[PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Jacopo Mondi]

Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
signal processor.

Datasheet:
https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../bindings/media/raspberrypi,pispbe.yaml    | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml

diff --git a/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
new file mode 100644
index 000000000000..50111b87ad81
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/raspberrypi,pispbe.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
+
+maintainers:
+  - Dave Stevenson <dave.stevenson@raspberrypi.com>
+  - David Plowman <david.plowman@raspberrypi.com>
+  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+  - Naushir Patuck <naush@raspberrypi.com>
+  - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
+
+description: |
+  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
+  processor that fetches images in Bayer or Grayscale format from DRAM memory
+  in tiles and produces images consumable by applications.
+
+  The full ISP documentation is available at:
+  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
+
+properties:
+  compatible:
+    const: raspberrypi,pispbe
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: isp_be
+
+  iommus:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    rpi1 {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        isp: pisp_be@880000  {
+             compatible = "raspberrypi,pispbe";
+             reg = <0x10 0x00880000  0x0 0x4000>;
+             interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
+             clocks = <&firmware_clocks 7>;
+             clock-names = "isp_be";
+             iommus = <&iommu2>;
+        };
+    };
--
2.43.0

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Krzysztof Kozlowski]

On 09/02/2024 17:48, Jacopo Mondi wrote:
> Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> signal processor.
> 
> Datasheet:
> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> 


> +---
> +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> +
> +maintainers:
> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> +  - David Plowman <david.plowman@raspberrypi.com>
> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +  - Naushir Patuck <naush@raspberrypi.com>
> +  - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

I assume all folks are fine being here?

> +
> +description: |
> +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> +  in tiles and produces images consumable by applications.
> +
> +  The full ISP documentation is available at:
> +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> +
> +properties:
> +  compatible:
> +    const: raspberrypi,pispbe

Nothing more specific? No model name, no version? It's quite generic
compatible which in general should not be allowed. I would assume that
at least version of Pi could denote some sort of a model... unless
version is detectable?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: isp_be

You can skip clock-names if they have only one entry and it matches
block name. Quite useless.

> +
> +  iommus:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    rpi1 {

soc {

> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        isp: pisp_be@880000  {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
so: isp@

and drop unused label

> +             compatible = "raspberrypi,pispbe";
ds,
Krzysztof

Re: Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Jacopo Mondi]

Hi Krzysztof

On Mon, Feb 12, 2024 at 09:12:11AM +0100, Krzysztof Kozlowski wrote:
> On 09/02/2024 17:48, Jacopo Mondi wrote:
> > Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> > signal processor.
> >
> > Datasheet:
> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> >
>
>
> > +---
> > +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> > +
> > +maintainers:
> > +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> > +  - David Plowman <david.plowman@raspberrypi.com>
> > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +  - Naushir Patuck <naush@raspberrypi.com>
> > +  - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
>
> I assume all folks are fine being here?
>

I admint I haven't ask any single one of them, and I listed all of
them thinking of "maintainers of PiSP", but when it comes to bindings
only we can shorten the list if preferred ?

> > +
> > +description: |
> > +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> > +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> > +  in tiles and produces images consumable by applications.
> > +
> > +  The full ISP documentation is available at:
> > +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: raspberrypi,pispbe
>
> Nothing more specific? No model name, no version? It's quite generic
> compatible which in general should not be allowed. I would assume that
> at least version of Pi could denote some sort of a model... unless
> version is detectable?
>

The driver matches on a version register and that should be enough to
handle quirks which are specific to an IP revision in the driver
itself.

Considering how minimal the integration with the SoC is (one clock, one
interrupt and one optional iommu reference) even if we'll get future
revisions of the SoC I don't think there will be any need to match on
a dedicated compatible for bindings-validation purposes.

However I understand that to be future-proof it's good practice to
allow a more flexible scheme, so we can have a generic fallback and a
revision-specific entry.

Would

  compatible:
    items:
      - enum:
        - raspberrypi,pipspbe-bcm2712
      - const: raspberrypi,pispbe

do in this case ?

Also, let's see what RPi think as they are certainly more informed
than me on what a good revision-specific match could be

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: isp_be
>
> You can skip clock-names if they have only one entry and it matches
> block name. Quite useless.
>

ack

> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    rpi1 {
>
> soc {
>

Are you sure ? This will only ever live in the 'rp1' node.

> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        isp: pisp_be@880000  {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> so: isp@
>
> and drop unused label

ok

Thanks
  j

PS:
on v6.8-rc1 I'm seeing

LINT    Documentation/devicetree/bindings
usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA] [--list-files]
                [-f {parsable,standard,colored,github,auto}] [-s] [--no-warnings] [-v]
                [FILE_OR_DIR ...]

when running dt_binding_check

My setup should be reasonably up-to-date, is it me only seeing this ?


>
> > +             compatible = "raspberrypi,pispbe";
> ds,
> Krzysztof
>

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Krzysztof Kozlowski]

On 12/02/2024 09:50, Jacopo Mondi wrote:

>>> +properties:
>>> +  compatible:
>>> +    const: raspberrypi,pispbe
>>
>> Nothing more specific? No model name, no version? It's quite generic
>> compatible which in general should not be allowed. I would assume that
>> at least version of Pi could denote some sort of a model... unless
>> version is detectable?
>>
> 
> The driver matches on a version register and that should be enough to
> handle quirks which are specific to an IP revision in the driver
> itself.
> 
> Considering how minimal the integration with the SoC is (one clock, one
> interrupt and one optional iommu reference) even if we'll get future
> revisions of the SoC I don't think there will be any need to match on
> a dedicated compatible for bindings-validation purposes.
> 
> However I understand that to be future-proof it's good practice to
> allow a more flexible scheme, so we can have a generic fallback and a
> revision-specific entry.
> 
> Would
> 
>   compatible:
>     items:
>       - enum:
>         - raspberrypi,pipspbe-bcm2712

bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
rather Pi model?

>       - const: raspberrypi,pispbe
> 
> do in this case ?
> 
> Also, let's see what RPi think as they are certainly more informed
> than me on what a good revision-specific match could be

I am fine with auto-detection, though.

...

>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    rpi1 {
>>
>> soc {
>>
> 
> Are you sure ? This will only ever live in the 'rp1' node.

What is "rp1" node? Does not look like a generic name.


> 
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +
>>> +        isp: pisp_be@880000  {
>>
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>> so: isp@
>>
>> and drop unused label
> 
> ok
> 
> Thanks
>   j
> 
> PS:
> on v6.8-rc1 I'm seeing
> 
> LINT    Documentation/devicetree/bindings
> usage: yamllint [-h] [-] [-c CONFIG_FILE | -d CONFIG_DATA] [--list-files]
>                 [-f {parsable,standard,colored,github,auto}] [-s] [--no-warnings] [-v]
>                 [FILE_OR_DIR ...]
> 
> when running dt_binding_check
> 
> My setup should be reasonably up-to-date, is it me only seeing this ?

I think you need to update your yamllint.

Best regards,
Krzysztof

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Tomi Valkeinen]

Hi,

On 12/02/2024 11:05, Krzysztof Kozlowski wrote:
> On 12/02/2024 09:50, Jacopo Mondi wrote:
> 
>>>> +properties:
>>>> +  compatible:
>>>> +    const: raspberrypi,pispbe
>>>
>>> Nothing more specific? No model name, no version? It's quite generic
>>> compatible which in general should not be allowed. I would assume that
>>> at least version of Pi could denote some sort of a model... unless
>>> version is detectable?
>>>
>>
>> The driver matches on a version register and that should be enough to
>> handle quirks which are specific to an IP revision in the driver
>> itself.
>>
>> Considering how minimal the integration with the SoC is (one clock, one
>> interrupt and one optional iommu reference) even if we'll get future
>> revisions of the SoC I don't think there will be any need to match on
>> a dedicated compatible for bindings-validation purposes.
>>
>> However I understand that to be future-proof it's good practice to
>> allow a more flexible scheme, so we can have a generic fallback and a
>> revision-specific entry.
>>
>> Would
>>
>>    compatible:
>>      items:
>>        - enum:
>>          - raspberrypi,pipspbe-bcm2712
> 
> bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
> rather Pi model?

Indeed, this is something I don't get. If the BE is in the bcm2712, is 
it not a broadcom IP? Why is raspberrypi in the compatible name at all?

Naush, Dave?

>>        - const: raspberrypi,pispbe
>>
>> do in this case ?
>>
>> Also, let's see what RPi think as they are certainly more informed
>> than me on what a good revision-specific match could be
> 
> I am fine with auto-detection, though.
> 
> ...
> 
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> +
>>>> +    rpi1 {
>>>
>>> soc {
>>>
>>
>> Are you sure ? This will only ever live in the 'rp1' node.
> 
> What is "rp1" node? Does not look like a generic name.

I don't think this is right. RP1 is a separate chip, an IO controller, 
on raspberrypi 5. BE is not in the RP1.

  Tomi

Re: Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Jacopo Mondi]

Hi Tomi

On Tue, Feb 13, 2024 at 09:28:33AM +0200, Tomi Valkeinen wrote:
> Hi,
>
> On 12/02/2024 11:05, Krzysztof Kozlowski wrote:
> > On 12/02/2024 09:50, Jacopo Mondi wrote:
> >
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: raspberrypi,pispbe
> > > >
> > > > Nothing more specific? No model name, no version? It's quite generic
> > > > compatible which in general should not be allowed. I would assume that
> > > > at least version of Pi could denote some sort of a model... unless
> > > > version is detectable?
> > > >
> > >
> > > The driver matches on a version register and that should be enough to
> > > handle quirks which are specific to an IP revision in the driver
> > > itself.
> > >
> > > Considering how minimal the integration with the SoC is (one clock, one
> > > interrupt and one optional iommu reference) even if we'll get future
> > > revisions of the SoC I don't think there will be any need to match on
> > > a dedicated compatible for bindings-validation purposes.
> > >
> > > However I understand that to be future-proof it's good practice to
> > > allow a more flexible scheme, so we can have a generic fallback and a
> > > revision-specific entry.
> > >
> > > Would
> > >
> > >    compatible:
> > >      items:
> > >        - enum:
> > >          - raspberrypi,pipspbe-bcm2712
> >
> > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
> > rather Pi model?
>
> Indeed, this is something I don't get. If the BE is in the bcm2712, is it
> not a broadcom IP? Why is raspberrypi in the compatible name at all?
>
> Naush, Dave?
>
> > >        - const: raspberrypi,pispbe
> > >
> > > do in this case ?
> > >
> > > Also, let's see what RPi think as they are certainly more informed
> > > than me on what a good revision-specific match could be
> >
> > I am fine with auto-detection, though.
> >
> > ...
> >
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > > > +
> > > > > +    rpi1 {
> > > >
> > > > soc {
> > > >
> > >
> > > Are you sure ? This will only ever live in the 'rp1' node.
> >
> > What is "rp1" node? Does not look like a generic name.
>
> I don't think this is right. RP1 is a separate chip, an IO controller, on
> raspberrypi 5. BE is not in the RP1.
>

Ah yes indeed, bad copy and paste from me. I'll s/rpi/soc as suggested
by Krzysztof

>  Tomi
>
>

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Naushir Patuck]

Hi Tomi,

On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> On 12/02/2024 11:05, Krzysztof Kozlowski wrote:
> > On 12/02/2024 09:50, Jacopo Mondi wrote:
> >
> >>>> +properties:
> >>>> +  compatible:
> >>>> +    const: raspberrypi,pispbe
> >>>
> >>> Nothing more specific? No model name, no version? It's quite generic
> >>> compatible which in general should not be allowed. I would assume that
> >>> at least version of Pi could denote some sort of a model... unless
> >>> version is detectable?
> >>>
> >>
> >> The driver matches on a version register and that should be enough to
> >> handle quirks which are specific to an IP revision in the driver
> >> itself.
> >>
> >> Considering how minimal the integration with the SoC is (one clock, one
> >> interrupt and one optional iommu reference) even if we'll get future
> >> revisions of the SoC I don't think there will be any need to match on
> >> a dedicated compatible for bindings-validation purposes.
> >>
> >> However I understand that to be future-proof it's good practice to
> >> allow a more flexible scheme, so we can have a generic fallback and a
> >> revision-specific entry.
> >>
> >> Would
> >>
> >>    compatible:
> >>      items:
> >>        - enum:
> >>          - raspberrypi,pipspbe-bcm2712
> >
> > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
> > rather Pi model?
>
> Indeed, this is something I don't get. If the BE is in the bcm2712, is
> it not a broadcom IP? Why is raspberrypi in the compatible name at all?
>
> Naush, Dave?

The Backend (and Frontend) IP are both owned solely by Raspberry Pi,
and the BE is instantiated on the BCM2712.  So I think "raspberry" in
the compatible string is correct here.

>
> >>        - const: raspberrypi,pispbe
> >>
> >> do in this case ?
> >>
> >> Also, let's see what RPi think as they are certainly more informed
> >> than me on what a good revision-specific match could be
> >
> > I am fine with auto-detection, though.
> >
> > ...
> >
> >>>> +
> >>>> +examples:
> >>>> +  - |
> >>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>>> +
> >>>> +    rpi1 {
> >>>
> >>> soc {
> >>>
> >>
> >> Are you sure ? This will only ever live in the 'rp1' node.
> >
> > What is "rp1" node? Does not look like a generic name.
>
> I don't think this is right. RP1 is a separate chip, an IO controller,
> on raspberrypi 5. BE is not in the RP1.

Oops, missed this.  Yes, I think it should be the "soc" node.

Regards,
Naush

>
>   Tomi
>

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Laurent Pinchart]

On Tue, Feb 13, 2024 at 08:35:39AM +0000, Naushir Patuck wrote:
> On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen wrote:
> > On 12/02/2024 11:05, Krzysztof Kozlowski wrote:
> > > On 12/02/2024 09:50, Jacopo Mondi wrote:
> > >
> > >>>> +properties:
> > >>>> +  compatible:
> > >>>> +    const: raspberrypi,pispbe
> > >>>
> > >>> Nothing more specific? No model name, no version? It's quite generic
> > >>> compatible which in general should not be allowed. I would assume that
> > >>> at least version of Pi could denote some sort of a model... unless
> > >>> version is detectable?
> > >>
> > >> The driver matches on a version register and that should be enough to
> > >> handle quirks which are specific to an IP revision in the driver
> > >> itself.
> > >>
> > >> Considering how minimal the integration with the SoC is (one clock, one
> > >> interrupt and one optional iommu reference) even if we'll get future
> > >> revisions of the SoC I don't think there will be any need to match on
> > >> a dedicated compatible for bindings-validation purposes.
> > >>
> > >> However I understand that to be future-proof it's good practice to
> > >> allow a more flexible scheme, so we can have a generic fallback and a
> > >> revision-specific entry.
> > >>
> > >> Would
> > >>
> > >>    compatible:
> > >>      items:
> > >>        - enum:
> > >>          - raspberrypi,pipspbe-bcm2712
> > >
> > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
> > > rather Pi model?
> >
> > Indeed, this is something I don't get. If the BE is in the bcm2712, is
> > it not a broadcom IP? Why is raspberrypi in the compatible name at all?
> >
> > Naush, Dave?
> 
> The Backend (and Frontend) IP are both owned solely by Raspberry Pi,
> and the BE is instantiated on the BCM2712.  So I think "raspberry" in
> the compatible string is correct here.

Following what we do with other SoCs, we could have

	compatible = "brcm,pispbe-bcm2712", "raspberrypi,pispbe";

However, that scheme is mostly used when SoC vendor license IP cores
from third parties. Here, while the SoC is indeed manufactured by
Broadcom, it's a Raspberry Pi-specific SoC.

I don't have a personal preference.

> > >>        - const: raspberrypi,pispbe
> > >>
> > >> do in this case ?
> > >>
> > >> Also, let's see what RPi think as they are certainly more informed
> > >> than me on what a good revision-specific match could be
> > >
> > > I am fine with auto-detection, though.
> > >
> > > ...
> > >
> > >>>> +
> > >>>> +examples:
> > >>>> +  - |
> > >>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > >>>> +
> > >>>> +    rpi1 {
> > >>>
> > >>> soc {
> > >>>
> > >>
> > >> Are you sure ? This will only ever live in the 'rp1' node.
> > >
> > > What is "rp1" node? Does not look like a generic name.
> >
> > I don't think this is right. RP1 is a separate chip, an IO controller,
> > on raspberrypi 5. BE is not in the RP1.
> 
> Oops, missed this.  Yes, I think it should be the "soc" node.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Conor Dooley]

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

On Tue, Feb 13, 2024 at 12:44:05PM +0200, Laurent Pinchart wrote:
> On Tue, Feb 13, 2024 at 08:35:39AM +0000, Naushir Patuck wrote:
> > On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen wrote:
> > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote:
> > > > On 12/02/2024 09:50, Jacopo Mondi wrote:
> > > >
> > > >>>> +properties:
> > > >>>> +  compatible:
> > > >>>> +    const: raspberrypi,pispbe
> > > >>>
> > > >>> Nothing more specific? No model name, no version? It's quite generic
> > > >>> compatible which in general should not be allowed. I would assume that
> > > >>> at least version of Pi could denote some sort of a model... unless
> > > >>> version is detectable?
> > > >>
> > > >> The driver matches on a version register and that should be enough to
> > > >> handle quirks which are specific to an IP revision in the driver
> > > >> itself.
> > > >>
> > > >> Considering how minimal the integration with the SoC is (one clock, one
> > > >> interrupt and one optional iommu reference) even if we'll get future
> > > >> revisions of the SoC I don't think there will be any need to match on
> > > >> a dedicated compatible for bindings-validation purposes.
> > > >>
> > > >> However I understand that to be future-proof it's good practice to
> > > >> allow a more flexible scheme, so we can have a generic fallback and a
> > > >> revision-specific entry.
> > > >>
> > > >> Would
> > > >>
> > > >>    compatible:
> > > >>      items:
> > > >>        - enum:
> > > >>          - raspberrypi,pipspbe-bcm2712
> > > >
> > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
> > > > rather Pi model?
> > >
> > > Indeed, this is something I don't get. If the BE is in the bcm2712, is
> > > it not a broadcom IP? Why is raspberrypi in the compatible name at all?
> > >
> > > Naush, Dave?
> > 
> > The Backend (and Frontend) IP are both owned solely by Raspberry Pi,
> > and the BE is instantiated on the BCM2712.  So I think "raspberry" in
> > the compatible string is correct here.
> 
> Following what we do with other SoCs, we could have
> 
> 	compatible = "brcm,pispbe-bcm2712", "raspberrypi,pispbe";
> 
> However, that scheme is mostly used when SoC vendor license IP cores
> from third parties. Here, while the SoC is indeed manufactured by
> Broadcom, it's a Raspberry Pi-specific SoC.
> 
> I don't have a personal preference.

I'd be okay with what you propose here, I think it is a better
reflection of what is going on than that in the original patch etc.

Cheers,
Conor.

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

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Rob Herring]

On Tue, Feb 13, 2024 at 12:44:05PM +0200, Laurent Pinchart wrote:
> On Tue, Feb 13, 2024 at 08:35:39AM +0000, Naushir Patuck wrote:
> > On Tue, 13 Feb 2024 at 07:28, Tomi Valkeinen wrote:
> > > On 12/02/2024 11:05, Krzysztof Kozlowski wrote:
> > > > On 12/02/2024 09:50, Jacopo Mondi wrote:
> > > >
> > > >>>> +properties:
> > > >>>> +  compatible:
> > > >>>> +    const: raspberrypi,pispbe
> > > >>>
> > > >>> Nothing more specific? No model name, no version? It's quite generic
> > > >>> compatible which in general should not be allowed. I would assume that
> > > >>> at least version of Pi could denote some sort of a model... unless
> > > >>> version is detectable?
> > > >>
> > > >> The driver matches on a version register and that should be enough to
> > > >> handle quirks which are specific to an IP revision in the driver
> > > >> itself.
> > > >>
> > > >> Considering how minimal the integration with the SoC is (one clock, one
> > > >> interrupt and one optional iommu reference) even if we'll get future
> > > >> revisions of the SoC I don't think there will be any need to match on
> > > >> a dedicated compatible for bindings-validation purposes.
> > > >>
> > > >> However I understand that to be future-proof it's good practice to
> > > >> allow a more flexible scheme, so we can have a generic fallback and a
> > > >> revision-specific entry.
> > > >>
> > > >> Would
> > > >>
> > > >>    compatible:
> > > >>      items:
> > > >>        - enum:
> > > >>          - raspberrypi,pipspbe-bcm2712
> > > >
> > > > bcm2712 is manufactured by Broadcom, not Raspberry Pi, so it should be
> > > > rather Pi model?
> > >
> > > Indeed, this is something I don't get. If the BE is in the bcm2712, is
> > > it not a broadcom IP? Why is raspberrypi in the compatible name at all?
> > >
> > > Naush, Dave?
> > 
> > The Backend (and Frontend) IP are both owned solely by Raspberry Pi,
> > and the BE is instantiated on the BCM2712.  So I think "raspberry" in
> > the compatible string is correct here.
> 
> Following what we do with other SoCs, we could have
> 
> 	compatible = "brcm,pispbe-bcm2712", "raspberrypi,pispbe";

Nit: brcm,bcm7212-pispbe

Otherwise looks fine to me.

Rob

Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Naushir Patuck]

Hi Krzysztof and Jacopo,

On Mon, 12 Feb 2024 at 08:12, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/02/2024 17:48, Jacopo Mondi wrote:
> > Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> > signal processor.
> >
> > Datasheet:
> > https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> >
>
>
> > +---
> > +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> > +
> > +maintainers:
> > +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> > +  - David Plowman <david.plowman@raspberrypi.com>
> > +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +  - Naushir Patuck <naush@raspberrypi.com>
> > +  - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
>
> I assume all folks are fine being here?

Although I am fine with my name above, I think it might be easier to use:

Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>

to follow other Raspberry Pi drivers.

>
> > +
> > +description: |
> > +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> > +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> > +  in tiles and produces images consumable by applications.
> > +
> > +  The full ISP documentation is available at:
> > +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: raspberrypi,pispbe
>
> Nothing more specific? No model name, no version? It's quite generic
> compatible which in general should not be allowed. I would assume that
> at least version of Pi could denote some sort of a model... unless
> version is detectable?

There is a version register that is tested for different HW variants.  The
expectation is variant handling in the driver would happen based on this version
register.  Do you think that is sufficient to keep the compatible string as-is?

Thanks,
Naush

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: isp_be
>
> You can skip clock-names if they have only one entry and it matches
> block name. Quite useless.
>
> > +
> > +  iommus:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    rpi1 {
>
> soc {
>
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        isp: pisp_be@880000  {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> so: isp@
>
> and drop unused label
>
> > +             compatible = "raspberrypi,pispbe";
> ds,
> Krzysztof
>