mbox series

[v4,0/5] ARM: Add GXP I2C Support

Message ID 20230125184438.28483-1-nick.hawkins@hpe.com
Headers show
Series ARM: Add GXP I2C Support | expand

Message

Hawkins, Nick Jan. 25, 2023, 6:44 p.m. UTC
From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP SoC supports 10 I2C engines. Each I2C engine is completely
independent and can function both as an I2C master and I2C slave. The
I2C master can operate in a multi master environment. The engines support
a scalable speed from 8kHZ to 1.5 Mhz.

---

Changes since v3:
 *Switch engine variable to u32
 *Disable IRQ on device remove with register write instead
 *Provided even greater description with the use of Phandle
Changes since v2:
 *Disable IRQ on a device remove
 *Remove use of I2C_CLASS_DEPRECATED
 *Use i2c_parse_fw_timings instead of of_property_read_u32
 *Remove redundant dev_err_probe as platform_get_irq already has one
 *Used __iomem instead of res->start to find physical address
 *Use BIT in gxp_i2c_irq_handler
 *Made value u8 instead of u16 for u8 read
 *Provided a better description of Phandle in yaml
Changes since v1:
 *Removed yaml documentation of hpe,gxp-sysreg as it has been
  applied to syscon.yaml
 *Made i2cX a generic node name i2c in dts file
 *Added status field to the dtsi and the dts for i2c bus
 *Removed unnecessary size-cells and address-cells from yaml
 *Removed phandle from hpe,sysreg-phandle
 *Changed hpe,i2c-max-bus-freq to clock-frequency
 *Removed rogue tab in structure definition
 *Removed use of __iomem *base local variables as it was
  unnecessary
 *Switched #if IS_ENABLED() -> if (IS_ENABLED()) inside
  functions
 *Removed use of pr_* functions
 *Removed informational prints in register and unregister
  functions
 *Removed print from interrupt handler
 *Removed informational prints from probe function
 *Switched dev_err -> dev_err_probe in probe function
 *Used the respective helper for mapping the resource to
  __iomem

Nick Hawkins (5):
  i2c: hpe: Add GXP SoC I2C Controller
  dt-bindings: i2c: Add hpe,gxp-i2c
  ARM: dts: hpe: Add I2C Topology
  ARM: multi_v7_defconfig: add gxp i2c module
  MAINTAINERS: Add HPE GXP I2C Support

 .../devicetree/bindings/i2c/hpe,gxp-i2c.yaml  |  59 ++
 MAINTAINERS                                   |   2 +
 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts      | 109 ++++
 arch/arm/boot/dts/hpe-gxp.dtsi                | 125 ++++
 arch/arm/configs/multi_v7_defconfig           |   1 +
 drivers/i2c/busses/Kconfig                    |   7 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-gxp.c                  | 603 ++++++++++++++++++
 8 files changed, 907 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/hpe,gxp-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-gxp.c

Comments

Rob Herring Jan. 26, 2023, 1:38 p.m. UTC | #1
On Wed, Jan 25, 2023 at 3:32 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
>
> > > + hpe,sysreg:
> > > + $ref: /schemas/types.yaml#/definitions/phandle
> > > + description:
> > > + Phandle to the global status and enable interrupt registers shared
> > > + between each I2C engine controller instance. It enables the I2C
> > > + engine controller to act as both a master or slave by being able to
> > > + arm and respond to interrupts from its engine. Each bit in the
> > > + registers represent the respective bit position.
>
>
> > Each bit represents the bit position?
>
> Yes what I mean here is that bit 0 represents engine 0, bit 1 represents
> engine 1 and so on. I will reword this how you have below.
>
> > AIUI, each I2C instance has a bit in it needs to control. How does the
> > driver know what instance (and therefore the correct bit)? Typically you
> > would have a 2nd cell here with that information.
>
> We are currently using the memory area designated reg to determine
> which engine we are on.
>
> Here is a snippet from patch 1 of this patchset that introduces the driver:
> /* Use physical memory address to determine which I2C engine this is. */
> +       drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;
>
> This works because each engine is 0x100 apart.

Ah, that works fine then.

Reviewed-by: Rob Herring <robh@kernel.org>
Wolfram Sang Jan. 28, 2023, 6:38 p.m. UTC | #2
On Wed, Jan 25, 2023 at 12:44:38PM -0600, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Add the I2C controller source and bindings.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>

I assume this better goes via the arm-soc-tree as well to reduce merge
conflicts, so:

Acked-by: Wolfram Sang <wsa@kernel.org>

Let me know if I should pick this instead. Will review the driver in the
next days.
Hawkins, Nick Feb. 7, 2023, 9:53 p.m. UTC | #3
> Let me know if I should pick this instead. Will review the driver in the
> next days.

Please include it. There may be warnings generated if MAINTAINERS
does not list the files at the time of your commit.

Thank you,

-Nick Hawkins
Hawkins, Nick Feb. 15, 2023, 7:26 p.m. UTC | #4
> > Let me know if I should pick this instead. Will review the driver in the
> > next days.

> Please include it. There may be warnings generated if MAINTAINERS
does not list the files at the time of your commit.

> Thank you,

> -Nick Hawkins

Greetings Wolfram,

Just a gentle reminder on this review.

Thank you for your time,

-Nick Hawkins
Wolfram Sang Feb. 15, 2023, 8:29 p.m. UTC | #5
Hi Nick,

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index e50f9603d189..8b3951e0ca5c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1457,4 +1457,11 @@ config I2C_VIRTIO
>            This driver can also be built as a module. If so, the module
>            will be called i2c-virtio.
>  
> +config I2C_GXP
> +	tristate "GXP I2C Interface"
> +	depends on ARCH_HPE_GXP || COMPILE_TEST
> +	help
> +	  This enables support for GXP I2C interface. The I2C engines can be
> +	  either I2C master or I2C slaves.
> +

Shouldn't this be in the "I2C system bus drivers (mostly embedded /
system-on-chip)" section? (alphabetically sorted)

> +static bool i2c_global_init_done;

Can't we avoid this static by checking if i2cg_map is NULL/non-NULL?

> +
> +enum {
> +	GXP_I2C_IDLE = 0,
> +	GXP_I2C_ADDR_PHASE,
> +	GXP_I2C_RDATA_PHASE,
> +	GXP_I2C_WDATA_PHASE,
> +	GXP_I2C_ADDR_NACK,
> +	GXP_I2C_DATA_NACK,
> +	GXP_I2C_ERROR,
> +	GXP_I2C_COMP
> +};
> +
> +struct gxp_i2c_drvdata {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct i2c_timings t;
> +	u32 engine;
> +	int irq;
> +	struct completion completion;
> +	struct i2c_adapter adapter;
> +	struct i2c_msg *curr_msg;
> +	int msgs_remaining;
> +	int msgs_num;
> +	u8 *buf;
> +	size_t buf_remaining;
> +	unsigned char state;
> +	struct i2c_client *slave;
> +	unsigned char stopped;
> +};
> +
> +static struct regmap *i2cg_map;
> +
> +static void gxp_i2c_start(struct gxp_i2c_drvdata *drvdata)
> +{
> +	u16 value;
> +
> +	drvdata->buf = drvdata->curr_msg->buf;
> +	drvdata->buf_remaining = drvdata->curr_msg->len;
> +
> +	/* Note: Address in struct i2c_msg is 7 bits */
> +	value = drvdata->curr_msg->addr << 9;
> +
> +	if (drvdata->curr_msg->flags & I2C_M_RD) {
> +		/* Read */
> +		value |= 0x05;
> +	} else {
> +		/* Write */
> +		value |= 0x01;
> +	}

I'd write it more concise as:

	value |= drvdata->curr_msg->flags & I2C_M_RD ? 0x05 : 0x01;

But this is a matter of taste.

> +
> +	drvdata->state = GXP_I2C_ADDR_PHASE;
> +	writew(value, drvdata->base + GXP_I2CMCMD);
> +}
> +
> +static int gxp_i2c_master_xfer(struct i2c_adapter *adapter,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	int ret;
> +	struct gxp_i2c_drvdata *drvdata = i2c_get_adapdata(adapter);
> +	unsigned long time_left;
> +
> +	drvdata->msgs_remaining = num;
> +	drvdata->curr_msg = msgs;
> +	drvdata->msgs_num = num;
> +	reinit_completion(&drvdata->completion);
> +
> +	gxp_i2c_start(drvdata);
> +
> +	time_left = wait_for_completion_timeout(&drvdata->completion,
> +						adapter->timeout);
> +	ret = num - drvdata->msgs_remaining;
> +	if (time_left == 0) {
> +		switch (drvdata->state) {
> +		case GXP_I2C_WDATA_PHASE:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:write Data phase timeout at msg[%d]\n",
> +			ret);

Please don't write error message to the log on timeouts. They can
happen. Think of an EEPROM which is busy because it needs to erase a
page. Upper layers need to handle this correctly, the user doesn't need
to know about it.

> +			break;
> +		case GXP_I2C_RDATA_PHASE:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:read Data phase timeout at msg[%d]\n",
> +			ret);
> +			break;
> +		case GXP_I2C_ADDR_PHASE:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:Addr phase timeout\n");
> +			break;
> +		default:
> +			dev_err(drvdata->dev,
> +				"gxp_i2c_start:i2c transfer timeout state=%d\n",
> +			drvdata->state);
> +			break;
> +		}
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (drvdata->state == GXP_I2C_ADDR_NACK) {
> +		dev_err(drvdata->dev,
> +			"gxp_i2c_start:No ACK for address phase\n");
> +		return -EIO;
> +	} else if (drvdata->state == GXP_I2C_DATA_NACK) {
> +		dev_err(drvdata->dev, "gxp_i2c_start:No ACK for data phase\n");
> +		return -EIO;

Same here for NACK. Otherwise i2cdetect will flood your logfile.

> +	}
> +
> +	return ret;
> +}
> +
> +static u32 gxp_i2c_func(struct i2c_adapter *adap)
> +{
> +	if (IS_ENABLED(CONFIG_I2C_SLAVE))
> +		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
> +
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int gxp_i2c_reg_slave(struct i2c_client *slave)
> +{
> +	struct gxp_i2c_drvdata *drvdata = i2c_get_adapdata(slave->adapter);
> +
> +	if (drvdata->slave)
> +		return -EBUSY;
> +
> +	if (slave->flags & I2C_CLIENT_TEN)
> +		return -EAFNOSUPPORT;
> +
> +	drvdata->slave = slave;
> +
> +	writeb(slave->addr << 1, drvdata->base + GXP_I2COWNADR);
> +	writeb(0x69, drvdata->base + GXP_I2CSCMD);

Does it make sense to have #defines for the magic values for I2CSCMD?
BTW, is the datasheet public?

...

> +static void gxp_i2c_init(struct gxp_i2c_drvdata *drvdata)
> +{
> +	drvdata->state = GXP_I2C_IDLE;
> +	writeb(2000000 / drvdata->t.bus_freq_hz,
> +	       drvdata->base + GXP_I2CFREQDIV);
> +	writeb(0x32, drvdata->base + GXP_I2CFLTFAIR);
> +	writeb(0x0a, drvdata->base + GXP_I2CTMOEDG);
> +	writeb(0x00, drvdata->base + GXP_I2CCYCTIM);
> +	writeb(0x00, drvdata->base + GXP_I2CSNPAA);
> +	writeb(0x00, drvdata->base + GXP_I2CADVFEAT);
> +	writeb(0xF0, drvdata->base + GXP_I2CSCMD);
> +	writeb(0x80, drvdata->base + GXP_I2CMCMD);
> +	writeb(0x00, drvdata->base + GXP_I2CEVTERR);
> +	writeb(0x00, drvdata->base + GXP_I2COWNADR);

Also here, lots of magic values. Can we do something about it?

> +}
> +
> +static int gxp_i2c_probe(struct platform_device *pdev)
> +{
> +	struct gxp_i2c_drvdata *drvdata;
> +	int rc;
> +	struct i2c_adapter *adapter;
> +
> +	if (!i2c_global_init_done) {
> +		i2cg_map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							   "hpe,sysreg");
> +		if (IS_ERR(i2cg_map)) {
> +			return dev_err_probe(&pdev->dev, IS_ERR(i2cg_map),
> +					     "failed to map i2cg_handle\n");
> +		}
> +
> +		/* Disable interrupt */
> +		regmap_update_bits(i2cg_map, GXP_I2CINTEN, 0x00000FFF, 0);
> +		i2c_global_init_done = true;
> +	}
> +
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata),
> +			       GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, drvdata);
> +	drvdata->dev = &pdev->dev;
> +	init_completion(&drvdata->completion);
> +
> +	drvdata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
> +
> +	/* Use physical memory address to determine which I2C engine this is. */
> +	drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;

This breaks on my 64-bit test-build, so it will also fail with
COMPILE_TEST.

drivers/i2c/busses/i2c-gxp.c: In function ‘gxp_i2c_probe’:
drivers/i2c/busses/i2c-gxp.c:533:28: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
  533 |         drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;

The rest looks good to me.

Except for removing the hardcoded values, the other fixes should be
fairly simple, I think. So, hardcoded values could be changed
incrementally maybe. If this is possible at all. I still plan to have
this driver in 6.3.

Happy hacking,

   Wolfram
Hawkins, Nick Feb. 16, 2023, 8:02 p.m. UTC | #6
> Does it make sense to have #defines for the magic values for I2CSCMD?
> BTW, is the datasheet public?

I will work on the defines and get rid of all the magic values.
Unfortunately, there is no public spec available currently.
Hopefully, we will have one someday though.

> > + /* Use physical memory address to determine which I2C engine this is. */
> > + drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;


> This breaks on my 64-bit test-build, so it will also fail with
> COMPILE_TEST.


> drivers/i2c/busses/i2c-gxp.c: In function ‘gxp_i2c_probe’:
> drivers/i2c/busses/i2c-gxp.c:533:28: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> 533 | drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;

I am currently unable to reproduce this error. I even set W=2.
Would replacing (u32) with (unsigned long) resolve it?

Thanks,

-Nick Hawkins
Wolfram Sang Feb. 16, 2023, 8:47 p.m. UTC | #7
Hi Nick,

> I will work on the defines and get rid of all the magic values.

Cool, thank you!

> Unfortunately, there is no public spec available currently.
> Hopefully, we will have one someday though.

Then, the #defines are even more helpful!

> > drivers/i2c/busses/i2c-gxp.c: In function ‘gxp_i2c_probe’:
> > drivers/i2c/busses/i2c-gxp.c:533:28: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
> > 533 | drvdata->engine = ((u32)drvdata->base & 0xf00) >> 8;
> 
> I am currently unable to reproduce this error. I even set W=2.

Interesting. I have this version:

gcc (Debian 10.2.1-6) 10.2.1 20210110

> Would replacing (u32) with (unsigned long) resolve it?

Or size_t.

Happy hacking,

   Wolfram