Re: [PATCH v3 3/3] PCI: ultrarisc: Add UltraRISC DP1000 PCIe Root Complex driver

[Jia Wang <wangjia@ultrarisc.com>]

On 2026-04-23 18:34 +0530, Manivannan Sadhasivam wrote:
> On Thu, Apr 23, 2026 at 06:14:48PM +0800, Jia Wang wrote:
> 
> [...]
> 
> > > > +	int ret;
> > > > +
> > > > +	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > > > +	if (!pcie)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > +	if (!pci)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	pci->dev = dev;
> > > > +	pci->ops = &dw_pcie_ops;
> > > > +
> > > > +	/* Set a default value suitable for at most 16 in and 16 out windows */
> > > > +	pci->atu_size = SZ_8K;
> > > > +	pci->max_link_speed = 4;
> > > 
> > > Get this from DT please... This can change between SoC revisions.
> > >
> > 
> > During the v2 DT binding review, Krzysztof pointed out that since the
> > properties are fixed for this hardware, they are deducible from the
> > compatible string and should be dropped from the DT bindings.
> > 
> > The v2 discussion is here:
> > https://lore.kernel.org/all/c60a712e-ecf3-4926-9947-0e593cdb921d@kernel.org/
> > 
> > I then moved them into the driver for v3. If future SoC revisions introduce
> > variations, I'll update the driver to handle them accordingly.
> > 
> > Does this make sense?
> > 
> 
> There are two uses of this property:
> 
> 1. To provide default Max Link Speed of the Root Port(s) if the hardware default
> value is wrong.'
> 
> 2. To override the hardware default to workaround the hardware issues like
> broken PCB routing etc...
> 
> For the first usecase, you need to check if this value different from
> PCI_EXP_LNKCAP_SLS field of LNKCAP register? If not, you don't need to set it
> in soc.dtsi/dt-binding and the DWC core will extract this value from the DBI
> register.
> 
> For the second case, you don't need to define it in your controller dt-binding
> with a default value and also in your soc.dtsi. Since this property is defined
> in the dtschema [1], you can just include the property in board DTS file to
> workaround hardware issues (board specific) and dtbs_check will not complain.
> 
> NOTE: For both cases, you don't need driver changes, since DWC core will handle
> it.
> 
> - Mani
> 
> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/pci/pci-bus-common.yaml#L117
>

Thanks for the detailed explanation, Mani.

That makes perfect sense. I have confirmed that the hardware default value
for our SoC is correct and there is currently no need for board-level
overrides.

Following your suggestion, I will remove the hardcoded max-link-speed from
the driver in v4 and rely on the DWC core to handle it. If a board-level
workaround becomes necessary in the future, we can add the property in the
board DTS file without touching the driver.

> > > > +	pcie->pci = pci;
> > > > +
> > > > +	pp = &pci->pp;
> > > > +
> > > > +	platform_set_drvdata(pdev, pcie);
> > > > +
> > > > +	pp->irq = platform_get_irq(pdev, 1);
> > > > +	if (pp->irq < 0)
> > > > +		return pp->irq;
> > > 
> > > Who is requesting this IRQ?
> > > 
> > 
> > Not needed. Will remove it in v4.
> > 
> > > > +
> > > > +	pp->num_vectors = MAX_MSI_IRQS;
> > > 
> > > Are you sure your controller supports 256 MSIs with one SPI interrupt? It is
> > > possible, but want to make sure it is the case.
> > > 
> > 
> > Yes. The controller implements 8 MSI control blocks (i = 0..7), each
> > providing 32 vectors, so 256 MSIs are supported. This is documented in the
> > controller IP specification.
> >
> 
> Ok, thanks for confirming.
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்
> 

Best Regards,
Jia Wang