| 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 |
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>; > + }; > + }; > + }; > + }; > +...
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.
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.
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. >
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 ?
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. >
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.
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
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. > + }; > + }; > + }; > + }; > +...
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. > > > + }; > > + }; > > + }; > > + }; > > +...
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. > > > > > + }; > > > + }; > > > + }; > > > + }; > > > +... >
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. > > > > > > > + }; > > > > + }; > > > > + }; > > > > + }; > > > > +...
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.
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 --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>; + }; + }; + }; + }; +...