| Message ID | 20230426071434.452717-3-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti: > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate > with SFP. > > Introduce the property "i2c-dw-flags" to match device data for software > node case. Since IO resource was mapped on the ethernet driver, add a model > quirk to get resource from platform info. > > The exists IP limitations are dealt as workarounds: > - IP does not support interrupt mode, it works on polling mode. > - Additionally set FIFO depth address the chip issue. Thanks for an update, my comments below. ... > goto done_nolock; > } > > + if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) { > + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > + goto done_nolock; > + } switch (dev->flags & MODEL_MASK) { case AMD: ... case WANGXUN: ... default: break; } ... > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev->dev); > + struct resource *r; > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!r) > + return -ENODEV; > + > + dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > + > + return PTR_ERR_OR_ZERO(dev->base); > +} Redundant. See below. ... > case MODEL_BAIKAL_BT1: > ret = bt1_i2c_request_regs(dev); > break; > + case MODEL_WANGXUN_SP: > + ret = txgbe_i2c_request_regs(dev); How is it different to... > + break; > default: > dev->base = devm_platform_ioremap_resource(pdev, 0); ...this one? ... > dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > + if (!dev->flags) No need to check this. Just define priorities (I would go with above to be higher priority). > + device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags); Needs to be added to the Device Tree bindings I believe. But wait, don't we have other ways to detect your hardware at probe time and initialize flags respectively?
On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@gmail.com wrote: > Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti: > > Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate > > with SFP. > > > > Introduce the property "i2c-dw-flags" to match device data for software > > node case. Since IO resource was mapped on the ethernet driver, add a model > > quirk to get resource from platform info. > > > > The exists IP limitations are dealt as workarounds: > > - IP does not support interrupt mode, it works on polling mode. > > - Additionally set FIFO depth address the chip issue. > > Thanks for an update, my comments below. > > ... > > > goto done_nolock; > > } > > > > + if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) { > > + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); > > + goto done_nolock; > > + } > > switch (dev->flags & MODEL_MASK) { > case AMD: > ... > case WANGXUN: > ... > default: > break; > } > > ... > > > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev->dev); > > + struct resource *r; > > + > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!r) > > + return -ENODEV; > > + > > + dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > > + > > + return PTR_ERR_OR_ZERO(dev->base); > > +} > > Redundant. See below. > > ... > > > case MODEL_BAIKAL_BT1: > > ret = bt1_i2c_request_regs(dev); > > break; > > + case MODEL_WANGXUN_SP: > > + ret = txgbe_i2c_request_regs(dev); > > How is it different to... > > > + break; > > default: > > dev->base = devm_platform_ioremap_resource(pdev, 0); > > ...this one? > devm_platform_ioremap_resource() has one more devm_request_mem_region() operation than devm_ioremap(). By my test, this memory cannot be re-requested, only re-mapped. > ... > > > dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > > > + if (!dev->flags) > > No need to check this. Just define priorities (I would go with above to be > higher priority). > > > + device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags); > > Needs to be added to the Device Tree bindings I believe. > > But wait, don't we have other ways to detect your hardware at probe time and > initialize flags respectively? > I2C is connected to our NIC chip with no PCI ID, so I register a platform device for it. Please see the 4/9 patch. Software nodes are used to pass the device structure but no DT and ACPI. I haven't found another way to initialize flags yet, other than the platform data used in the previous patch (it seems to be an obsolete way). > -- > With Best Regards, > Andy Shevchenko > > >
On Thu, Apr 27, 2023 at 5:15 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@gmail.com wrote: > > Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti: ... > > > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) > > > +{ > > > + struct platform_device *pdev = to_platform_device(dev->dev); > > > + struct resource *r; > > > + > > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!r) > > > + return -ENODEV; > > > + > > > + dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > > > + > > > + return PTR_ERR_OR_ZERO(dev->base); > > > +} > > > > Redundant. See below. ... > > > case MODEL_BAIKAL_BT1: > > > ret = bt1_i2c_request_regs(dev); > > > break; > > > + case MODEL_WANGXUN_SP: > > > + ret = txgbe_i2c_request_regs(dev); > > > > How is it different to... > > > > > + break; > > > default: > > > dev->base = devm_platform_ioremap_resource(pdev, 0); > > > > ...this one? > > devm_platform_ioremap_resource() has one more devm_request_mem_region() > operation than devm_ioremap(). By my test, this memory cannot be re-requested, > only re-mapped. Yeah, which makes a point that the mother driver requests a region that doesn't belong to it. You need to split that properly in the mother driver and avoid requesting it there. Is it feasible? If not, why? ... > > > dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > > > > > + if (!dev->flags) > > > > No need to check this. Just define priorities (I would go with above to be > > higher priority). > > > > > + device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags); > > > > Needs to be added to the Device Tree bindings I believe. > > > > But wait, don't we have other ways to detect your hardware at probe time and > > initialize flags respectively? > > I2C is connected to our NIC chip with no PCI ID, so I register a platform device for it. > Please see the 4/9 patch. Software nodes are used to pass the device structure but > no DT and ACPI. I haven't found another way to initialize flags yet, other than the > platform data used in the previous patch (it seems to be an obsolete way). You can share a common data structure between the mother driver and her children. In that case you may access it via `dev_get_drvdata(pdev.dev->parent)` call. OTOH, the property, if only Linux (kernel) specific for internal usage, should be named accordingly, or be prepared to have one in Device Tree / ACPI / etc. Examples: USB dwc3 driver (see "linux," ones), or intel-lpss-pci.c/intel-lpss-acpi.c (see the SPI type).
On Thursday, April 27, 2023 1:57 PM, Andy Shevchenko wrote: > On Thu, Apr 27, 2023 at 5:15 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > > On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@gmail.com wrote: > > > Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti: > > ... > > > > > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) > > > > +{ > > > > + struct platform_device *pdev = to_platform_device(dev->dev); > > > > + struct resource *r; > > > > + > > > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > + if (!r) > > > > + return -ENODEV; > > > > + > > > > + dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > > > > + > > > > + return PTR_ERR_OR_ZERO(dev->base); > > > > +} > > > > > > Redundant. See below. > > ... > > > > > case MODEL_BAIKAL_BT1: > > > > ret = bt1_i2c_request_regs(dev); > > > > break; > > > > + case MODEL_WANGXUN_SP: > > > > + ret = txgbe_i2c_request_regs(dev); > > > > > > How is it different to... > > > > > > > + break; > > > > default: > > > > dev->base = devm_platform_ioremap_resource(pdev, 0); > > > > > > ...this one? > > > > devm_platform_ioremap_resource() has one more devm_request_mem_region() > > operation than devm_ioremap(). By my test, this memory cannot be re-requested, > > only re-mapped. > > Yeah, which makes a point that the mother driver requests a region > that doesn't belong to it. You need to split that properly in the > mother driver and avoid requesting it there. Is it feasible? If not, > why? The I2C region belongs to the middle part of the total region. It was not considered to split because the mother driver implement I2C bus master driver itself in the previous patch. But is it suitable for splitting? After splitting, I get two virtual address, and each time I read/write to a register, I have to determine which region it belongs to...Right? > > ... > > > > > dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); > > > > > > > + if (!dev->flags) > > > > > > No need to check this. Just define priorities (I would go with above to be > > > higher priority). > > > > > > > + device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags); > > > > > > Needs to be added to the Device Tree bindings I believe. > > > > > > But wait, don't we have other ways to detect your hardware at probe time and > > > initialize flags respectively? > > > > I2C is connected to our NIC chip with no PCI ID, so I register a platform device for it. > > Please see the 4/9 patch. Software nodes are used to pass the device structure but > > no DT and ACPI. I haven't found another way to initialize flags yet, other than the > > platform data used in the previous patch (it seems to be an obsolete way). > > You can share a common data structure between the mother driver and > her children. In that case you may access it via > `dev_get_drvdata(pdev.dev->parent)` call. > I'd like to try it. > OTOH, the property, if only Linux (kernel) specific for internal > usage, should be named accordingly, or be prepared to have one in > Device Tree / ACPI / etc. Examples: USB dwc3 driver (see "linux," > ones), or intel-lpss-pci.c/intel-lpss-acpi.c (see the SPI type). > > -- > With Best Regards, > Andy Shevchenko >
> You can share a common data structure between the mother driver and > her children. In that case you may access it via > `dev_get_drvdata(pdev.dev->parent)` call. > Unfortunately, the driver data already stores the private structure of the adapter, so no more data can be stored.
On Thu, Apr 27, 2023 at 9:58 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > On Thursday, April 27, 2023 1:57 PM, Andy Shevchenko wrote: > > On Thu, Apr 27, 2023 at 5:15 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote: > > > On Wednesday, April 26, 2023 11:45 PM, andy.shevchenko@gmail.com wrote: > > > > Wed, Apr 26, 2023 at 03:14:27PM +0800, Jiawen Wu kirjoitti: ... > > > > > +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) > > > > > +{ > > > > > + struct platform_device *pdev = to_platform_device(dev->dev); > > > > > + struct resource *r; > > > > > + > > > > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > + if (!r) > > > > > + return -ENODEV; > > > > > + > > > > > + dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); > > > > > + > > > > > + return PTR_ERR_OR_ZERO(dev->base); > > > > > +} > > > > > > > > Redundant. See below. ... > > > > > case MODEL_BAIKAL_BT1: > > > > > ret = bt1_i2c_request_regs(dev); > > > > > break; > > > > > + case MODEL_WANGXUN_SP: > > > > > + ret = txgbe_i2c_request_regs(dev); > > > > > > > > How is it different to... > > > > > > > > > + break; > > > > > default: > > > > > dev->base = devm_platform_ioremap_resource(pdev, 0); > > > > > > > > ...this one? > > > > > > devm_platform_ioremap_resource() has one more devm_request_mem_region() > > > operation than devm_ioremap(). By my test, this memory cannot be re-requested, > > > only re-mapped. > > > > Yeah, which makes a point that the mother driver requests a region > > that doesn't belong to it. You need to split that properly in the > > mother driver and avoid requesting it there. Is it feasible? If not, > > why? > > The I2C region belongs to the middle part of the total region. It was not considered to > split because the mother driver implement I2C bus master driver itself in the previous > patch. But is it suitable for splitting? After splitting, I get two virtual address, and each > time I read/write to a register, I have to determine which region it belongs to...Right? If it's in the middle there are two (maybe more, but I can't right now come up with) possible solutions: 1/ mother driver can provide a regmap, then in the children drivers the dev_get_regmap(pdev->dev.parent) will return it (see intel_soc_pmic_*.c set of drivers, IIRC they work with this schema in mind) 2/ in the mother driver you need to open code remapping resource in a way that it does ioremap and request regions separately. The problem with your current approach is that you request a region in the driver which does not handle that part of IO space and belongs to another one. It's a clear layering violation. (Note, we already have one big driver that does something like this and we have to learn from it not to be trapped by making the same mistake).
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 0dc6b1ce663f..a7c2e67ccbf6 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -575,6 +575,14 @@ int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev) unsigned int param; int ret; + /* DW_IC_COMP_PARAM_1 not implement for IP issue */ + if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) { + dev->tx_fifo_depth = 4; + dev->rx_fifo_depth = 0; + + return 0; + } + /* * Try to detect the FIFO depth if not set by interface driver, * the depth could be from 2 to 256 from HW spec. diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 050d8c63ad3c..c686198e12d2 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -303,6 +303,7 @@ struct dw_i2c_dev { #define MODEL_MSCC_OCELOT BIT(8) #define MODEL_BAIKAL_BT1 BIT(9) #define MODEL_AMD_NAVI_GPU BIT(10) +#define MODEL_WANGXUN_SP BIT(11) #define MODEL_MASK GENMASK(11, 8) /* diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 55ea91a63382..fe85c2ac79d7 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -354,6 +354,68 @@ static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, return 0; } +static int i2c_dw_poll_tx_empty(struct dw_i2c_dev *dev) +{ + u32 val; + + return regmap_read_poll_timeout(dev->map, DW_IC_RAW_INTR_STAT, val, + val & DW_IC_INTR_TX_EMPTY, + 100, 1000); +} + +static int i2c_dw_poll_rx_full(struct dw_i2c_dev *dev) +{ + u32 val; + + return regmap_read_poll_timeout(dev->map, DW_IC_RAW_INTR_STAT, val, + val & DW_IC_INTR_RX_FULL, + 100, 1000); +} + +static int txgbe_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, + int num_msgs) +{ + struct dw_i2c_dev *dev = i2c_get_adapdata(adap); + int msg_idx, buf_len, data_idx, ret; + unsigned int val, stop = 0; + u8 *buf; + + dev->msgs = msgs; + dev->msgs_num = num_msgs; + i2c_dw_xfer_init(dev); + regmap_write(dev->map, DW_IC_INTR_MASK, 0); + + for (msg_idx = 0; msg_idx < num_msgs; msg_idx++) { + buf = msgs[msg_idx].buf; + buf_len = msgs[msg_idx].len; + + for (data_idx = 0; data_idx < buf_len; data_idx++) { + if (msg_idx == num_msgs - 1 && data_idx == buf_len - 1) + stop |= BIT(9); + + if (msgs[msg_idx].flags & I2C_M_RD) { + regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | stop); + + ret = i2c_dw_poll_rx_full(dev); + if (ret) + return ret; + + regmap_read(dev->map, DW_IC_DATA_CMD, &val); + buf[data_idx] = val; + } else { + ret = i2c_dw_poll_tx_empty(dev); + if (ret) + return ret; + + regmap_write(dev->map, DW_IC_DATA_CMD, + buf[data_idx] | stop); + } + } + } + + return num_msgs; +} + /* * Initiate (and continue) low level master read/write transaction. * This function is only called from i2c_dw_isr, and pumping i2c_msg @@ -568,6 +630,11 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) goto done_nolock; } + if ((dev->flags & MODEL_MASK) == MODEL_WANGXUN_SP) { + ret = txgbe_i2c_dw_xfer_quirk(adap, msgs, num); + goto done_nolock; + } + reinit_completion(&dev->cmd_complete); dev->msgs = msgs; dev->msgs_num = num; @@ -848,7 +915,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) return 0; } -static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev) +static int i2c_dw_poll_adap_quirk(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; int ret; @@ -862,6 +929,17 @@ static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev) return ret; } +static bool i2c_dw_is_model_poll(struct dw_i2c_dev *dev) +{ + switch (dev->flags & MODEL_MASK) { + case MODEL_AMD_NAVI_GPU: + case MODEL_WANGXUN_SP: + return true; + default: + return false; + } +} + int i2c_dw_probe_master(struct dw_i2c_dev *dev) { struct i2c_adapter *adap = &dev->adapter; @@ -917,8 +995,8 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev) adap->dev.parent = dev->dev; i2c_set_adapdata(adap, dev); - if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) - return amd_i2c_adap_quirk(dev); + if (i2c_dw_is_model_poll(dev)) + return i2c_dw_poll_adap_quirk(dev); if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { irq_flags = IRQF_NO_SUSPEND; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 74182db03a88..5504579cd622 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -168,6 +168,20 @@ static inline int dw_i2c_of_configure(struct platform_device *pdev) } #endif +static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev) +{ + struct platform_device *pdev = to_platform_device(dev->dev); + struct resource *r; + + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!r) + return -ENODEV; + + dev->base = devm_ioremap(&pdev->dev, r->start, resource_size(r)); + + return PTR_ERR_OR_ZERO(dev->base); +} + static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev) { pm_runtime_disable(dev->dev); @@ -185,6 +199,9 @@ static int dw_i2c_plat_request_regs(struct dw_i2c_dev *dev) case MODEL_BAIKAL_BT1: ret = bt1_i2c_request_regs(dev); break; + case MODEL_WANGXUN_SP: + ret = txgbe_i2c_request_regs(dev); + break; default: dev->base = devm_platform_ioremap_resource(pdev, 0); ret = PTR_ERR_OR_ZERO(dev->base); @@ -278,6 +295,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) return -ENOMEM; dev->flags = (uintptr_t)device_get_match_data(&pdev->dev); + if (!dev->flags) + device_property_read_u32(&pdev->dev, "i2c-dw-flags", &dev->flags); + dev->dev = &pdev->dev; dev->irq = irq; platform_set_drvdata(pdev, dev);
Wangxun 10Gb ethernet chip is connected to Designware I2C, to communicate with SFP. Introduce the property "i2c-dw-flags" to match device data for software node case. Since IO resource was mapped on the ethernet driver, add a model quirk to get resource from platform info. The exists IP limitations are dealt as workarounds: - IP does not support interrupt mode, it works on polling mode. - Additionally set FIFO depth address the chip issue. Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/i2c/busses/i2c-designware-common.c | 8 ++ drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-master.c | 84 ++++++++++++++++++++- drivers/i2c/busses/i2c-designware-platdrv.c | 20 +++++ 4 files changed, 110 insertions(+), 3 deletions(-)