[PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[Andre Przywara]

The Arm PL050 "Keyboard/Mouse Interface" is an Arm system IP providing a
PS/2 compatible serial interface.

Add a simple DT schema binding, based on the TRM[1], the existing DTs and
the Linux driver.

[1] https://developer.arm.com/documentation/ddi0143/latest

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/serio/amba-pl050.yaml | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serio/amba-pl050.yaml

diff --git a/Documentation/devicetree/bindings/serio/amba-pl050.yaml b/Documentation/devicetree/bindings/serio/amba-pl050.yaml
new file mode 100644
index 0000000000000..9732a84550098
--- /dev/null
+++ b/Documentation/devicetree/bindings/serio/amba-pl050.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serio/amba-pl050.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm Ltd. PrimeCell PL050 PS/2 Keyboard/Mouse Interface
+
+maintainers:
+  - Andre Przywara <andre.przywara@arm.com>
+
+description:
+  The Arm PrimeCell PS2 Keyboard/Mouse Interface (KMI) is an AMBA compliant
+  peripheral that can be used to implement a keyboard or mouse interface that
+  is IBM PS2 or AT compatible.
+
+# We need a select here so we don't match all nodes with 'arm,primecell'
+select:
+  properties:
+    compatible:
+      contains:
+        const: arm,pl050
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - const: arm,pl050
+      - const: arm,primecell
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: KMI reference clock, used to generate the bus timing
+      - description: APB register access clock
+
+  clock-names:
+    items:
+      - const: KMIREFCLK
+      - const: apb_pclk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    kmi@70000 {
+            compatible = "arm,pl050", "arm,primecell";
+            reg = <0x070000 0x1000>;
+            interrupts = <8>;
+            clocks = <&mb_clk24mhz>, <&soc_smc50mhz>;
+            clock-names = "KMIREFCLK", "apb_pclk";
+    };
+
+...
-- 
2.25.1

Re: [PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[Krzysztof Kozlowski]

On 27/04/2022 13:25, Andre Przywara wrote:
> The Arm PL050 "Keyboard/Mouse Interface" is an Arm system IP providing a
> PS/2 compatible serial interface.
> 
> Add a simple DT schema binding, based on the TRM[1], the existing DTs and
> the Linux driver.
> 
> [1] https://developer.arm.com/documentation/ddi0143/latest
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/serio/amba-pl050.yaml | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serio/amba-pl050.yaml
> 
> diff --git a/Documentation/devicetree/bindings/serio/amba-pl050.yaml b/Documentation/devicetree/bindings/serio/amba-pl050.yaml
> new file mode 100644
> index 0000000000000..9732a84550098
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serio/amba-pl050.yaml

Filename: vendor,device

> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serio/amba-pl050.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Arm Ltd. PrimeCell PL050 PS/2 Keyboard/Mouse Interface
> +
> +maintainers:
> +  - Andre Przywara <andre.przywara@arm.com>
> +
> +description:
> +  The Arm PrimeCell PS2 Keyboard/Mouse Interface (KMI) is an AMBA compliant
> +  peripheral that can be used to implement a keyboard or mouse interface that
> +  is IBM PS2 or AT compatible.
> +
> +# We need a select here so we don't match all nodes with 'arm,primecell'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: arm,pl050
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: arm,pl050
> +      - const: arm,primecell
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: KMI reference clock, used to generate the bus timing
> +      - description: APB register access clock
> +
> +  clock-names:
> +    items:
> +      - const: KMIREFCLK

lowercase letters only

> +      - const: apb_pclk
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    kmi@70000 {

Generic node names, so "serio".

> +            compatible = "arm,pl050", "arm,primecell";
> +            reg = <0x070000 0x1000>;
> +            interrupts = <8>;
> +            clocks = <&mb_clk24mhz>, <&soc_smc50mhz>;
> +            clock-names = "KMIREFCLK", "apb_pclk";
> +    };
> +
> +...


Best regards,
Krzysztof

Re: [PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[Andre Przywara]

On Thu, 28 Apr 2022 09:07:53 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

Hi Krzysztof,

many thanks for having a look and your comments (here and on the other
patches). I will fix them and send a new version ASAP.
One thing below:

> On 27/04/2022 13:25, Andre Przywara wrote:
> > The Arm PL050 "Keyboard/Mouse Interface" is an Arm system IP providing a
> > PS/2 compatible serial interface.
> > 
> > Add a simple DT schema binding, based on the TRM[1], the existing DTs and
> > the Linux driver.
> > 
> > [1] https://developer.arm.com/documentation/ddi0143/latest
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  .../devicetree/bindings/serio/amba-pl050.yaml | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/serio/amba-pl050.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/serio/amba-pl050.yaml b/Documentation/devicetree/bindings/serio/amba-pl050.yaml
> > new file mode 100644
> > index 0000000000000..9732a84550098
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serio/amba-pl050.yaml  
> 
> Filename: vendor,device
> 
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/serio/amba-pl050.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Arm Ltd. PrimeCell PL050 PS/2 Keyboard/Mouse Interface
> > +
> > +maintainers:
> > +  - Andre Przywara <andre.przywara@arm.com>
> > +
> > +description:
> > +  The Arm PrimeCell PS2 Keyboard/Mouse Interface (KMI) is an AMBA compliant
> > +  peripheral that can be used to implement a keyboard or mouse interface that
> > +  is IBM PS2 or AT compatible.
> > +
> > +# We need a select here so we don't match all nodes with 'arm,primecell'
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: arm,pl050
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: arm,pl050
> > +      - const: arm,primecell
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: KMI reference clock, used to generate the bus timing
> > +      - description: APB register access clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: KMIREFCLK  
> 
> lowercase letters only

I am afraid this ship has sailed: the various DTs in the kernel tree use
it in that way, and the Linux driver insists on that spelling. So by
changing this we would break both the existing DT's compliance and also
existing Linux kernels.
So is lowercase something that is mandated by DT schema, or can we just
make an exception here?

Cheers,
Andre

> > +      - const: apb_pclk
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    kmi@70000 {  
> 
> Generic node names, so "serio".
> 
> > +            compatible = "arm,pl050", "arm,primecell";
> > +            reg = <0x070000 0x1000>;
> > +            interrupts = <8>;
> > +            clocks = <&mb_clk24mhz>, <&soc_smc50mhz>;
> > +            clock-names = "KMIREFCLK", "apb_pclk";
> > +    };
> > +
> > +...  
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[Krzysztof Kozlowski]

On 28/04/2022 19:27, Andre Przywara wrote:

>>> +  clock-names:
>>> +    items:
>>> +      - const: KMIREFCLK  
>>
>> lowercase letters only
> 
> I am afraid this ship has sailed: the various DTs in the kernel tree use
> it in that way, and the Linux driver insists on that spelling. 

The driver is coming from ancient times, so it is understandable it has
some old coding style. But it definitely not sailed away. :)

> So by
> changing this we would break both the existing DT's compliance and also
> existing Linux kernels.
> So is lowercase something that is mandated by DT schema, or can we just
> make an exception here?

This uppercase clock name affects even ARM64 devices, so it seems the
device is still being used. Therefore I propose to add new clock name,
old as deprecated and support both in the driver:

	kmi->clk = clk_get(&dev->dev, "kmirefclk");
	if (IS_ERR(kmi->clk)) {
		kmi->clk = clk_get(&dev->dev, "KMIREFCLK");

and convert the DTS as well later on.

Best regards,
Krzysztof

Re: [PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[Krzysztof Kozlowski]

On 29/04/2022 08:29, Krzysztof Kozlowski wrote:
> The driver is coming from ancient times, so it is understandable it has
> some old coding style. But it definitely not sailed away. :)
> 
>> So by
>> changing this we would break both the existing DT's compliance and also
>> existing Linux kernels.
>> So is lowercase something that is mandated by DT schema, or can we just
>> make an exception here?
> 
> This uppercase clock name affects even ARM64 devices, so it seems the
> device is still being used. Therefore I propose to add new clock name,
> old as deprecated and support both in the driver:
> 
> 	kmi->clk = clk_get(&dev->dev, "kmirefclk");
> 	if (IS_ERR(kmi->clk)) {
> 		kmi->clk = clk_get(&dev->dev, "KMIREFCLK");
> 
> and convert the DTS as well later on.

On the other hand, I don't find this as that important if you do not
have time for it, so I am fine with the exception and uppercase name.

Best regards,
Krzysztof

Re: [PATCH 05/11] dt-bindings: serio: add Arm PL050 DT schema

[Andre Przywara]

On Fri, 29 Apr 2022 08:35:02 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

Hi Krzysztof,

> On 29/04/2022 08:29, Krzysztof Kozlowski wrote:
> > The driver is coming from ancient times, so it is understandable it has
> > some old coding style. But it definitely not sailed away. :)

What I meant with this was: there are DTs and drivers out there, for years
now, and this binding is just documenting that state.

I understand that in an ideal world you start with the binding, then write
drivers and DT based on that, but we missed that opportunity years ago.
It actually looks like this uppercase clock name predates the git history,
so this was probably the internal board file clock name twenty years ago,
and just got converted into the DT clock name.

> >> So by
> >> changing this we would break both the existing DT's compliance and also
> >> existing Linux kernels.
> >> So is lowercase something that is mandated by DT schema, or can we just
> >> make an exception here?  
> > 
> > This uppercase clock name affects even ARM64 devices, so it seems the
> > device is still being used. Therefore I propose to add new clock name,
> > old as deprecated and support both in the driver:
> > 
> > 	kmi->clk = clk_get(&dev->dev, "kmirefclk");
> > 	if (IS_ERR(kmi->clk)) {
> > 		kmi->clk = clk_get(&dev->dev, "KMIREFCLK");
> > 
> > and convert the DTS as well later on.  
> 
> On the other hand, I don't find this as that important if you do not
> have time for it, so I am fine with the exception and uppercase name.

Thanks, and yeah, I would really like to not change that. For once, this
device really doesn't have a big future (it's a PS/2 keyboard/mouse
controller after all), and the most prominent users are the fast models /
FVPs, where I wouldn't be aware of anyone actually using that device
there. (I don't even know how to do that). Other than that there is the
Juno board, but I need to disable the KMI driver there because it
electrically interferes with the USB PHY, so is not really usable there as
well, unless you sacrifice USB altogether.

So it would just create a lot of churn, for no real benefit.

Thanks!
Andre