Re: [PATCH v9 16/16] PCI: imx6: Add iMX95 Endpoint (EP) support

From: Frank Li
Date: Tue Feb 13 2024 - 10:37:55 EST

Next message: Mark Brown: "Re: [PATCH] regulator (max5970): Fix regulator child node name"
Previous message: Vlastimil Babka: "Re: [PATCH v8 3/5] mm,page_owner: Display all stacks and their count"
In reply to: Robin Murphy: "Re: [PATCH v9 16/16] PCI: imx6: Add iMX95 Endpoint (EP) support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Feb 13, 2024 at 01:23:45PM +0000, Robin Murphy wrote:
> On 13/02/2024 10:38 am, Lorenzo Pieralisi wrote:
> > [+Christoph, Robin - just checking with you if the DMA mask handling is
> > what you expect from drivers, don't want to merge code that breaks your
> > expectations]
>
> I don't really understand how all the endpoint stuff fits together, but my
> hunch would be that setting the DMA masks in imx6_add_pcie_ep() might be
> more sensible, unless perhaps there's an implied intent to account for eDMA

pci_epc_mem_alloc_addr() ioremap PCIe EP side outbound memory. Without it
it will be mapped to lower 4G space / SWIOTLB. Generally, outbound is quite
big in some chips, such imx95, (4GB). So it need set dmamask to make sure
it can be mapped to > 4G space.

> channels in root complex mode as well (and so assuming that eDMA and
> endpoint mode play nicely together sharing the same platform device) - as

There are eDMA in host side, but still not find a usecase yet. So it is not
enabled yet. I just need add flags IMX6_PCIE_FLAG_SUPPORT_64BIT in imx95
host if needs.

Anyways, if you like, I can move it into imx6_add_pcie_ep() at next
version.

Frank

> we've discussed before across various threads, setting DMA masks for a host
> bridge itself doesn't really make sense (and hence it leaves the gap where
> we can get away with co-opting them for the MSI address hack); it's any
> additional DMA-initiating functionality within a root complex which should
> own that responsibility.
>
> FWIW it looks like the equivalent change for Layerscape *is* specific to
> endpoint mode, but I don't know how relevant that is to i.MX given that
> they're unrelated hardware configurations.

Previous i.MX PCI outbound space < 4G space. And only have 32bit address
access in system bus. So needn't set DMA mask. i.MX95 change it and PCIe
part is inheriate from layerscape.

Frank
>
> Thanks,
> Robin.
>
> > On Fri, Jan 19, 2024 at 12:11:22PM -0500, Frank Li wrote:
> > > Add iMX95 EP support and add 64bit address support. Internal bus bridge for
> > > PCI support 64bit dma address in iMX95. Hence, call
> > > dma_set_mask_and_coherent() to set 64 bit DMA mask.
> > >
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > > ---
> > >
> > > Notes:
> > > Change from v8 to v9
> > > - update fixme comments
> > > - update BAR1 comments
> > > - Add mani's review tag
> > > Change from v7 to v8
> > > - Update commit message
> > > - Using Fixme
> > > - Update clks_cnts by ARRAY_SIZE
> > > Change from v4 to v7
> > > - none
> > > Change from v3 to v4
> > > - change align to 4k for imx95
> > > Change from v1 to v3
> > > - new patches at v3
> > >
> > > drivers/pci/controller/dwc/pci-imx6.c | 47 +++++++++++++++++++++++++++
> > > 1 file changed, 47 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > > index bbaa45c08323b..7889318f6555a 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -75,6 +75,7 @@ enum imx6_pcie_variants {
> > > IMX8MQ_EP,
> > > IMX8MM_EP,
> > > IMX8MP_EP,
> > > + IMX95_EP,
> > > };
> > > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> > > @@ -84,6 +85,7 @@ enum imx6_pcie_variants {
> > > #define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4)
> > > #define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5)
> > > #define IMX6_PCIE_FLAG_HAS_SERDES BIT(6)
> > > +#define IMX6_PCIE_FLAG_SUPPORT_64BIT BIT(7)
> > > #define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
> > > @@ -616,6 +618,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> > > break;
> > > case IMX7D:
> > > case IMX95:
> > > + case IMX95_EP:
> > > break;
> > > case IMX8MM:
> > > case IMX8MM_EP:
> > > @@ -1050,6 +1053,23 @@ static const struct pci_epc_features imx8m_pcie_epc_features = {
> > > .align = SZ_64K,
> > > };
> > > +/*
> > > + * BAR# | Default BAR enable | Default BAR Type | Default BAR Size | BAR Sizing Scheme
> > > + * ================================================================================================
> > > + * BAR0 | Enable | 64-bit | 1 MB | Programmable Size
> > > + * BAR1 | Disable | 32-bit | 64 KB | Fixed Size
> > > + * BAR1 should be disabled if BAR0 is 64bit.
> > > + * BAR2 | Enable | 32-bit | 1 MB | Programmable Size
> > > + * BAR3 | Enable | 32-bit | 64 KB | Programmable Size
> > > + * BAR4 | Enable | 32-bit | 1M | Programmable Size
> > > + * BAR5 | Enable | 32-bit | 64 KB | Programmable Size
> > > + */
> > > +static const struct pci_epc_features imx95_pcie_epc_features = {
> > > + .msi_capable = true,
> > > + .bar_fixed_size[1] = SZ_64K,
> > > + .align = SZ_4K,
> > > +};
> > > +
> > > static const struct pci_epc_features*
> > > imx6_pcie_ep_get_features(struct dw_pcie_ep *ep)
> > > {
> > > @@ -1092,6 +1112,15 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
> > > pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
> > > + /*
> > > + * FIXME: Ideally, dbi2 base address should come from DT. But since only IMX95 is defining
> > > + * "dbi2" in DT, "dbi_base2" is set to NULL here for that platform alone so that the DWC
> > > + * core code can fetch that from DT. But once all platform DTs were fixed, this and the
> > > + * above "dbi_base2" setting should be removed.
> > > + */
> > > + if (imx6_pcie->drvdata->variant == IMX95_EP)
> > > + pci->dbi_base2 = NULL;
> > > +
> > > ret = dw_pcie_ep_init(ep);
> > > if (ret) {
> > > dev_err(dev, "failed to initialize endpoint\n");
> > > @@ -1345,6 +1374,9 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > "unable to find iomuxc registers\n");
> > > }
> > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_SUPPORT_64BIT))
> > > + dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > > +
> > > /* Grab PCIe PHY Tx Settings */
> > > if (of_property_read_u32(node, "fsl,tx-deemph-gen1",
> > > &imx6_pcie->tx_deemph_gen1))
> > > @@ -1563,6 +1595,20 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > .mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
> > > .epc_features = &imx8m_pcie_epc_features,
> > > },
> > > + [IMX95_EP] = {
> > > + .variant = IMX95_EP,
> > > + .flags = IMX6_PCIE_FLAG_HAS_SERDES |
> > > + IMX6_PCIE_FLAG_SUPPORT_64BIT,
> > > + .clk_names = imx8mq_clks,
> > > + .clks_cnt = ARRAY_SIZE(imx8mq_clks),
> > > + .ltssm_off = IMX95_PE0_GEN_CTRL_3,
> > > + .ltssm_mask = IMX95_PCIE_LTSSM_EN,
> > > + .mode_off[0] = IMX95_PE0_GEN_CTRL_1,
> > > + .mode_mask[0] = IMX95_PCIE_DEVICE_TYPE,
> > > + .init_phy = imx95_pcie_init_phy,
> > > + .epc_features = &imx95_pcie_epc_features,
> > > + .mode = DW_PCIE_EP_TYPE,
> > > + },
> > > };
> > > static const struct of_device_id imx6_pcie_of_match[] = {
> > > @@ -1577,6 +1623,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
> > > { .compatible = "fsl,imx8mq-pcie-ep", .data = &drvdata[IMX8MQ_EP], },
> > > { .compatible = "fsl,imx8mm-pcie-ep", .data = &drvdata[IMX8MM_EP], },
> > > { .compatible = "fsl,imx8mp-pcie-ep", .data = &drvdata[IMX8MP_EP], },
> > > + { .compatible = "fsl,imx95-pcie-ep", .data = &drvdata[IMX95_EP], },
> > > {},
> > > };
> > > --
> > > 2.34.1
> > >

Next message: Mark Brown: "Re: [PATCH] regulator (max5970): Fix regulator child node name"
Previous message: Vlastimil Babka: "Re: [PATCH v8 3/5] mm,page_owner: Display all stacks and their count"
In reply to: Robin Murphy: "Re: [PATCH v9 16/16] PCI: imx6: Add iMX95 Endpoint (EP) support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]