diff mbox series

[v2,2/5] dt-bindings: media: Add bindings for ARM mali-c55

Message ID 20240214141906.245685-3-dan.scally@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series Add Arm Mali-C55 Image Signal Processor Driver | expand

Commit Message

Dan Scally Feb. 14, 2024, 2:19 p.m. UTC
Add the yaml binding for ARM's Mali-C55 Image Signal Processor.

Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
Changes in v2:

	- Added clocks information
	- Fixed the warnings raised by Rob

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

Comments

Laurent Pinchart Feb. 14, 2024, 2:28 p.m. UTC | #1
Hi Dan,

Thank you for the patch.

On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> 
> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
> 
> 	- Added clocks information
> 	- Fixed the warnings raised by Rob
> 
>  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
>  1 file changed, 77 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..30038cfec3a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> @@ -0,0 +1,77 @@
> +# 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 <dan.scally@ideasonboard.com>
> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +
> +properties:
> +  compatible:
> +    const: arm,mali-c55
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: ISP video clock

I wonder if we need this clock. Granted, it's an input clock to the ISP,
but it's part of the input video bus. I don't expect anyone would ever
need to control it manually, it should be provided by the video source
automatically.

> +      - description: ISP AXI clock
> +      - description: ISP AHB-lite clock

These two other clocks look good to me.

> +
> +  clock-names:
> +    items:
> +      - const: vclk
> +      - const: aclk
> +      - const: hclk
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/properties/endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - ports
> +
> +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";
> +      interrupts = <0>;
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +          isp_in: endpoint {
> +              remote-endpoint = <&mipi_out>;
> +          };
> +        };
> +      };
> +    };
> +...
Conor Dooley Feb. 14, 2024, 5:37 p.m. UTC | #2
On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > 
> > Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > Changes in v2:
> > 
> > 	- Added clocks information
> > 	- Fixed the warnings raised by Rob
> > 
> >  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> >  1 file changed, 77 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..30038cfec3a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > @@ -0,0 +1,77 @@
> > +# 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 <dan.scally@ideasonboard.com>
> > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: arm,mali-c55
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: ISP video clock
> 
> I wonder if we need this clock. Granted, it's an input clock to the ISP,
> but it's part of the input video bus. I don't expect anyone would ever
> need to control it manually, it should be provided by the video source
> automatically.

I'd say that if there's a clock controller providing this clock, even if
it is implicit in the video feed it's good to have here. Being able to
increment the refcount on that clock would be good, even if you don't
actually control it manually?

> 
> > +      - description: ISP AXI clock
> > +      - description: ISP AHB-lite clock
> 
> These two other clocks look good to me.
> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: vclk
> > +      - const: aclk
> > +      - const: hclk

Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
tree clock names you've provided - they're all clocks, so having "clk"
in them is just noise :)

Cheers,
Conor.
Laurent Pinchart Feb. 15, 2024, 11:02 a.m. UTC | #3
On Wed, Feb 14, 2024 at 05:37:17PM +0000, Conor Dooley wrote:
> On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
> > On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> > > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > > 
> > > Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > ---
> > > Changes in v2:
> > > 
> > > 	- Added clocks information
> > > 	- Fixed the warnings raised by Rob
> > > 
> > >  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> > >  1 file changed, 77 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..30038cfec3a4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > > @@ -0,0 +1,77 @@
> > > +# 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 <dan.scally@ideasonboard.com>
> > > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: arm,mali-c55
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: ISP video clock
> > 
> > I wonder if we need this clock. Granted, it's an input clock to the ISP,
> > but it's part of the input video bus. I don't expect anyone would ever
> > need to control it manually, it should be provided by the video source
> > automatically.
> 
> I'd say that if there's a clock controller providing this clock, even if
> it is implicit in the video feed it's good to have here. Being able to
> increment the refcount on that clock would be good, even if you don't
> actually control it manually?

I don't expect there would be a clock controller to directly reference
in most cases. It depends a bit on where the clock domain crossing
between the source (often a CSI-2 receiver) and the ISP is. If it's
implemented in glue logic bundled with the ISP, which wouldn't be
described in DT as a separate node, then there's a higher chance we'll
have a clock controller for vclk. If it's implemented in the source,
vclk will just come from the source, which won't be listed as a clock
controller.

One option would be to make this clock optional, by moving it to the end
of the clocks list, and setting

	minItems: 2
	maxItems: 3

> > > +      - description: ISP AXI clock
> > > +      - description: ISP AHB-lite clock
> > 
> > These two other clocks look good to me.
> > 
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: vclk
> > > +      - const: aclk
> > > +      - const: hclk
> 
> Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
> tree clock names you've provided - they're all clocks, so having "clk"
> in them is just noise :)

As far as I understand, the names proposed by Dan come directly from the
IP core documentation.
Dan Scally Feb. 16, 2024, 1:09 p.m. UTC | #4
Hi both

On 15/02/2024 11:02, Laurent Pinchart wrote:
> On Wed, Feb 14, 2024 at 05:37:17PM +0000, Conor Dooley wrote:
>> On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
>>> On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
>>>> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
>>>>
>>>> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>> ---
>>>> Changes in v2:
>>>>
>>>> 	- Added clocks information
>>>> 	- Fixed the warnings raised by Rob
>>>>
>>>>   .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
>>>>   1 file changed, 77 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..30038cfec3a4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
>>>> @@ -0,0 +1,77 @@
>>>> +# 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 <dan.scally@ideasonboard.com>
>>>> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: arm,mali-c55
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: ISP video clock
>>> I wonder if we need this clock. Granted, it's an input clock to the ISP,
>>> but it's part of the input video bus. I don't expect anyone would ever
>>> need to control it manually, it should be provided by the video source
>>> automatically.
>> I'd say that if there's a clock controller providing this clock, even if
>> it is implicit in the video feed it's good to have here. Being able to
>> increment the refcount on that clock would be good, even if you don't
>> actually control it manually?
> I don't expect there would be a clock controller to directly reference
> in most cases. It depends a bit on where the clock domain crossing
> between the source (often a CSI-2 receiver) and the ISP is. If it's
> implemented in glue logic bundled with the ISP, which wouldn't be
> described in DT as a separate node, then there's a higher chance we'll
> have a clock controller for vclk. If it's implemented in the source,
> vclk will just come from the source, which won't be listed as a clock
> controller.
>
> One option would be to make this clock optional, by moving it to the end
> of the clocks list, and setting
>
> 	minItems: 2
> 	maxItems: 3
>
>>>> +      - description: ISP AXI clock
>>>> +      - description: ISP AHB-lite clock
>>> These two other clocks look good to me.
>>>
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: vclk
>>>> +      - const: aclk
>>>> +      - const: hclk
>> Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
>> tree clock names you've provided - they're all clocks, so having "clk"
>> in them is just noise :)
> As far as I understand, the names proposed by Dan come directly from the
> IP core documentation.


This is the case, but I do take Conor's point that more descriptive names might be nicer - if I'm 
honest I just didn't think about it particularly given "Xclk" is such a common name for them 
already, but having been poked into thinking about it I do agree.

>
Laurent Pinchart Feb. 16, 2024, 1:27 p.m. UTC | #5
On Fri, Feb 16, 2024 at 01:09:15PM +0000, Daniel Scally wrote:
> On 15/02/2024 11:02, Laurent Pinchart wrote:
> > On Wed, Feb 14, 2024 at 05:37:17PM +0000, Conor Dooley wrote:
> >> On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
> >>> On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> >>>> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> >>>>
> >>>> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >>>> ---
> >>>> Changes in v2:
> >>>>
> >>>> 	- Added clocks information
> >>>> 	- Fixed the warnings raised by Rob
> >>>>
> >>>>   .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> >>>>   1 file changed, 77 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..30038cfec3a4
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> >>>> @@ -0,0 +1,77 @@
> >>>> +# 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 <dan.scally@ideasonboard.com>
> >>>> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>>> +
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    const: arm,mali-c55
> >>>> +
> >>>> +  reg:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  interrupts:
> >>>> +    maxItems: 1
> >>>> +
> >>>> +  clocks:
> >>>> +    items:
> >>>> +      - description: ISP video clock
> >>>
> >>> I wonder if we need this clock. Granted, it's an input clock to the ISP,
> >>> but it's part of the input video bus. I don't expect anyone would ever
> >>> need to control it manually, it should be provided by the video source
> >>> automatically.
> >>
> >> I'd say that if there's a clock controller providing this clock, even if
> >> it is implicit in the video feed it's good to have here. Being able to
> >> increment the refcount on that clock would be good, even if you don't
> >> actually control it manually?
> >
> > I don't expect there would be a clock controller to directly reference
> > in most cases. It depends a bit on where the clock domain crossing
> > between the source (often a CSI-2 receiver) and the ISP is. If it's
> > implemented in glue logic bundled with the ISP, which wouldn't be
> > described in DT as a separate node, then there's a higher chance we'll
> > have a clock controller for vclk. If it's implemented in the source,
> > vclk will just come from the source, which won't be listed as a clock
> > controller.
> >
> > One option would be to make this clock optional, by moving it to the end
> > of the clocks list, and setting
> >
> > 	minItems: 2
> > 	maxItems: 3
> >
> >>>> +      - description: ISP AXI clock
> >>>> +      - description: ISP AHB-lite clock
> >>>
> >>> These two other clocks look good to me.
> >>>
> >>>> +
> >>>> +  clock-names:
> >>>> +    items:
> >>>> +      - const: vclk
> >>>> +      - const: aclk
> >>>> +      - const: hclk
> >>
> >> Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
> >> tree clock names you've provided - they're all clocks, so having "clk"
> >> in them is just noise :)
> >
> > As far as I understand, the names proposed by Dan come directly from the
> > IP core documentation.
> 
> This is the case, but I do take Conor's point that more descriptive names might be nicer - if I'm 
> honest I just didn't think about it particularly given "Xclk" is such a common name for them 
> already, but having been poked into thinking about it I do agree.

Isn't the usual practice in DT bindings is to name GPIOs, clocks and reset
signals based on the hardware documentation ?
Dan Scally Feb. 16, 2024, 2:45 p.m. UTC | #6
On 16/02/2024 13:27, Laurent Pinchart wrote:
> On Fri, Feb 16, 2024 at 01:09:15PM +0000, Daniel Scally wrote:
>> On 15/02/2024 11:02, Laurent Pinchart wrote:
>>> On Wed, Feb 14, 2024 at 05:37:17PM +0000, Conor Dooley wrote:
>>>> On Wed, Feb 14, 2024 at 04:28:25PM +0200, Laurent Pinchart wrote:
>>>>> On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
>>>>>> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
>>>>>>
>>>>>> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
>>>>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>>
>>>>>> 	- Added clocks information
>>>>>> 	- Fixed the warnings raised by Rob
>>>>>>
>>>>>>    .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
>>>>>>    1 file changed, 77 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..30038cfec3a4
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
>>>>>> @@ -0,0 +1,77 @@
>>>>>> +# 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 <dan.scally@ideasonboard.com>
>>>>>> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: arm,mali-c55
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  interrupts:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  clocks:
>>>>>> +    items:
>>>>>> +      - description: ISP video clock
>>>>> I wonder if we need this clock. Granted, it's an input clock to the ISP,
>>>>> but it's part of the input video bus. I don't expect anyone would ever
>>>>> need to control it manually, it should be provided by the video source
>>>>> automatically.
>>>> I'd say that if there's a clock controller providing this clock, even if
>>>> it is implicit in the video feed it's good to have here. Being able to
>>>> increment the refcount on that clock would be good, even if you don't
>>>> actually control it manually?
>>> I don't expect there would be a clock controller to directly reference
>>> in most cases. It depends a bit on where the clock domain crossing
>>> between the source (often a CSI-2 receiver) and the ISP is. If it's
>>> implemented in glue logic bundled with the ISP, which wouldn't be
>>> described in DT as a separate node, then there's a higher chance we'll
>>> have a clock controller for vclk. If it's implemented in the source,
>>> vclk will just come from the source, which won't be listed as a clock
>>> controller.
>>>
>>> One option would be to make this clock optional, by moving it to the end
>>> of the clocks list, and setting
>>>
>>> 	minItems: 2
>>> 	maxItems: 3
>>>
>>>>>> +      - description: ISP AXI clock
>>>>>> +      - description: ISP AHB-lite clock
>>>>> These two other clocks look good to me.
>>>>>
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    items:
>>>>>> +      - const: vclk
>>>>>> +      - const: aclk
>>>>>> +      - const: hclk
>>>> Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
>>>> tree clock names you've provided - they're all clocks, so having "clk"
>>>> in them is just noise :)
>>> As far as I understand, the names proposed by Dan come directly from the
>>> IP core documentation.
>> This is the case, but I do take Conor's point that more descriptive names might be nicer - if I'm
>> honest I just didn't think about it particularly given "Xclk" is such a common name for them
>> already, but having been poked into thinking about it I do agree.
> Isn't the usual practice in DT bindings is to name GPIOs, clocks and reset
> signals based on the hardware documentation ?


Ah - I don't know honestly. If that's so then yeah - these are the names the documentation prescribes.

>
Conor Dooley Feb. 16, 2024, 7:07 p.m. UTC | #7
On Fri, Feb 16, 2024 at 02:45:31PM +0000, Dan Scally wrote:

> > > > > > > +      - description: ISP AXI clock
> > > > > > > +      - description: ISP AHB-lite clock
> > > > > > These two other clocks look good to me.
> > > > > > 
> > > > > > > +
> > > > > > > +  clock-names:
> > > > > > > +    items:
> > > > > > > +      - const: vclk
> > > > > > > +      - const: aclk
> > > > > > > +      - const: hclk
> > > > > Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
> > > > > tree clock names you've provided - they're all clocks, so having "clk"
> > > > > in them is just noise :)
> > > > As far as I understand, the names proposed by Dan come directly from the
> > > > IP core documentation.
> > > This is the case, but I do take Conor's point that more descriptive names might be nicer - if I'm
> > > honest I just didn't think about it particularly given "Xclk" is such a common name for them
> > > already, but having been poked into thinking about it I do agree.
> > Isn't the usual practice in DT bindings is to name GPIOs, clocks and reset
> > signals based on the hardware documentation ?
> 
> 
> Ah - I don't know honestly. If that's so then yeah - these are the names the documentation prescribes.

If a direct doc match is what you're going for, then sure, keep it.
Rob Herring (Arm) Feb. 22, 2024, 6:07 p.m. UTC | #8
On Fri, Feb 16, 2024 at 07:07:58PM +0000, Conor Dooley wrote:
> On Fri, Feb 16, 2024 at 02:45:31PM +0000, Dan Scally wrote:
> 
> > > > > > > > +      - description: ISP AXI clock
> > > > > > > > +      - description: ISP AHB-lite clock
> > > > > > > These two other clocks look good to me.
> > > > > > > 
> > > > > > > > +
> > > > > > > > +  clock-names:
> > > > > > > > +    items:
> > > > > > > > +      - const: vclk
> > > > > > > > +      - const: aclk
> > > > > > > > +      - const: hclk
> > > > > > Why not "video" "axi" "ahb-lite"? There's 3 useful letters between the
> > > > > > tree clock names you've provided - they're all clocks, so having "clk"
> > > > > > in them is just noise :)
> > > > > As far as I understand, the names proposed by Dan come directly from the
> > > > > IP core documentation.
> > > > This is the case, but I do take Conor's point that more descriptive names might be nicer - if I'm
> > > > honest I just didn't think about it particularly given "Xclk" is such a common name for them
> > > > already, but having been poked into thinking about it I do agree.
> > > Isn't the usual practice in DT bindings is to name GPIOs, clocks and reset
> > > signals based on the hardware documentation ?
> > 
> > 
> > Ah - I don't know honestly. If that's so then yeah - these are the names the documentation prescribes.
> 
> If a direct doc match is what you're going for, then sure, keep it.

pclk, aclk, and hclk are generally the names used for APB, AXI, and AHB 
bus clocks, so I'd stick with them. Though we also have cases of the bus 
names used... 

Rob
Sakari Ailus Feb. 26, 2024, 11:54 a.m. UTC | #9
Hi Dan,

On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> 
> Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
> 
> 	- Added clocks information
> 	- Fixed the warnings raised by Rob
> 
>  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
>  1 file changed, 77 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..30038cfec3a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> @@ -0,0 +1,77 @@
> +# 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 <dan.scally@ideasonboard.com>
> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +
> +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
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/properties/endpoint
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - ports
> +
> +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";
> +      interrupts = <0>;
> +
> +      ports {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          reg = <0>;
> +          isp_in: endpoint {
> +              remote-endpoint = <&mipi_out>;

I suppose this is a CSI-2 interface with D-PHY?

How many lanes do you have and is lane remapping / polarity inversion
supported?

This should be reflected in bindings.

> +          };
> +        };
> +      };
> +    };
> +...
Laurent Pinchart Feb. 26, 2024, 12:04 p.m. UTC | #10
Hi Sakari,

On Mon, Feb 26, 2024 at 11:54:22AM +0000, Sakari Ailus wrote:
> On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > 
> > Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> > Changes in v2:
> > 
> > 	- Added clocks information
> > 	- Fixed the warnings raised by Rob
> > 
> >  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> >  1 file changed, 77 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..30038cfec3a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > @@ -0,0 +1,77 @@
> > +# 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 <dan.scally@ideasonboard.com>
> > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +
> > +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
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +
> > +        properties:
> > +          endpoint:
> > +            $ref: /schemas/graph.yaml#/properties/endpoint
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +  - ports
> > +
> > +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";
> > +      interrupts = <0>;
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 {
> > +          reg = <0>;
> > +          isp_in: endpoint {
> > +              remote-endpoint = <&mipi_out>;
> 
> I suppose this is a CSI-2 interface with D-PHY?

No, that's an internal parallel bus. Depending on the SoC integration,
it can be connected to a CSI-2 receiver, a DMA engine, or a mux to
select between different sources.

> How many lanes do you have and is lane remapping / polarity inversion
> supported?
> 
> This should be reflected in bindings.
> 
> > +          };
> > +        };
> > +      };
> > +    };
> > +...
Sakari Ailus Feb. 26, 2024, 12:20 p.m. UTC | #11
Hi Laurent,

On Mon, Feb 26, 2024 at 02:04:31PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Mon, Feb 26, 2024 at 11:54:22AM +0000, Sakari Ailus wrote:
> > On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> > > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > > 
> > > Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > ---
> > > Changes in v2:
> > > 
> > > 	- Added clocks information
> > > 	- Fixed the warnings raised by Rob
> > > 
> > >  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> > >  1 file changed, 77 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..30038cfec3a4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > > @@ -0,0 +1,77 @@
> > > +# 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 <dan.scally@ideasonboard.com>
> > > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > +
> > > +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
> > > +
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +
> > > +        properties:
> > > +          endpoint:
> > > +            $ref: /schemas/graph.yaml#/properties/endpoint
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - clocks
> > > +  - clock-names
> > > +  - ports
> > > +
> > > +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";
> > > +      interrupts = <0>;
> > > +
> > > +      ports {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        port@0 {
> > > +          reg = <0>;
> > > +          isp_in: endpoint {
> > > +              remote-endpoint = <&mipi_out>;
> > 
> > I suppose this is a CSI-2 interface with D-PHY?
> 
> No, that's an internal parallel bus. Depending on the SoC integration,
> it can be connected to a CSI-2 receiver, a DMA engine, or a mux to
> select between different sources.

The name suggests otherwise. Maybe change that to something more
descriptive?

> 
> > How many lanes do you have and is lane remapping / polarity inversion
> > supported?
> > 
> > This should be reflected in bindings.
> > 
> > > +          };
> > > +        };
> > > +      };
> > > +    };
> > > +...
>
Laurent Pinchart Feb. 26, 2024, 12:58 p.m. UTC | #12
On Mon, Feb 26, 2024 at 12:20:15PM +0000, Sakari Ailus wrote:
> On Mon, Feb 26, 2024 at 02:04:31PM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 26, 2024 at 11:54:22AM +0000, Sakari Ailus wrote:
> > > On Wed, Feb 14, 2024 at 02:19:03PM +0000, Daniel Scally wrote:
> > > > Add the yaml binding for ARM's Mali-C55 Image Signal Processor.
> > > > 
> > > > Acked-by: Nayden Kanchev <nayden.kanchev@arm.com>
> > > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > > ---
> > > > Changes in v2:
> > > > 
> > > > 	- Added clocks information
> > > > 	- Fixed the warnings raised by Rob
> > > > 
> > > >  .../bindings/media/arm,mali-c55.yaml          | 77 +++++++++++++++++++
> > > >  1 file changed, 77 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..30038cfec3a4
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
> > > > @@ -0,0 +1,77 @@
> > > > +# 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 <dan.scally@ideasonboard.com>
> > > > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > +
> > > > +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
> > > > +
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/properties/port

Adding

        description: Input parallel video bus

here would be useful.

> > > > +
> > > > +        properties:
> > > > +          endpoint:
> > > > +            $ref: /schemas/graph.yaml#/properties/endpoint
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - interrupts
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - ports
> > > > +
> > > > +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";
> > > > +      interrupts = <0>;
> > > > +
> > > > +      ports {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        port@0 {
> > > > +          reg = <0>;
> > > > +          isp_in: endpoint {
> > > > +              remote-endpoint = <&mipi_out>;
> > > 
> > > I suppose this is a CSI-2 interface with D-PHY?
> > 
> > No, that's an internal parallel bus. Depending on the SoC integration,
> > it can be connected to a CSI-2 receiver, a DMA engine, or a mux to
> > select between different sources.
> 
> The name suggests otherwise. Maybe change that to something more
> descriptive?

We could rename mipi_out to csi2_rx_out, sure.

> > > How many lanes do you have and is lane remapping / polarity inversion
> > > supported?
> > > 
> > > This should be reflected in bindings.
> > > 
> > > > +          };
> > > > +        };
> > > > +      };
> > > > +    };
> > > > +...
Sakari Ailus Feb. 26, 2024, 1:37 p.m. UTC | #13
Hi Laurent,

On Mon, Feb 26, 2024 at 02:58:18PM +0200, Laurent Pinchart wrote:
> > > > > +              remote-endpoint = <&mipi_out>;
> > > > 
> > > > I suppose this is a CSI-2 interface with D-PHY?
> > > 
> > > No, that's an internal parallel bus. Depending on the SoC integration,
> > > it can be connected to a CSI-2 receiver, a DMA engine, or a mux to
> > > select between different sources.
> > 
> > The name suggests otherwise. Maybe change that to something more
> > descriptive?
> 
> We could rename mipi_out to csi2_rx_out, sure.

Sounds good to me.
Dan Scally Feb. 26, 2024, 1:42 p.m. UTC | #14
Hi Sakari - thanks for reviews!

On 26/02/2024 13:37, Sakari Ailus wrote:
> Hi Laurent,
>
> On Mon, Feb 26, 2024 at 02:58:18PM +0200, Laurent Pinchart wrote:
>>>>>> +              remote-endpoint = <&mipi_out>;
>>>>> I suppose this is a CSI-2 interface with D-PHY?
>>>> No, that's an internal parallel bus. Depending on the SoC integration,
>>>> it can be connected to a CSI-2 receiver, a DMA engine, or a mux to
>>>> select between different sources.
>>> The name suggests otherwise. Maybe change that to something more
>>> descriptive?
>> We could rename mipi_out to csi2_rx_out, sure.
> Sounds good to me.
>
OK, will do - and likewise the description you suggested above.
diff mbox series

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..30038cfec3a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/arm,mali-c55.yaml
@@ -0,0 +1,77 @@ 
+# 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 <dan.scally@ideasonboard.com>
+  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+
+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
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+
+        properties:
+          endpoint:
+            $ref: /schemas/graph.yaml#/properties/endpoint
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - ports
+
+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";
+      interrupts = <0>;
+
+      ports {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        port@0 {
+          reg = <0>;
+          isp_in: endpoint {
+              remote-endpoint = <&mipi_out>;
+          };
+        };
+      };
+    };
+...