Message ID | 20250507071315.394857-1-herve.codina@bootlin.com |
---|---|
Headers | show |
Series | lan966x pci device: Add support for SFPs | expand |
On Wed, May 07, 2025 at 09:13:03AM +0200, Herve Codina wrote: > The lan966x_pci.dtso file contains descriptions related to both the > LAN966x PCI device chip and the LAN966x PCI device board where the chip > is soldered. > > Split the file in order to have: > - lan966x_pci.dtsi > The description related to the PCI chip. > > - lan966x_pci.dtso > The description of the PCI board. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > This avoid to use the code for an other board. > > In order to be more generic and to allow support for other boards (PCI > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > it to PCI Vendor/Device IDs handled by the driver. > > This structure contains information related to the PCI board such as > information related to the dtbo describing the board we have to load. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> How big is the dtbo ? This is going in the right direction. I'm just wondering if each dtbo should be wrapped in its own very slim PCI driver, which simply registers its lan966x_pci_info structure to a core driver. Only the needed dtbo will then be loaded into memory as a module, not them all. Pretty much all the pieces are here, so it can be done later. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, 8 May 2025 at 00:24, Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > > > This avoid to use the code for an other board. > > > > In order to be more generic and to allow support for other boards (PCI > > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > > it to PCI Vendor/Device IDs handled by the driver. > > > > This structure contains information related to the PCI board such as > > information related to the dtbo describing the board we have to load. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > How big is the dtbo ? > > This is going in the right direction. I'm just wondering if each dtbo > should be wrapped in its own very slim PCI driver, which simply > registers its lan966x_pci_info structure to a core driver. Only the > needed dtbo will then be loaded into memory as a module, not them all. Alternatively, the dtbo could be loaded through request_firmware(). That could lead to a generic support option in the PCI core, which would fallback to loading pci-<vid>-<pid>.dtbo when no driver is available. > Pretty much all the pieces are here, so it can be done later. Exactly. Gr{oetje,eeting}s, Geert
On Wed, 7 May 2025 18:10:40 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, May 07, 2025 at 09:12:51AM +0200, Herve Codina wrote: > > The code set directly dev->fwnode. > > > > Use the dedicated helper to perform this operation. > > ... > > > @@ -1046,7 +1046,7 @@ static void mock_companion(struct acpi_device *adev, struct device *dev) > > { > > device_initialize(&adev->dev); > > fwnode_init(&adev->fwnode, NULL); > > - dev->fwnode = &adev->fwnode; > > + device_set_node(dev, &adev->fwnode); > > adev->fwnode.dev = dev; > > } > > This code is questionable to begin with. Can the original author explain what > is the motivation behind this as the only callers of fwnode_init() are deep > core pieces _and_ this only module. Why?! > More likely to happen if CXL folk are +CC. Added. Dan, maybe one for you?
On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: > The simple-pm-bus drivers handles several simple bus. When it is used bus --> busses ? > with busses other than a compatible "simple-pm-bus", it don't populate > its child devices during its probe. > > This confuses fw_devlink and results in wrong or missing devlinks. > > Once a driver is bound to a device and the probe() has been called, > device_links_driver_bound() is called. > > This function performs operation based on the following assumption: > If a child firmware node of the bound device is not added as a > device, it will never be added. > > Among operations done on fw_devlinks of those "never be added" devices, > device_links_driver_bound() changes their supplier. > > With devices attached to a simple-bus compatible device, this change > leads to wrong devlinks where supplier of devices points to the device > parent (i.e. simple-bus compatible device) instead of the device itself > (i.e. simple-bus child). > > When the device attached to the simple-bus is removed, because devlinks > are not correct, its consumers are not removed first. > > In order to have correct devlinks created, make the simple-pm-bus driver > compliant with the devlink assumption and create its child devices > during its probe. ... > if (match && match->data) { > if (of_property_match_string(np, "compatible", match->compatible) == 0) Side note, there is an fwnode_is_device_compatible() API for such cases. And IIRC there is also OF variant of it. > - return 0; > + goto populate; > else > return -ENODEV; > } ... > + if (pdev->dev.of_node) Why do you need this check? AFAICS it dups the one the call has already in it. > + of_platform_depopulate(&pdev->dev);
On Wed, May 07, 2025 at 09:12:59AM +0200, Herve Codina wrote: > PCI drivers can use a device-tree overlay to describe the hardware > available on the PCI board. This is the case, for instance, of the > LAN966x PCI device driver. > > Adding some more nodes in the device-tree overlay adds some more > consumer/supplier relationship between devices instantiated from this > overlay. > > Those fw_node consumer/supplier relationships are handled by fw_devlink > and are created based on the device-tree parsing done by the > of_fwnode_add_links() function. > > Those consumer/supplier links are needed in order to ensure a correct PM > runtime management and a correct removal order between devices. > > For instance, without those links a supplier can be removed before its > consumers is removed leading to all kind of issue if this consumer still are removed OR consumer > want the use the already removed supplier. > > The support for the usage of an overlay from a PCI driver has been added > on x86 systems in commit 1f340724419ed ("PCI: of: Create device tree PCI > host bridge node"). > > In the past, support for fw_devlink on x86 had been tried but this > support has been removed in commit 4a48b66b3f52 ("of: property: Disable > fw_devlink DT support for X86"). Indeed, this support was breaking some > x86 systems such as OLPC system and the regression was reported in [0]. > > Instead of disabling this support for all x86 system, a first approach > would be to use a finer grain and disable this support only for the > possible problematic subset of x86 systems (at least OLPC and CE4100). > > This first approach could still leads to issues. Indeed, the list of > possible problematic system and the way to identify them using Kconfig > symbols is not well defined and so some system can be missed leading to > kernel regressions on those missing systems. > > Use an other way and enable the support on x86 system only when this > support is needed by some specific feature. The usage of a device-tree > overlay by a PCI driver and thus the creation of PCI device-tree nodes > is a feature that needs it. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > link: https://lore.kernel.org/lkml/3c1f2473-92ad-bfc4-258e-a5a08ad73dd0@web.de/ [0] Link: (mind capitalisation) Otherwise LGTM, FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > This avoid to use the code for an other board. > > In order to be more generic and to allow support for other boards (PCI > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > it to PCI Vendor/Device IDs handled by the driver. > > This structure contains information related to the PCI board such as > information related to the dtbo describing the board we have to load. ... > static struct pci_device_id lan966x_pci_ids[] = { > - { PCI_DEVICE(PCI_VENDOR_ID_EFAR, 0x9660) }, > + { PCI_VDEVICE(EFAR, 0x9660), (kernel_ulong_t)&evb_lan9662_nic_info }, PCI_DEVICE_DATA() ? > { } > };
Hi Herve, On Wed, May 07, 2025 at 09:13:01AM +0200, Herve Codina wrote: > The AT91 I2C driver depends on ARCH_AT91. > > This I2C controller can be used by the LAN966x PCI device and so > it needs to be available when the LAN966x PCI device is enabled. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Acked-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Andy, On Thu, 8 May 2025 17:27:52 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: > > The simple-pm-bus drivers handles several simple bus. When it is used > > bus --> busses ? Yes sure. > > > with busses other than a compatible "simple-pm-bus", it don't populate > > its child devices during its probe. > > > > This confuses fw_devlink and results in wrong or missing devlinks. > > > > Once a driver is bound to a device and the probe() has been called, > > device_links_driver_bound() is called. > > > > This function performs operation based on the following assumption: > > If a child firmware node of the bound device is not added as a > > device, it will never be added. > > > > Among operations done on fw_devlinks of those "never be added" devices, > > device_links_driver_bound() changes their supplier. > > > > With devices attached to a simple-bus compatible device, this change > > leads to wrong devlinks where supplier of devices points to the device > > parent (i.e. simple-bus compatible device) instead of the device itself > > (i.e. simple-bus child). > > > > When the device attached to the simple-bus is removed, because devlinks > > are not correct, its consumers are not removed first. > > > > In order to have correct devlinks created, make the simple-pm-bus driver > > compliant with the devlink assumption and create its child devices > > during its probe. > > ... > > > if (match && match->data) { > > if (of_property_match_string(np, "compatible", match->compatible) == 0) > > Side note, there is an fwnode_is_device_compatible() API for such cases. And IIRC > there is also OF variant of it. fwnode_device_is_compatible() checked for all compatible string. I mean, if we have compatible = "foo,custom-bus", "simple-bus"; fwnode_device_is_compatible() checking against "simple-bus" returns true. Here, we want "simple-bus" as the first position in the compatible string. In other word, we want to match the more specific compatible string as mentioned in the comment. > > > - return 0; > > + goto populate; > > else > > return -ENODEV; > > } > > ... > > > + if (pdev->dev.of_node) > > Why do you need this check? AFAICS it dups the one the call has already in it. of_platform_populate() was called only if an OF node is present. I want to call of_platform_depopulate() on removal also only if an OF node is present. I don't see the other call that duplicated this check. Can you clarify? > > > + of_platform_depopulate(&pdev->dev); > Best regards, Hervé
On Mon, May 19, 2025 at 01:58:18PM +0200, Herve Codina wrote: > On Thu, 8 May 2025 17:27:52 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: ... > > > if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > Side note, there is an fwnode_is_device_compatible() API for such cases. And IIRC > > there is also OF variant of it. > > fwnode_device_is_compatible() checked for all compatible string. I mean, if > we have compatible = "foo,custom-bus", "simple-bus"; > fwnode_device_is_compatible() checking against "simple-bus" returns true. > > Here, we want "simple-bus" as the first position in the compatible string. > In other word, we want to match the more specific compatible string as > mentioned in the comment. I admit I'm not an expert in DT, but why is the compatibility position dependent? ... > > > + if (pdev->dev.of_node) > > > > Why do you need this check? AFAICS it dups the one the call has already in it. > > of_platform_populate() was called only if an OF node is present. > I want to call of_platform_depopulate() on removal also only if an OF node > is present. > > I don't see the other call that duplicated this check. > > Can you clarify? The of_...() is already NULL-aware (AFAICS), why do you need the duplicated check? > > > + of_platform_depopulate(&pdev->dev);
Hi Andy, On Mon, 19 May 2025 15:06:33 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, May 19, 2025 at 01:58:18PM +0200, Herve Codina wrote: > > On Thu, 8 May 2025 17:27:52 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, May 07, 2025 at 09:12:47AM +0200, Herve Codina wrote: > > ... > > > > > if (of_property_match_string(np, "compatible", match->compatible) == 0) > > > > > > Side note, there is an fwnode_is_device_compatible() API for such cases. And IIRC > > > there is also OF variant of it. > > > > fwnode_device_is_compatible() checked for all compatible string. I mean, if > > we have compatible = "foo,custom-bus", "simple-bus"; > > fwnode_device_is_compatible() checking against "simple-bus" returns true. > > > > Here, we want "simple-bus" as the first position in the compatible string. > > In other word, we want to match the more specific compatible string as > > mentioned in the comment. > > I admit I'm not an expert in DT, but why is the compatibility position > dependent? > > ... > > > > > + if (pdev->dev.of_node) > > > > > > Why do you need this check? AFAICS it dups the one the call has already in it. > > > > of_platform_populate() was called only if an OF node is present. > > I want to call of_platform_depopulate() on removal also only if an OF node > > is present. > > > > I don't see the other call that duplicated this check. > > > > Can you clarify? > > The of_...() is already NULL-aware (AFAICS), why do you need the duplicated > check? Oh, I see. I was focused on previous of_device_get_match_data() call. My bad. Indeed, you're right, I can call directly of_platform_depopulate() without checking pdev->dev.of_node before the call. The check is already present in of_platform_depopulate() itself. I will do that in the next iteration. Thanks for pointing out. Best regards, Hervé
Hi Andrew, On Thu, 8 May 2025 00:24:03 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > > > This avoid to use the code for an other board. > > > > In order to be more generic and to allow support for other boards (PCI > > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > > it to PCI Vendor/Device IDs handled by the driver. > > > > This structure contains information related to the PCI board such as > > information related to the dtbo describing the board we have to load. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > How big is the dtbo ? With current series applied, the dtbo is 6029 Bytes. Best regards, Hervé
Hi Andy, On Thu, 8 May 2025 22:21:41 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: > > Only one device-tree overlay (lan966x_evb_lan9662_nic.dtbo) is handled > > and this overlay is directly referenced in lan966x_pci_load_overlay(). > > > > This avoid to use the code for an other board. > > > > In order to be more generic and to allow support for other boards (PCI > > Vendor/Device IDs), introduce the lan966x_pci_info structure and attach > > it to PCI Vendor/Device IDs handled by the driver. > > > > This structure contains information related to the PCI board such as > > information related to the dtbo describing the board we have to load. > > ... > > > static struct pci_device_id lan966x_pci_ids[] = { > > - { PCI_DEVICE(PCI_VENDOR_ID_EFAR, 0x9660) }, > > + { PCI_VDEVICE(EFAR, 0x9660), (kernel_ulong_t)&evb_lan9662_nic_info }, > > PCI_DEVICE_DATA() ? PCI_DEVICE_DATA() need the device ID defined using a #define in the form PCI_DEVICE_ID_##vend##_##dev PCI_VDEVICE() allows the device ID value passed as an integer in the same way as for PCI_DEVICE(). Also, according to its kdoc, it allows the next field to follow as the device private data. IMHO, I think PCI_VDEVICE() use is correct here and I will keep it. Best regards, Hervé
On Mon, May 19, 2025 at 05:00:04PM +0200, Herve Codina wrote: > On Thu, 8 May 2025 22:21:41 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, May 07, 2025 at 09:13:05AM +0200, Herve Codina wrote: ... > > > static struct pci_device_id lan966x_pci_ids[] = { > > > - { PCI_DEVICE(PCI_VENDOR_ID_EFAR, 0x9660) }, > > > + { PCI_VDEVICE(EFAR, 0x9660), (kernel_ulong_t)&evb_lan9662_nic_info }, > > > > PCI_DEVICE_DATA() ? > > PCI_DEVICE_DATA() need the device ID defined using a #define in the form > PCI_DEVICE_ID_##vend##_##dev > > PCI_VDEVICE() allows the device ID value passed as an integer in the same > way as for PCI_DEVICE(). > > Also, according to its kdoc, it allows the next field to follow as the > device private data. > > IMHO, I think PCI_VDEVICE() use is correct here and I will keep it. It's correct, no doubts, but using PCI_DEVICE_DATA() makes sense when you need to supply driver_data. In particular it will take care of needed castings and also as you noticed asks users to apply the regular pattern for PCI ID definitions. Moreover, the 0x9660 is used in two drivers and it's a good candidate to be in pci_ids.h. (Note drivers/pci/quirks.c:6286)