[RFC PATCH 0/2] Enable PCIe1 on J7AHP

[RFC PATCH 0/2] Enable PCIe1 on J7AHP

[Achal Verma]

PCIe1 instance on J7AHP EVM EP connector has reference clock connection
from serdes unlike PCIe0 for which reference clock connection is from
on-board clock generator. To enable PCIe1 instance, ACSPCIE clock buffer
pads have to be enabled to get reference clock output available to PCIe1 EP

This enables clock source select and ACSPCIE clock buffer pads.

Achal Verma (2):
  dt-bindings: PCI: ti,j721e-pci-*: Add "ti,syscon-pcie-refclk-out"
    property
  pci: j721e: Enable reference clock output from serdes

 .../bindings/pci/ti,j721e-pci-host.yaml       | 53 ++++++++++
 .../pci/controller/cadence/pci-j721e-host.c   | 96 +++++++++++++++++++
 2 files changed, 149 insertions(+)

-- 
2.25.1

[RFC PATCH 1/2] dt-bindings: PCI: ti,j721e-pci-*: Add "ti,syscon-pcie-refclk-out" property

[Achal Verma]

Added "ti,syscon-pcie-refclk-out" property to specify the ACSPCIE clock
buffer register offset in SYSCON, which would be used to enable serdes
reference clock output.

Signed-off-by: Achal Verma <a-verma1@ti.com>
---
 .../bindings/pci/ti,j721e-pci-host.yaml       | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
index a2c5eaea57f5..27bdc52282c4 100644
--- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
@@ -44,6 +44,18 @@ properties:
           - description: pcie_ctrl register offset within SYSCON
     description: Specifier for configuring PCIe mode and link speed.
 
+  ti,syscon-pcie-refclk-out:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: Phandle to the SYSCON entry
+          - description: lock2_kick0 register offset within SYSCON
+          - description: lock2_kick1 register offset within SYSCON
+          - description: acspcie_ctrl register offset within SYSCON
+          - description: pcie_refclk_clksel register offset within SYSCON
+          - description: clock source index to source ref clock
+    description: Specifier for enabling ACSPCIe clock buffer for reference clock output.
+
   power-domains:
     maxItems: 1
 
@@ -99,6 +111,7 @@ required:
   - reg
   - reg-names
   - ti,syscon-pcie-ctrl
+  - ti,syscon-pcie-refclk-out
   - max-link-speed
   - num-lanes
   - power-domains
@@ -153,3 +166,43 @@ examples:
             dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
         };
     };
+
+  -
+    #include <dt-bindings/mux/mux.h>
+    #include <dt-bindings/mux/ti-serdes.h>
+    #include <dt-bindings/phy/phy.h>
+    #include <dt-bindings/phy/phy-ti.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie1_rc: pcie@2910000 {
+                compatible = "ti,j784s4-pcie-host";
+                reg = <0x00 0x02910000 0x00 0x1000>,
+                      <0x00 0x02917000 0x00 0x400>,
+                      <0x00 0x0d800000 0x00 0x00800000>,
+                      <0x00 0x18000000 0x00 0x00001000>;
+                reg-names = "intd_cfg", "user_cfg", "reg", "cfg";
+                interrupt-names = "link_state";
+                interrupts = <GIC_SPI 330 IRQ_TYPE_EDGE_RISING>;
+                device_type = "pci";
+                ti,syscon-pcie-ctrl = <&scm_conf 0x4074>;
+                ti,syscon-pcie-refclk-out = <&scm_conf 0x9008 0x900c 0x18090 0x8074 0x1>;
+                max-link-speed = <3>;
+                num-lanes = <4>;
+                power-domains = <&k3_pds 333 TI_SCI_PD_EXCLUSIVE>;
+                clocks = <&k3_clks 333 0>;
+                clock-names = "fck";
+                #address-cells = <3>;
+                #size-cells = <2>;
+                bus-range = <0x0 0xff>;
+                vendor-id = <0x104c>;
+                device-id = <0xb013>;
+                msi-map = <0x0 &gic_its 0x10000 0x10000>;
+                dma-coherent;
+                ranges = <0x01000000 0x0 0x18001000  0x00 0x18001000  0x0 0x0010000>,
+                         <0x02000000 0x0 0x18011000  0x00 0x18011000  0x0 0x7fef000>;
+                dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
+        };
+    };
-- 
2.25.1

Re: [RFC PATCH 1/2] dt-bindings: PCI: ti,j721e-pci-*: Add "ti,syscon-pcie-refclk-out" property

[Krzysztof Kozlowski]

On 05/09/2023 13:48, Achal Verma wrote:
> Added "ti,syscon-pcie-refclk-out" property to specify the ACSPCIE clock
> buffer register offset in SYSCON, which would be used to enable serdes
> reference clock output.
> 
> Signed-off-by: Achal Verma <a-verma1@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-host.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> index a2c5eaea57f5..27bdc52282c4 100644
> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml
> @@ -44,6 +44,18 @@ properties:
>            - description: pcie_ctrl register offset within SYSCON
>      description: Specifier for configuring PCIe mode and link speed.
>  
> +  ti,syscon-pcie-refclk-out:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: Phandle to the SYSCON entry
> +          - description: lock2_kick0 register offset within SYSCON
> +          - description: lock2_kick1 register offset within SYSCON
> +          - description: acspcie_ctrl register offset within SYSCON
> +          - description: pcie_refclk_clksel register offset within SYSCON
> +          - description: clock source index to source ref clock
> +    description: Specifier for enabling ACSPCIe clock buffer for reference clock output.

No, syscon is not a way to avoid creating clock/reset/power controllers.
NAK.


>    power-domains:
>      maxItems: 1
>  
> @@ -99,6 +111,7 @@ required:
>    - reg
>    - reg-names
>    - ti,syscon-pcie-ctrl
> +  - ti,syscon-pcie-refclk-out

So an ABI break?

>    - max-link-speed
>    - num-lanes
>    - power-domains
> @@ -153,3 +166,43 @@ examples:
>              dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x10000 0x0>;
>          };
>      };
> +
> +  -
> +    #include <dt-bindings/mux/mux.h>
> +    #include <dt-bindings/mux/ti-serdes.h>
> +    #include <dt-bindings/phy/phy.h>
> +    #include <dt-bindings/phy/phy-ti.h>
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie1_rc: pcie@2910000 {
> +                compatible = "ti,j784s4-pcie-host";
> +                reg = <0x00 0x02910000 0x00 0x1000>,

No need for new example. It's anyway wrongly formatted...


Best regards,
Krzysztof

Re: [RFC PATCH 1/2] dt-bindings: PCI: ti,j721e-pci-*: Add "ti,syscon-pcie-refclk-out" property

[Rob Herring]

On Tue, 05 Sep 2023 17:18:15 +0530, Achal Verma wrote:
> Added "ti,syscon-pcie-refclk-out" property to specify the ACSPCIE clock
> buffer register offset in SYSCON, which would be used to enable serdes
> reference clock output.
> 
> Signed-off-by: Achal Verma <a-verma1@ti.com>
> ---
>  .../bindings/pci/ti,j721e-pci-host.yaml       | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:171:6: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:172:6: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:173:6: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:174:6: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:177:10: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:178:10: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:180:9: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:197:18: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:198:18: [error] missing starting space in comment (comments)
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:199:17: [warning] wrong indentation: expected 8 but found 16 (indentation)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/pci/ti,j721e-pci-host.example.dts'
Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:180:9: expected <block end>, but found '<block mapping start>'
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/pci/ti,j721e-pci-host.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "/usr/bin/yamllint", line 33, in <module>
    sys.exit(load_entry_point('yamllint==1.29.0', 'console_scripts', 'yamllint')())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3/dist-packages/yamllint/cli.py", line 228, in run
    prob_level = show_problems(problems, file, args_format=args.format,
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3/dist-packages/yamllint/cli.py", line 113, in show_problems
    for problem in problems:
  File "/usr/lib/python3/dist-packages/yamllint/linter.py", line 200, in _run
    for problem in get_cosmetic_problems(buffer, conf, filepath):
  File "/usr/lib/python3/dist-packages/yamllint/linter.py", line 137, in get_cosmetic_problems
    for problem in rule.check(rule_conf,
  File "/usr/lib/python3/dist-packages/yamllint/rules/indentation.py", line 583, in check
    yield from _check(conf, token, prev, next, nextnext, context)
  File "/usr/lib/python3/dist-packages/yamllint/rules/indentation.py", line 344, in _check
    if expected < 0:
       ^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'NoneType' and 'int'
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml:180:9: expected <block end>, but found '<block mapping start>'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230905114816.2993628-2-a-verma1@ti.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[RFC PATCH 2/2] pci: j721e: Enable reference clock output from serdes

[Achal Verma]

PCIe1 in J7AHP EVM has EP side connector reference clock connection from
serdes named SOC_SERDES0_REFCLK(PCIE_REFCLK_OUT) unlike PCIe0 which has
reference clock connection from on-board serdes. To enable this reference
clock out, ACSPCIE clock buffer pads have to be enabled.

This change enables ACSPCIE clock buffer pads and select clock source for
reference clock output.

Signed-off-by: Achal Verma <a-verma1@ti.com>
---
 .../pci/controller/cadence/pci-j721e-host.c   | 96 +++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/pci/controller/cadence/pci-j721e-host.c b/drivers/pci/controller/cadence/pci-j721e-host.c
index af47aa090159..106472920f68 100644
--- a/drivers/pci/controller/cadence/pci-j721e-host.c
+++ b/drivers/pci/controller/cadence/pci-j721e-host.c
@@ -10,10 +10,21 @@
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #include "pcie-cadence.h"
 #include "pci-j721e.h"
 
+#define CTRL_MMR_LOCK2_MASK		0xFFFFFFFF
+#define CTRL_MMR_LOCK2_KICK0_UNLOCK_VAL	0x68EF3490
+#define CTRL_MMR_LOCK2_KICK1_UNLOCK_VAL	0xD172BC5A
+#define CTRL_MMR_LOCK_KICK_LOCK_VAL	0xFFFFFFFF
+#define CTRL_MMR_ACSPCIE_PAD_MASK	0xFFFFFFFF
+#define CTRL_MMR_ACSPCIE_PAD_EN		0x01000000
+#define PCIE_REFCLK_CLKSEL_OUT_EN	BIT(8)
+#define PCIE_REFCLK_CLKSEL_MASK	GENMASK(1, 0)
+
 static int cdns_ti_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 *value)
 {
@@ -79,6 +90,84 @@ static const struct of_device_id of_j721e_pcie_host_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_j721e_pcie_host_match);
 
+static int j721e_enable_acspcie(struct j721e_pcie *pcie)
+{
+	struct device *dev = pcie->cdns_pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct of_phandle_args args;
+	unsigned int lock2_kick0_offset, lock2_kick1_offset;
+	unsigned int acspcie_pad_offset, refclk_clksel_offset;
+	unsigned int refclk_clksel_source;
+	struct regmap *syscon;
+	u32 val = 0, mask = 0;
+	int ret;
+
+	/*
+	 * If property ti,syscon-pcie-refclk-out exists, believe PCIe connector
+	 * requires PCIe ref clock from Serdes, so enable ACSPCIE pads and mux
+	 * to source out PCIe ref clock else ref clock has to be supplied from
+	 * on-board clock generator.
+	 */
+	syscon = syscon_regmap_lookup_by_phandle(node, "ti,syscon-pcie-refclk-out");
+	if (IS_ERR(syscon))
+		return 0;
+
+	ret = of_parse_phandle_with_fixed_args(node, "ti,syscon-pcie-refclk-out", 5,
+						0, &args);
+	if (ret) {
+		dev_err(dev, "Failed to read ti,syscon-pcie-refclk-out property\n");
+		return ret;
+	}
+
+	lock2_kick0_offset = args.args[0];
+	lock2_kick1_offset = args.args[1];
+	acspcie_pad_offset = args.args[2];
+	refclk_clksel_offset = args.args[3];
+	refclk_clksel_source = args.args[4];
+
+	/* Un-lock Partition 2 : 8000h to 9FFFh */
+	mask = CTRL_MMR_LOCK2_MASK;
+	val = CTRL_MMR_LOCK2_KICK0_UNLOCK_VAL;
+	ret = regmap_update_bits(syscon, lock2_kick0_offset, mask, val);
+	if (ret)
+		goto err;
+
+	val = CTRL_MMR_LOCK2_KICK1_UNLOCK_VAL;
+	ret = regmap_update_bits(syscon, lock2_kick1_offset, mask, val);
+	if (ret)
+		goto err;
+
+	/* Enable ACSPCIe PADS  */
+	mask = CTRL_MMR_ACSPCIE_PAD_MASK;
+	val = CTRL_MMR_ACSPCIE_PAD_EN;
+	ret = regmap_update_bits(syscon, acspcie_pad_offset, mask, val);
+	if (ret)
+		goto err;
+
+	/* PCIE_REFCLKx_CLKSEL : EN + ref_clk_source */
+	mask = PCIE_REFCLK_CLKSEL_OUT_EN | PCIE_REFCLK_CLKSEL_MASK;
+	val = PCIE_REFCLK_CLKSEL_OUT_EN | refclk_clksel_source;
+	ret = regmap_update_bits(syscon, refclk_clksel_offset, mask, val);
+	if (ret)
+		goto err;
+
+	/* Re-lock Partition 2 : 8000h to 9FFFh */
+	mask = CTRL_MMR_LOCK_KICK_LOCK_VAL;
+	val = CTRL_MMR_LOCK_KICK_LOCK_VAL;
+	ret = regmap_update_bits(syscon, lock2_kick0_offset, mask, val);
+	if (ret)
+		goto err;
+
+	ret = regmap_update_bits(syscon, lock2_kick1_offset, mask, val);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	dev_err(dev, "Failed to enable ref clock out\n");
+	return ret;
+}
+
 static int j721e_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -121,6 +210,13 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Enable ACSPCIe clock buffer to source out reference clock for EP
+	 */
+	ret = j721e_enable_acspcie(pcie);
+	if (ret < 0)
+		return ret;
+
 	pcie->perst_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(pcie->perst_gpiod)) {
 		ret = PTR_ERR(pcie->perst_gpiod);
-- 
2.25.1

Re: [RFC PATCH 2/2] pci: j721e: Enable reference clock output from serdes

[Bjorn Helgaas]

Previous j721e subject line history is like this:

  c86f4bd6008e ("PCI: j721e: Convert to platform remove callback returning void")
  053ca37c87af ("PCI: j721e: Initialize pcie->cdns_pcie before using it")
  19e863828acf ("PCI: j721e: Drop redundant struct device *")
  72de208f2bda ("PCI: j721e: Drop pointless of_device_get_match_data() cast")
  496bb18483cc ("PCI: j721e: Fix j721e_pcie_probe() error path")
  c8a375a8e15a ("PCI: j721e: Add PCIe support for AM64")

Match capitalization style, i.e., "PCI: " instead of "pci: "

On Tue, Sep 05, 2023 at 05:18:16PM +0530, Achal Verma wrote:
> PCIe1 in J7AHP EVM has EP side connector reference clock connection from
> serdes named SOC_SERDES0_REFCLK(PCIE_REFCLK_OUT) unlike PCIe0 which has
> reference clock connection from on-board serdes. To enable this reference
> clock out, ACSPCIE clock buffer pads have to be enabled.
> 
> This change enables ACSPCIE clock buffer pads and select clock source for
> reference clock output.

s/This change enables/Enable/
s/and select/and selects/

> +static int j721e_enable_acspcie(struct j721e_pcie *pcie)
> +{
> +	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	struct of_phandle_args args;
> +	unsigned int lock2_kick0_offset, lock2_kick1_offset;
> +	unsigned int acspcie_pad_offset, refclk_clksel_offset;
> +	unsigned int refclk_clksel_source;
> +	struct regmap *syscon;
> +	u32 val = 0, mask = 0;

Looks like these initializations are unnecessary?

> +	syscon = syscon_regmap_lookup_by_phandle(node, "ti,syscon-pcie-refclk-out");

Looks like this and the of_parse_phandle_with_fixed_args() below don't
fit in 80 columns like the rest of the file.

> +	ret = of_parse_phandle_with_fixed_args(node, "ti,syscon-pcie-refclk-out", 5,
> +						0, &args);

> +	/* Enable ACSPCIe PADS  */

Spurious extra space at end of comment.

> +	/*
> +	 * Enable ACSPCIe clock buffer to source out reference clock for EP
> +	 */

Looks like it could be a single-line comment, e.g.,

  /* Enable ACSPCIe clock buffer to source out reference clock for EP */