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

Commit Message

Frank Li July 29, 2024, 8:18 p.m. UTC

From: Richard Zhu <[e-mail]>

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.

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.

Signed-off-by: Richard Zhu <[e-mail]>
Reviewed-by: Manivannan Sadhasivam <[e-mail]>
Signed-off-by: Frank Li <[e-mail]>
---
 drivers/pci/controller/dwc/pci-imx6.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Bjorn Helgaas Sept. 3, 2024, 1:49 a.m. UTC | #1

On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote:
> From: Richard Zhu <[e-mail]>
> 
> 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.

> 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.

> +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?

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

Frank Li Sept. 3, 2024, 8:35 p.m. UTC | #2

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 <[e-mail]>
> >
> > 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);
> > +}

Bjorn Helgaas Sept. 3, 2024, 9:09 p.m. UTC | #3

On Tue, Sep 03, 2024 at 04:35:47PM -0400, Frank Li wrote:
> 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 <[e-mail]>
> > >
> > > 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.

Yes.  But using "Notably" here connects them by suggesting that the
address space translation is a major part of what came before.  Weird
subtlety of English usage, I guess.

Krzysztof, if you have a chance, just s/Notably/Also/ here.

Bjorn Helgaas Sept. 11, 2024, 2:07 p.m. UTC | #4

[+cc Qianqiang]

On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote:
> From: Richard Zhu <[e-mail]>
> 
> 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.
> 
> 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.

> +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;
> +	return (cpu_addr - offset);
> +}

I'm sure that with enough effort, we could prove "entry" cannot be
NULL here, but I'm not sure I want to spend the effort, and we're
going to end up with more patches like this:

  https://lore.kernel.org/r/[e-mail]

I propose this minor change:

  entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
  if (!entry)
    return cpu_addr;

  return cpu_addr - entry->offset;

I still think we should get rid of the .cpu_addr_fixup() callback if
possible.  But that's a discussion for another day.

Bjorn

Frank Li Sept. 11, 2024, 3:19 p.m. UTC | #5

On Wed, Sep 11, 2024 at 09:07:21AM -0500, Bjorn Helgaas wrote:
> [+cc Qianqiang]
>
> On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote:
> > From: Richard Zhu <[e-mail]>
> >
> > 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.
> >
> > 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.
>
> > +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;
> > +	return (cpu_addr - offset);
> > +}
>
> I'm sure that with enough effort, we could prove "entry" cannot be
> NULL here, but I'm not sure I want to spend the effort, and we're
> going to end up with more patches like this:
>
>   https://lore.kernel.org/r/[e-mail]
>
> I propose this minor change:
>
>   entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
>   if (!entry)
>     return cpu_addr;
>
>   return cpu_addr - entry->offset;
>
> I still think we should get rid of the .cpu_addr_fixup() callback if
> possible.  But that's a discussion for another day.

Stop these fake alarm from some tools's scan. entry never be NULL here.
I am working on EP side by involve a "ranges" support like RC side.

Or just omit this kinds of patches.

Frank

>
> Bjorn

Bjorn Helgaas Sept. 11, 2024, 4:33 p.m. UTC | #6

On Wed, Sep 11, 2024 at 11:19:33AM -0400, Frank Li wrote:
> On Wed, Sep 11, 2024 at 09:07:21AM -0500, Bjorn Helgaas wrote:
> > [+cc Qianqiang]
> >
> > On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote:
> > > From: Richard Zhu <[e-mail]>
> > >
> > > 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.
> > >
> > > 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.
> >
> > > +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;
> > > +	return (cpu_addr - offset);
> > > +}
> >
> > I'm sure that with enough effort, we could prove "entry" cannot be
> > NULL here, but I'm not sure I want to spend the effort, and we're
> > going to end up with more patches like this:
> >
> >   https://lore.kernel.org/r/[e-mail]
> >
> > I propose this minor change:
> >
> >   entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> >   if (!entry)
> >     return cpu_addr;
> >
> >   return cpu_addr - entry->offset;
> >
> > I still think we should get rid of the .cpu_addr_fixup() callback if
> > possible.  But that's a discussion for another day.
> 
> Stop these fake alarm from some tools's scan. entry never be NULL here.
> I am working on EP side by involve a "ranges" support like RC side.
> 
> Or just omit this kinds of patches.

As I said initially, we probably *could* prove that "entry" can never
be NULL here, but why should I have to spend the effort to do that?
The "windows" list is not even built in this file, so it's not
trivial.  And even if "entry" can't be NULL now, what's to prevent
that assumption from breaking in the future?

I don't think there's anything wrong with checking for NULL here, and
it avoids copy/pasting this somewhere where it *does* matter.  So I'm
in favor of this kind of patch.

Bjorn

Frank Li Sept. 11, 2024, 6:07 p.m. UTC | #7

On Wed, Sep 11, 2024 at 11:33:56AM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 11, 2024 at 11:19:33AM -0400, Frank Li wrote:
> > On Wed, Sep 11, 2024 at 09:07:21AM -0500, Bjorn Helgaas wrote:
> > > [+cc Qianqiang]
> > >
> > > On Mon, Jul 29, 2024 at 04:18:18PM -0400, Frank Li wrote:
> > > > From: Richard Zhu <[e-mail]>
> > > >
> > > > 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.
> > > >
> > > > 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.
> > >
> > > > +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;
> > > > +	return (cpu_addr - offset);
> > > > +}
> > >
> > > I'm sure that with enough effort, we could prove "entry" cannot be
> > > NULL here, but I'm not sure I want to spend the effort, and we're
> > > going to end up with more patches like this:
> > >
> > >   https://lore.kernel.org/r/[e-mail]
> > >
> > > I propose this minor change:
> > >
> > >   entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> > >   if (!entry)
> > >     return cpu_addr;
> > >
> > >   return cpu_addr - entry->offset;
> > >
> > > I still think we should get rid of the .cpu_addr_fixup() callback if
> > > possible.  But that's a discussion for another day.
> >
> > Stop these fake alarm from some tools's scan. entry never be NULL here.
> > I am working on EP side by involve a "ranges" support like RC side.
> >
> > Or just omit this kinds of patches.
>
> As I said initially, we probably *could* prove that "entry" can never
> be NULL here, but why should I have to spend the effort to do that?
> The "windows" list is not even built in this file, so it's not
> trivial.  And even if "entry" can't be NULL now, what's to prevent
> that assumption from breaking in the future?
>
> I don't think there's anything wrong with checking for NULL here, and
> it avoids copy/pasting this somewhere where it *does* matter.  So I'm
> in favor of this kind of patch.

I am fine for this change.

Frank

>
> Bjorn

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 91aab0288fdcb..4928cea05f6fe 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -65,6 +65,7 @@  enum imx_pcie_variants {
 	IMX8MQ,
 	IMX8MM,
 	IMX8MP,
+	IMX8Q,
 	IMX95,
 	IMX8MQ_EP,
 	IMX8MM_EP,
@@ -80,6 +81,7 @@  enum imx_pcie_variants {
 #define IMX_PCIE_FLAG_HAS_PHY_RESET		BIT(5)
 #define IMX_PCIE_FLAG_HAS_SERDES		BIT(6)
 #define IMX_PCIE_FLAG_SUPPORT_64BIT		BIT(7)
+#define IMX_PCIE_FLAG_CPU_ADDR_FIXUP		BIT(8)
 
 #define imx_check_flag(pci, val)     (pci->drvdata->flags & val)
 
@@ -1011,6 +1013,22 @@  static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
 		regulator_disable(imx_pcie->vpcie);
 }
 
+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;
+
+	return (cpu_addr - offset);
+}
+
 static const struct dw_pcie_host_ops imx_pcie_host_ops = {
 	.init = imx_pcie_host_init,
 	.deinit = imx_pcie_host_exit,
@@ -1019,6 +1037,7 @@  static const struct dw_pcie_host_ops imx_pcie_host_ops = {
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = imx_pcie_start_link,
 	.stop_link = imx_pcie_stop_link,
+	.cpu_addr_fixup = imx_pcie_cpu_addr_fixup,
 };
 
 static void imx_pcie_ep_init(struct dw_pcie_ep *ep)
@@ -1461,6 +1480,7 @@  static const char * const imx6q_clks[] = {"pcie_bus", "pcie", "pcie_phy"};
 static const char * const imx8mm_clks[] = {"pcie_bus", "pcie", "pcie_aux"};
 static const char * const imx8mq_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"};
 static const char * const imx6sx_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"};
+static const char * const imx8q_clks[] = {"mstr", "slv", "dbi"};
 
 static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX6Q] = {
@@ -1564,6 +1584,13 @@  static const struct imx_pcie_drvdata drvdata[] = {
 		.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
 		.enable_ref_clk = imx8mm_pcie_enable_ref_clk,
 	},
+	[IMX8Q] = {
+		.variant = IMX8Q,
+		.flags = IMX_PCIE_FLAG_HAS_PHYDRV |
+			 IMX_PCIE_FLAG_CPU_ADDR_FIXUP,
+		.clk_names = imx8q_clks,
+		.clks_cnt = ARRAY_SIZE(imx8q_clks),
+	},
 	[IMX95] = {
 		.variant = IMX95,
 		.flags = IMX_PCIE_FLAG_HAS_SERDES,
@@ -1641,6 +1668,7 @@  static const struct of_device_id imx_pcie_of_match[] = {
 	{ .compatible = "fsl,imx8mq-pcie", .data = &drvdata[IMX8MQ], },
 	{ .compatible = "fsl,imx8mm-pcie", .data = &drvdata[IMX8MM], },
 	{ .compatible = "fsl,imx8mp-pcie", .data = &drvdata[IMX8MP], },
+	{ .compatible = "fsl,imx8q-pcie", .data = &drvdata[IMX8Q], },
 	{ .compatible = "fsl,imx95-pcie", .data = &drvdata[IMX95], },
 	{ .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
 	{ .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },