| Message ID | 20230426071434.452717-1-jiawenwu@trustnetic.com |
|---|---|
| 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).