Re: [PATCH v8 11/11] PCI: imx6: Add i.MX8Q PCIe root complex (RC) support

[Frank Li]

On Mon, Sep 02, 2024 at 08:49:27PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote:
> > From: Richard Zhu <hongxing.zhu@xxxxxxx>
> >
> > Implement i.MX8Q (i.MX8QM, i.MX8QXP, and i.MX8DXL) PCIe RC support. While
> > the controller resembles that of iMX8MP, the PHY differs significantly.
> > Notably, there's a distinction between PCI bus addresses and CPU addresses.
>
> This bus/CPU address distinction is unrelated to the PHY despite the
> fact that this phrasing suggests they might be related.

This just list two indepentent differences.

>
> > Introduce IMX_PCIE_FLAG_CPU_ADDR_FIXUP in drvdata::flags to indicate driver
> > need the cpu_addr_fixup() callback to facilitate CPU address to PCI bus
> > address conversion according to "ranges" property.
>
> I actually don't understand why the .cpu_addr_fixup() callback exists
> at all.  I guess this is my lack of understanding here, but on the
> ACPI side, if CPU addresses and PCI bus addresses are different, ACPI
> tells us how to convert them.  It seems like it should be analogous
> for DT.

DT can tell how to convert it by ranges. But dwc core use addr_fixup()

drivers/pci/controller/dwc/pcie-designware.c

int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
                              const struct dw_pcie_ob_atu_cfg *atu)
{

        ...
        if (pci->ops && pci->ops->cpu_addr_fixup)
                cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
                                     ^^^

        dwc driver should parser dt range BUT it use callback
        cpu_addr_fixup() to get pci space address, then config ATU.

        Ideally dwc driver can fetch such informaiton from dt to do that.
        But because some history reason, some driver hardcode by
        mask some higher bit instead of using dt's ranges.

        And another possible reason is that EP have not ranges property in
	DT, this code shared between RC and EP. So it use fixup functions.

        ...

}

>
> > +static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> > +{
> > +	struct imx_pcie *imx_pcie = to_imx_pcie(pcie);
> > +	struct dw_pcie_rp *pp = &pcie->pp;
> > +	struct resource_entry *entry;
> > +	unsigned int offset;
> > +
> > +	if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_CPU_ADDR_FIXUP))
> > +		return cpu_addr;
> > +
> > +	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> > +	offset = entry->offset;
>
> I would have assumed that if the DT is correct, "offset" will be zero
> for platforms where PCI bus addresses are identical to CPU addresses,
> so we could (and *should*) do this for all platforms, not just IMX8Q.
> But I must be missing something?

EP mode have not ranges property and pp->bridge is NULL in EP mode.

That's another reason why only add RC function in this patch series. we
need more time to figure out how to get such offset informaiton from dt
when work as EP mode.

Frank

}

>
> > +	return (cpu_addr - offset);
> > +}