[PATCH v3 0/2] Add ACSPCIE refclk support on J784S4-EVM

[PATCH v3 0/2] Add ACSPCIE refclk support on J784S4-EVM

[Siddharth Vadapalli]

Hello,

This series adds support to drive out the reference clock required by
the PCIe Endpoint device using the ACSPCIE buffer. Series __doesn't__
have any dependencies as the dependent patch:
https://lore.kernel.org/r/20240729064012.1915674-1-s-vadapalli@ti.com/
which was mentioned in the v2 series has been merged.

Series is based on linux-next tagged next-20240826.

v2:
https://lore.kernel.org/r/20240729092855.1945700-1-s-vadapalli@ti.com/
Changes since v2:
- Rebased series on next-20240826.

v1:
https://lore.kernel.org/r/20240715120936.1150314-1-s-vadapalli@ti.com/
Changes since v1:
- Patch 1/3 of the v1 series has been posted separately at:
  https://lore.kernel.org/r/20240729064012.1915674-1-s-vadapalli@ti.com/
- Collected Acked-by tag from:
  Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
  for Patch 2/3 of the v1 series which is patch 1/2 of this series:
  https://lore.kernel.org/r/1caa0c9a-1de7-41db-be2b-557b49f4a248@kernel.org/
- Addressed Bjorn's feedback on Patch 3/3 of v1 series at:
  https://lore.kernel.org/r/20240725211841.GA859405@bhelgaas/
  which is patch 2/2 of this series.

Regards,
Siddharth.

Siddharth Vadapalli (2):
  dt-bindings: PCI: ti,j721e-pci-host: Add ACSPCIE proxy control
    property
  PCI: j721e: Enable ACSPCIE Refclk if "ti,syscon-acspcie-proxy-ctrl"
    exists

 .../bindings/pci/ti,j721e-pci-host.yaml       | 10 +++++
 drivers/pci/controller/cadence/pci-j721e.c    | 38 +++++++++++++++++++
 2 files changed, 48 insertions(+)

-- 
2.40.1

[PATCH v3 1/2] dt-bindings: PCI: ti,j721e-pci-host: Add ACSPCIE proxy control property

[Siddharth Vadapalli]

Add the "ti,syscon-acspcie-proxy-ctrl" device-tree property which is
used to obtain a reference to the ACSPCIE Proxy Control register along
with the details of the PAD IO Buffer output enable bits.

The ACSPCIE Proxy Control register is used to drive the reference clock
for the PCIe Endpoint device via the PAD IO Buffers of the ACSPCIE module.
The ACSPCIE module can be used as an alternative to either an on-board
clock generator or an external clock generator for providing the reference
clock to the PCIe Endpoint device.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

v2:
https://lore.kernel.org/r/20240729092855.1945700-2-s-vadapalli@ti.com/
Changes since v2:
- Rebased patch on next-20240826.

v1:
https://lore.kernel.org/r/20240715120936.1150314-3-s-vadapalli@ti.com/
Changes since v1:
- Collected Acked-by tag from:
  Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
  https://lore.kernel.org/r/1caa0c9a-1de7-41db-be2b-557b49f4a248@kernel.org/

 .../devicetree/bindings/pci/ti,j721e-pci-host.yaml     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index 15a2658ceeef..69b499c96c71 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -38,6 +38,16 @@ properties:
       - const: reg
       - const: cfg
 
+  ti,syscon-acspcie-proxy-ctrl:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to the ACSPCIE Proxy Control Register
+          - description: Bitmask corresponding to the PAD IO Buffer
+                         output enable fields (Active Low).
+    description: Specifier for enabling the ACSPCIE PAD outputs to drive
+                 the reference clock to the Endpoint device.
+
   ti,syscon-pcie-ctrl:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
-- 
2.40.1

[PATCH v3 2/2] PCI: j721e: Enable ACSPCIE Refclk if "ti,syscon-acspcie-proxy-ctrl" exists

[Siddharth Vadapalli]

The ACSPCIE module is capable of driving the reference clock required by
the PCIe Endpoint device. It is an alternative to on-board and external
reference clock generators. Enabling the output from the ACSPCIE module's
PAD IO Buffers requires clearing the "PAD IO disable" bits of the
ACSPCIE_PROXY_CTRL register in the CTRL_MMR register space.

Add support to enable the ACSPCIE reference clock output using the optional
device-tree property "ti,syscon-acspcie-proxy-ctrl".

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

v2:
https://lore.kernel.org/r/20240729092855.1945700-3-s-vadapalli@ti.com/
Changes since v2:
- Rebased patch on next-20240826.

v1:
https://lore.kernel.org/r/20240715120936.1150314-4-s-vadapalli@ti.com/
Changes since v1:
- Addressed Bjorn's feedback at:
  https://lore.kernel.org/r/20240725211841.GA859405@bhelgaas/
  with the following changes:
  1) Updated $subject and commit message to indicate that this patch
  enables ACSPCIE reference clock output if the DT property is present.
  2) Updated macro and comments to indicate that the BITS correspond to
  disabling ACSPCIE output, due to which clearing them enables the
  reference clock output.
  3) Replaced "PAD" with "refclk" both in the function name and in the
  error prints.
  4) Wrapped lines to be within the 80 character limit to match the rest
  of the driver.

 drivers/pci/controller/cadence/pci-j721e.c | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 85718246016b..ed42b2229483 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -44,6 +44,7 @@ enum link_status {
 #define J721E_MODE_RC			BIT(7)
 #define LANE_COUNT(n)			((n) << 8)
 
+#define ACSPCIE_PAD_DISABLE_MASK	GENMASK(1, 0)
 #define GENERATION_SEL_MASK		GENMASK(1, 0)
 
 struct j721e_pcie {
@@ -220,6 +221,34 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
 	return ret;
 }
 
+static int j721e_enable_acspcie_refclk(struct j721e_pcie *pcie,
+				       struct regmap *syscon)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+	struct device_node *node = dev->of_node;
+	u32 mask = ACSPCIE_PAD_DISABLE_MASK;
+	struct of_phandle_args args;
+	u32 val;
+	int ret;
+
+	ret = of_parse_phandle_with_fixed_args(node,
+					       "ti,syscon-acspcie-proxy-ctrl",
+					       1, 0, &args);
+	if (!ret) {
+		/* Clear PAD IO disable bits to enable refclk output */
+		val = ~(args.args[0]);
+		ret = regmap_update_bits(syscon, 0, mask, val);
+		if (ret)
+			dev_err(dev, "failed to enable ACSPCIE refclk: %d\n",
+				ret);
+	} else {
+		dev_err(dev,
+			"ti,syscon-acspcie-proxy-ctrl has invalid arguments\n");
+	}
+
+	return ret;
+}
+
 static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
 {
 	struct device *dev = pcie->cdns_pcie->dev;
@@ -259,6 +288,15 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
 		return ret;
 	}
 
+	/* Enable ACSPCIE refclk output if the optional property exists */
+	syscon = syscon_regmap_lookup_by_phandle_optional(node,
+						"ti,syscon-acspcie-proxy-ctrl");
+	if (syscon) {
+		ret = j721e_enable_acspcie_refclk(pcie, syscon);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.40.1

Re: [PATCH v3 2/2] PCI: j721e: Enable ACSPCIE Refclk if "ti,syscon-acspcie-proxy-ctrl" exists

[Bjorn Helgaas]

On Tue, Aug 27, 2024 at 11:25:48AM +0530, Siddharth Vadapalli wrote:
> The ACSPCIE module is capable of driving the reference clock required by
> the PCIe Endpoint device. It is an alternative to on-board and external
> reference clock generators. Enabling the output from the ACSPCIE module's
> PAD IO Buffers requires clearing the "PAD IO disable" bits of the
> ACSPCIE_PROXY_CTRL register in the CTRL_MMR register space.
> 
> Add support to enable the ACSPCIE reference clock output using the optional
> device-tree property "ti,syscon-acspcie-proxy-ctrl".
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> v2:
> https://lore.kernel.org/r/20240729092855.1945700-3-s-vadapalli@ti.com/
> Changes since v2:
> - Rebased patch on next-20240826.
> 
> v1:
> https://lore.kernel.org/r/20240715120936.1150314-4-s-vadapalli@ti.com/
> Changes since v1:
> - Addressed Bjorn's feedback at:
>   https://lore.kernel.org/r/20240725211841.GA859405@bhelgaas/
>   with the following changes:
>   1) Updated $subject and commit message to indicate that this patch
>   enables ACSPCIE reference clock output if the DT property is present.
>   2) Updated macro and comments to indicate that the BITS correspond to
>   disabling ACSPCIE output, due to which clearing them enables the
>   reference clock output.
>   3) Replaced "PAD" with "refclk" both in the function name and in the
>   error prints.
>   4) Wrapped lines to be within the 80 character limit to match the rest
>   of the driver.
> 
>  drivers/pci/controller/cadence/pci-j721e.c | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 85718246016b..ed42b2229483 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -44,6 +44,7 @@ enum link_status {
>  #define J721E_MODE_RC			BIT(7)
>  #define LANE_COUNT(n)			((n) << 8)
>  
> +#define ACSPCIE_PAD_DISABLE_MASK	GENMASK(1, 0)
>  #define GENERATION_SEL_MASK		GENMASK(1, 0)
>  
>  struct j721e_pcie {
> @@ -220,6 +221,34 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
>  	return ret;
>  }
>  
> +static int j721e_enable_acspcie_refclk(struct j721e_pcie *pcie,
> +				       struct regmap *syscon)
> +{
> +	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	u32 mask = ACSPCIE_PAD_DISABLE_MASK;
> +	struct of_phandle_args args;
> +	u32 val;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_fixed_args(node,
> +					       "ti,syscon-acspcie-proxy-ctrl",
> +					       1, 0, &args);
> +	if (!ret) {
> +		/* Clear PAD IO disable bits to enable refclk output */
> +		val = ~(args.args[0]);
> +		ret = regmap_update_bits(syscon, 0, mask, val);
> +		if (ret)
> +			dev_err(dev, "failed to enable ACSPCIE refclk: %d\n",
> +				ret);
> +	} else {
> +		dev_err(dev,
> +			"ti,syscon-acspcie-proxy-ctrl has invalid arguments\n");
> +	}

I should have mentioned this the first time, but this would be easier
to read if structured as:

  ret = of_parse_phandle_with_fixed_args(...);
  if (ret) {
    dev_err(...);
    return ret;
  }

  /* Clear PAD IO disable bits to enable refclk output */
  val = ~(args.args[0]);
  ret = regmap_update_bits(syscon, 0, mask, val);
  if (ret) {
    dev_err(...);
    return ret;
  }

  return 0;

> +	return ret;
> +}
> +
>  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>  {
>  	struct device *dev = pcie->cdns_pcie->dev;
> @@ -259,6 +288,15 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>  		return ret;
>  	}
>  
> +	/* Enable ACSPCIE refclk output if the optional property exists */
> +	syscon = syscon_regmap_lookup_by_phandle_optional(node,
> +						"ti,syscon-acspcie-proxy-ctrl");
> +	if (syscon) {
> +		ret = j721e_enable_acspcie_refclk(pcie, syscon);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;

Not as dramatic here, but I think the following would be a little
simpler since the final "return" isn't used for two purposes
((1) syscon property absent, (2) syscon present and refclk
successfully enabled):

  syscon = syscon_regmap_lookup_by_phandle_optional(...);
  if (!syscon)
    return 0;

  return j721e_enable_acspcie_refclk(...);

>  }
>  
> -- 
> 2.40.1
> 

Re: [PATCH v3 2/2] PCI: j721e: Enable ACSPCIE Refclk if "ti,syscon-acspcie-proxy-ctrl" exists

[Siddharth Vadapalli]

On Wed, Aug 28, 2024 at 04:19:06PM -0500, Bjorn Helgaas wrote:

Hello Bjorn,

> On Tue, Aug 27, 2024 at 11:25:48AM +0530, Siddharth Vadapalli wrote:
> > The ACSPCIE module is capable of driving the reference clock required by
> > the PCIe Endpoint device. It is an alternative to on-board and external
> > reference clock generators. Enabling the output from the ACSPCIE module's
> > PAD IO Buffers requires clearing the "PAD IO disable" bits of the
> > ACSPCIE_PROXY_CTRL register in the CTRL_MMR register space.
> > 
> > Add support to enable the ACSPCIE reference clock output using the optional
> > device-tree property "ti,syscon-acspcie-proxy-ctrl".
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> > 

[...]

> > +
> > +	ret = of_parse_phandle_with_fixed_args(node,
> > +					       "ti,syscon-acspcie-proxy-ctrl",
> > +					       1, 0, &args);
> > +	if (!ret) {
> > +		/* Clear PAD IO disable bits to enable refclk output */
> > +		val = ~(args.args[0]);
> > +		ret = regmap_update_bits(syscon, 0, mask, val);
> > +		if (ret)
> > +			dev_err(dev, "failed to enable ACSPCIE refclk: %d\n",
> > +				ret);
> > +	} else {
> > +		dev_err(dev,
> > +			"ti,syscon-acspcie-proxy-ctrl has invalid arguments\n");
> > +	}
> 
> I should have mentioned this the first time, but this would be easier
> to read if structured as:
> 
>   ret = of_parse_phandle_with_fixed_args(...);
>   if (ret) {
>     dev_err(...);
>     return ret;
>   }
> 
>   /* Clear PAD IO disable bits to enable refclk output */
>   val = ~(args.args[0]);
>   ret = regmap_update_bits(syscon, 0, mask, val);
>   if (ret) {
>     dev_err(...);
>     return ret;
>   }
> 
>   return 0;

Yes, this removes the nested IF conditions and is definitely a cleaner
approach. I will update this in the next version of the patch.

> 
> > +	return ret;
> > +}
> > +
> >  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->cdns_pcie->dev;
> > @@ -259,6 +288,15 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable ACSPCIE refclk output if the optional property exists */
> > +	syscon = syscon_regmap_lookup_by_phandle_optional(node,
> > +						"ti,syscon-acspcie-proxy-ctrl");
> > +	if (syscon) {
> > +		ret = j721e_enable_acspcie_refclk(pcie, syscon);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	return 0;
> 
> Not as dramatic here, but I think the following would be a little
> simpler since the final "return" isn't used for two purposes
> ((1) syscon property absent, (2) syscon present and refclk
> successfully enabled):
> 
>   syscon = syscon_regmap_lookup_by_phandle_optional(...);
>   if (!syscon)
>     return 0;
> 
>   return j721e_enable_acspcie_refclk(...);

Sure. I will implement the above in the next patch. Thank you for
reviewing this patch and sharing your feedback.

Regards,
Siddharth.