| Message ID | 20250507071315.394857-24-herve.codina@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | lan966x pci device: Add support for SFPs | expand |
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, 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 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)
diff --git a/drivers/misc/lan966x_pci.c b/drivers/misc/lan966x_pci.c index b28066c96534..e136bb17c678 100644 --- a/drivers/misc/lan966x_pci.c +++ b/drivers/misc/lan966x_pci.c @@ -18,10 +18,6 @@ #include <linux/pci_ids.h> #include <linux/slab.h> -/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */ -extern char __dtbo_lan966x_evb_lan9662_nic_begin[]; -extern char __dtbo_lan966x_evb_lan9662_nic_end[]; - struct pci_dev_intr_ctrl { struct pci_dev *pci_dev; struct irq_domain *irq_domain; @@ -118,17 +114,23 @@ static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev) return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl); } +struct lan966x_pci_info { + void *dtbo_begin; + void *dtbo_end; +}; + struct lan966x_pci { struct device *dev; int ovcs_id; + const struct lan966x_pci_info *info; }; static int lan966x_pci_load_overlay(struct lan966x_pci *data) { - u32 dtbo_size = __dtbo_lan966x_evb_lan9662_nic_end - __dtbo_lan966x_evb_lan9662_nic_begin; - void *dtbo_start = __dtbo_lan966x_evb_lan9662_nic_begin; + const struct lan966x_pci_info *info = data->info; - return of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, dev_of_node(data->dev)); + return of_overlay_fdt_apply(info->dtbo_begin, info->dtbo_end - info->dtbo_begin, + &data->ovcs_id, dev_of_node(data->dev)); } static void lan966x_pci_unload_overlay(struct lan966x_pci *data) @@ -169,6 +171,9 @@ static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i pci_set_drvdata(pdev, data); data->dev = dev; + data->info = (const struct lan966x_pci_info *)id->driver_data; + if (!data->info) + return -EINVAL; ret = lan966x_pci_load_overlay(data); if (ret) @@ -196,8 +201,17 @@ static void lan966x_pci_remove(struct pci_dev *pdev) lan966x_pci_unload_overlay(data); } +/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */ +extern char __dtbo_lan966x_evb_lan9662_nic_begin[]; +extern char __dtbo_lan966x_evb_lan9662_nic_end[]; + +static struct lan966x_pci_info evb_lan9662_nic_info = { + .dtbo_begin = __dtbo_lan966x_evb_lan9662_nic_begin, + .dtbo_end = __dtbo_lan966x_evb_lan9662_nic_end, +}; + 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 }, { } }; MODULE_DEVICE_TABLE(pci, lan966x_pci_ids);
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> --- drivers/misc/lan966x_pci.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)