[PATCH 0/3] Add support for ACSPCIE refclk output on J784S4-EVM

[PATCH 0/3] Add support for ACSPCIE refclk output 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 is based on linux-next tagged next-20240715.

Series has been tested with the following device-tree change:
https://gist.github.com/Siddharth-Vadapalli-at-TI/8206ea3a89753c309e4c82d825c75dae
on J784S4-EVM with an NVMe SSD connected to the PCIe connector
corresponding to the PCIe1 instance of PCIe. Logs:
https://gist.github.com/Siddharth-Vadapalli-at-TI/d825068cfe55bba7a869469c1ef64ddd

Regards,
Siddharth.

Siddharth Vadapalli (3):
  dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible
  dt-bindings: PCI: ti,j721e-pci-host: Add ACSPCIE proxy control
    property
  PCI: j721e: Add support for enabling ACSPCIE PAD IO Buffer output

 .../devicetree/bindings/mfd/syscon.yaml       |  1 +
 .../bindings/pci/ti,j721e-pci-host.yaml       | 10 ++++++
 drivers/pci/controller/cadence/pci-j721e.c    | 33 +++++++++++++++++++
 3 files changed, 44 insertions(+)

-- 
2.40.1

[PATCH 1/3] dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible

[Siddharth Vadapalli]

The ACSPCIE_PROXY_CTRL registers within the CTRL_MMR space of TI's J784S4
SoC are used to drive the reference clock to the PCIe Endpoint device via
the PAD IO Buffers. Add the compatible for allowing the PCIe driver to
obtain the regmap for the ACSPCIE_CTRL register within the System
Controller device-tree node in order to enable the PAD IO Buffers.

The Technical Reference Manual for J784S4 SoC with details of the
ASCPCIE_CTRL registers is available at:
https://www.ti.com/lit/zip/spruj52

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 9dc594ea3654..13cbc6fe996e 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -113,6 +113,7 @@ select:
           - ti,am625-dss-oldi-io-ctrl
           - ti,am62p-cpsw-mac-efuse
           - ti,am654-dss-oldi-io-ctrl
+          - ti,j784s4-acspcie-proxy-ctrl
           - ti,j784s4-pcie-ctrl
           - ti,keystone-pllctrl
   required:
-- 
2.40.1

Re: [PATCH 1/3] dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible

[Krzysztof Kozlowski]

On 15/07/2024 14:09, Siddharth Vadapalli wrote:
> The ACSPCIE_PROXY_CTRL registers within the CTRL_MMR space of TI's J784S4
> SoC are used to drive the reference clock to the PCIe Endpoint device via
> the PAD IO Buffers. Add the compatible for allowing the PCIe driver to
> obtain the regmap for the ACSPCIE_CTRL register within the System
> Controller device-tree node in order to enable the PAD IO Buffers.
> 
> The Technical Reference Manual for J784S4 SoC with details of the
> ASCPCIE_CTRL registers is available at:
> https://www.ti.com/lit/zip/spruj52

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: (subset) [PATCH 1/3] dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible

[Lee Jones]

On Mon, 15 Jul 2024 17:39:34 +0530, Siddharth Vadapalli wrote:
> The ACSPCIE_PROXY_CTRL registers within the CTRL_MMR space of TI's J784S4
> SoC are used to drive the reference clock to the PCIe Endpoint device via
> the PAD IO Buffers. Add the compatible for allowing the PCIe driver to
> obtain the regmap for the ACSPCIE_CTRL register within the System
> Controller device-tree node in order to enable the PAD IO Buffers.
> 
> The Technical Reference Manual for J784S4 SoC with details of the
> ASCPCIE_CTRL registers is available at:
> https://www.ti.com/lit/zip/spruj52
> 
> [...]

Applied, thanks!

[1/3] dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible
      commit: d86ce301dcf715ea2d5147bb013a29f722bf5d0b

--
Lee Jones [李琼斯]

Re: (subset) [PATCH 1/3] dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible

[Siddharth Vadapalli]

On Thu, Jul 25, 2024 at 11:23:34AM +0100, Lee Jones wrote:

Hello Lee,

> On Mon, 15 Jul 2024 17:39:34 +0530, Siddharth Vadapalli wrote:
> > The ACSPCIE_PROXY_CTRL registers within the CTRL_MMR space of TI's J784S4
> > SoC are used to drive the reference clock to the PCIe Endpoint device via
> > the PAD IO Buffers. Add the compatible for allowing the PCIe driver to
> > obtain the regmap for the ACSPCIE_CTRL register within the System
> > Controller device-tree node in order to enable the PAD IO Buffers.
> > 
> > The Technical Reference Manual for J784S4 SoC with details of the
> > ASCPCIE_CTRL registers is available at:
> > https://www.ti.com/lit/zip/spruj52
> > 
> > [...]
> 
> Applied, thanks!
> 
> [1/3] dt-bindings: mfd: syscon: Add ti,j784s4-acspcie-proxy-ctrl compatible
>       commit: d86ce301dcf715ea2d5147bb013a29f722bf5d0b

I don't see the commit in the MFD tree [1] and Linux-Next. Therefore I
am assuming that this patch was not committed and will be posting the v2
series with this patch included.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/log/?h=for-mfd-next

Regards,
Siddharth.

[PATCH 2/3] 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>
---
 .../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

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

[Krzysztof Kozlowski]

On 15/07/2024 14:09, Siddharth Vadapalli wrote:
> 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>

Best regards,
Krzysztof

[PATCH 3/3] PCI: j721e: Add support for enabling ACSPCIE PAD IO Buffer output

[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.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 85718246016b..2fa0eff68a8a 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_ENABLE_MASK		GENMASK(1, 0)
 #define GENERATION_SEL_MASK		GENMASK(1, 0)
 
 struct j721e_pcie {
@@ -220,6 +221,30 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
 	return ret;
 }
 
+static int j721e_acspcie_pad_enable(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_ENABLE_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) {
+		/* PAD Enable Bits have to be cleared to in order to enable output */
+		val = ~(args.args[0]);
+		ret = regmap_update_bits(syscon, 0, mask, val);
+		if (ret)
+			dev_err(dev, "Enabling ACSPCIE PAD output failed: %d\n", ret);
+	} else {
+		dev_err(dev, "ti,syscon-acspcie-proxy-ctrl has invalid parameters\n");
+	}
+
+	return ret;
+}
+
 static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
 {
 	struct device *dev = pcie->cdns_pcie->dev;
@@ -259,6 +284,14 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
 		return ret;
 	}
 
+	/* Enable ACSPCIe PAD IO Buffers if the optional property exists */
+	syscon = syscon_regmap_lookup_by_phandle_optional(node, "ti,syscon-acspcie-proxy-ctrl");
+	if (syscon) {
+		ret = j721e_acspcie_pad_enable(pcie, syscon);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.40.1

Re: [PATCH 3/3] PCI: j721e: Add support for enabling ACSPCIE PAD IO Buffer output

[Bjorn Helgaas]

On Mon, Jul 15, 2024 at 05:39:36PM +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.

And I guess this patch actually *does* enable the ACSPCIE PAD IO
Buffer output?

This commit log tells me what is *required* to enable the output, but
it doesn't actually say whether the patch *does* enable the output.

Similarly, if this patch enables ACSPCIE PAD IO Buffer output, I would
make the subject be:

  PCI: j721e: Enable ACSPCIE Refclk output when DT property is present

> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/pci/controller/cadence/pci-j721e.c | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 85718246016b..2fa0eff68a8a 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_ENABLE_MASK		GENMASK(1, 0)
>  #define GENERATION_SEL_MASK		GENMASK(1, 0)
>  
>  struct j721e_pcie {
> @@ -220,6 +221,30 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
>  	return ret;
>  }
>  
> +static int j721e_acspcie_pad_enable(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_ENABLE_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) {
> +		/* PAD Enable Bits have to be cleared to in order to enable output */

Most of this file fits in 80 columns (printf strings are an exception
so they're easier to find with grep).  It'd be nice if your new code
and comments fit in 80 columns as well.

An easy fix for the comment would be:

  /* Clear PAD Enable bits to enable output */

Although it sounds non-sensical to *clear* enable bits to enable
something, and the commit log talks about clearing PAD IO *disable*
bits, so maybe you meant this instead?

  /* Clear PAD IO disable bits to enable output */

If the logical operation here is to enable driving Refclk, I think the
function name and error messages might be more informative if they
mentioned "refclk" instead of "PAD".

> +		val = ~(args.args[0]);
> +		ret = regmap_update_bits(syscon, 0, mask, val);
> +		if (ret)
> +			dev_err(dev, "Enabling ACSPCIE PAD output failed: %d\n", ret);
> +	} else {
> +		dev_err(dev, "ti,syscon-acspcie-proxy-ctrl has invalid parameters\n");
> +	}
> +
> +	return ret;
> +}
> +
>  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>  {
>  	struct device *dev = pcie->cdns_pcie->dev;
> @@ -259,6 +284,14 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
>  		return ret;
>  	}
>  
> +	/* Enable ACSPCIe PAD IO Buffers if the optional property exists */

Is the canonical name "ACSPCIE" or "ACSPCIe"?  You used "ACSPCIE"
above?

> +	syscon = syscon_regmap_lookup_by_phandle_optional(node, "ti,syscon-acspcie-proxy-ctrl");
> +	if (syscon) {
> +		ret = j721e_acspcie_pad_enable(pcie, syscon);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.40.1
> 

Re: [PATCH 3/3] PCI: j721e: Add support for enabling ACSPCIE PAD IO Buffer output

[Siddharth Vadapalli]

On Thu, Jul 25, 2024 at 04:18:41PM -0500, Bjorn Helgaas wrote:

Hello Bjorn,

> On Mon, Jul 15, 2024 at 05:39:36PM +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.
> 
> And I guess this patch actually *does* enable the ACSPCIE PAD IO
> Buffer output?
> 
> This commit log tells me what is *required* to enable the output, but
> it doesn't actually say whether the patch *does* enable the output.
> 
> Similarly, if this patch enables ACSPCIE PAD IO Buffer output, I would
> make the subject be:
> 
>   PCI: j721e: Enable ACSPCIE Refclk output when DT property is present

I will update the commit message and the $subject to clearly indicate
that the patch enables the reference clock output from the ACSPCIE module.

> 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> >  drivers/pci/controller/cadence/pci-j721e.c | 33 ++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index 85718246016b..2fa0eff68a8a 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c

[...]

> > +
> > +	ret = of_parse_phandle_with_fixed_args(node, "ti,syscon-acspcie-proxy-ctrl",
> > +					       1, 0, &args);
> > +	if (!ret) {
> > +		/* PAD Enable Bits have to be cleared to in order to enable output */
> 
> Most of this file fits in 80 columns (printf strings are an exception
> so they're easier to find with grep).  It'd be nice if your new code
> and comments fit in 80 columns as well.

I will wrap the lines to the 80 character limit.

> 
> An easy fix for the comment would be:
> 
>   /* Clear PAD Enable bits to enable output */
> 
> Although it sounds non-sensical to *clear* enable bits to enable
> something, and the commit log talks about clearing PAD IO *disable*
> bits, so maybe you meant this instead?
> 
>   /* Clear PAD IO disable bits to enable output */

Thank you for the suggestion. This is much better and I will update the
comment.

> 
> If the logical operation here is to enable driving Refclk, I think the
> function name and error messages might be more informative if they
> mentioned "refclk" instead of "PAD".

While the Hardware terminology is "PAD", looking at it again, I agree
that using "refclk" will be a better choice for describing the objective
of the function, as well as the outcome in case of a failure.

> 
> > +		val = ~(args.args[0]);
> > +		ret = regmap_update_bits(syscon, 0, mask, val);
> > +		if (ret)
> > +			dev_err(dev, "Enabling ACSPCIE PAD output failed: %d\n", ret);
> > +	} else {
> > +		dev_err(dev, "ti,syscon-acspcie-proxy-ctrl has invalid parameters\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->cdns_pcie->dev;
> > @@ -259,6 +284,14 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable ACSPCIe PAD IO Buffers if the optional property exists */
> 
> Is the canonical name "ACSPCIE" or "ACSPCIe"?  You used "ACSPCIE"
> above?

It is "ACSPCIE" and I have mentioned it that way consistently at all
places including the dt-bindings patches but have accidentally written
"ACSPCIe" above. I will fix this.

Thank you for reviewing this patch.

Regards,
Siddharth.