[PATCH 0/3] DTC fixes for Arm pl022 bindings

[PATCH 0/3] DTC fixes for Arm pl022 bindings

[Kuldeep Singh]

This patchset helps in resolving dtc warnings for arm pl022 bindings.
Fixing requires updation to clock and clock-names properties.
One more dts fix is required to eliminate all warnings which will be
sent from other tree.

Please note, this patch is based on top of
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git, for-next

Kuldeep Singh (3):
  dt-bindings: spi: Update clocks property for ARM pl022
  dt-bindings: spi: Update clock-names property for ARM pl022
  dt-bindings: spi: Add spiclk to clock-names property in pl022

 .../devicetree/bindings/spi/spi-pl022.yaml          | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/3] dt-bindings: spi: Update clocks property for ARM pl022

[Kuldeep Singh]

Add missing minItems property to clocks in ARM pl022 bindings.

This also helps in resolving below dtc warnings:
arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clocks: [[4]] is too short
    From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml
arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clock-names: ['apb_pclk'] is too short
    From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml

Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
---
 Documentation/devicetree/bindings/spi/spi-pl022.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
index 6d633728fc2b..7d36e15db5b3 100644
--- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
@@ -34,6 +34,7 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 1
     maxItems: 2
 
   clock-names:
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: spi: Update clocks property for ARM pl022

[Robin Murphy]

On 2022-02-28 12:43, Kuldeep Singh wrote:
> Add missing minItems property to clocks in ARM pl022 bindings.
> 
> This also helps in resolving below dtc warnings:
> arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clocks: [[4]] is too short
>      From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml
> arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clock-names: ['apb_pclk'] is too short
>      From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml

Who says that minItems is missing? Looking at the PL022 TRM[1] it seems 
clear that SSPCLK is pretty fundamental to useful operation. If that DT 
ever worked, it must be that the same clock is wired to both inputs, and 
the fact that that's how the neighbouring PL011 is described is strongly 
suggestive.

If the point of schema is to find errors in DTs, doesn't it make more 
sense to fix the DTs than to weaken the schema just to shut it up?

Of course in this particular case there's also the question of whether 
the most humane way to "fix" the Seattle DTs is to simply delete them, 
but that's orthogonal.

Robin.

[1] https://developer.arm.com/documentation/ddi0194/h/?lang=en

> Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
> ---
>   Documentation/devicetree/bindings/spi/spi-pl022.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
> index 6d633728fc2b..7d36e15db5b3 100644
> --- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
> @@ -34,6 +34,7 @@ properties:
>       maxItems: 1
>   
>     clocks:
> +    minItems: 1
>       maxItems: 2
>   
>     clock-names:

Re: [PATCH 1/3] dt-bindings: spi: Update clocks property for ARM pl022

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On Mon, Feb 28, 2022 at 02:26:12PM +0000, Robin Murphy wrote:

> Who says that minItems is missing? Looking at the PL022 TRM[1] it seems
> clear that SSPCLK is pretty fundamental to useful operation. If that DT ever
> worked, it must be that the same clock is wired to both inputs, and the fact
> that that's how the neighbouring PL011 is described is strongly suggestive.

Well, it could also be that the clock is wired to some other clock which
is always on (which I guess is why the driver allows this in the first
place, there's a lot of sloppy code around stuff like that in the tree).

> If the point of schema is to find errors in DTs, doesn't it make more sense
> to fix the DTs than to weaken the schema just to shut it up?

I do agree on this point though, given that the clock is actually
required for useful operation what's happening here is that the schema
is detecting an error in the use of the binding.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: spi: Update clocks property for ARM pl022

[Robin Murphy]

On 2022-02-28 15:11, Mark Brown wrote:
> On Mon, Feb 28, 2022 at 02:26:12PM +0000, Robin Murphy wrote:
> 
>> Who says that minItems is missing? Looking at the PL022 TRM[1] it seems
>> clear that SSPCLK is pretty fundamental to useful operation. If that DT ever
>> worked, it must be that the same clock is wired to both inputs, and the fact
>> that that's how the neighbouring PL011 is described is strongly suggestive.
> 
> Well, it could also be that the clock is wired to some other clock which
> is always on (which I guess is why the driver allows this in the first
> place, there's a lot of sloppy code around stuff like that in the tree).

I wouldn't say the driver "allows" it, so much as it just blindly grabs 
the first clock assuming it's SSPCLK per the binding, and thus it will 
happen to work out if the underlying physical clock is the same as, or 
equivalent to, the APB PCLK. Otherwise, it's already into some degree of 
not working properly, by virtue of reading the wrong clock rate.

Robin.

Re: [PATCH 1/3] dt-bindings: spi: Update clocks property for ARM pl022

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Mon, Feb 28, 2022 at 03:27:08PM +0000, Robin Murphy wrote:
> On 2022-02-28 15:11, Mark Brown wrote:

> > Well, it could also be that the clock is wired to some other clock which
> > is always on (which I guess is why the driver allows this in the first
> > place, there's a lot of sloppy code around stuff like that in the tree).

> I wouldn't say the driver "allows" it, so much as it just blindly grabs the
> first clock assuming it's SSPCLK per the binding, and thus it will happen to
> work out if the underlying physical clock is the same as, or equivalent to,
> the APB PCLK. Otherwise, it's already into some degree of not working
> properly, by virtue of reading the wrong clock rate.

Ah, the APB clock requirement is inherited from the AMBA implementation
isn't it?  We really ought to be extending an AMBA binding here...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: spi: Update clocks property for ARM pl022

[Robin Murphy]

On 2022-02-28 15:46, Mark Brown wrote:
> On Mon, Feb 28, 2022 at 03:27:08PM +0000, Robin Murphy wrote:
>> On 2022-02-28 15:11, Mark Brown wrote:
> 
>>> Well, it could also be that the clock is wired to some other clock which
>>> is always on (which I guess is why the driver allows this in the first
>>> place, there's a lot of sloppy code around stuff like that in the tree).
> 
>> I wouldn't say the driver "allows" it, so much as it just blindly grabs the
>> first clock assuming it's SSPCLK per the binding, and thus it will happen to
>> work out if the underlying physical clock is the same as, or equivalent to,
>> the APB PCLK. Otherwise, it's already into some degree of not working
>> properly, by virtue of reading the wrong clock rate.
> 
> Ah, the APB clock requirement is inherited from the AMBA implementation
> isn't it?  We really ought to be extending an AMBA binding here...

Yup, both the "apb_pclk" clock specifier and the "arm,primecell" 
compatible technically belong to the common AMBA binding, but I'm not 
sure whether schema has the ability to compose at such fine-grained a 
level :/

Robin.

[PATCH 2/3] dt-bindings: spi: Update clock-names property for ARM pl022

[Kuldeep Singh]

Below are the possibilities of pl022 clock-names property:
    - apb_pclk
    - [sspclk, SSPCLK, spiclk], apb_pclk

The current schema refers to second case and does not consider first
one. Hence, resolve the below dtc warnings by updating clock properties:

arch/arm64/boot/dts/lg/lg1313-ref.dt.yaml: spi@fe900000: clock-names:0:
'apb_pclk' is not one of ['SSPCLK', 'sspclk']
    From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml

Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
---
 Documentation/devicetree/bindings/spi/spi-pl022.yaml | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
index 7d36e15db5b3..5f6926a58b15 100644
--- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
@@ -38,11 +38,14 @@ properties:
     maxItems: 2
 
   clock-names:
-    items:
+    oneOf:
       - enum:
-          - SSPCLK
-          - sspclk
-      - const: apb_pclk
+          - apb_pclk
+      - items:
+          - enum:
+              - sspclk
+              - SSPCLK
+          - const: apb_pclk
 
   pl022,autosuspend-delay:
     description: delay in ms following transfer completion before the
-- 
2.25.1

[PATCH 3/3] dt-bindings: spi: Add spiclk to clock-names property in pl022

[Kuldeep Singh]

Fix below dtc warning by making necessary addition of "spiclk" in
clock-names property.

arch/arm64/boot/dts/broadcom/stingray/bcm958742t.dt.yaml: spi@190000:
clock-names:0: 'spiclk' is not one of ['SSPCLK', 'sspclk']
    From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml
arch/arm64/boot/dts/broadcom/northstar2/ns2-svk.dt.yaml: spi@66190000:
clock-names:0: 'spiclk' is not one of ['SSPCLK', 'sspclk']
    From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml

Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
---
 Documentation/devicetree/bindings/spi/spi-pl022.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
index 5f6926a58b15..fb3075a0c7fd 100644
--- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
@@ -45,6 +45,7 @@ properties:
           - enum:
               - sspclk
               - SSPCLK
+              - spiclk
           - const: apb_pclk
 
   pl022,autosuspend-delay:
-- 
2.25.1

Re: [PATCH 3/3] dt-bindings: spi: Add spiclk to clock-names property in pl022

[Robin Murphy]

On 2022-02-28 12:43, Kuldeep Singh wrote:
> Fix below dtc warning by making necessary addition of "spiclk" in
> clock-names property.
> 
> arch/arm64/boot/dts/broadcom/stingray/bcm958742t.dt.yaml: spi@190000:
> clock-names:0: 'spiclk' is not one of ['SSPCLK', 'sspclk']
>      From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml
> arch/arm64/boot/dts/broadcom/northstar2/ns2-svk.dt.yaml: spi@66190000:
> clock-names:0: 'spiclk' is not one of ['SSPCLK', 'sspclk']
>      From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml

As before, what makes the binding at fault rather than that DT? The 
PL022's actual input is named SSPCLK, not SPICLK, so why should a driver 
which wants to look up that clock by name expect to look for "spiclk"?

Robin.

> Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
> ---
>   Documentation/devicetree/bindings/spi/spi-pl022.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
> index 5f6926a58b15..fb3075a0c7fd 100644
> --- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
> @@ -45,6 +45,7 @@ properties:
>             - enum:
>                 - sspclk
>                 - SSPCLK
> +              - spiclk
>             - const: apb_pclk
>   
>     pl022,autosuspend-delay:

Re: [PATCH 3/3] dt-bindings: spi: Add spiclk to clock-names property in pl022

[Kuldeep Singh]

On Mon, Feb 28, 2022 at 02:36:23PM +0000, Robin Murphy wrote:
> On 2022-02-28 12:43, Kuldeep Singh wrote:
> > Fix below dtc warning by making necessary addition of "spiclk" in
> > clock-names property.
> > 
> > arch/arm64/boot/dts/broadcom/stingray/bcm958742t.dt.yaml: spi@190000:
> > clock-names:0: 'spiclk' is not one of ['SSPCLK', 'sspclk']
> >      From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml
> > arch/arm64/boot/dts/broadcom/northstar2/ns2-svk.dt.yaml: spi@66190000:
> > clock-names:0: 'spiclk' is not one of ['SSPCLK', 'sspclk']
> >      From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml
> 
> As before, what makes the binding at fault rather than that DT? The PL022's
> actual input is named SSPCLK, not SPICLK, so why should a driver which wants
> to look up that clock by name expect to look for "spiclk"?
 
That's right. It's the DT which is at the fault of defining spiclk
instead of sspclk and need to be fixed in DT itself. I didn't take a
look at pl022 doc and acted on the basis of DT info.

Moreover, DT also uses sspclk and SSPCLK names interchangeably which are
anyway same. This also require updation to follow single convention.

Appreciate your comments and valuable inputs.

-- 
Best Regards
Kuldeep