[PATCH 1/2] clk: keystone: syscon-clk: Allow the clock node to not be of type syscon

[PATCH 1/2] clk: keystone: syscon-clk: Allow the clock node to not be of type syscon

[Andrew Davis]

There is a helper device_node_to_regmap() we can use that does not force
this clock DT node to be a "syscon" node. It should work the same in
this case but allow us to remove the unneeded "syscon" compatible.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/clk/keystone/syscon-clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/keystone/syscon-clk.c b/drivers/clk/keystone/syscon-clk.c
index 5d7cc83682da..bd5cec0bd12d 100644
--- a/drivers/clk/keystone/syscon-clk.c
+++ b/drivers/clk/keystone/syscon-clk.c
@@ -101,10 +101,10 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 	if (!data)
 		return -EINVAL;
 
-	regmap = syscon_node_to_regmap(dev->of_node);
+	regmap = device_node_to_regmap(dev->of_node);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap),
-				     "failed to find parent regmap\n");
+				     "failed to get regmap\n");
 
 	num_clks = 0;
 	for (p = data; p->name; p++)
-- 
2.39.2

[PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Andrew Davis]

This node's register space is not accessed by any other node, which
is the traditional use for the "syscon" hint. It looks to have been
added here to make use of a Linux kernel helper syscon_node_to_regmap().
The Linux driver now uses a more appropriate helper that does not
require the hint, so let's remove it from the binding.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 .../devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml     | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml b/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
index 66765116aff5..64b8bce5962c 100644
--- a/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
+++ b/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
@@ -16,7 +16,6 @@ properties:
           - ti,am654-ehrpwm-tbclk
           - ti,am64-epwm-tbclk
           - ti,am62-epwm-tbclk
-      - const: syscon
 
   "#clock-cells":
     const: 1
@@ -33,8 +32,8 @@ additionalProperties: false
 
 examples:
   - |
-    ehrpwm_tbclk: syscon@4140 {
-        compatible = "ti,am654-ehrpwm-tbclk", "syscon";
+    ehrpwm_tbclk: clock@4140 {
+        compatible = "ti,am654-ehrpwm-tbclk";
         reg = <0x4140 0x18>;
         #clock-cells = <1>;
     };
-- 
2.39.2

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Vignesh Raghavendra]

On 17/05/23 00:16, Andrew Davis wrote:
> This node's register space is not accessed by any other node, which
> is the traditional use for the "syscon" hint. 

Unfortunately that's not the case across SoCs. Eg AM65x See  TRM section
Table 5-582. CTRLMMR_EPWM0_CTRL Register Field Descriptions

TB_CLKEN is clubbed with SYNCIN_SEL and ePWM tripzone configuration
signals which may require register to be shared with other drivers in future


> It looks to have been
> added here to make use of a Linux kernel helper syscon_node_to_regmap().
> The Linux driver now uses a more appropriate helper that does not
> require the hint, so let's remove it from the binding.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  .../devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml     | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml b/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
> index 66765116aff5..64b8bce5962c 100644
> --- a/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
> +++ b/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
> @@ -16,7 +16,6 @@ properties:
>            - ti,am654-ehrpwm-tbclk
>            - ti,am64-epwm-tbclk
>            - ti,am62-epwm-tbclk
> -      - const: syscon
>  
>    "#clock-cells":
>      const: 1
> @@ -33,8 +32,8 @@ additionalProperties: false
>  
>  examples:
>    - |
> -    ehrpwm_tbclk: syscon@4140 {
> -        compatible = "ti,am654-ehrpwm-tbclk", "syscon";
> +    ehrpwm_tbclk: clock@4140 {
> +        compatible = "ti,am654-ehrpwm-tbclk";
>          reg = <0x4140 0x18>;
>          #clock-cells = <1>;
>      };

-- 
Regards
Vignesh

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Krzysztof Kozlowski]

On Tue, 16 May 2023 13:46:26 -0500, Andrew Davis wrote:
> This node's register space is not accessed by any other node, which
> is the traditional use for the "syscon" hint. It looks to have been
> added here to make use of a Linux kernel helper syscon_node_to_regmap().
> The Linux driver now uses a more appropriate helper that does not
> require the hint, so let's remove it from the binding.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  .../devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml     | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 

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:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.example.dtb: scm-conf@100000: clock-controller@4140:compatible: ['ti,am654-ehrpwm-tbclk', 'syscon'] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.example.dtb: clock-controller@4140: compatible: ['ti,am654-ehrpwm-tbclk', 'syscon'] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml

See https://patchwork.ozlabs.org/patch/1782200

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Krzysztof Kozlowski]

On 16/05/2023 20:46, Andrew Davis wrote:
> This node's register space is not accessed by any other node, which
> is the traditional use for the "syscon" hint. It looks to have been
> added here to make use of a Linux kernel helper syscon_node_to_regmap().
> The Linux driver now uses a more appropriate helper that does not
> require the hint, so let's remove it from the binding.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

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


Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Andrew Davis]

On 5/16/23 11:36 PM, Vignesh Raghavendra wrote:
> 
> 
> On 17/05/23 00:16, Andrew Davis wrote:
>> This node's register space is not accessed by any other node, which
>> is the traditional use for the "syscon" hint.
> 
> Unfortunately that's not the case across SoCs. Eg AM65x See  TRM section
> Table 5-582. CTRLMMR_EPWM0_CTRL Register Field Descriptions
> 

Not sure what version of the TRM you have, latest (Rev. E) has this
register as Table 5-636.. but I found it and see your point here.

> TB_CLKEN is clubbed with SYNCIN_SEL and ePWM tripzone configuration
> signals which may require register to be shared with other drivers in future
> 

This looks to only be a problem in AM65x, all later devices we have fixed
the issue and now group the clock enable bits all together.

Do we actually expect this to be an issue and have a user of these
other bits? If so then we modeled this region wrong in AM65x DT, these
registers are not "tbclk gate registers" any more then they are to the
other functions they provide. These registers should be a syscon node
and then each function within should be a child node.

syscon@4140 {
	compatible = "ti,am654-epwm-crtl", "syscon";
	reg = <0x4140 0x18>;

	ehrpwm_tbclk: clock {
		compatible = "ti,am654-ehrpwm-tbclk";
		#clock-cells = <1>;
	};

	pwm_mux: mux-controller {
		compatible = "mmio-mux";
		#mux-control-cells = <1>;
	};
};

Something like that. That way we do not give preference to one device
and have to have it give out shared registers.

Either that or split the binding compatible, one for AM65x with syscon
and one for all later device compatibles that do not share the register:

compatible:
     oneOf:
       - items:
           - const: ti,am654-ehrpwm-tbclk
           - const: syscon
       - items:
         - enum:
           - ti,am64-epwm-tbclk
           - ti,am62-epwm-tbclk

Would rather the first option.

Andrew

> 
>> It looks to have been
>> added here to make use of a Linux kernel helper syscon_node_to_regmap().
>> The Linux driver now uses a more appropriate helper that does not
>> require the hint, so let's remove it from the binding.
>>
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   .../devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml     | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml b/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
>> index 66765116aff5..64b8bce5962c 100644
>> --- a/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
>> +++ b/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
>> @@ -16,7 +16,6 @@ properties:
>>             - ti,am654-ehrpwm-tbclk
>>             - ti,am64-epwm-tbclk
>>             - ti,am62-epwm-tbclk
>> -      - const: syscon
>>   
>>     "#clock-cells":
>>       const: 1
>> @@ -33,8 +32,8 @@ additionalProperties: false
>>   
>>   examples:
>>     - |
>> -    ehrpwm_tbclk: syscon@4140 {
>> -        compatible = "ti,am654-ehrpwm-tbclk", "syscon";
>> +    ehrpwm_tbclk: clock@4140 {
>> +        compatible = "ti,am654-ehrpwm-tbclk";
>>           reg = <0x4140 0x18>;
>>           #clock-cells = <1>;
>>       };
> 

Re: [PATCH 1/2] clk: keystone: syscon-clk: Allow the clock node to not be of type syscon

[Stephen Boyd]

Quoting Andrew Davis (2023-05-16 11:46:25)
> There is a helper device_node_to_regmap() we can use that does not force
> this clock DT node to be a "syscon" node. It should work the same in
> this case but allow us to remove the unneeded "syscon" compatible.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Applied to clk-next

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Stephen Boyd]

Quoting Andrew Davis (2023-05-16 11:46:26)
> This node's register space is not accessed by any other node, which
> is the traditional use for the "syscon" hint. It looks to have been
> added here to make use of a Linux kernel helper syscon_node_to_regmap().
> The Linux driver now uses a more appropriate helper that does not
> require the hint, so let's remove it from the binding.
> 
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---

Applied to clk-next

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Rob Herring]

On Wed, May 17, 2023 at 1:53 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On Tue, 16 May 2023 13:46:26 -0500, Andrew Davis wrote:
> > This node's register space is not accessed by any other node, which
> > is the traditional use for the "syscon" hint. It looks to have been
> > added here to make use of a Linux kernel helper syscon_node_to_regmap().
> > The Linux driver now uses a more appropriate helper that does not
> > require the hint, so let's remove it from the binding.
> >
> > Signed-off-by: Andrew Davis <afd@ti.com>
> > ---
> >  .../devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml     | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
>
> 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:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.example.dtb: scm-conf@100000: clock-controller@4140:compatible: ['ti,am654-ehrpwm-tbclk', 'syscon'] is too long
>         From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.example.dtb: clock-controller@4140: compatible: ['ti,am654-ehrpwm-tbclk', 'syscon'] is too long
>         From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml

Now failing in linux-next.

Rob

Re: [PATCH 2/2] dt-bindings: clock: ehrpwm: Remove unneeded syscon compatible

[Andrew Davis]

On 6/23/23 2:59 PM, Rob Herring wrote:
> On Wed, May 17, 2023 at 1:53 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On Tue, 16 May 2023 13:46:26 -0500, Andrew Davis wrote:
>>> This node's register space is not accessed by any other node, which
>>> is the traditional use for the "syscon" hint. It looks to have been
>>> added here to make use of a Linux kernel helper syscon_node_to_regmap().
>>> The Linux driver now uses a more appropriate helper that does not
>>> require the hint, so let's remove it from the binding.
>>>
>>> Signed-off-by: Andrew Davis <afd@ti.com>
>>> ---
>>>   .../devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml     | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>
>> 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:
>>
>> dtschema/dtc warnings/errors:
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.example.dtb: scm-conf@100000: clock-controller@4140:compatible: ['ti,am654-ehrpwm-tbclk', 'syscon'] is too long
>>          From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.yaml
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/ti,j721e-system-controller.example.dtb: clock-controller@4140: compatible: ['ti,am654-ehrpwm-tbclk', 'syscon'] is too long
>>          From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/ti,am654-ehrpwm-tbclk.yaml
> 
> Now failing in linux-next.
> 

Sent fix: https://lore.kernel.org/lkml/20230623201519.194269-1-afd@ti.com/T/#u

Andrew