RE: [PATCH v2 1/3] dt-bindings: gpu: mali-bifrost: Document RZ/G2L support
Hi,
> Subject: RE: [PATCH v2 1/3] dt-bindings: gpu: mali-bifrost: Document
> RZ/G2L support
>
> Hi Robin,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v2 1/3] dt-bindings: gpu: mali-bifrost: Document
> > RZ/G2L support
> >
> > On 2021-12-06 15:00, Biju Das wrote:
> > > The Renesas RZ/G2{L, LC} SoC (a.k.a R9A07G044) has a Bifrost
> > > Mali-G31 GPU, add a compatible string for it.
> > >
> > > Signed-off-by: Biju Das
> > > Reviewed-by: Lad Prabhakar
> > > ---
> > > v1->v2:
> > > * Updated minItems for resets as 2
> > > * Documented optional property reset-names
> > > * Documented reset-names as required property for RZ/G2L SoC.
> > > ---
> > > .../bindings/gpu/arm,mali-bifrost.yaml| 39
> ++-
> > > 1 file changed, 37 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > > b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > > index 6f98dd55fb4c..c3b2f4ddd520 100644
> > > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > > @@ -19,6 +19,7 @@ properties:
> > > - amlogic,meson-g12a-mali
> > > - mediatek,mt8183-mali
> > > - realtek,rtd1619-mali
> > > + - renesas,r9a07g044-mali
> > > - rockchip,px30-mali
> > > - rockchip,rk3568-mali
> > > - const: arm,mali-bifrost # Mali Bifrost GPU model/revision
> > > is fully discoverable @@ -27,19 +28,30 @@ properties:
> > > maxItems: 1
> > >
> > > interrupts:
> > > +minItems: 3
> > > items:
> > > - description: Job interrupt
> > > - description: MMU interrupt
> > > - description: GPU interrupt
> > > + - description: EVENT interrupt
> >
> > I believe we haven't bothered with the "Event" interrupt so far since
> > there's no real meaningful use for it - it seems the downstream
> > binding for Arm's kbase driver doesn't mention it either.
>
> I agree.
> But DT binding describes the H/W. Our SoC, mention about Event interrupt.
> That is the reason I have documented it.
> >
> > > interrupt-names:
> > > +minItems: 3
> > > items:
> > > - const: job
> > > - const: mmu
> > > - const: gpu
> > > + - const: event
> > >
> > > clocks:
> > > -maxItems: 1
> > > +minItems: 1
> > > +maxItems: 3
> > > +
> > > + clock-names:
> > > +items:
> > > + - const: gpu
> > > + - const: bus
> > > + - const: bus_ace
> >
> > Note that the Bifrost GPUs themselves all only have a single external
> > clock and reset (unexcitingly named "CLK" and "RESETn" respectively,
> > FWIW). I can't help feeling wary that defining additional names for
> > vendor integration details in the core binding may quickly grow into a
> > mess of mutually-incompatible sets of values, for no great benefit. At
> > the very least, it would seem more sensible to put them in the
> > SoC-specific conditional schemas.
>
I agree, All optional properties like clock-names and reset-names should go in
the SoC-specific conditional schemas.
I will make clock-names and reset-names to true and handle it in the
SoC-specific conditional schemas.
I will send V3, incorporating the above.
Regards,
Biju
>
> >
> > Robin.
> >
> > >
> > > mali-supply: true
> > >
> > > @@ -52,7 +64,14 @@ properties:
> > > maxItems: 3
> > >
> > > resets:
> > > -maxItems: 2
> > > +minItems: 2
> > > +maxItems: 3
> > > +
> > > + reset-names:
> > > +items:
> > > + - const: rst
> > > + - const: axi_rst
> > > + - const: ace_rst
> > >
> > > "#cooling-cells":
> > > const: 2
> > > @@ -113,6 +132,22 @@ allOf:
> > > - sram-supply
> > > - power-domains
> > > - power-domain-names
> > > + - if:
> > > + properties:
> > > +compatible:
> > > + contains:
> > > +const: renesas,r9a07g044-mali
> > > +then:
> > > + properties:
> > > +interrupt-names:
> > > + minItems: 4
> > > +clock-names:
> > > + minItems: 3
> > > + required:
> > > +- clock-names
> > > +- power-domains
> > > +- resets
> > > +- reset-names
> > > else:
> > > properties:
> > > power-domains:
> > >
RE: [PATCH v2 1/3] dt-bindings: gpu: mali-bifrost: Document RZ/G2L support
Hi Robin,
Thanks for the feedback.
> Subject: Re: [PATCH v2 1/3] dt-bindings: gpu: mali-bifrost: Document
> RZ/G2L support
>
> On 2021-12-06 15:00, Biju Das wrote:
> > The Renesas RZ/G2{L, LC} SoC (a.k.a R9A07G044) has a Bifrost Mali-G31
> > GPU, add a compatible string for it.
> >
> > Signed-off-by: Biju Das
> > Reviewed-by: Lad Prabhakar
> > ---
> > v1->v2:
> > * Updated minItems for resets as 2
> > * Documented optional property reset-names
> > * Documented reset-names as required property for RZ/G2L SoC.
> > ---
> > .../bindings/gpu/arm,mali-bifrost.yaml| 39 ++-
> > 1 file changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > index 6f98dd55fb4c..c3b2f4ddd520 100644
> > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> > @@ -19,6 +19,7 @@ properties:
> > - amlogic,meson-g12a-mali
> > - mediatek,mt8183-mali
> > - realtek,rtd1619-mali
> > + - renesas,r9a07g044-mali
> > - rockchip,px30-mali
> > - rockchip,rk3568-mali
> > - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is
> > fully discoverable @@ -27,19 +28,30 @@ properties:
> > maxItems: 1
> >
> > interrupts:
> > +minItems: 3
> > items:
> > - description: Job interrupt
> > - description: MMU interrupt
> > - description: GPU interrupt
> > + - description: EVENT interrupt
>
> I believe we haven't bothered with the "Event" interrupt so far since
> there's no real meaningful use for it - it seems the downstream binding
> for Arm's kbase driver doesn't mention it either.
I agree.
But DT binding describes the H/W. Our SoC, mention about Event interrupt.
That is the reason I have documented it.
I am ok for keeping it or removing it. Please let me know.
>
> > interrupt-names:
> > +minItems: 3
> > items:
> > - const: job
> > - const: mmu
> > - const: gpu
> > + - const: event
> >
> > clocks:
> > -maxItems: 1
> > +minItems: 1
> > +maxItems: 3
> > +
> > + clock-names:
> > +items:
> > + - const: gpu
> > + - const: bus
> > + - const: bus_ace
>
> Note that the Bifrost GPUs themselves all only have a single external
> clock and reset (unexcitingly named "CLK" and "RESETn" respectively,
> FWIW). I can't help feeling wary that defining additional names for vendor
> integration details in the core binding may quickly grow into a mess of
> mutually-incompatible sets of values, for no great benefit. At the very
> least, it would seem more sensible to put them in the SoC-specific
> conditional schemas.
Initially GPU was not working on our platform. Then after debugging found that
it needs, bus clock
to make it work. This information is missing in dt binding and I need to find
this info from source code.
That is the reason, even if it is optional, I have documented with same name
here.
Regards,
Biju
>
> Robin.
>
> >
> > mali-supply: true
> >
> > @@ -52,7 +64,14 @@ properties:
> > maxItems: 3
> >
> > resets:
> > -maxItems: 2
> > +minItems: 2
> > +maxItems: 3
> > +
> > + reset-names:
> > +items:
> > + - const: rst
> > + - const: axi_rst
> > + - const: ace_rst
> >
> > "#cooling-cells":
> > const: 2
> > @@ -113,6 +132,22 @@ allOf:
> > - sram-supply
> > - power-domains
> > - power-domain-names
> > + - if:
> > + properties:
> > +compatible:
> > + contains:
> > +const: renesas,r9a07g044-mali
> > +then:
> > + properties:
> > +interrupt-names:
> > + minItems: 4
> > +clock-names:
> > + minItems: 3
> > + required:
> > +- clock-names
> > +- power-domains
> > +- resets
> > +- reset-names
> > else:
> > properties:
> > power-domains:
> >
Re: [PATCH v2 1/3] dt-bindings: gpu: mali-bifrost: Document RZ/G2L support
On 2021-12-06 15:00, Biju Das wrote:
The Renesas RZ/G2{L, LC} SoC (a.k.a R9A07G044) has a Bifrost Mali-G31 GPU,
add a compatible string for it.
Signed-off-by: Biju Das
Reviewed-by: Lad Prabhakar
---
v1->v2:
* Updated minItems for resets as 2
* Documented optional property reset-names
* Documented reset-names as required property for RZ/G2L SoC.
---
.../bindings/gpu/arm,mali-bifrost.yaml| 39 ++-
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
index 6f98dd55fb4c..c3b2f4ddd520 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
@@ -19,6 +19,7 @@ properties:
- amlogic,meson-g12a-mali
- mediatek,mt8183-mali
- realtek,rtd1619-mali
+ - renesas,r9a07g044-mali
- rockchip,px30-mali
- rockchip,rk3568-mali
- const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully
discoverable
@@ -27,19 +28,30 @@ properties:
maxItems: 1
interrupts:
+minItems: 3
items:
- description: Job interrupt
- description: MMU interrupt
- description: GPU interrupt
+ - description: EVENT interrupt
I believe we haven't bothered with the "Event" interrupt so far since
there's no real meaningful use for it - it seems the downstream binding
for Arm's kbase driver doesn't mention it either.
interrupt-names:
+minItems: 3
items:
- const: job
- const: mmu
- const: gpu
+ - const: event
clocks:
-maxItems: 1
+minItems: 1
+maxItems: 3
+
+ clock-names:
+items:
+ - const: gpu
+ - const: bus
+ - const: bus_ace
Note that the Bifrost GPUs themselves all only have a single external
clock and reset (unexcitingly named "CLK" and "RESETn" respectively,
FWIW). I can't help feeling wary that defining additional names for
vendor integration details in the core binding may quickly grow into a
mess of mutually-incompatible sets of values, for no great benefit. At
the very least, it would seem more sensible to put them in the
SoC-specific conditional schemas.
Robin.
mali-supply: true
@@ -52,7 +64,14 @@ properties:
maxItems: 3
resets:
-maxItems: 2
+minItems: 2
+maxItems: 3
+
+ reset-names:
+items:
+ - const: rst
+ - const: axi_rst
+ - const: ace_rst
"#cooling-cells":
const: 2
@@ -113,6 +132,22 @@ allOf:
- sram-supply
- power-domains
- power-domain-names
+ - if:
+ properties:
+compatible:
+ contains:
+const: renesas,r9a07g044-mali
+then:
+ properties:
+interrupt-names:
+ minItems: 4
+clock-names:
+ minItems: 3
+ required:
+- clock-names
+- power-domains
+- resets
+- reset-names
else:
properties:
power-domains:
