Message-ID: <20240807120956.30c8264e@bootlin.com>
Date: Wed, 7 Aug 2024 12:09:56 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>, Simon Horman
 <horms@...nel.org>, Lee Jones <lee@...nel.org>, Arnd Bergmann
 <arnd@...db.de>, Derek Kiernan <derek.kiernan@....com>, Dragan Cvetic
 <dragan.cvetic@....com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>, Philipp Zabel
 <p.zabel@...gutronix.de>, Lars Povlsen <lars.povlsen@...rochip.com>, Steen
 Hegelund <Steen.Hegelund@...rochip.com>, Daniel Machon
 <daniel.machon@...rochip.com>, UNGLinuxDriver@...rochip.com, Rob Herring
 <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Horatiu Vultur
 <horatiu.vultur@...rochip.com>, Andrew Lunn <andrew@...n.ch>,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 devicetree@...r.kernel.org, Allan Nielsen <allan.nielsen@...rochip.com>,
 Luca Ceresoli <luca.ceresoli@...tlin.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v4 1/8] misc: Add support for LAN966x PCI device

Hi Andy,

On Mon, 5 Aug 2024 22:13:38 +0200
Andy Shevchenko <andy.shevchenko@...il.com> wrote:

> On Mon, Aug 5, 2024 at 12:19 PM Herve Codina <herve.codina@...tlin.com> 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.  
> 
> ...
> 
> + device.h

Will be added.

> 
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>  
> 
> > +#include <linux/pci.h>  
> 
> > +#include <linux/pci_ids.h>  
> 
> AFAIU pci_ids..h is guaranteed to be included by pci.h, but having it
> here explicitly doesn't make it worse, so up to you.

I will keep pci_ids.h

> 
> > +#include <linux/slab.h>  
> 
> ...
> 
> > +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 IRQ_RETVAL(!ret);  
> 
> Hmm... I dunno if it was me who suggested IRQ_RETVAL() here, but it
> usually makes sense for the cases where ret is not inverted.
> 
> Perhaps
> 
>   if (ret)
>     return NONE;
>   return HANDLED;
> 
> is slightly better in this case?

Right. I will use a more compact version:
  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);  
> 
> Before even trying to get it via APIs? (see below as well)
> Also, when is it possible to have 0 here?

pdev->irq can be 0 if the PCI device did not request any IRQ
(i.e. PCI_INTERRUPT_PIN in PCI config header is 0).

I use that to check whether or not INTx is supported.

Even if this code is present in the LAN966x PCI driver, it can be use as a
starting point for other drivers and may be moved to a common part in the
future.

Do you think I should remove it ?

If keeping it is fine, I will add a comment.

> 
> > +       fwnode = dev_fwnode(&pdev->dev);
> > +       if (!fwnode)
> > +               return ERR_PTR(-ENODEV);
> > +
> > +       intr_ctrl = kmalloc(sizeof(*intr_ctrl), GFP_KERNEL);  
> 
> Hmm... Why not use __free()?

Well, just because I am not used to using __free() and I didn't think
about it.

I will use it in the next operation.

> 
> > +       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;
> > +       }  
> 
> I am wondering if you even need this in case you want solely INTx.

I have the feeling that it is needed.
pci_alloc_irq_vectors() will call pci_intx() which in turn enables INT
clearing PCI_COMMAND_INTX_DISABLE flag in the PCI_COMMAND config word.

> 
> > +       intr_ctrl->irq = pci_irq_vector(pdev, 0);  
> 
> Don't remember documentation by heart for this, but the implementation
> suggests that it can be called without the above for retrieving INTx.

So, with the above said, I will keep both pci_alloc_irq_vectors() and
pci_irq_vector() calls.

> 
> > +       ret = request_irq(intr_ctrl->irq, pci_dev_irq_handler, IRQF_SHARED,
> > +                         dev_name(&pdev->dev), intr_ctrl);  
> 
> pci_name() ? (IIRC the macro name)

Indeed, will be changed.

> 
> > +       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 devm_pci_dev_remove_intr_ctrl(void *data)
> > +{  
> 
> > +       struct pci_dev_intr_ctrl *intr_ctrl = data;  
> 
> It can be eliminated
> 
> static void devm_pci_...(void *intr_ctrl)

I will update.

> 
> > +       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, dev_of_node(data->dev));
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;  
> 
> return of_overlay_fdt_apply() ?

Yes indeed.

> 
> > +}  
> 
> ...
> 
> > +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;
> > +
> > +       /*
> > +        * 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 this of_node.
> > +        */
> > +       if (!dev_of_node(dev)) {  
> 
> > +               dev_err(dev, "Missing of_node for device\n");
> > +               return -EINVAL;  
> 
> return dev_err_probe() ?

Yes, I will update.

> 
> > +       }
> > +
> > +       /* 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;
> > +
> > +       pci_set_drvdata(pdev, data);
> > +       data->dev = dev;
> > +
> > +       ret = lan966x_pci_load_overlay(data);
> > +       if (ret)
> > +               return ret;
> > +
> > +       pci_set_master(pdev);
> > +
> > +       ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
> > +       if (ret)
> > +               goto err_unload_overlay;
> > +
> > +       return 0;
> > +
> > +err_unload_overlay:
> > +       lan966x_pci_unload_overlay(data);
> > +       return ret;
> > +}  
> 
> ...
> 
> > +#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>  
> 
> Alphabetical order?

Yes indeed.

Thanks for the review.

Best regards,
Hervé

Powered by blists - more mailing lists