[PATCH 0/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

[PATCH 0/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

[David Oberhollenzer]

This patch set adds a device tree setting to the PIPE3 PHY for
the reference clock selection of the ACSPCIe clock buffer stage.

The clock buffer can operate in either RX mode (clock pins are input,
recover the clock from the bus) or in TX mode (generate the reference
clock for the bus).

This was tested on a board using a phyCORE-AM57x SoM.

David Oberhollenzer (2):
  phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode
  dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

 .../devicetree/bindings/phy/ti,phy-usb3.yaml  |  9 +++++++
 drivers/phy/ti/phy-ti-pipe3.c                 | 27 +++++++++++++++++++
 2 files changed, 36 insertions(+)

-- 
2.55.0

[PATCH 1/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

[David Oberhollenzer]

Besides power control, the PHY control logic in the OMAP PCIe PHY
subsystem has a configurable reference clock selection. It can
either use the ljcb_clkp/ljcb_clkn pin as inputs, recovering the
clock from the bus (RX mode), or generate the reference clock
internally, driving the bus pins as outputs (TX mode).

This patch adds a device tree setting to the PIPE3 PHY, allowing
the syscon node, register offset and RX/TX mode setting to be
attached and set up correctly.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 drivers/phy/ti/phy-ti-pipe3.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index b5543b5c674c..801a6239328e 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -115,6 +115,9 @@
 #define MEM_CDR_LOS_SOURCE_MASK		GENMASK(10, 9)
 #define MEM_CDR_LOS_SOURCE_SHIFT	9
 
+#define PCIE_TX_RX_CTRL_MASK		GENMASK(17, 16)
+#define PCIE_TX_RX_CTRL_SHIFT		16
+
 /*
  * This is an Empirical value that works, need to confirm the actual
  * value required for the PIPE3PHY_PLL_CONFIGURATION2.PLL_IDLE status
@@ -785,6 +788,24 @@ static int ti_pipe3_get_pll_base(struct ti_pipe3 *phy)
 	return PTR_ERR_OR_ZERO(phy->pll_ctrl_base);
 }
 
+static int ti_pipe3_acspcie_tx_rx_mode(struct ti_pipe3 *phy)
+{
+	struct device_node *np = phy->dev->of_node;
+	struct regmap *regmap;
+	unsigned int args[2];
+
+	regmap = syscon_regmap_lookup_by_phandle_args(np,
+						      "syscon-acspcie-tx-rx",
+						      2, args);
+	if (IS_ERR(regmap)) {
+		dev_warn(phy->dev, "can't get syscon-acspcie-tx-rx\n");
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(regmap, args[0], PCIE_TX_RX_CTRL_MASK,
+				  args[1] << PCIE_TX_RX_CTRL_SHIFT);
+}
+
 static int ti_pipe3_probe(struct platform_device *pdev)
 {
 	struct ti_pipe3 *phy;
@@ -807,6 +828,12 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	phy->dpll_map = data->dpll_map;
 	phy->settings = data->settings;
 
+	if (phy->mode == PIPE3_MODE_PCIE) {
+		ret = ti_pipe3_acspcie_tx_rx_mode(phy);
+		if (ret)
+			dev_warn(dev, "failed to set ACSPCIe TX/RX mode\n");
+	}
+
 	ret = ti_pipe3_get_pll_base(phy);
 	if (ret)
 		return ret;
-- 
2.55.0

Re: [PATCH 1/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe deferral is broken and errors are masked and ignored when fetching the syscon-acspcie-tx-rx property.
--

--- Patch [1]: [PATCH 1/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode ---
commit 033ddba9bd8c07b8f722ae7d9285da5fbd94d88e
Author: David Oberhollenzer <david.oberhollenzer@sigma-star.at>

phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

This commit adds a device tree setting to configure the ACSPCIe TX/RX mode
for the OMAP PCIe PHY subsystem. It uses a syscon regmap to apply the
configuration during probe.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index b5543b5c674c8..801a6239328e1 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c

[ ... ]

> @@ -785,6 +788,24 @@ static int ti_pipe3_get_pll_base(struct ti_pipe3 *phy)
>  	return PTR_ERR_OR_ZERO(phy->pll_ctrl_base);
>  }
>  
> +static int ti_pipe3_acspcie_tx_rx_mode(struct ti_pipe3 *phy)
> +{
> +	struct device_node *np = phy->dev->of_node;
> +	struct regmap *regmap;
> +	unsigned int args[2];
> +
> +	regmap = syscon_regmap_lookup_by_phandle_args(np,
> +						      "syscon-acspcie-tx-rx",
> +						      2, args);
> +	if (IS_ERR(regmap)) {
> +		dev_warn(phy->dev, "can't get syscon-acspcie-tx-rx\n");
> +		return -EINVAL;

[Severity: High]
Does this code inadvertently break probe deferral?

If the syscon provider is not yet ready, syscon_regmap_lookup_by_phandle_args()
will return -EPROBE_DEFER. Masking this actual error code into -EINVAL could
prevent the driver from probing later.

> +	}
> +
> +	return regmap_update_bits(regmap, args[0], PCIE_TX_RX_CTRL_MASK,
> +				  args[1] << PCIE_TX_RX_CTRL_SHIFT);
> +}
> +
>  static int ti_pipe3_probe(struct platform_device *pdev)
>  {
>  	struct ti_pipe3 *phy;
> @@ -807,6 +828,12 @@ static int ti_pipe3_probe(struct platform_device *pdev)
>  	phy->dpll_map = data->dpll_map;
>  	phy->settings = data->settings;
>  
> +	if (phy->mode == PIPE3_MODE_PCIE) {
> +		ret = ti_pipe3_acspcie_tx_rx_mode(phy);
> +		if (ret)
> +			dev_warn(dev, "failed to set ACSPCIe TX/RX mode\n");
> +	}
> +
>  	ret = ti_pipe3_get_pll_base(phy);

[Severity: High]
Is it intentional that the error returned from ti_pipe3_acspcie_tx_rx_mode()
is ignored?

The value of ret is captured but immediately overwritten by
ti_pipe3_get_pll_base(). This causes the probe to proceed even if the syscon
lookup fails or requires deferral.

>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804111849.92046-1-david.oberhollenzer@sigma-star.at?part=1

Re: [PATCH 1/2] phy: ti: ti-pipe3: allow configuring ACSPCIe TX/RX mode

[David Oberhollenzer]

Hi,

On 8/4/26 1:27 PM, sashiko-bot@kernel.org wrote:
>> +
>> +	regmap = syscon_regmap_lookup_by_phandle_args(np,
>> +						      "syscon-acspcie-tx-rx",
>> +						      2, args);
>> +	if (IS_ERR(regmap)) {
>> +		dev_warn(phy->dev, "can't get syscon-acspcie-tx-rx\n");
>> +		return -EINVAL;
> 
> [Severity: High]
> Does this code inadvertently break probe deferral?
> 
> If the syscon provider is not yet ready, syscon_regmap_lookup_by_phandle_args()
> will return -EPROBE_DEFER.

It will not. syscon_regmap_lookup_by_phandle_args() internally wraps
of_parse_phandle_with_fixed_args(), the implementation of which can
either return -EINVAL or -ENODEV.

The only additional error path in syscon_regmap_lookup_by_phandle_args()
also returns -ENODEV. At worst, this will mask -ENODEV as -EINVAL.

> 
>> +	if (phy->mode == PIPE3_MODE_PCIE) {
>> +		ret = ti_pipe3_acspcie_tx_rx_mode(phy);
>> +		if (ret)
>> +			dev_warn(dev, "failed to set ACSPCIe TX/RX mode\n");
>> +	}
>> +
>>   	ret = ti_pipe3_get_pll_base(phy);
> > Is it intentional that the error returned from ti_pipe3_acspcie_tx_rx_mode()
> is ignored?

Yes, I tried to following established convention by modeling the implementation
after other drivers that need to set syscon register bits during probe.

For instance, the dra7 pcie driver. See: dra7xx_pcie_unaligned_memaccess() or
dra7xx_pcie_configure_two_lane(). Those follow the same structure,
returning -EINVAL on lookup failure or the result from regmap_update_bits().

Where the setting is optional, a warning is issued on failure, but the probe
then simply continues.

Thanks,

David

[PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

[David Oberhollenzer]

Add syscon-acspcie-tx-rx property to the PHY bindings.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
index 84f538aa587c..4fb017df2898 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
@@ -52,6 +52,15 @@ properties:
       enum: [wkupclk, sysclk, refclk, dpll_ref,
              dpll_ref_m2, phy-div, div-clk]
 
+  syscon-acspcie-tx-rx:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    maxItems: 1
+    items:
+      items:
+        - description: Phandle to the system control module
+        - description: Register offset of CTRL_CORE_SMA_SW_6
+        - description: Setting for PCIE_TX_RX_CONTROL
+
   syscon-phy-power:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     maxItems: 1
-- 
2.55.0

Re: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Error handling in `ti_pipe3_acspcie_tx_rx_mode` breaks probe deferral and causes warning spam on older device trees
- [Low] Schema Validation Incompleteness: `syscon-acspcie-tx-rx` allows invalid device tree configurations
--

--- Patch [2]: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting ---
commit c7627fd3448f76b00b303ca95c47a9daed73d133
Author: David Oberhollenzer <david.oberhollenzer@sigma-star.at>

dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting
    
Add syscon-acspcie-tx-rx property to the PHY bindings.

> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> index 84f538aa587c4..4fb017df28985 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> @@ -52,6 +52,15 @@ properties:
>        enum: [wkupclk, sysclk, refclk, dpll_ref,
>               dpll_ref_m2, phy-div, div-clk]
>  
> +  syscon-acspcie-tx-rx:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    maxItems: 1
> +    items:
> +      items:
> +        - description: Phandle to the system control module
> +        - description: Register offset of CTRL_CORE_SMA_SW_6
> +        - description: Setting for PCIE_TX_RX_CONTROL
> +

[Severity: Low]
Does this allow invalid device tree configurations to pass validation?

Because this is defined in the top-level properties block and lacks an allOf
conditional if-then block, schema validation (dt_binding_check) will silently
pass if this PCIe-specific property is incorrectly added to SATA or USB3 PHY
nodes.

Should this property be restricted exclusively to the ti,phy-pipe3-pcie
compatible string?

>    syscon-phy-power:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      maxItems: 1

Also, looking at the corresponding driver changes that implement this new
property in drivers/phy/ti/phy-ti-pipe3.c:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_acspcie_tx_rx_mode() {
        ...
        regmap = syscon_regmap_lookup_by_phandle_args(np,
                                                      "syscon-acspcie-tx-rx",
                                                      2, args);
        if (IS_ERR(regmap)) {
                dev_warn(phy->dev, "can't get syscon-acspcie-tx-rx\n");
                return -EINVAL;
        }
        ...
}

[Severity: High]
Does this error handling break probe deferral?

If the syscon provider is not yet ready,
syscon_regmap_lookup_by_phandle_args() returns -EPROBE_DEFER, which is
converted to -EINVAL here.

Additionally, if the syscon-acspcie-tx-rx property is missing (which is
expected on older valid device trees), it returns -ENOENT, leading to
warning spam.

In the caller:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_probe() {
        ...
        if (phy->mode == PIPE3_MODE_PCIE) {
                ret = ti_pipe3_acspcie_tx_rx_mode(phy);
                if (ret)
                        dev_warn(dev, "failed to set ACSPCIe TX/RX mode\n");
        }
        ...
}

Because ti_pipe3_probe() ignores the error code, does this mean probe
succeeds even if the syscon provider deferred?

This could lead to silent hardware initialization failure on systems where
syscon is deferred.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804111849.92046-1-david.oberhollenzer@sigma-star.at?part=2

Re: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

[Krzysztof Kozlowski]

On Tue, Aug 04, 2026 at 01:18:49PM +0200, David Oberhollenzer wrote:
> Add syscon-acspcie-tx-rx property to the PHY bindings.

Why? Actually better to explain WHY in the property description.

> 
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> ---
>  Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> index 84f538aa587c..4fb017df2898 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-usb3.yaml
> @@ -52,6 +52,15 @@ properties:
>        enum: [wkupclk, sysclk, refclk, dpll_ref,
>               dpll_ref_m2, phy-div, div-clk]
>  
> +  syscon-acspcie-tx-rx:

Please organize the patch documenting the compatible (DT bindings)
before the patch using that compatible.
See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Not a valid naming, missing vendor prefix and syscon looks like
Linuxism, since you called it "system control module".

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    maxItems: 1
> +    items:
> +      items:

Missing -

Also, update the example.

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

[David Oberhollenzer]

Hi,

On 8/5/26 9:39 AM, Krzysztof Kozlowski wrote:
> 
> Not a valid naming, missing vendor prefix and syscon looks like
> Linuxism, since you called it "system control module".
> 

I agree that this is a very vendor specific kluge and I suspected
it might need a vendor prefix. However, I ultimately tried to stick
to existing conventions.

There are 3 more syscon properties defined for this device:
  - syscon-phy-power
  - syscon-pllreset
  - syscon-pcs

None of them have a vendor prefix, all of them use the
description "system control module".

I take it, the existing properties already break best practices
that were established later? Or is there another reason they
lack a vendor prefix?

>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    maxItems: 1
>> +    items:
>> +      items:
> 
> Missing -
> 
Likewise, I simply copied from the syscon blocks and made some
minor changes. Hence also the identical description. So this is
broken for the existing properties as well?

Thanks,

David

Re: [PATCH 2/2] dt-bindings: phy: ti,phy-usb3: document aspcie reference clock setting

[Krzysztof Kozlowski]

On 05/08/2026 10:42, David Oberhollenzer wrote:
> Hi,
> 
> On 8/5/26 9:39 AM, Krzysztof Kozlowski wrote:
>>
>> Not a valid naming, missing vendor prefix and syscon looks like
>> Linuxism, since you called it "system control module".
>>
> 
> I agree that this is a very vendor specific kluge and I suspected
> it might need a vendor prefix. However, I ultimately tried to stick
> to existing conventions.
> 
> There are 3 more syscon properties defined for this device:
>   - syscon-phy-power
>   - syscon-pllreset
>   - syscon-pcs

These, as the rest of the binding come, from 2015 or even earlier. A lot
changed during that time. It is a good idea to take recently reviewed
code as an example. It is a bad idea to take 10 yo code.

> 
> None of them have a vendor prefix, all of them use the
> description "system control module".
> 
> I take it, the existing properties already break best practices
> that were established later? Or is there another reason they
> lack a vendor prefix?

Just add vendor prefix to the new property.

> 
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    maxItems: 1
>>> +    items:
>>> +      items:
>>
>> Missing -
>>
> Likewise, I simply copied from the syscon blocks and made some
> minor changes. Hence also the identical description. So this is
> broken for the existing properties as well?

Hm, you are right, the syntax was actually correct. I missed maxItems.

Preferred syntax is for example in
Documentation/devicetree/bindings/crypto/intel,ixp4xx-crypto.yaml, but
your code was fine as well.



Best regards,
Krzysztof