by Manivannan Sadhasivam

[permalink] [raw]

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

On Mon, Jan 08, 2024 at 06:21:31PM -0500, Frank Li wrote:
> Add IMX6_PCIE_FLAG_HAS_PHYDRV bitmask define for drvdata::flags. Reduce
> switch-case structure for handling phy.
>

"Since some i.MX platforms make use of the separate PHY driver, use
IMX6_PCIE_FLAG_HAS_PHYDRV flag to identify them and get the reference to PHY
from DT. This simplifies the code."

> Signed-off-by: Frank Li <[email protected]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

- Mani

> ---
>
> Notes:
> Change from v7 to v8:
> - renmae IMX6_PCIE_FLAG_HAS_PHY to IMX6_PCIE_FLAG_HAS_PHYDRV
> Change from v6 to v7:
> - none
> Change from v4 to v5:
> - none, Keep IMX6_PCIE_FLAG_HAS_PHY to indicate dts mismatch when platform
> require phy suppport.
>
> Change from v1 to v3:
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 4912c6b08ecf8..adc90099ec7f8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -60,6 +60,9 @@ enum imx6_pcie_variants {
> #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0)
> #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1)
> #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2)
> +#define IMX6_PCIE_FLAG_HAS_PHYDRV BIT(3)
> +
> +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
>
> #define IMX6_PCIE_MAX_CLKS 6
>
> @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHYDRV)) {
> + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> + if (IS_ERR(imx6_pcie->phy))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> + "failed to get pcie phy\n");
> + }
> +
> switch (imx6_pcie->drvdata->variant) {
> case IMX7D:
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset),
> "failed to get pcie apps reset control\n");
>
> - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> - if (IS_ERR(imx6_pcie->phy))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy),
> - "failed to get pcie phy\n");
> -
> break;
> default:
> break;
> @@ -1458,14 +1463,17 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> },
> [IMX8MM] = {
> .variant = IMX8MM,
> - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_PHYDRV |
> + IMX6_PCIE_FLAG_HAS_APP_RESET,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> .clk_names = imx6_3clks_bus_pcie_aux,
> .clks_cnt = ARRAY_SIZE(imx6_3clks_bus_pcie_aux),
> },
> [IMX8MP] = {
> .variant = IMX8MP,
> - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_PHYDRV,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> .clk_names = imx6_3clks_bus_pcie_aux,
> .clks_cnt = ARRAY_SIZE(imx6_3clks_bus_pcie_aux),
> @@ -1479,6 +1487,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> },
> [IMX8MM_EP] = {
> .variant = IMX8MM_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_PHYDRV,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> .clk_names = imx6_3clks_bus_pcie_aux,
> @@ -1486,6 +1495,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_PHYDRV,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> .clk_names = imx6_3clks_bus_pcie_aux,
> --
> 2.34.1
>

--
மணிவண்ணன் சதாசிவம்

2024-01-19 08:37:34

by Manivannan Sadhasivam

[permalink] [raw]

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

On Mon, Jan 08, 2024 at 06:21:45PM -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.
>
> Signed-off-by: Frank Li <[email protected]>

One comment below. With that addressed,

Reviewed-by: Manivannan Sadhasivam <[email protected]>

> ---
>
> Notes:
> 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 | 46 +++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3f474d4749dce..69ba72c3a9c9c 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
> + * | (If BAR0 is 64-bit) | (if BAR0 is 32-bit) | (if BAR0 is 32-bit) | As Bar0 is 64bit

I think the above comment needs some fixup. I think you want to say that if BAR0
is 64bit then BAR1 should be disabled? If not, please clarify.

> + * 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,14 @@ static int imx6_add_pcie_ep(struct imx6_pcie *imx6_pcie,
>
> pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
>
> + /*
> + * FIXME: dbi2 information should fetch from dtb file. dw_pcie_ep_init() can get dbi_base2
> + * from "dbi2" if pci->dbi_base2 is NULL. All code related pcie_dbi2_offset should be
> + * removed after all dts added "dbi2" reg.
> + */

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.

- Mani

> + 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 +1373,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))
> @@ -1567,6 +1598,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 = imx6_4clks_bus_pcie_phy_aux,
> + .clks_cnt = ARRAY_SIZE(imx6_4clks_bus_pcie_phy_aux),
> + .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[] = {
> @@ -1581,6 +1626,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 v8 00/16] PCI: imx6: Clean up and add imx95 pci support

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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