by Manivannan Sadhasivam

[permalink] [raw]

Subject: Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK_* bitmask

On Mon, Dec 11, 2023 at 04:58:30PM -0500, Frank Li wrote:
> Refactors the clock handling logic in the imx6 PCI driver by adding
> HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes
> it more maintainable, as future additions of SOC support will only require
> straightforward changes. The drvdata::flags and a bitmask ensures a cleaner
> and more scalable switch-case structure for handling clocks.
>

Is there any necessity to validate each clock in the driver? I mean, can't you
just rely on devicetree to provide enough clocks for the functioning of the PCIe
controller?

If you can rely on devicetree (everyone should in an ideal world), then you can
just use devm_clk_bulk_get_all() to get all available clocks for the SoC and
just enable/disable whatever is available.

This will greatly simplify the code.

Only downside of this approach is, if the devicetree is not supplying enough
clocks, then it will be difficult to find why PCIe is not working. But this also
means that the devicetree is screwed.

- Mani

> Signed-off-by: Frank Li <[email protected]>
> ---
>
> Notes:
> Change from v1 to v3
> - none
>
> drivers/pci/controller/dwc/pci-imx6.c | 84 +++++++++++++--------------
> 1 file changed, 42 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 74703362aeec7..8a9b527934f80 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -60,6 +60,10 @@ 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_CLK_INBOUND_AXI BIT(3)
> +#define IMX6_PCIE_FLAG_HAS_CLK_AUX BIT(4)
> +
> +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val)
>
> struct imx6_pcie_drvdata {
> enum imx6_pcie_variants variant;
> @@ -550,19 +554,23 @@ static int imx6_pcie_attach_pd(struct device *dev)
>
> static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> {
> - struct dw_pcie *pci = imx6_pcie->pci;
> - struct device *dev = pci->dev;
> unsigned int offset;
> int ret = 0;
>
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) {
> ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_axi clock\n");
> - break;
> - }
> + if (ret)
> + return ret;
> + }
>
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) {
> + ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> + if (ret)
> + return ret;
> + }
> +
> + switch (imx6_pcie->drvdata->variant) {
> + case IMX6SX:
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> IMX6SX_GPR12_PCIE_TEST_POWERDOWN, 0);
> break;
> @@ -589,12 +597,6 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
> case IMX8MQ_EP:
> case IMX8MP:
> case IMX8MP_EP:
> - ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_aux clock\n");
> - break;
> - }
> -
> offset = imx6_pcie_grp_offset(imx6_pcie);
> /*
> * Set the over ride low and enabled
> @@ -614,10 +616,13 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>
> static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> {
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI))
> clk_disable_unprepare(imx6_pcie->pcie_inbound_axi);
> - break;
> +
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX))
> + clk_disable_unprepare(imx6_pcie->pcie_aux);
> +
> + switch (imx6_pcie->drvdata->variant) {
> case IMX6QP:
> case IMX6Q:
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
> @@ -631,14 +636,6 @@ static void imx6_pcie_disable_ref_clk(struct imx6_pcie *imx6_pcie)
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> break;
> - case IMX8MM:
> - case IMX8MM_EP:
> - case IMX8MQ:
> - case IMX8MQ_EP:
> - case IMX8MP:
> - case IMX8MP_EP:
> - clk_disable_unprepare(imx6_pcie->pcie_aux);
> - break;
> default:
> break;
> }
> @@ -1316,21 +1313,21 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> "pcie clock source missing or invalid\n");
>
> - switch (imx6_pcie->drvdata->variant) {
> - case IMX6SX:
> - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> - "pcie_inbound_axi");
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI)) {
> + imx6_pcie->pcie_inbound_axi = devm_clk_get(dev, "pcie_inbound_axi");
> if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> - "pcie_inbound_axi clock missing or invalid\n");
> - break;
> - case IMX8MQ:
> - case IMX8MQ_EP:
> + dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> + "pcie_inbound_axi clock missing or invalid\n");
> + }
> +
> + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_CLK_AUX)) {
> imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> if (IS_ERR(imx6_pcie->pcie_aux))
> return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> "pcie_aux clock source missing or invalid\n");
> - fallthrough;
> + }
> +
> + switch (imx6_pcie->drvdata->variant) {
> case IMX7D:
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> imx6_pcie->controller_id = 1;
> @@ -1353,10 +1350,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> case IMX8MM_EP:
> case IMX8MP:
> case IMX8MP_EP:
> - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> - if (IS_ERR(imx6_pcie->pcie_aux))
> - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> - "pcie_aux clock source missing or invalid\n");
> imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> "apps");
> if (IS_ERR(imx6_pcie->apps_reset))
> @@ -1482,7 +1475,8 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .variant = IMX6SX,
> .flags = IMX6_PCIE_FLAG_IMX6_PHY |
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> - IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_CLK_INBOUND_AXI,
> .gpr = "fsl,imx6q-iomuxc-gpr",
> },
> [IMX6QP] = {
> @@ -1500,30 +1494,36 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> },
> [IMX8MQ] = {
> .variant = IMX8MQ,
> + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> },
> [IMX8MM] = {
> .variant = IMX8MM,
> - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_CLK_AUX,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> },
> [IMX8MP] = {
> .variant = IMX8MP,
> - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND |
> + IMX6_PCIE_FLAG_HAS_CLK_AUX,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> },
> [IMX8MQ_EP] = {
> .variant = IMX8MQ_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mq-iomuxc-gpr",
> },
> [IMX8MM_EP] = {
> .variant = IMX8MM_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mm-iomuxc-gpr",
> },
> [IMX8MP_EP] = {
> .variant = IMX8MP_EP,
> + .flags = IMX6_PCIE_FLAG_HAS_CLK_AUX,
> .mode = DW_PCIE_EP_TYPE,
> .gpr = "fsl,imx8mp-iomuxc-gpr",
> },
> --
> 2.34.1
>

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

2023-12-12 16:51:20

2023-12-13 16:46:52

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

On Wed, Dec 13, 2023 at 10:39:20AM -0500, Frank Li wrote:
> On Wed, Dec 13, 2023 at 08:36:15AM -0600, Rob Herring wrote:
> > On Tue, Dec 12, 2023 at 06:28:43PM -0500, Frank Li wrote:
> > > On Tue, Dec 12, 2023 at 04:44:26PM -0600, Rob Herring wrote:
> > > > On Mon, Dec 11, 2023 at 04:58:37PM -0500, Frank Li wrote:
> > > > > From: Richard Zhu <[email protected]>
> > > > >
> > > > > Add i.MX95 PCIe "fsl,imx95-pcie" compatible string.
> > > > > Add "atu" and "serdes" to reg-names.
> > > > >
> > > > > Signed-off-by: Richard Zhu <[email protected]>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > Change from v2 to v3
> > > > > - Remove krzy's ACK tag
> > > > > - Add condition check for imx95, which required more reg-names then old
> > > > > platform, so need Krzy review again,
> > > > >
> > > > > Change from v1 to v2
> > > > > - add Krzy's ACK tag
> > > > >
> > > > > .../bindings/pci/fsl,imx6q-pcie.yaml | 18 ++++++++++++++++++
> > > > > 1 file changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > > index 81bbb8728f0f9..b8fcf8258f031 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
> > > > > @@ -29,6 +29,7 @@ properties:
> > > > > - fsl,imx8mq-pcie
> > > > > - fsl,imx8mm-pcie
> > > > > - fsl,imx8mp-pcie
> > > > > + - fsl,imx95-pcie
> > > > >
> > > > > reg:
> > > > > items:
> > > > > @@ -90,6 +91,22 @@ required:
> > > > > allOf:
> > > > > - $ref: /schemas/pci/snps,dw-pcie.yaml#
> > > > > - $ref: /schemas/pci/fsl,imx6q-pcie-common.yaml#
> > > > > + - if:
> > > > > + properties:
> > > > > + compatible:
> > > > > + enum:
> > > > > + - fsl,imx95-pcie
> > > > > + then:
> > > > > + properties:
> > > > > + reg:
> > > > > + minItems: 4
> > > > > + reg-names:
> > > > > + items:
> > > > > + - const: dbi
> > > > > + - const: serdes
> > > >
> > > > Did you test this? It should fail because 'serdes' would need to be
> > > > added to snps,dw-pcie.yaml.
> > >
> > > I run "make dt_binding_check DT_SCHEMA_FILES=/pci/", no error report.
> >
> > Only because you have no example. What about your actual .dts?
>
> I see. 95 is quite new. Still have not good base yet.
> I may just take take care this session.
>
> >
> > > And PCIe function can work.
> > >
> > > >
> > > > Is this really not a separate phy block?
> > >
> > > This is misc block, which included phy and also include some registers
> > > about SID for each PCI devices. I plan do it later.
> >
> > Sounds like it should be a separate node and use the phy binding. Do it
> > correctly from the start, not later. Later is an ABI break.
>
> Actually, I considerred phy binding. The major problem is LUT (look up
> table) for MSI and SMMU. LUT need be config according to some PCI device
> information. I have not find good hook for that at PHY driver.
>
> >
> > What is SID?
>
> Stream ID, each device master have SID, which pass to IOMMU and GIC ITS.
>
> Frank

Similar case at

commit c6523c4a301d3adff7ddcf57515b9c847beb7566
Author: Dmitry Baryshkov <[email protected]>
Date: Fri May 6 18:21:02 2022 +0300

dt-bindings: PCI: qcom: Specify reg-names explicitly

Instead of specifying the enum of possible reg-names, specify them
explicitly. This allows us to specify which chipsets need the "atu"
regions and which do not. Also it clearly describes which platforms
enumerate PCIe cores using the dbi region and which use parf region for
that.

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Lorenzo Pieralisi <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Acked-by: Rob Herring <[email protected]>

+ - const: dbi # DesignWare PCIe registers
+ - const: elbi # External local bus interface registers
+ - const: parf # Qualcomm specific registers
^^^^

+ - const: config # PCIe configuration space

Qualcomm called "part", nxp call "serdes"

Frank

>
> >
> > Rob

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

Subject: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK_* bitmask

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

Subject: [PATCH v3 04/13] PCI: imx6: Using "linux,pci-domain" as slot ID

Subject: [PATCH v3 02/13] PCI: imx6: Simplify phy handling by using by using IMX6_PCIE_FLAG_HAS_PHY

Subject: [PATCH v3 05/13] PCI: imx6: Simplify ltssm_enable() by using ltssm_off and ltssm_mask

Subject: [PATCH v3 07/13] PCI: imx6: Simplify switch-case logic by involve init_phy callback

Subject: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: [PATCH v3 06/13] PCI: imx6: Simplify configure_type() by using mode_off and mode_mask

Subject: [PATCH v3 09/13] PCI: imx6: Add iMX95 PCIe support

Subject: [PATCH v3 13/13] PCI: imx6: Add iMX95 Endpoint (EP) function support

Subject: [PATCH v3 10/13] PCI: imx6: Clean up get addr_space code

Subject: [PATCH v3 11/13] PCI: imx6: Add epc_features in imx6_pcie_drvdata

Subject: [PATCH v3 12/13] dt-bindings: imx6q-pcie: Add iMX95 pcie endpoint compatible string

Subject: Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK_* bitmask

Subject: Re: [PATCH v3 12/13] dt-bindings: imx6q-pcie: Add iMX95 pcie endpoint compatible string

Attachments:

Subject: Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK_* bitmask

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK_* bitmask

Subject: Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK_* bitmask

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: Re: [PATCH v3 12/13] dt-bindings: imx6q-pcie: Add iMX95 pcie endpoint compatible string

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: Re: [PATCH v3 08/13] dt-bindings: imx6q-pcie: Add imx95 pcie compatible string

Subject: [PATCH v3 03/13] PCI: imx6: Simplify reset handling by using by using _FLAG_HAS__RESET