| Message ID | 20230125184438.28483-1-nick.hawkins@hpe.com |
|---|---|
| Headers | show |
| Series | ARM: Add GXP I2C Support | expand |
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>
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.
> 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
> > 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
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
> 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
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
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