Re: [External] : [PATCH v7 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host driver

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Alok,

On 11/17/25 16:04, ALOK TIWARI wrote:
>> +
>> +/* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
>> +static int rzg3s_pcie_root_write(struct pci_bus *bus, unsigned int devfn,
>> +                 int where, int size, u32 val)
>> +{
>> +    struct rzg3s_pcie_host *host = bus->sysdata;
>> +
>> +    /* Enable access control to the CFGU */
>> +    writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
>> +               host->axi + RZG3S_PCI_PERM);
>> +
>> +    pci_generic_config_write(bus, devfn, where, size, val);
> 
> why ignore pci_generic_config_write ret ?

Missed it.

> 
>> +
>> +    /* Disable access control to the CFGU */
>> +    writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
>> +
>> +    return PCIBIOS_SUCCESSFUL;
>> +}

[...]

>> +static int rzg3s_pcie_init_msi(struct rzg3s_pcie_host *host)
>> +{
>> +    struct platform_device *pdev = to_platform_device(host->dev);
>> +    struct rzg3s_pcie_msi *msi = &host->msi;
>> +    struct device *dev = host->dev;
>> +    const char *devname;
>> +    int irq, ret;
>> +
>> +    ret = devm_mutex_init(dev, &msi->map_lock);
>> +    if (ret)
>> +        return ret;
>> +
>> +    msi->irq = platform_get_irq_byname(pdev, "msi");
>> +    if (msi->irq < 0)
>> +        return dev_err_probe(dev, irq ? irq : -EINVAL,
>> +                     "Failed to get MSI IRQ!\n");
> 
> irq is uninitialized. do you mean msi->irq?

Good catch, I'll update it.

> 
>> +
>> +    devname = devm_kasprintf(dev, GFP_KERNEL, "%s-msi", dev_name(dev));
>> +    if (!devname)
>> +        return -ENOMEM;
>> +
>> +    ret = rzg3s_pcie_msi_allocate_domains(msi);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /*
>> +     * Don't use devm_request_irq() as the driver uses non-devm helpers
>> +     * to control clocks. Mixing them may lead to subtle bugs.
>> +     */
>> +    ret = request_irq(msi->irq, rzg3s_pcie_msi_irq, 0, devname, host);
>> +    if (ret) {
>> +        dev_err_probe(dev, ret, "Failed to request IRQ: %d\n", ret);
>> +        goto free_domains;
>> +    }
>> +
>> +    ret = rzg3s_pcie_msi_setup(host);
>> +    if (ret) {
>> +        dev_err_probe(dev, ret, "Failed to setup MSI!\n");
>> +        goto free_irq;
>> +    }
>> +
>> +    return 0;
>> +
>> +free_irq:
>> +    free_irq(msi->irq, host);
>> +free_domains:
>> +    irq_domain_remove(msi->domain);
>> +    return ret;
>> +}
>> +
>> +static void rzg3s_pcie_intx_irq_ack(struct irq_data *d)
>> +{
>> +    struct rzg3s_pcie_host *host = irq_data_get_irq_chip_data(d);
>> +
>> +    guard(raw_spinlock_irqsave)(&host->hw_lock);
>> +
>> +    rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
>> +                   RZG3S_PCI_PINTRCVIS_INTX(d->hwirq),
>> +                   RZG3S_PCI_PINTRCVIS_INTX(d->hwirq));
>> +}
>> +
>> +static int
>> +rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>> +              int (*init_irqdomain)(struct rzg3s_pcie_host *host),
>> +              void (*teardown_irqdomain)(struct rzg3s_pcie_host *host))
>> +{
>> +    struct device *dev = host->dev;
>> +    int ret;
>> +
>> +    /* Set inbound windows */
>> +    ret = rzg3s_pcie_parse_map_dma_ranges(host);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret,
>> +                     "Failed to set inbound windows!\n");
>> +
>> +    /* Set outbound windows */
>> +    ret = rzg3s_pcie_parse_map_ranges(host);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret,
>> +                     "Failed to set outbound windows!\n");
>> +
>> +    ret = init_irqdomain(host);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "Failed to init IRQ doamin\n");
> 
> typo doamin -> domain

Same here.

Thank you for your review,
Claudiu