[v8,05/17] dt-bindings: media: Add bindings for ARM mali-c55

Commit Message

Dan Scally Nov. 6, 2024, 10:05 a.m. UTC

Add the yaml binding for ARM's Mali-C55 Image Signal Processor.

Acked-by: Nayden Kanchev <[e-mail]>
Co-developed-by: Jacopo Mondi <[e-mail]>
Signed-off-by: Jacopo Mondi <[e-mail]>
Signed-off-by: Daniel Scally <[e-mail]>
---
Changes in v8:

	- Added the video clock back in. Now that we have actual hardware it's
	  clear that it's necessary.
	- Added reset lines 
	- Dropped R-bs

Changes in v7:

	- None

Changes in v6:

	- None

Changes in v5:

	- None

Changes in v4:

	- Switched to port instead of ports

Changes in v3:

	- Dropped the video clock as suggested by Laurent. I didn't retain it
	for the purposes of the refcount since this driver will call .s_stream()
	for the sensor driver which will refcount the clock anyway.
	- Clarified that the port is a parallel input port rather (Sakari)

Changes in v2:

	- Added clocks information
	- Fixed the warnings raised by Rob

 .../bindings/media/arm,mali-c55.yaml          | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/arm,mali-c55.yaml

Comments

Krzysztof Kozlowski Nov. 6, 2024, 12:15 p.m. UTC | #1

On Wed, Nov 06, 2024 at 10:05:22AM +0000, Daniel Scally wrote:
> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> 
> Acked-by: Nayden Kanchev <[e-mail]>
> Co-developed-by: Jacopo Mondi <[e-mail]>
> Signed-off-by: Jacopo Mondi <[e-mail]>
> Signed-off-by: Daniel Scally <[e-mail]>
> ---
> Changes in v8:
> 
> 	- Added the video clock back in. Now that we have actual hardware it's
> 	  clear that it's necessary.
> 	- Added reset lines 
> 	- Dropped R-bs

These are trivial, so I wish you kept the review... but since you ask,
then comment further

I recommend using b4, so your cover letter changelog comes with nice
links to previous versions. I scrolled through entire cover letter for
this (for me that's almost the only point of cover letter) and could
not find them. Anyway, just a remark.


...

> +  resets:
> +    items:
> +      - description: vclk domain reset
> +      - description: aclk domain reset
> +      - description: hclk domain reset
> +
> +  reset-names:
> +    items:
> +      - const: vresetn

drop "reset", it's redundant and rather come here with logical name. I
wonder what "n" means as well. It's not a GPIO to be "inverted"...

I wonder about reset domains for clocks as well... is this just gate
clock misrepresented?

Best regards,
Krzysztof

Laurent Pinchart Nov. 6, 2024, 1:57 p.m. UTC | #2

Hi Krzysztof,

On Wed, Nov 06, 2024 at 01:15:23PM +0100, Krzysztof Kozlowski wrote:
> On Wed, Nov 06, 2024 at 10:05:22AM +0000, Daniel Scally wrote:
> > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > 
> > Acked-by: Nayden Kanchev <[e-mail]>
> > Co-developed-by: Jacopo Mondi <[e-mail]>
> > Signed-off-by: Jacopo Mondi <[e-mail]>
> > Signed-off-by: Daniel Scally <[e-mail]>
> > ---
> > Changes in v8:
> > 
> > 	- Added the video clock back in. Now that we have actual hardware it's
> > 	  clear that it's necessary.
> > 	- Added reset lines 
> > 	- Dropped R-bs
> 
> These are trivial, so I wish you kept the review... but since you ask,
> then comment further
> 
> I recommend using b4, so your cover letter changelog comes with nice
> links to previous versions. I scrolled through entire cover letter for
> this (for me that's almost the only point of cover letter) and could
> not find them. Anyway, just a remark.
> 
> 
> ...
> 
> > +  resets:
> > +    items:
> > +      - description: vclk domain reset
> > +      - description: aclk domain reset
> > +      - description: hclk domain reset
> > +
> > +  reset-names:
> > +    items:
> > +      - const: vresetn
> 
> drop "reset", it's redundant and rather come here with logical name. I
> wonder what "n" means as well. It's not a GPIO to be "inverted"...

The aresetn and hresetn names come directly from a hardware manual
(vresetn seems to be called rstn in that document though). As far as I
understand, they are the names of the external signals of the IP core.
I tend to pick the hardware names for clock and reset names. That makes
it easier for integrators, and from a driver point of view it doesn't
change much as DT names are just a convention anyway.

That being said, if there's a good reason to do otherwise (such as
standardizing property names to make handling through common code
possible), that's fine too.

> I wonder about reset domains for clocks as well... is this just gate
> clock misrepresented?

No, those are real reset signals. There can be clock gates external to
the IP, and those are handled by the clock providers. The IP has three
domains of logical that are synchronous to three different clocks, and
they have one reset signal each, hence the description mentioning "clock
domain".

Krzysztof Kozlowski Nov. 6, 2024, 2:29 p.m. UTC | #3

On 06/11/2024 14:57, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Wed, Nov 06, 2024 at 01:15:23PM +0100, Krzysztof Kozlowski wrote:
>> On Wed, Nov 06, 2024 at 10:05:22AM +0000, Daniel Scally wrote:
>>> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
>>>
>>> Acked-by: Nayden Kanchev <[e-mail]>
>>> Co-developed-by: Jacopo Mondi <[e-mail]>
>>> Signed-off-by: Jacopo Mondi <[e-mail]>
>>> Signed-off-by: Daniel Scally <[e-mail]>
>>> ---
>>> Changes in v8:
>>>
>>> 	- Added the video clock back in. Now that we have actual hardware it's
>>> 	  clear that it's necessary.
>>> 	- Added reset lines 
>>> 	- Dropped R-bs
>>
>> These are trivial, so I wish you kept the review... but since you ask,
>> then comment further
>>
>> I recommend using b4, so your cover letter changelog comes with nice
>> links to previous versions. I scrolled through entire cover letter for
>> this (for me that's almost the only point of cover letter) and could
>> not find them. Anyway, just a remark.
>>
>>
>> ...
>>
>>> +  resets:
>>> +    items:
>>> +      - description: vclk domain reset
>>> +      - description: aclk domain reset
>>> +      - description: hclk domain reset
>>> +
>>> +  reset-names:
>>> +    items:
>>> +      - const: vresetn
>>
>> drop "reset", it's redundant and rather come here with logical name. I
>> wonder what "n" means as well. It's not a GPIO to be "inverted"...
> 
> The aresetn and hresetn names come directly from a hardware manual
> (vresetn seems to be called rstn in that document though). As far as I
> understand, they are the names of the external signals of the IP core.
> I tend to pick the hardware names for clock and reset names. That makes
> it easier for integrators, and from a driver point of view it doesn't
> change much as DT names are just a convention anyway.
> 
> That being said, if there's a good reason to do otherwise (such as
> standardizing property names to make handling through common code
> possible), that's fine too.

If these are from manual then it is fine, although sometimes the names
are really pointless in manual...

Reviewed-by: Krzysztof Kozlowski <[e-mail]>

Best regards,
Krzysztof

Dan Scally Nov. 6, 2024, 4:38 p.m. UTC | #4

Hi Krzysztof - thanks for reviewing

On 06/11/2024 14:29, Krzysztof Kozlowski wrote:
> On 06/11/2024 14:57, Laurent Pinchart wrote:
>> Hi Krzysztof,
>>
>> On Wed, Nov 06, 2024 at 01:15:23PM +0100, Krzysztof Kozlowski wrote:
>>> On Wed, Nov 06, 2024 at 10:05:22AM +0000, Daniel Scally wrote:
>>>> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
>>>>
>>>> Acked-by: Nayden Kanchev <[e-mail]>
>>>> Co-developed-by: Jacopo Mondi <[e-mail]>
>>>> Signed-off-by: Jacopo Mondi <[e-mail]>
>>>> Signed-off-by: Daniel Scally <[e-mail]>
>>>> ---
>>>> Changes in v8:
>>>>
>>>> 	- Added the video clock back in. Now that we have actual hardware it's
>>>> 	  clear that it's necessary.
>>>> 	- Added reset lines
>>>> 	- Dropped R-bs
>>> These are trivial, so I wish you kept the review...

Fair enough, I just didn't want to be presumptuous


>>> but since you ask,
>>> then comment further
>>>
>>> I recommend using b4, so your cover letter changelog comes with nice
>>> links to previous versions. I scrolled through entire cover letter for
>>> this (for me that's almost the only point of cover letter) and could
>>> not find them. Anyway, just a remark.


Thanks for the recommendation - I'll take a look when I get time, but either way I can add a link to 
the previous versions next time.

>>>
>>>
>>> ...
>>>
>>>> +  resets:
>>>> +    items:
>>>> +      - description: vclk domain reset
>>>> +      - description: aclk domain reset
>>>> +      - description: hclk domain reset
>>>> +
>>>> +  reset-names:
>>>> +    items:
>>>> +      - const: vresetn
>>> drop "reset", it's redundant and rather come here with logical name. I
>>> wonder what "n" means as well. It's not a GPIO to be "inverted"...
>> The aresetn and hresetn names come directly from a hardware manual
>> (vresetn seems to be called rstn in that document though). As far as I
>> understand, they are the names of the external signals of the IP core.
>> I tend to pick the hardware names for clock and reset names. That makes
>> it easier for integrators, and from a driver point of view it doesn't
>> change much as DT names are just a convention anyway.
>>
>> That being said, if there's a good reason to do otherwise (such as
>> standardizing property names to make handling through common code
>> possible), that's fine too.
> If these are from manual then it is fine, although sometimes the names
> are really pointless in manual...
>
> Reviewed-by: Krzysztof Kozlowski <[e-mail]>
Thanks
>
> Best regards,
> Krzysztof
>

Laurent Pinchart Nov. 6, 2024, 9:54 p.m. UTC | #5

Hi Dan,

Thank you for the patch.

On Wed, Nov 06, 2024 at 10:05:22AM +0000, Daniel Scally wrote:
> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> 
> Acked-by: Nayden Kanchev <[e-mail]>
> Co-developed-by: Jacopo Mondi <[e-mail]>
> Signed-off-by: Jacopo Mondi <[e-mail]>
> Signed-off-by: Daniel Scally <[e-mail]>
> ---
> Changes in v8:
> 
> 	- Added the video clock back in. Now that we have actual hardware it's
> 	  clear that it's necessary.
> 	- Added reset lines 
> 	- Dropped R-bs
> 
> Changes in v7:
> 
> 	- None
> 
> Changes in v6:
> 
> 	- None
> 
> Changes in v5:
> 
> 	- None
> 
> Changes in v4:
> 
> 	- Switched to port instead of ports
> 
> Changes in v3:
> 
> 	- Dropped the video clock as suggested by Laurent. I didn't retain it
> 	for the purposes of the refcount since this driver will call .s_stream()
> 	for the sensor driver which will refcount the clock anyway.
> 	- Clarified that the port is a parallel input port rather (Sakari)
> 
> Changes in v2:
> 
> 	- Added clocks information
> 	- Fixed the warnings raised by Rob
> 
>  .../bindings/media/arm,mali-c55.yaml          | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/arm,mali-c55.yaml b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> new file mode 100644
> index 000000000000..efc88fd2c447
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/arm,mali-c55.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM Mali-C55 Image Signal Processor
> +
> +maintainers:
> +  - Daniel Scally <[e-mail]>
> +  - Jacopo Mondi <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: arm,mali-c55

I have a feeling we may need to add an SoC-specific compatible string to
support platform-specific integration quirks. Let's see if we will have
a use for that in the RZ/V2H. It can also be addressed later if needed.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: ISP Video Clock
> +      - description: ISP AXI clock
> +      - description: ISP AHB-lite clock
> +
> +  clock-names:
> +    items:
> +      - const: vclk
> +      - const: aclk
> +      - const: hclk
> +
> +  resets:
> +    items:
> +      - description: vclk domain reset
> +      - description: aclk domain reset
> +      - description: hclk domain reset
> +
> +  reset-names:
> +    items:
> +      - const: vresetn
> +      - const: aresetn
> +      - const: hresetn
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    description: Input parallel video bus

Doesn't the ISP have two input ports, for two exposures ? Depending on
the SoC integration, could they be connected to two different devices,
with the two connections having to be modelled in DT ? Looking at the
changelog you've switched from 'ports' to 'port' in v4, but I think we
should either have two ports right away, or be ready for a second port
later if SoC integrations requires that. That could be a good enough
reason to use 'ports' right away, even with a single port.

> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/graph.yaml#/properties/endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mali_c55: isp@400000 {
> +      compatible = "arm,mali-c55";
> +      reg = <0x400000 0x200000>;
> +      clocks = <&clk 0>, <&clk 1>, <&clk 2>;
> +      clock-names = "vclk", "aclk", "hclk";
> +      resets = <&resets 0>, <&resets 1>, <&resets 2>;
> +      reset-names = "vresetn", "aresetn", "hresetn";
> +      interrupts = <0>;
> +
> +      port {
> +        isp_in: endpoint {
> +            remote-endpoint = <&csi2_rx_out>;
> +        };
> +      };
> +    };
> +...

Patch

diff --git a/Documentation/devicetree/bindings/media/arm,mali-c55.yaml b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
new file mode 100644
index 000000000000..efc88fd2c447
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
@@ -0,0 +1,82 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/arm,mali-c55.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM Mali-C55 Image Signal Processor
+
+maintainers:
+  - Daniel Scally <[e-mail]>
+  - Jacopo Mondi <[e-mail]>
+
+properties:
+  compatible:
+    const: arm,mali-c55
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: ISP Video Clock
+      - description: ISP AXI clock
+      - description: ISP AHB-lite clock
+
+  clock-names:
+    items:
+      - const: vclk
+      - const: aclk
+      - const: hclk
+
+  resets:
+    items:
+      - description: vclk domain reset
+      - description: aclk domain reset
+      - description: hclk domain reset
+
+  reset-names:
+    items:
+      - const: vresetn
+      - const: aresetn
+      - const: hresetn
+
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description: Input parallel video bus
+
+    properties:
+      endpoint:
+        $ref: /schemas/graph.yaml#/properties/endpoint
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    mali_c55: isp@400000 {
+      compatible = "arm,mali-c55";
+      reg = <0x400000 0x200000>;
+      clocks = <&clk 0>, <&clk 1>, <&clk 2>;
+      clock-names = "vclk", "aclk", "hclk";
+      resets = <&resets 0>, <&resets 1>, <&resets 2>;
+      reset-names = "vresetn", "aresetn", "hresetn";
+      interrupts = <0>;
+
+      port {
+        isp_in: endpoint {
+            remote-endpoint = <&csi2_rx_out>;
+        };
+      };
+    };
+...