by Robin Murphy

[permalink] [raw]

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

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 channels in root complex mode as well (and so assuming that
eDMA and endpoint mode play nicely together sharing the same platform
device) - as 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.

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 <[email protected]>
>> Signed-off-by: Frank Li <[email protected]>
>> ---
>>
>> 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
>>

2024-02-13 15:37:56

by Frank Li

[permalink] [raw]

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

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 <[email protected]>
> > > Signed-off-by: Frank Li <[email protected]>
> > > ---
> > >
> > > 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
> > >

Subject: [PATCH v9 00/16] PCI: imx6: Clean up and add imx95 pci support

Subject: [PATCH v9 02/16] PCI: imx6: Simplify phy handling by using IMX6_PCIE_FLAG_HAS_PHYDRV

Subject: [PATCH v9 01/16] PCI: imx6: Simplify clock handling by using clk_bulk*() function

Subject: [PATCH v9 03/16] PCI: imx6: Simplify reset handling by using by using *_FLAG_HAS_*_RESET

Subject: [PATCH v9 06/16] PCI: imx6: Simplify ltssm_enable() by using ltssm_off and ltssm_mask

Subject: [PATCH v9 07/16] PCI: imx6: Simplify configure_type() by using mode_off and mode_mask

Subject: [PATCH v9 04/16] dt-bindings: imx6q-pcie: Add linux,pci-domain as required for iMX8MQ

Subject: [PATCH v9 08/16] PCI: imx6: Simplify switch-case logic by involve init_phy callback

Subject: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: [PATCH v9 09/16] dt-bindings: imx6q-pcie: Clean up irrationality clocks check

Subject: [PATCH v9 10/16] dt-bindings: imx6q-pcie: Restruct reg and reg-name

Subject: [PATCH v9 11/16] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: [PATCH v9 15/16] dt-bindings: imx6q-pcie: Add iMX95 pcie endpoint compatible string

Subject: [PATCH v9 13/16] PCI: imx6: Clean up get addr_space code

Subject: [PATCH v9 14/16] PCI: imx6: Add epc_features in imx6_pcie_drvdata

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

Subject: [PATCH v9 12/16] PCI: imx6: Add iMX95 PCIe Root Complex support

Subject: Re: [PATCH v9 10/16] dt-bindings: imx6q-pcie: Restruct reg and reg-name

Subject: Re: [PATCH v9 00/16] PCI: imx6: Clean up and add imx95 pci support

Subject: Re: [PATCH v9 00/16] PCI: imx6: Clean up and add imx95 pci support

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

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

Subject: Re: [PATCH v9 07/16] PCI: imx6: Simplify configure_type() by using mode_off and mode_mask

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

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

Subject: [PATCH v9 03/16] PCI: imx6: Simplify reset handling by using by using _FLAG_HAS__RESET