[v2,18/19] mfd: Add support for LAN966x PCI device

Commit Message

Herve Codina May 27, 2024, 4:14 p.m. UTC

Add a PCI driver that handles the LAN966x PCI device using a device-tree
overlay. This overlay is applied to the PCI device DT node and allows to
describe components that are present in the device.

The memory from the device-tree is remapped to the BAR memory thanks to
"ranges" properties computed at runtime by the PCI core during the PCI
enumeration.
The PCI device itself acts as an interrupt controller and is used as the
parent of the internal LAN966x interrupt controller to route the
interrupts to the assigned PCI INTx interrupt.

Signed-off-by: Herve Codina <[e-mail]>
---
 drivers/mfd/Kconfig          |  24 ++++
 drivers/mfd/Makefile         |   4 +
 drivers/mfd/lan966x_pci.c    | 229 +++++++++++++++++++++++++++++++++++
 drivers/mfd/lan966x_pci.dtso | 167 +++++++++++++++++++++++++
 drivers/pci/quirks.c         |   1 +
 5 files changed, 425 insertions(+)
 create mode 100644 drivers/mfd/lan966x_pci.c
 create mode 100644 drivers/mfd/lan966x_pci.dtso

Comments

Andy Shevchenko June 5, 2024, 8:24 p.m. UTC | #1

Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:
> Add a PCI driver that handles the LAN966x PCI device using a device-tree
> overlay. This overlay is applied to the PCI device DT node and allows to
> describe components that are present in the device.
> 
> The memory from the device-tree is remapped to the BAR memory thanks to
> "ranges" properties computed at runtime by the PCI core during the PCI
> enumeration.
> The PCI device itself acts as an interrupt controller and is used as the
> parent of the internal LAN966x interrupt controller to route the
> interrupts to the assigned PCI INTx interrupt.

...

> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>

> +#include <linux/kernel.h>

Why do you need this?

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>

General comment to the headers (in all your patches), try to follow IWYU
principle, i.e. include what you use explicitly and don't use "proxy" headers
such as kernel.h which basically shouldn't be used at all in the drivers.

...

> +static irqreturn_t pci_dev_irq_handler(int irq, void *data)
> +{
> +	struct pci_dev_intr_ctrl *intr_ctrl = data;
> +	int ret;
> +
> +	ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
> +	return ret ? IRQ_NONE : IRQ_HANDLED;

There is a macro for that IRQ_RETVAL() IIRC.

> +}

...

> +static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev)
> +{
> +	struct pci_dev_intr_ctrl *intr_ctrl;
> +
> +	intr_ctrl = pci_dev_create_intr_ctrl(pdev);

> +

Redundant blank line.

> +	if (IS_ERR(intr_ctrl))
> +		return PTR_ERR(intr_ctrl);
> +
> +	return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl);
> +}

...

> +static int lan966x_pci_load_overlay(struct lan966x_pci *data)
> +{
> +	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
> +	void *dtbo_start = __dtbo_lan966x_pci_begin;
> +	int ret;
> +
> +	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);

dev_of_node() ?

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

...

> +static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct lan966x_pci *data;
> +	int ret;

> +	if (!dev->of_node) {
> +		dev_err(dev, "Missing of_node for device\n");
> +		return -EINVAL;
> +	}

Why do you need this? The code you have in _create_intr_ctrl() will take care
already for this case.

> +	/* Need to be done before devm_pci_dev_create_intr_ctrl.
> +	 * It allocates an IRQ and so pdev->irq is updated

Missing period at the end.

> +	 */
> +	ret = pcim_enable_device(pdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pci_dev_create_intr_ctrl(pdev);
> +	if (ret)
> +		return ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, data);
> +	data->dev = dev;
> +	data->pci_dev = pdev;
> +
> +	ret = lan966x_pci_load_overlay(data);
> +	if (ret)
> +		return ret;

> +	pci_set_master(pdev);

You don't use MSI, what is this for?

> +	ret = of_platform_default_populate(dev->of_node, NULL, dev);

dev_of_node()

> +	if (ret)
> +		goto err_unload_overlay;
> +
> +	return 0;
> +
> +err_unload_overlay:
> +	lan966x_pci_unload_overlay(data);
> +	return ret;
> +}

...

> +static void lan966x_pci_remove(struct pci_dev *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct lan966x_pci *data = dev_get_drvdata(dev);

platform_get_drvdata()

> +	of_platform_depopulate(dev);
> +
> +	lan966x_pci_unload_overlay(data);

> +	pci_clear_master(pdev);

No need to call this excplicitly when pcim_enable_device() was called.

> +}

...

> +static struct pci_device_id lan966x_pci_ids[] = {
> +	{ PCI_DEVICE(0x1055, 0x9660) },

Don't you have VENDOR_ID defined somewhere?

> +	{ 0, }

Unneeded ' 0, ' part

> +};

Bjorn Helgaas June 5, 2024, 9:34 p.m. UTC | #2

On Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina wrote:
> Add a PCI driver that handles the LAN966x PCI device using a device-tree
> overlay. This overlay is applied to the PCI device DT node and allows to
> describe components that are present in the device.
> 
> The memory from the device-tree is remapped to the BAR memory thanks to
> "ranges" properties computed at runtime by the PCI core during the PCI
> enumeration.
> The PCI device itself acts as an interrupt controller and is used as the
> parent of the internal LAN966x interrupt controller to route the
> interrupts to the assigned PCI INTx interrupt.

Several patches in this series have things like this where it appears
the last two sentences are intended to be separate paragraphs, but the
only way to tell is that the first ends with a short line, which is
annoying to read.

Perhaps either add a blank line between or rewrap the whole thing into
a single paragraph that fills 75 columns or so?

Herve Codina June 20, 2024, 4:43 p.m. UTC | #3

My bad, I wrongly answered first in private.
-> Resend my answers with people in Cc

Andy, I will also resend your reply.

Sorry for this mistake.

Herve

On Thu, 20 Jun 2024 17:56:46 +0200
Herve Codina <[e-mail]> wrote:

> Hi Andy,
> 
> On Wed, 5 Jun 2024 23:24:43 +0300
> Andy Shevchenko <[e-mail]> wrote:
> 
> > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:
> > > Add a PCI driver that handles the LAN966x PCI device using a device-tree
> > > overlay. This overlay is applied to the PCI device DT node and allows to
> > > describe components that are present in the device.
> > > 
> > > The memory from the device-tree is remapped to the BAR memory thanks to
> > > "ranges" properties computed at runtime by the PCI core during the PCI
> > > enumeration.
> > > The PCI device itself acts as an interrupt controller and is used as the
> > > parent of the internal LAN966x interrupt controller to route the
> > > interrupts to the assigned PCI INTx interrupt.  
> > 
> > ...
> > 
> > > +#include <linux/irq.h>
> > > +#include <linux/irqdomain.h>  
> > 
> > > +#include <linux/kernel.h>  
> > 
> > Why do you need this?
> > 
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/slab.h>  
> > 
> > General comment to the headers (in all your patches), try to follow IWYU
> > principle, i.e. include what you use explicitly and don't use "proxy" headers
> > such as kernel.h which basically shouldn't be used at all in the drivers.
> 
> Sure, I will remove unneeded header inclusion.
> 
> > 
> > ...
> > 
> > > +static irqreturn_t pci_dev_irq_handler(int irq, void *data)
> > > +{
> > > +	struct pci_dev_intr_ctrl *intr_ctrl = data;
> > > +	int ret;
> > > +
> > > +	ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
> > > +	return ret ? IRQ_NONE : IRQ_HANDLED;  
> > 
> > There is a macro for that IRQ_RETVAL() IIRC.
> 
> Didn't known about that. Thanks for pointing out!
> I will use it :)
> 
> > 
> > > +}  
> > 
> > ...
> > 
> > > +static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev)
> > > +{
> > > +	struct pci_dev_intr_ctrl *intr_ctrl;
> > > +
> > > +	intr_ctrl = pci_dev_create_intr_ctrl(pdev);  
> > 
> > > +  
> > 
> > Redundant blank line.
> 
> Will be removed.
> 
> > 
> > > +	if (IS_ERR(intr_ctrl))
> > > +		return PTR_ERR(intr_ctrl);
> > > +
> > > +	return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl);
> > > +}  
> > 
> > ...
> > 
> > > +static int lan966x_pci_load_overlay(struct lan966x_pci *data)
> > > +{
> > > +	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
> > > +	void *dtbo_start = __dtbo_lan966x_pci_begin;
> > > +	int ret;
> > > +
> > > +	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);  
> > 
> > dev_of_node() ?
> 
> Yes indeed.
> 
> > 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}  
> > 
> > ...
> > 
> > > +static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct lan966x_pci *data;
> > > +	int ret;  
> > 
> > > +	if (!dev->of_node) {
> > > +		dev_err(dev, "Missing of_node for device\n");
> > > +		return -EINVAL;
> > > +	}  
> > 
> > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > already for this case.
> 
> The code in _create_intr_ctrl checks for fwnode and not an of_node.
> 
> The check here is to ensure that an of_node is available as it will be use
> for DT overlay loading.
> 
> I will keep the check here and use dev_of_node() instead of dev->of_node.
> 
> > 
> > > +	/* Need to be done before devm_pci_dev_create_intr_ctrl.
> > > +	 * It allocates an IRQ and so pdev->irq is updated  
> > 
> > Missing period at the end.
> 
> Will be added.
> 
> > 
> > > +	 */
> > > +	ret = pcim_enable_device(pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = devm_pci_dev_create_intr_ctrl(pdev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > +	if (!data)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_set_drvdata(dev, data);
> > > +	data->dev = dev;
> > > +	data->pci_dev = pdev;
> > > +
> > > +	ret = lan966x_pci_load_overlay(data);
> > > +	if (ret)
> > > +		return ret;  
> > 
> > > +	pci_set_master(pdev);  
> > 
> > You don't use MSI, what is this for?
> 
> DMA related.
> Allows the PCI device to be master on the bus and so initiate transactions.
> 
> Did I misunderstood ?
> 
> > 
> > > +	ret = of_platform_default_populate(dev->of_node, NULL, dev);  
> > 
> > dev_of_node()
> 
> Yes, sure.
> 
> > 
> > > +	if (ret)
> > > +		goto err_unload_overlay;
> > > +
> > > +	return 0;
> > > +
> > > +err_unload_overlay:
> > > +	lan966x_pci_unload_overlay(data);
> > > +	return ret;
> > > +}  
> > 
> > ...
> > 
> > > +static void lan966x_pci_remove(struct pci_dev *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct lan966x_pci *data = dev_get_drvdata(dev);  
> > 
> > platform_get_drvdata()
> 
> platform_get_drvdata() is related to platform_device.
> There is no platform_device here but a pci_dev.
> 
> I will use pci_get_drvdata() here and update probe() to
> use pci_set_drvdata() for consistency.
> 
> > 
> > > +	of_platform_depopulate(dev);
> > > +
> > > +	lan966x_pci_unload_overlay(data);  
> > 
> > > +	pci_clear_master(pdev);  
> > 
> > No need to call this excplicitly when pcim_enable_device() was called.
> 
> You're right. I will remove this call.
> 
> > 
> > > +}  
> > 
> > ...
> > 
> > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > +	{ PCI_DEVICE(0x1055, 0x9660) },  
> > 
> > Don't you have VENDOR_ID defined somewhere?
> 
> No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> 
> I will patch pci-ids.h to create:
>   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
>   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> As part of this patch, I will update lan743x_main.h to remove its own #define
> 
> And use PCI_VENDOR_ID_MCHP in this series.
> 
> > 
> > > +	{ 0, }  
> > 
> > Unneeded ' 0, ' part
> 
> Will be removed.
> 
> > 
> > > +};  
> > 
> 
> Thanks a lot for your review.
> 
> Best regards,
> Hervé

Herve Codina June 20, 2024, 4:43 p.m. UTC | #4

My bad, I wrongly answered first in private.
I already eesend my answers with people in Cc

Now, this is the Andy's your reply.

Sorry for this mistake.

Herve

On Thu, 20 Jun 2024 18:07:16 +0200
Andy Shevchenko <[e-mail]> wrote:

> On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <[e-mail]> wrote:
> > On Wed, 5 Jun 2024 23:24:43 +0300
> > Andy Shevchenko <[e-mail]> wrote:  
> > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:  
> 
> ...
> 
> > > > +   if (!dev->of_node) {
> > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > +           return -EINVAL;
> > > > +   }  
> > >
> > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > already for this case.  
> >
> > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> >
> > The check here is to ensure that an of_node is available as it will be use
> > for DT overlay loading.  
> 
> So, what exactly do you want to check? fwnode check covers this.
> 
> > I will keep the check here and use dev_of_node() instead of dev->of_node.  
> 
> It needs to be well justified as from a coding point of view this is a
> duplication.
> 
> ...
> 
> > > > +   pci_set_master(pdev);  
> > >
> > > You don't use MSI, what is this for?  
> >
> > DMA related.
> > Allows the PCI device to be master on the bus and so initiate transactions.
> >
> > Did I misunderstood ?  
> 
> So, you mean that the PCI device may initiate DMA transactions and
> they are not related to MSI, correct?
> 
> ...
> 
> > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > +   { PCI_DEVICE(0x1055, 0x9660) },  
> > >
> > > Don't you have VENDOR_ID defined somewhere?  
> >
> > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> >
> > I will patch pci-ids.h to create:
> >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > As part of this patch, I will update lan743x_main.h to remove its own #define
> >
> > And use PCI_VENDOR_ID_MCHP in this series.  
> 
> Okay, but I don't think (but I haven't checked) we have something like
> this ever done there. In any case it's up to Bjorn how to implement
> this.
> 
> > > > +   { 0, }  
> > >
> > > Unneeded ' 0, ' part  
> >
> > Will be removed.
> >  
> > > > +};  
>

Herve Codina June 20, 2024, 5:19 p.m. UTC | #5

Hi Andy,

On Thu, 20 Jun 2024 18:43:09 +0200
Herve Codina <[e-mail]> wrote:

> My bad, I wrongly answered first in private.
> I already eesend my answers with people in Cc
> 
> Now, this is the Andy's your reply.
> 
> Sorry for this mistake.
> 
> Herve
> 
> On Thu, 20 Jun 2024 18:07:16 +0200
> Andy Shevchenko <[e-mail]> wrote:
> 
> > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <[e-mail]> wrote:  
> > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > Andy Shevchenko <[e-mail]> wrote:    
> > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:    
> > 
> > ...
> >   
> > > > > +   if (!dev->of_node) {
> > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > +           return -EINVAL;
> > > > > +   }    
> > > >
> > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > already for this case.    
> > >
> > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > >
> > > The check here is to ensure that an of_node is available as it will be use
> > > for DT overlay loading.    
> > 
> > So, what exactly do you want to check? fwnode check covers this.
> >   
> > > I will keep the check here and use dev_of_node() instead of dev->of_node.    
> > 
> > It needs to be well justified as from a coding point of view this is a
> > duplication.

On DT based system, if a fwnode is set it is an of_node.
On ACPI, if a fwnode is set is is an acpi_node.

The core PCI, when it successfully creates the DT node for a device
(CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
So we can have a device with:
 - fwnode from ACPI
 - of_node from core PCI creation

This driver needs the of_node to load the overlay.
Even if the core PCI cannot create a DT node for the PCI device right
now, I don't expect this LAN855x PCI driver updated when the core PCI
is able to create this PCI device DT node.

> > 
> > ...
> >   
> > > > > +   pci_set_master(pdev);    
> > > >
> > > > You don't use MSI, what is this for?    
> > >
> > > DMA related.
> > > Allows the PCI device to be master on the bus and so initiate transactions.
> > >
> > > Did I misunderstood ?    
> > 
> > So, you mean that the PCI device may initiate DMA transactions and
> > they are not related to MSI, correct?

That's my understanding.
Right now, the internal LAN966x DMA controller is not used but it will be
used in a near future.

> > 
> > ...
> >   
> > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > +   { PCI_DEVICE(0x1055, 0x9660) },    
> > > >
> > > > Don't you have VENDOR_ID defined somewhere?    
> > >
> > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > >
> > > I will patch pci-ids.h to create:
> > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > >
> > > And use PCI_VENDOR_ID_MCHP in this series.    
> > 
> > Okay, but I don't think (but I haven't checked) we have something like
> > this ever done there. In any case it's up to Bjorn how to implement
> > this.

Right, I wait for Bjorn reply before changing anything.

Best regards,
Hervé

Andy Shevchenko June 21, 2024, 3:45 p.m. UTC | #6

On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <[e-mail]> wrote:
> On Thu, 20 Jun 2024 18:43:09 +0200
> Herve Codina <[e-mail]> wrote:
> > On Thu, 20 Jun 2024 18:07:16 +0200
> > Andy Shevchenko <[e-mail]> wrote:
> > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <[e-mail]> wrote:
> > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > Andy Shevchenko <[e-mail]> wrote:
> > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:

...

> > > > > > +   if (!dev->of_node) {
> > > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > > +           return -EINVAL;
> > > > > > +   }
> > > > >
> > > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > > already for this case.
> > > >
> > > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > > >
> > > > The check here is to ensure that an of_node is available as it will be use
> > > > for DT overlay loading.
> > >
> > > So, what exactly do you want to check? fwnode check covers this.
> > >
> > > > I will keep the check here and use dev_of_node() instead of dev->of_node.
> > >
> > > It needs to be well justified as from a coding point of view this is a
> > > duplication.
>
> On DT based system, if a fwnode is set it is an of_node.
> On ACPI, if a fwnode is set is is an acpi_node.
>
> The core PCI, when it successfully creates the DT node for a device
> (CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
> So we can have a device with:
>  - fwnode from ACPI
>  - of_node from core PCI creation

Does PCI device creation not set fwnode?

> This driver needs the of_node to load the overlay.
> Even if the core PCI cannot create a DT node for the PCI device right
> now, I don't expect this LAN855x PCI driver updated when the core PCI
> is able to create this PCI device DT node.

If it's really needed, I think the correct call here is is_of_node()
to show exactly why it's not a duplication. It also needs a comment on
top of this call.

...

> > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > >
> > > > > Don't you have VENDOR_ID defined somewhere?
> > > >
> > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > >
> > > > I will patch pci-ids.h to create:
> > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > > >
> > > > And use PCI_VENDOR_ID_MCHP in this series.
> > >
> > > Okay, but I don't think (but I haven't checked) we have something like
> > > this ever done there. In any case it's up to Bjorn how to implement
> > > this.
>
> Right, I wait for Bjorn reply before changing anything.

But we already have the vendor ID with the same value. Even if the
company was acquired, the old ID still may be used. In that case an
update on PCI IDs can go in a separate change justifying it. In any
case, I would really want to hear from Bjorn on this and if nothing
happens, to use the existing vendor ID for now to speed up the series
to be reviewed/processed.

Bjorn Helgaas June 21, 2024, 6:49 p.m. UTC | #7

On Fri, Jun 21, 2024 at 05:45:05PM +0200, Andy Shevchenko wrote:
> On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <[e-mail]> wrote:
> > On Thu, 20 Jun 2024 18:43:09 +0200
> > Herve Codina <[e-mail]> wrote:
> > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > Andy Shevchenko <[e-mail]> wrote:
> > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <[e-mail]> wrote:
> > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > Andy Shevchenko <[e-mail]> wrote:
> > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:

> > > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > > >
> > > > > > Don't you have VENDOR_ID defined somewhere?
> > > > >
> > > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC in 2012
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > > >
> > > > > I will patch pci-ids.h to create:
> > > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > > As part of this patch, I will update lan743x_main.h to remove its own #define
> > > > >
> > > > > And use PCI_VENDOR_ID_MCHP in this series.
> > > >
> > > > Okay, but I don't think (but I haven't checked) we have something like
> > > > this ever done there. In any case it's up to Bjorn how to implement
> > > > this.
> >
> > Right, I wait for Bjorn reply before changing anything.
> 
> But we already have the vendor ID with the same value. Even if the
> company was acquired, the old ID still may be used. In that case an
> update on PCI IDs can go in a separate change justifying it. In any
> case, I would really want to hear from Bjorn on this and if nothing
> happens, to use the existing vendor ID for now to speed up the series
> to be reviewed/processed.

We have "#define PCI_VENDOR_ID_EFAR 0x1055" in pci_ids.h, but
https://pcisig.com/membership/member-companies?combine=1055 shows no
results, so it *looks* like EFAR/SMSC/MCHP are currently squatting on
that ID without it being officially assigned.

I think MCHP needs to register 0x1055 with the PCI-SIG
([e-mail]) if it wants to continue using it.
The vendor is responsible for managing the Device ID space, so this
registration includes the burden of tracking all the Device IDs that
were assigned by EFAR and SMSC and now MCHP so there are no conflicts.

I don't want to change the existing PCI_VENDOR_ID_EFAR, and I also
don't want to add a PCI_VENDOR_ID_MCHP for 0x1055 until that ID has
been registered with the PCI-SIG.

So I propose that you use PCI_VENDOR_ID_EFAR for now, and if/when MCHP
registers 0x1055 with PCI-SIG so it is unambiguously owned by MCHP, we
can add "#define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_EFAR" or similar.
As Andy points out, this would be a separate logical change in its own
patch.

Bjorn

Herve Codina June 24, 2024, 8:20 a.m. UTC | #8

On Fri, 21 Jun 2024 17:45:05 +0200
Andy Shevchenko <[e-mail]> wrote:

> On Thu, Jun 20, 2024 at 7:19 PM Herve Codina <[e-mail]> wrote:
> > On Thu, 20 Jun 2024 18:43:09 +0200
> > Herve Codina <[e-mail]> wrote:  
> > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > Andy Shevchenko <[e-mail]> wrote:  
> > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina <[e-mail]> wrote:  
> > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > Andy Shevchenko <[e-mail]> wrote:  
> > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina kirjoitti:  
> 
> ...
> 
> > > > > > > +   if (!dev->of_node) {
> > > > > > > +           dev_err(dev, "Missing of_node for device\n");
> > > > > > > +           return -EINVAL;
> > > > > > > +   }  
> > > > > >
> > > > > > Why do you need this? The code you have in _create_intr_ctrl() will take care
> > > > > > already for this case.  
> > > > >
> > > > > The code in _create_intr_ctrl checks for fwnode and not an of_node.
> > > > >
> > > > > The check here is to ensure that an of_node is available as it will be use
> > > > > for DT overlay loading.  
> > > >
> > > > So, what exactly do you want to check? fwnode check covers this.
> > > >  
> > > > > I will keep the check here and use dev_of_node() instead of dev->of_node.  
> > > >
> > > > It needs to be well justified as from a coding point of view this is a
> > > > duplication.  
> >
> > On DT based system, if a fwnode is set it is an of_node.
> > On ACPI, if a fwnode is set is is an acpi_node.
> >
> > The core PCI, when it successfully creates the DT node for a device
> > (CONFIG_PCI_DYNAMIC_OF_NODES) set the of_node of this device.
> > So we can have a device with:
> >  - fwnode from ACPI
> >  - of_node from core PCI creation  
> 
> Does PCI device creation not set fwnode?

No and IMHO it is correct.
This device has the fwnode that point to an ACPI node: The description used
for device creation.

The of_node set is created based on PCI known information.
This of_node, at PCI level is not used to create the PCI device but is created
based on an already existing PCI device.

> 
> > This driver needs the of_node to load the overlay.
> > Even if the core PCI cannot create a DT node for the PCI device right
> > now, I don't expect this LAN855x PCI driver updated when the core PCI
> > is able to create this PCI device DT node.  
> 
> If it's really needed, I think the correct call here is is_of_node()
> to show exactly why it's not a duplication. It also needs a comment on
> top of this call.

is_of_node() will not returns the expected result.
It will return false as the fwnode->ops of the device is not related to 
of_node ops but ACPI node ops :(

What do you thing it I keep the of_node test using dev_of_node() and add the
following comment:
	--- 8< ---
	/*
	 * On ACPI system, fwnode can point to the ACPI node.
	 * This driver needs an of_node to be used as the device-tree overlay
	 * target. This of_node should be set by the PCI core if it succeeds in
	 * creating it (CONFIG_PCI_DYNAMIC_OF_NODES feature).
	 * Check here for the validity of the of_node.
	 */
	if (!dev_of_node(dev)) {
		dev_err(dev, "Missing of_node for device\n");
		return -EINVAL;
	}
	--- 8< ---

Let me know if this can be ok.

Hervé

Steen Hegelund June 24, 2024, 11:46 a.m. UTC | #9

Hi Bjorn,

I am not sure what went wrong here.

I have seen that lspci lists 'Microchip / SMSC' for the 0x1055 Vendor
ID value and as mentioned previously there has been a number of
aquicisions over the years, so that the ID has been absorbed but not
necessarily re-registered.

Anyway I have started an investigation, so we can determine what
up/down in this.

I agree that for now this will have to be PCI_VENDOR_ID_EFAR, and I
will return with an update as soon as I know more.

Best Regards
Steen

On Fri, 2024-06-21 at 13:49 -0500, Bjorn Helgaas wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jun 21, 2024 at 05:45:05PM +0200, Andy Shevchenko wrote:
> > On Thu, Jun 20, 2024 at 7:19 PM Herve Codina
> > <[e-mail]> wrote:
> > > On Thu, 20 Jun 2024 18:43:09 +0200
> > > Herve Codina <[e-mail]> wrote:
> > > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > > Andy Shevchenko <[e-mail]> wrote:
> > > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina
> > > > > <[e-mail]> wrote:
> > > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > > Andy Shevchenko <[e-mail]> wrote:
> > > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina
> > > > > > > kirjoitti:
> 
> > > > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > > > > 
> > > > > > > Don't you have VENDOR_ID defined somewhere?
> > > > > > 
> > > > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC
> > > > > > in 2012
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > > > > 
> > > > > > I will patch pci-ids.h to create:
> > > > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > > > As part of this patch, I will update lan743x_main.h to
> > > > > > remove its own #define
> > > > > > 
> > > > > > And use PCI_VENDOR_ID_MCHP in this series.
> > > > > 
> > > > > Okay, but I don't think (but I haven't checked) we have
> > > > > something like
> > > > > this ever done there. In any case it's up to Bjorn how to
> > > > > implement
> > > > > this.
> > > 
> > > Right, I wait for Bjorn reply before changing anything.
> > 
> > But we already have the vendor ID with the same value. Even if the
> > company was acquired, the old ID still may be used. In that case an
> > update on PCI IDs can go in a separate change justifying it. In any
> > case, I would really want to hear from Bjorn on this and if nothing
> > happens, to use the existing vendor ID for now to speed up the
> > series
> > to be reviewed/processed.
> 
> We have "#define PCI_VENDOR_ID_EFAR 0x1055" in pci_ids.h, but
> https://pcisig.com/membership/member-companies?combine=1055 shows no
> results, so it *looks* like EFAR/SMSC/MCHP are currently squatting on
> that ID without it being officially assigned.
> 
> I think MCHP needs to register 0x1055 with the PCI-SIG
> ([e-mail]) if it wants to continue using it.
> The vendor is responsible for managing the Device ID space, so this
> registration includes the burden of tracking all the Device IDs that
> were assigned by EFAR and SMSC and now MCHP so there are no
> conflicts.
> 
> I don't want to change the existing PCI_VENDOR_ID_EFAR, and I also
> don't want to add a PCI_VENDOR_ID_MCHP for 0x1055 until that ID has
> been registered with the PCI-SIG.
> 
> So I propose that you use PCI_VENDOR_ID_EFAR for now, and if/when
> MCHP
> registers 0x1055 with PCI-SIG so it is unambiguously owned by MCHP,
> we
> can add "#define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_EFAR" or similar.
> As Andy points out, this would be a separate logical change in its
> own
> patch.
> 
> Bjorn

Herve Codina June 26, 2024, 6:52 a.m. UTC | #10

Hi Steen, Bjorn, Andy,

On Mon, 24 Jun 2024 13:46:32 +0200
Steen Hegelund <[e-mail]> wrote:

> Hi Bjorn,
> 
> I am not sure what went wrong here.
> 
> I have seen that lspci lists 'Microchip / SMSC' for the 0x1055 Vendor
> ID value and as mentioned previously there has been a number of
> aquicisions over the years, so that the ID has been absorbed but not
> necessarily re-registered.
> 
> Anyway I have started an investigation, so we can determine what
> up/down in this.
> 
> I agree that for now this will have to be PCI_VENDOR_ID_EFAR, and I
> will return with an update as soon as I know more.
> 

Right, PCI_VENDOR_ID_EFAR will be directly used in the next iteration.

Best regards,
Hervé

Steen Hegelund June 26, 2024, 7:17 a.m. UTC | #11

Hi Bjorn, and Herve,

Bill Mahany from Microchip has now been in contact with PCI-SIG, and
has been able to get confirmation that Vendor ID 0x1055 is still
belonging to Microchip even though this is not visible on their
webpage.

I have attached a snippet of the conversation.

I hope this settles the matter of the Vendor ID 0x1055.

Best Regards
Steen

=== Copied from the conversation ===

   Hi Bill,
    
   Thank you for your email. We can confirm VID 4181 (1055 Hex) is
   Microchip’s. This is not listed on the website because we are only
   able to list one VID per company.
    
   Please feel free to contact us if you have any questions.
   
   Best Regards,
   
   PCI-SIG Administration
   
   Main: 503-619-0569 | Fax: [phone]
   3855 SW 153rd Drive, Beaverton, OR 97003 USA
   Email: [e-mail]
   
   
    
   www.pcisig.com | Connect with us on LinkedIn and Twitter @pci_sig
    
   From: [e-mail] (administration)
   <[e-mail]>
   Sent: Monday, June 24, 2024 11:07 AM
   To: [e-mail]
   Subject: RE: vendor id missing from member companies list
    
   Hello once again.
    
   Please refer to the below. Could you please reverify that Microchip
   Technology is still in control of 4181 (0x1055).
    
   Apparently, the lack of a search result for 4181 (0x1055) at
   https://pcisig.com/membership/member-companiesis
   Is still causing issues in the Linux community -
   https://lore.kernel.org/all/20240621184923.GA1398370@bhelgaas/
    
   Thanks
    
   Bill Mahany
    
On Mon, 2024-06-24 at 13:46 +0200, Steen Hegelund wrote:
> Hi Bjorn,
> 
> I am not sure what went wrong here.
> 
> I have seen that lspci lists 'Microchip / SMSC' for the 0x1055 Vendor
> ID value and as mentioned previously there has been a number of
> aquicisions over the years, so that the ID has been absorbed but not
> necessarily re-registered.
> 
> Anyway I have started an investigation, so we can determine what
> up/down in this.
> 
> I agree that for now this will have to be PCI_VENDOR_ID_EFAR, and I
> will return with an update as soon as I know more.
> 
> Best Regards
> Steen
> 
> On Fri, 2024-06-21 at 13:49 -0500, Bjorn Helgaas wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Fri, Jun 21, 2024 at 05:45:05PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jun 20, 2024 at 7:19 PM Herve Codina
> > > <[e-mail]> wrote:
> > > > On Thu, 20 Jun 2024 18:43:09 +0200
> > > > Herve Codina <[e-mail]> wrote:
> > > > > On Thu, 20 Jun 2024 18:07:16 +0200
> > > > > Andy Shevchenko <[e-mail]> wrote:
> > > > > > On Thu, Jun 20, 2024 at 5:56 PM Herve Codina
> > > > > > <[e-mail]> wrote:
> > > > > > > On Wed, 5 Jun 2024 23:24:43 +0300
> > > > > > > Andy Shevchenko <[e-mail]> wrote:
> > > > > > > > Mon, May 27, 2024 at 06:14:45PM +0200, Herve Codina
> > > > > > > > kirjoitti:
> > 
> > > > > > > > > +static struct pci_device_id lan966x_pci_ids[] = {
> > > > > > > > > +   { PCI_DEVICE(0x1055, 0x9660) },
> > > > > > > > 
> > > > > > > > Don't you have VENDOR_ID defined somewhere?
> > > > > > > 
> > > > > > > No and 0x1055 is taken by PCI_VENDOR_ID_EFAR in pci-ids.h
> > > > > > > but SMSC acquired EFAR late 1990's and MCHP acquired SMSC
> > > > > > > in 2012
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/microchip/lan743x_main.h#L851
> > > > > > > 
> > > > > > > I will patch pci-ids.h to create:
> > > > > > >   #define PCI_VENDOR_ID_SMSC PCI_VENDOR_ID_EFAR
> > > > > > >   #define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_SMSC
> > > > > > > As part of this patch, I will update lan743x_main.h to
> > > > > > > remove its own #define
> > > > > > > 
> > > > > > > And use PCI_VENDOR_ID_MCHP in this series.
> > > > > > 
> > > > > > Okay, but I don't think (but I haven't checked) we have
> > > > > > something like
> > > > > > this ever done there. In any case it's up to Bjorn how to
> > > > > > implement
> > > > > > this.
> > > > 
> > > > Right, I wait for Bjorn reply before changing anything.
> > > 
> > > But we already have the vendor ID with the same value. Even if
> > > the
> > > company was acquired, the old ID still may be used. In that case
> > > an
> > > update on PCI IDs can go in a separate change justifying it. In
> > > any
> > > case, I would really want to hear from Bjorn on this and if
> > > nothing
> > > happens, to use the existing vendor ID for now to speed up the
> > > series
> > > to be reviewed/processed.
> > 
> > We have "#define PCI_VENDOR_ID_EFAR 0x1055" in pci_ids.h, but
> > https://pcisig.com/membership/member-companies?combine=1055 shows
> > no
> > results, so it *looks* like EFAR/SMSC/MCHP are currently squatting
> > on
> > that ID without it being officially assigned.
> > 
> > I think MCHP needs to register 0x1055 with the PCI-SIG
> > ([e-mail]) if it wants to continue using it.
> > The vendor is responsible for managing the Device ID space, so this
> > registration includes the burden of tracking all the Device IDs
> > that
> > were assigned by EFAR and SMSC and now MCHP so there are no
> > conflicts.
> > 
> > I don't want to change the existing PCI_VENDOR_ID_EFAR, and I also
> > don't want to add a PCI_VENDOR_ID_MCHP for 0x1055 until that ID has
> > been registered with the PCI-SIG.
> > 
> > So I propose that you use PCI_VENDOR_ID_EFAR for now, and if/when
> > MCHP
> > registers 0x1055 with PCI-SIG so it is unambiguously owned by MCHP,
> > we
> > can add "#define PCI_VENDOR_ID_MCHP PCI_VENDOR_ID_EFAR" or similar.
> > As Andy points out, this would be a separate logical change in its
> > own
> > patch.
> > 
> > Bjorn
>

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 266b4f54af60..15db144bc09b 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -144,6 +144,30 @@  config MFD_ATMEL_FLEXCOM
 	  by the probe function of this MFD driver according to a device tree
 	  property.
 
+config MFD_LAN966X_PCI
+	tristate "Microchip LAN966x PCIe Support"
+	depends on PCI
+	select OF
+	select OF_OVERLAY
+	select IRQ_DOMAIN
+	help
+	  This enables the support for the LAN966x PCIe device.
+	  This is used to drive the LAN966x PCIe device from the host system
+	  to which it is connected.
+
+	  This driver uses an overlay to load other drivers to support for
+	  LAN966x internal components.
+	  Even if this driver does not depend on these other drivers, in order
+	  to have a fully functional board, the following drivers are needed:
+	    - fixed-clock (COMMON_CLK)
+	    - lan966x-oic (LAN966X_OIC)
+	    - lan966x-cpu-syscon (MFD_SYSCON)
+	    - lan966x-switch-reset (RESET_MCHP_SPARX5)
+	    - lan966x-pinctrl (PINCTRL_OCELOT)
+	    - lan966x-serdes (PHY_LAN966X_SERDES)
+	    - lan966x-miim (MDIO_MSCC_MIIM)
+	    - lan966x-switch (LAN966X_SWITCH)
+
 config MFD_ATMEL_HLCDC
 	tristate "Atmel HLCDC (High-end LCD Controller)"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index c66f07edcd0e..165a9674ff48 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -284,3 +284,7 @@  rsmu-i2c-objs			:= rsmu_core.o rsmu_i2c.o
 rsmu-spi-objs			:= rsmu_core.o rsmu_spi.o
 obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu-i2c.o
 obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu-spi.o
+
+lan966x-pci-objs		:= lan966x_pci.o
+lan966x-pci-objs		+= lan966x_pci.dtbo.o
+obj-$(CONFIG_MFD_LAN966X_PCI)	+= lan966x-pci.o
diff --git a/drivers/mfd/lan966x_pci.c b/drivers/mfd/lan966x_pci.c
new file mode 100644
index 000000000000..a0a59860928f
--- /dev/null
+++ b/drivers/mfd/lan966x_pci.c
@@ -0,0 +1,229 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip LAN966x PCI driver
+ *
+ * Copyright (c) 2024 Microchip Technology Inc. and its subsidiaries.
+ *
+ * Authors:
+ *	Clément Léger <[e-mail]>
+ *	Hervé Codina <[e-mail]>
+ */
+
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */
+extern char __dtbo_lan966x_pci_begin[];
+extern char __dtbo_lan966x_pci_end[];
+
+struct pci_dev_intr_ctrl {
+	struct pci_dev *pci_dev;
+	struct irq_domain *irq_domain;
+	int irq;
+};
+
+static int pci_dev_irq_domain_map(struct irq_domain *d, unsigned int virq, irq_hw_number_t hw)
+{
+	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
+	return 0;
+}
+
+static const struct irq_domain_ops pci_dev_irq_domain_ops = {
+	.map = pci_dev_irq_domain_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static irqreturn_t pci_dev_irq_handler(int irq, void *data)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl = data;
+	int ret;
+
+	ret = generic_handle_domain_irq(intr_ctrl->irq_domain, 0);
+	return ret ? IRQ_NONE : IRQ_HANDLED;
+}
+
+static struct pci_dev_intr_ctrl *pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl;
+	struct fwnode_handle *fwnode;
+	int ret;
+
+	if (!pdev->irq)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	fwnode = dev_fwnode(&pdev->dev);
+	if (!fwnode)
+		return ERR_PTR(-ENODEV);
+
+	intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);
+	if (!intr_ctrl)
+		return ERR_PTR(-ENOMEM);
+
+	intr_ctrl->pci_dev = pdev;
+
+	intr_ctrl->irq_domain = irq_domain_create_linear(fwnode, 1, &pci_dev_irq_domain_ops,
+							 intr_ctrl);
+	if (!intr_ctrl->irq_domain) {
+		pci_err(pdev, "Failed to create irqdomain\n");
+		ret = -ENOMEM;
+		goto err_free_intr_ctrl;
+	}
+
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
+	if (ret < 0) {
+		pci_err(pdev, "Unable alloc irq vector (%d)\n", ret);
+		goto err_remove_domain;
+	}
+	intr_ctrl->irq = pci_irq_vector(pdev, 0);
+	ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
+			  dev_name(&pdev->dev), intr_ctrl);
+	if (ret) {
+		pci_err(pdev, "Unable to request irq %d (%d)\n", intr_ctrl->irq, ret);
+		goto err_free_irq_vector;
+	}
+
+	return intr_ctrl;
+
+err_free_irq_vector:
+	pci_free_irq_vectors(pdev);
+err_remove_domain:
+	irq_domain_remove(intr_ctrl->irq_domain);
+err_free_intr_ctrl:
+	kfree(intr_ctrl);
+	return ERR_PTR(ret);
+}
+
+static void pci_dev_remove_intr_ctrl(struct pci_dev_intr_ctrl *intr_ctrl)
+{
+	free_irq(intr_ctrl->irq, intr_ctrl);
+	pci_free_irq_vectors(intr_ctrl->pci_dev);
+	irq_dispose_mapping(irq_find_mapping(intr_ctrl->irq_domain, 0));
+	irq_domain_remove(intr_ctrl->irq_domain);
+	kfree(intr_ctrl);
+}
+
+static void devm_pci_dev_remove_intr_ctrl(void *data)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl = data;
+
+	pci_dev_remove_intr_ctrl(intr_ctrl);
+}
+
+static int devm_pci_dev_create_intr_ctrl(struct pci_dev *pdev)
+{
+	struct pci_dev_intr_ctrl *intr_ctrl;
+
+	intr_ctrl = pci_dev_create_intr_ctrl(pdev);
+
+	if (IS_ERR(intr_ctrl))
+		return PTR_ERR(intr_ctrl);
+
+	return devm_add_action_or_reset(&pdev->dev, devm_pci_dev_remove_intr_ctrl, intr_ctrl);
+}
+
+struct lan966x_pci {
+	struct device *dev;
+	struct pci_dev *pci_dev;
+	int ovcs_id;
+};
+
+static int lan966x_pci_load_overlay(struct lan966x_pci *data)
+{
+	u32 dtbo_size = __dtbo_lan966x_pci_end - __dtbo_lan966x_pci_begin;
+	void *dtbo_start = __dtbo_lan966x_pci_begin;
+	int ret;
+
+	ret = of_overlay_fdt_apply(dtbo_start, dtbo_size, &data->ovcs_id, data->dev->of_node);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void lan966x_pci_unload_overlay(struct lan966x_pci *data)
+{
+	of_overlay_remove(&data->ovcs_id);
+}
+
+static int lan966x_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data;
+	int ret;
+
+	if (!dev->of_node) {
+		dev_err(dev, "Missing of_node for device\n");
+		return -EINVAL;
+	}
+
+	/* Need to be done before devm_pci_dev_create_intr_ctrl.
+	 * It allocates an IRQ and so pdev->irq is updated
+	 */
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return ret;
+
+	ret = devm_pci_dev_create_intr_ctrl(pdev);
+	if (ret)
+		return ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, data);
+	data->dev = dev;
+	data->pci_dev = pdev;
+
+	ret = lan966x_pci_load_overlay(data);
+	if (ret)
+		return ret;
+
+	pci_set_master(pdev);
+
+	ret = of_platform_default_populate(dev->of_node, NULL, dev);
+	if (ret)
+		goto err_unload_overlay;
+
+	return 0;
+
+err_unload_overlay:
+	lan966x_pci_unload_overlay(data);
+	return ret;
+}
+
+static void lan966x_pci_remove(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct lan966x_pci *data = dev_get_drvdata(dev);
+
+	of_platform_depopulate(dev);
+
+	lan966x_pci_unload_overlay(data);
+
+	pci_clear_master(pdev);
+}
+
+static struct pci_device_id lan966x_pci_ids[] = {
+	{ PCI_DEVICE(0x1055, 0x9660) },
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, lan966x_pci_ids);
+
+static struct pci_driver lan966x_pci_driver = {
+	.name = "mchp_lan966x_pci",
+	.id_table = lan966x_pci_ids,
+	.probe = lan966x_pci_probe,
+	.remove = lan966x_pci_remove,
+};
+module_pci_driver(lan966x_pci_driver);
+
+MODULE_AUTHOR("Herve Codina <[e-mail]>");
+MODULE_DESCRIPTION("Microchip LAN966x PCI driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/lan966x_pci.dtso b/drivers/mfd/lan966x_pci.dtso
new file mode 100644
index 000000000000..041f4319e4cd
--- /dev/null
+++ b/drivers/mfd/lan966x_pci.dtso
@@ -0,0 +1,167 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Microchip UNG
+ */
+
+#include <dt-bindings/clock/microchip,lan966x.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/mfd/atmel-flexcom.h>
+#include <dt-bindings/phy/phy-lan966x-serdes.h>
+#include <dt-bindings/gpio/gpio.h>
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	fragment@0 {
+		target-path="";
+		__overlay__ {
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			pci-ep-bus@0 {
+				compatible = "simple-bus";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				/*
+				 * map @0xe2000000 (32MB) to BAR0 (CPU)
+				 * map @0xe0000000 (16MB) to BAR1 (AMBA)
+				 */
+				ranges = <0xe2000000 0x00 0x00 0x00 0x2000000
+				          0xe0000000 0x01 0x00 0x00 0x1000000>;
+
+				oic: oic@e00c0120 {
+					compatible = "microchip,lan966x-oic";
+					#interrupt-cells = <2>;
+					interrupt-controller;
+					interrupts = <0>; /* PCI INTx assigned interrupt */
+					reg = <0xe00c0120 0x190>;
+				};
+
+				cpu_clk: cpu_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <600000000>;  // CPU clock = 600MHz
+				};
+
+				ddr_clk: ddr_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <30000000>;  // Fabric clock = 30MHz
+				};
+
+				sys_clk: sys_clk {
+					compatible = "fixed-clock";
+					#clock-cells = <0>;
+					clock-frequency = <15625000>;  // System clock = 15.625MHz
+				};
+
+				cpu_ctrl: syscon@e00c0000 {
+					compatible = "microchip,lan966x-cpu-syscon", "syscon";
+					reg = <0xe00c0000 0xa8>;
+				};
+
+				reset: reset@e200400c {
+					compatible = "microchip,lan966x-switch-reset";
+					reg = <0xe200400c 0x4>;
+					reg-names = "gcb";
+					#reset-cells = <1>;
+					cpu-syscon = <&cpu_ctrl>;
+				};
+
+				gpio: pinctrl@e2004064 {
+					compatible = "microchip,lan966x-pinctrl";
+					reg = <0xe2004064 0xb4>,
+					      <0xe2010024 0x138>;
+					resets = <&reset 0>;
+					reset-names = "switch";
+					gpio-controller;
+					#gpio-cells = <2>;
+					gpio-ranges = <&gpio 0 0 78>;
+					interrupt-parent = <&oic>;
+					interrupt-controller;
+					interrupts = <17 IRQ_TYPE_LEVEL_HIGH>;
+					#interrupt-cells = <2>;
+
+					tod_pins: tod_pins {
+						pins = "GPIO_36";
+						function = "ptpsync_1";
+					};
+
+					fc0_a_pins: fcb4-i2c-pins {
+						/* RXD, TXD */
+						pins = "GPIO_9", "GPIO_10";
+						function = "fc0_a";
+					};
+
+				};
+
+				serdes: serdes@e202c000 {
+					compatible = "microchip,lan966x-serdes";
+					reg = <0xe202c000 0x9c>,
+					      <0xe2004010 0x4>;
+					#phy-cells = <2>;
+				};
+
+				mdio1: mdio@e200413c {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					compatible = "microchip,lan966x-miim";
+					reg = <0xe200413c 0x24>,
+					      <0xe2010020 0x4>;
+
+					resets = <&reset 0>;
+					reset-names = "switch";
+
+					lan966x_phy0: ethernet-lan966x_phy@1 {
+						reg = <1>;
+					};
+
+					lan966x_phy1: ethernet-lan966x_phy@2 {
+						reg = <2>;
+					};
+				};
+
+				switch: switch@e0000000 {
+					compatible = "microchip,lan966x-switch";
+					reg = <0xe0000000 0x0100000>,
+					      <0xe2000000 0x0800000>;
+					reg-names = "cpu", "gcb";
+
+					interrupt-parent = <&oic>;
+					interrupts = <12 IRQ_TYPE_LEVEL_HIGH>,
+						     <9 IRQ_TYPE_LEVEL_HIGH>;
+					interrupt-names = "xtr", "ana";
+
+					resets = <&reset 0>;
+					reset-names = "switch";
+
+					pinctrl-names = "default";
+					pinctrl-0 = <&tod_pins>;
+
+					ethernet-ports {
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						port0: port@0 {
+							phy-handle = <&lan966x_phy0>;
+
+							reg = <0>;
+							phy-mode = "gmii";
+							phys = <&serdes 0 CU(0)>;
+						};
+
+						port1: port@1 {
+							phy-handle = <&lan966x_phy1>;
+
+							reg = <1>;
+							phy-mode = "gmii";
+							phys = <&serdes 1 CU(1)>;
+						};
+					};
+				};
+			};
+		};
+	};
+};
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 568410e64ce6..4bfc3f2aafa4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6241,6 +6241,7 @@  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa76e, dpc_log_size);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
+DECLARE_PCI_FIXUP_FINAL(0x1055, 0x9660, of_pci_make_dev_node);
 
 /*
  * Devices known to require a longer delay before first config space access