[PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Yoshihiro Shimoda]

Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
each cache IPMMU has own module id. So, update descriptions of
renesas,ipmmu-main property for R-Car Gen4.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
 the latest datasheet undocumented the register. So, update the propeties
 description. Note that the second argument is not used on the driver.
 So no behavior change.

 .../bindings/iommu/renesas,ipmmu-vmsa.yaml          | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
index 72308a4c14e7..7f63ecb467e6 100644
--- a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
+++ b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
@@ -76,14 +76,15 @@ properties:
     items:
       - items:
           - description: phandle to main IPMMU
-          - description: the interrupt bit number associated with the particular
-              cache IPMMU device. The interrupt bit number needs to match the main
-              IPMMU IMSSTR register. Only used by cache IPMMU instances.
+          - description: The interrupt bit number or module id associated with
+              the particular cache IPMMU device. The interrupt bit number needs
+              to match the main IPMMU IMSSTR register. Only used by cache IPMMU
+              instances.
     description:
       Reference to the main IPMMU phandle plus 1 cell. The cell is
-      the interrupt bit number associated with the particular cache IPMMU
-      device. The interrupt bit number needs to match the main IPMMU IMSSTR
-      register. Only used by cache IPMMU instances.
+      the interrupt bit number or module id associated with the particular
+      cache IPMMU device. The interrupt bit number needs to match the main
+      IPMMU IMSSTR register. Only used by cache IPMMU instances.
 
 required:
   - compatible
-- 
2.25.1

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Krzysztof Kozlowski]

On 23/01/2023 02:29, Yoshihiro Shimoda wrote:
> Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> each cache IPMMU has own module id. So, update descriptions of
> renesas,ipmmu-main property for R-Car Gen4.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
>  the latest datasheet undocumented the register. So, update the propeties
>  description. Note that the second argument is not used on the driver.
>  So no behavior change.
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> each cache IPMMU has own module id. So, update descriptions of
> renesas,ipmmu-main property for R-Car Gen4.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> ---
>  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
>  the latest datasheet undocumented the register. So, update the propeties
>  description. Note that the second argument is not used on the driver.

DT describes hardware, not software policy.

>  So no behavior change.

So where do we get the module id numbers to use, if they are no longer
documented in the Hardware Manual?

> --- a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> +++ b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> @@ -76,14 +76,15 @@ properties:
>      items:
>        - items:
>            - description: phandle to main IPMMU
> -          - description: the interrupt bit number associated with the particular
> -              cache IPMMU device. The interrupt bit number needs to match the main
> -              IPMMU IMSSTR register. Only used by cache IPMMU instances.
> +          - description: The interrupt bit number or module id associated with
> +              the particular cache IPMMU device. The interrupt bit number needs
> +              to match the main IPMMU IMSSTR register. Only used by cache IPMMU
> +              instances.
>      description:
>        Reference to the main IPMMU phandle plus 1 cell. The cell is
> -      the interrupt bit number associated with the particular cache IPMMU
> -      device. The interrupt bit number needs to match the main IPMMU IMSSTR
> -      register. Only used by cache IPMMU instances.
> +      the interrupt bit number or module id associated with the particular
> +      cache IPMMU device. The interrupt bit number needs to match the main
> +      IPMMU IMSSTR register. Only used by cache IPMMU instances.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > each cache IPMMU has own module id. So, update descriptions of
> > renesas,ipmmu-main property for R-Car Gen4.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks for your patch!

Thank you for your review!

> > ---
> >  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> >  the latest datasheet undocumented the register. So, update the propeties
> >  description. Note that the second argument is not used on the driver.
> 
> DT describes hardware, not software policy.

I think so.

> >  So no behavior change.
> 
> So where do we get the module id numbers to use, if they are no longer
> documented in the Hardware Manual?

If so, we cannot get the module id numbers. So, should we use other
information which is completely fixed instead? I have some ideas:
1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
2) Sequential numbers from register base offset.
   In R-Car S4: ipmmu_rt0 is the first node from register base offset,
   and ipmmu_rt1 is the second one.
   So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
3) Using base address upper 16-bits. 
   In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.

Perhaps, the option 1) is reasonable, I think. But, what do you think?

Best regards,
Yoshihiro Shimoda

> > --- a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> > +++ b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> > @@ -76,14 +76,15 @@ properties:
> >      items:
> >        - items:
> >            - description: phandle to main IPMMU
> > -          - description: the interrupt bit number associated with the particular
> > -              cache IPMMU device. The interrupt bit number needs to match the main
> > -              IPMMU IMSSTR register. Only used by cache IPMMU instances.
> > +          - description: The interrupt bit number or module id associated with
> > +              the particular cache IPMMU device. The interrupt bit number needs
> > +              to match the main IPMMU IMSSTR register. Only used by cache IPMMU
> > +              instances.
> >      description:
> >        Reference to the main IPMMU phandle plus 1 cell. The cell is
> > -      the interrupt bit number associated with the particular cache IPMMU
> > -      device. The interrupt bit number needs to match the main IPMMU IMSSTR
> > -      register. Only used by cache IPMMU instances.
> > +      the interrupt bit number or module id associated with the particular
> > +      cache IPMMU device. The interrupt bit number needs to match the main
> > +      IPMMU IMSSTR register. Only used by cache IPMMU instances.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Geert Uytterhoeven]

Hi Shimoda-san,

On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> > On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > > each cache IPMMU has own module id. So, update descriptions of
> > > renesas,ipmmu-main property for R-Car Gen4.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

> > > ---
> > >  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> > >  the latest datasheet undocumented the register. So, update the propeties
> > >  description. Note that the second argument is not used on the driver.
> >
> > DT describes hardware, not software policy.
>
> I think so.
>
> > >  So no behavior change.
> >
> > So where do we get the module id numbers to use, if they are no longer
> > documented in the Hardware Manual?
>
> If so, we cannot get the module id numbers. So, should we use other
> information which is completely fixed instead? I have some ideas:
> 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> 2) Sequential numbers from register base offset.
>    In R-Car S4: ipmmu_rt0 is the first node from register base offset,
>    and ipmmu_rt1 is the second one.
>    So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> 3) Using base address upper 16-bits.
>    In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
>
> Perhaps, the option 1) is reasonable, I think. But, what do you think?

I would not make up numbers, as that would cause confusion with SoCs
where the numbers do match the hardware.
As the driver doesn't use the module id number (it already loops
over all domains, instead of checking IMSSTR, probably because of
historical (R-Car Gen2) reasons?), what about dropping it from the
property? I.e. add "minItems: 1", possibly only when compatible with
renesas,rcar-gen4-ipmmu-vmsa?

BTW, the related IMCTR register is still documented, and the driver
does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
hardware (documentation) people intend this to be used...
Perhaps IMCTR_INTEN will be removed/undocumented, too?
Or perhaps the removal/undocumentation of IMSSTR was a mistake?

> > > --- a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> > > +++ b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> > > @@ -76,14 +76,15 @@ properties:
> > >      items:
> > >        - items:
> > >            - description: phandle to main IPMMU
> > > -          - description: the interrupt bit number associated with the particular
> > > -              cache IPMMU device. The interrupt bit number needs to match the main
> > > -              IPMMU IMSSTR register. Only used by cache IPMMU instances.
> > > +          - description: The interrupt bit number or module id associated with
> > > +              the particular cache IPMMU device. The interrupt bit number needs
> > > +              to match the main IPMMU IMSSTR register. Only used by cache IPMMU
> > > +              instances.
> > >      description:
> > >        Reference to the main IPMMU phandle plus 1 cell. The cell is
> > > -      the interrupt bit number associated with the particular cache IPMMU
> > > -      device. The interrupt bit number needs to match the main IPMMU IMSSTR
> > > -      register. Only used by cache IPMMU instances.
> > > +      the interrupt bit number or module id associated with the particular
> > > +      cache IPMMU device. The interrupt bit number needs to match the main
> > > +      IPMMU IMSSTR register. Only used by cache IPMMU instances.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Robin Murphy]

On 2023-01-25 08:54, Geert Uytterhoeven wrote:
> Hi Shimoda-san,
> 
> On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>> From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
>>> On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>> Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
>>>> each cache IPMMU has own module id. So, update descriptions of
>>>> renesas,ipmmu-main property for R-Car Gen4.
>>>>
>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
>>>> ---
>>>>   The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
>>>>   the latest datasheet undocumented the register. So, update the propeties
>>>>   description. Note that the second argument is not used on the driver.
>>>
>>> DT describes hardware, not software policy.
>>
>> I think so.
>>
>>>>   So no behavior change.
>>>
>>> So where do we get the module id numbers to use, if they are no longer
>>> documented in the Hardware Manual?
>>
>> If so, we cannot get the module id numbers. So, should we use other
>> information which is completely fixed instead? I have some ideas:
>> 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
>> 2) Sequential numbers from register base offset.
>>     In R-Car S4: ipmmu_rt0 is the first node from register base offset,
>>     and ipmmu_rt1 is the second one.
>>     So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
>> 3) Using base address upper 16-bits.
>>     In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
>>
>> Perhaps, the option 1) is reasonable, I think. But, what do you think?
> 
> I would not make up numbers, as that would cause confusion with SoCs
> where the numbers do match the hardware.
> As the driver doesn't use the module id number (it already loops
> over all domains, instead of checking IMSSTR, probably because of
> historical (R-Car Gen2) reasons?), what about dropping it from the
> property? I.e. add "minItems: 1", possibly only when compatible with
> renesas,rcar-gen4-ipmmu-vmsa?

Right, if there really is no meaningful ID for this model then its 
binding should not require one.

> BTW, the related IMCTR register is still documented, and the driver
> does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
> hardware (documentation) people intend this to be used...
> Perhaps IMCTR_INTEN will be removed/undocumented, too?
> Or perhaps the removal/undocumentation of IMSSTR was a mistake?

I guess it should be pretty straightforward to just try reading the 
expected IMSSTR register locations on this SoC to double-check whether 
anything is there.

Thanks,
Robin.

RE: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Yoshihiro Shimoda]

Hi Robin,

> From: Robin Murphy, Sent: Wednesday, January 25, 2023 7:42 PM
> 
> On 2023-01-25 08:54, Geert Uytterhoeven wrote:
> > Hi Shimoda-san,
> >
> > On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> >>> From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> >>> On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> >>> <yoshihiro.shimoda.uh@renesas.com> wrote:
> >>>> Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> >>>> each cache IPMMU has own module id. So, update descriptions of
> >>>> renesas,ipmmu-main property for R-Car Gen4.
> >>>>
> >>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> >>>> ---
> >>>>   The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> >>>>   the latest datasheet undocumented the register. So, update the propeties
> >>>>   description. Note that the second argument is not used on the driver.
> >>>
> >>> DT describes hardware, not software policy.
> >>
> >> I think so.
> >>
> >>>>   So no behavior change.
> >>>
> >>> So where do we get the module id numbers to use, if they are no longer
> >>> documented in the Hardware Manual?
> >>
> >> If so, we cannot get the module id numbers. So, should we use other
> >> information which is completely fixed instead? I have some ideas:
> >> 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> >> 2) Sequential numbers from register base offset.
> >>     In R-Car S4: ipmmu_rt0 is the first node from register base offset,
> >>     and ipmmu_rt1 is the second one.
> >>     So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> >> 3) Using base address upper 16-bits.
> >>     In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
> >>
> >> Perhaps, the option 1) is reasonable, I think. But, what do you think?
> >
> > I would not make up numbers, as that would cause confusion with SoCs
> > where the numbers do match the hardware.
> > As the driver doesn't use the module id number (it already loops
> > over all domains, instead of checking IMSSTR, probably because of
> > historical (R-Car Gen2) reasons?), what about dropping it from the
> > property? I.e. add "minItems: 1", possibly only when compatible with
> > renesas,rcar-gen4-ipmmu-vmsa?
> 
> Right, if there really is no meaningful ID for this model then its
> binding should not require one.

Thank you for your comment. I got it.

> > BTW, the related IMCTR register is still documented, and the driver
> > does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
> > hardware (documentation) people intend this to be used...
> > Perhaps IMCTR_INTEN will be removed/undocumented, too?
> > Or perhaps the removal/undocumentation of IMSSTR was a mistake?
> 
> I guess it should be pretty straightforward to just try reading the
> expected IMSSTR register locations on this SoC to double-check whether
> anything is there.

As I mentioned on other email [1], we should not access IMSSTR to avoid
a potential issue...

[1]
https://lore.kernel.org/all/TYBPR01MB5341F8DC36EAD659209A2BDDD8CF9@TYBPR01MB5341.jpnprd01.prod.outlook.com/

Best regards,
Yoshihiro Shimoda

> Thanks,
> Robin.

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Geert Uytterhoeven]

Hi Robin,

On Wed, Jan 25, 2023 at 11:42 AM Robin Murphy <robin.murphy@arm.com> wrote:
> On 2023-01-25 08:54, Geert Uytterhoeven wrote:
> > BTW, the related IMCTR register is still documented, and the driver
> > does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
> > hardware (documentation) people intend this to be used...
> > Perhaps IMCTR_INTEN will be removed/undocumented, too?
> > Or perhaps the removal/undocumentation of IMSSTR was a mistake?
>
> I guess it should be pretty straightforward to just try reading the
> expected IMSSTR register locations on this SoC to double-check whether
> anything is there.

FTR, I did try this.  Unfortunately the IMSSTR register value is zero
by default, and interrupts are generated only for error conditions (how
to trigger them?), so my debug print in ipmmu_irq() never triggered.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Rob Herring]

On Wed, Jan 25, 2023 at 10:42:13AM +0000, Robin Murphy wrote:
> On 2023-01-25 08:54, Geert Uytterhoeven wrote:
> > Hi Shimoda-san,
> > 
> > On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> > > > On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > > > > each cache IPMMU has own module id. So, update descriptions of
> > > > > renesas,ipmmu-main property for R-Car Gen4.
> > > > > 
> > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > 
> > > > > ---
> > > > >   The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> > > > >   the latest datasheet undocumented the register. So, update the propeties
> > > > >   description. Note that the second argument is not used on the driver.
> > > > 
> > > > DT describes hardware, not software policy.
> > > 
> > > I think so.
> > > 
> > > > >   So no behavior change.
> > > > 
> > > > So where do we get the module id numbers to use, if they are no longer
> > > > documented in the Hardware Manual?
> > > 
> > > If so, we cannot get the module id numbers. So, should we use other
> > > information which is completely fixed instead? I have some ideas:
> > > 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> > > 2) Sequential numbers from register base offset.
> > >     In R-Car S4: ipmmu_rt0 is the first node from register base offset,
> > >     and ipmmu_rt1 is the second one.
> > >     So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> > > 3) Using base address upper 16-bits.
> > >     In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
> > > 
> > > Perhaps, the option 1) is reasonable, I think. But, what do you think?
> > 
> > I would not make up numbers, as that would cause confusion with SoCs
> > where the numbers do match the hardware.
> > As the driver doesn't use the module id number (it already loops
> > over all domains, instead of checking IMSSTR, probably because of
> > historical (R-Car Gen2) reasons?), what about dropping it from the
> > property? I.e. add "minItems: 1", possibly only when compatible with
> > renesas,rcar-gen4-ipmmu-vmsa?
> 
> Right, if there really is no meaningful ID for this model then its binding
> should not require one.

I agree, however that makes parsing the property a pain (for both the 
schema and driver). This property is a matrix. The number of entries is 
already variable. If both dimensions are variable, we have to then look 
at the compatible to know how to parse it. I would go with option 1.

A 4th option is a new property.

Rob

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Geert Uytterhoeven]

Hi Rob,

On Mon, Jan 30, 2023 at 8:36 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Jan 25, 2023 at 10:42:13AM +0000, Robin Murphy wrote:
> > On 2023-01-25 08:54, Geert Uytterhoeven wrote:
> > > On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> > > > > On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> > > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > > > > > each cache IPMMU has own module id. So, update descriptions of
> > > > > > renesas,ipmmu-main property for R-Car Gen4.
> > > > > >
> > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > > > > ---
> > > > > >   The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> > > > > >   the latest datasheet undocumented the register. So, update the propeties
> > > > > >   description. Note that the second argument is not used on the driver.
> > > > >
> > > > > DT describes hardware, not software policy.
> > > >
> > > > I think so.
> > > >
> > > > > >   So no behavior change.
> > > > >
> > > > > So where do we get the module id numbers to use, if they are no longer
> > > > > documented in the Hardware Manual?
> > > >
> > > > If so, we cannot get the module id numbers. So, should we use other
> > > > information which is completely fixed instead? I have some ideas:
> > > > 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> > > > 2) Sequential numbers from register base offset.
> > > >     In R-Car S4: ipmmu_rt0 is the first node from register base offset,
> > > >     and ipmmu_rt1 is the second one.
> > > >     So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> > > > 3) Using base address upper 16-bits.
> > > >     In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
> > > >
> > > > Perhaps, the option 1) is reasonable, I think. But, what do you think?
> > >
> > > I would not make up numbers, as that would cause confusion with SoCs
> > > where the numbers do match the hardware.
> > > As the driver doesn't use the module id number (it already loops
> > > over all domains, instead of checking IMSSTR, probably because of
> > > historical (R-Car Gen2) reasons?), what about dropping it from the
> > > property? I.e. add "minItems: 1", possibly only when compatible with
> > > renesas,rcar-gen4-ipmmu-vmsa?
> >
> > Right, if there really is no meaningful ID for this model then its binding
> > should not require one.
>
> I agree, however that makes parsing the property a pain (for both the
> schema and driver). This property is a matrix. The number of entries is
> already variable. If both dimensions are variable, we have to then look
> at the compatible to know how to parse it. I would go with option 1.

But it does not have to be two-dimensional.
The second dimension was added in commit 39bd2b6a3783b899 ("dt-bindings
 Improve phandle-array schemas"), but is not needed.
Can this be simplified again?

The driver doesn't care, it just checks for the presence of the
property, i.e. treats it as a boolean flag.

> A 4th option is a new property.

If all else fails, a new boolean flag would work...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Robin Murphy]

On 2023-01-31 08:20, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Mon, Jan 30, 2023 at 8:36 PM Rob Herring <robh@kernel.org> wrote:
>> On Wed, Jan 25, 2023 at 10:42:13AM +0000, Robin Murphy wrote:
>>> On 2023-01-25 08:54, Geert Uytterhoeven wrote:
>>>> On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
>>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>>>> From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
>>>>>> On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
>>>>>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>>>>>>> Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
>>>>>>> each cache IPMMU has own module id. So, update descriptions of
>>>>>>> renesas,ipmmu-main property for R-Car Gen4.
>>>>>>>
>>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>>
>>>>>>> ---
>>>>>>>    The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
>>>>>>>    the latest datasheet undocumented the register. So, update the propeties
>>>>>>>    description. Note that the second argument is not used on the driver.
>>>>>>
>>>>>> DT describes hardware, not software policy.
>>>>>
>>>>> I think so.
>>>>>
>>>>>>>    So no behavior change.
>>>>>>
>>>>>> So where do we get the module id numbers to use, if they are no longer
>>>>>> documented in the Hardware Manual?
>>>>>
>>>>> If so, we cannot get the module id numbers. So, should we use other
>>>>> information which is completely fixed instead? I have some ideas:
>>>>> 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
>>>>> 2) Sequential numbers from register base offset.
>>>>>      In R-Car S4: ipmmu_rt0 is the first node from register base offset,
>>>>>      and ipmmu_rt1 is the second one.
>>>>>      So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
>>>>> 3) Using base address upper 16-bits.
>>>>>      In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
>>>>>
>>>>> Perhaps, the option 1) is reasonable, I think. But, what do you think?
>>>>
>>>> I would not make up numbers, as that would cause confusion with SoCs
>>>> where the numbers do match the hardware.
>>>> As the driver doesn't use the module id number (it already loops
>>>> over all domains, instead of checking IMSSTR, probably because of
>>>> historical (R-Car Gen2) reasons?), what about dropping it from the
>>>> property? I.e. add "minItems: 1", possibly only when compatible with
>>>> renesas,rcar-gen4-ipmmu-vmsa?
>>>
>>> Right, if there really is no meaningful ID for this model then its binding
>>> should not require one.
>>
>> I agree, however that makes parsing the property a pain (for both the
>> schema and driver). This property is a matrix. The number of entries is
>> already variable. If both dimensions are variable, we have to then look
>> at the compatible to know how to parse it. I would go with option 1.
> 
> But it does not have to be two-dimensional.
> The second dimension was added in commit 39bd2b6a3783b899 ("dt-bindings
>   Improve phandle-array schemas"), but is not needed.
> Can this be simplified again?

Maybe the answer is that it's a cell-array of one item which contains a 
bare phandle and an (optional) integer that happens to look a bit like a 
phandle argument but isn't?

Robin.

> The driver doesn't care, it just checks for the presence of the
> property, i.e. treats it as a boolean flag.
> 
>> A 4th option is a new property.
> 
> If all else fails, a new boolean flag would work...
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

RE: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Wednesday, January 25, 2023 5:55 PM
> 
> Hi Shimoda-san,
> 
> On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> > > On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > > > each cache IPMMU has own module id. So, update descriptions of
> > > > renesas,ipmmu-main property for R-Car Gen4.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> > > > ---
> > > >  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> > > >  the latest datasheet undocumented the register. So, update the propeties
> > > >  description. Note that the second argument is not used on the driver.
> > >
> > > DT describes hardware, not software policy.
> >
> > I think so.
> >
> > > >  So no behavior change.
> > >
> > > So where do we get the module id numbers to use, if they are no longer
> > > documented in the Hardware Manual?
> >
> > If so, we cannot get the module id numbers. So, should we use other
> > information which is completely fixed instead? I have some ideas:
> > 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> > 2) Sequential numbers from register base offset.
> >    In R-Car S4: ipmmu_rt0 is the first node from register base offset,
> >    and ipmmu_rt1 is the second one.
> >    So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> > 3) Using base address upper 16-bits.
> >    In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
> >
> > Perhaps, the option 1) is reasonable, I think. But, what do you think?
> 
> I would not make up numbers, as that would cause confusion with SoCs
> where the numbers do match the hardware.

I see.

> As the driver doesn't use the module id number (it already loops
> over all domains, instead of checking IMSSTR, probably because of
> historical (R-Car Gen2) reasons?), what about dropping it from the
> property? I.e. add "minItems: 1", possibly only when compatible with
> renesas,rcar-gen4-ipmmu-vmsa?

It looks a good idea to me.

> BTW, the related IMCTR register is still documented, and the driver
> does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
> hardware (documentation) people intend this to be used...
> Perhaps IMCTR_INTEN will be removed/undocumented, too?
> Or perhaps the removal/undocumentation of IMSSTR was a mistake?

I don't think that IMCTR_INTEN will be removed/undocumented too because
the IMCTR register is related to IMSTR register, not IMSSTR register ;)
                                 ~~~~~               ~~~~~~
About undocumentation of IMSSTR, I found that accessing the register is
possible to cause a potential issue on both R-Car Gen3/4. That's why
the IMSSTR register is undocumented on R-Car Gen4. I'm not sure whether
R-Car Gen3 will be undocumented too in the future though, but at least,
we should not access the IMSSTR register on both R-Car Gen3/Gen4.
# I'm not sure, but that's why the driver doesn't check IMSSTR to avoid
# the potential issue??

So, to simplify the dt-bindings, just removing the second property without
any condition is better, I think. But, what do you think?

Best regards,
Yoshihiro Shimoda

> > > > --- a/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> > > > +++ b/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.yaml
> > > > @@ -76,14 +76,15 @@ properties:
> > > >      items:
> > > >        - items:
> > > >            - description: phandle to main IPMMU
> > > > -          - description: the interrupt bit number associated with the particular
> > > > -              cache IPMMU device. The interrupt bit number needs to match the main
> > > > -              IPMMU IMSSTR register. Only used by cache IPMMU instances.
> > > > +          - description: The interrupt bit number or module id associated with
> > > > +              the particular cache IPMMU device. The interrupt bit number needs
> > > > +              to match the main IPMMU IMSSTR register. Only used by cache IPMMU
> > > > +              instances.
> > > >      description:
> > > >        Reference to the main IPMMU phandle plus 1 cell. The cell is
> > > > -      the interrupt bit number associated with the particular cache IPMMU
> > > > -      device. The interrupt bit number needs to match the main IPMMU IMSSTR
> > > > -      register. Only used by cache IPMMU instances.
> > > > +      the interrupt bit number or module id associated with the particular
> > > > +      cache IPMMU device. The interrupt bit number needs to match the main
> > > > +      IPMMU IMSSTR register. Only used by cache IPMMU instances.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Jan 26, 2023 at 1:55 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Wednesday, January 25, 2023 5:55 PM
> > On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> > > > On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > > > > each cache IPMMU has own module id. So, update descriptions of
> > > > > renesas,ipmmu-main property for R-Car Gen4.
> > > > >
> > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > > > > ---
> > > > >  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> > > > >  the latest datasheet undocumented the register. So, update the propeties
> > > > >  description. Note that the second argument is not used on the driver.
> > > >
> > > > DT describes hardware, not software policy.
> > >
> > > I think so.
> > >
> > > > >  So no behavior change.
> > > >
> > > > So where do we get the module id numbers to use, if they are no longer
> > > > documented in the Hardware Manual?
> > >
> > > If so, we cannot get the module id numbers. So, should we use other
> > > information which is completely fixed instead? I have some ideas:
> > > 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> > > 2) Sequential numbers from register base offset.
> > >    In R-Car S4: ipmmu_rt0 is the first node from register base offset,
> > >    and ipmmu_rt1 is the second one.
> > >    So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> > > 3) Using base address upper 16-bits.
> > >    In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
> > >
> > > Perhaps, the option 1) is reasonable, I think. But, what do you think?
> >
> > I would not make up numbers, as that would cause confusion with SoCs
> > where the numbers do match the hardware.
>
> I see.
>
> > As the driver doesn't use the module id number (it already loops
> > over all domains, instead of checking IMSSTR, probably because of
> > historical (R-Car Gen2) reasons?), what about dropping it from the
> > property? I.e. add "minItems: 1", possibly only when compatible with
> > renesas,rcar-gen4-ipmmu-vmsa?
>
> It looks a good idea to me.
>
> > BTW, the related IMCTR register is still documented, and the driver
> > does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
> > hardware (documentation) people intend this to be used...
> > Perhaps IMCTR_INTEN will be removed/undocumented, too?
> > Or perhaps the removal/undocumentation of IMSSTR was a mistake?
>
> I don't think that IMCTR_INTEN will be removed/undocumented too because
> the IMCTR register is related to IMSTR register, not IMSSTR register ;)
>                                  ~~~~~               ~~~~~~
> About undocumentation of IMSSTR, I found that accessing the register is
> possible to cause a potential issue on both R-Car Gen3/4. That's why
> the IMSSTR register is undocumented on R-Car Gen4. I'm not sure whether
> R-Car Gen3 will be undocumented too in the future though, but at least,
> we should not access the IMSSTR register on both R-Car Gen3/Gen4.
> # I'm not sure, but that's why the driver doesn't check IMSSTR to avoid
> # the potential issue??

IC

> So, to simplify the dt-bindings, just removing the second property without
> any condition is better, I think. But, what do you think?

So just add "minItems: 1", and leave out the second value of the
"renesas,ipmmu-main" property when adding support for new SoCs.
I don't think there is an immediate need to remove the existing second
value on already supported SoCs, as these values are not incorrect
hardware description.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] dt-bindings: iommu: renesas,ipmmu-vmsa: Update descriptions for R-Car Gen4

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven <geert@linux-m68k.org>, Sent: Thursday, January 26, 2023 5:38 PM
> 
> On Thu, Jan 26, 2023 at 1:55 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Geert Uytterhoeven, Sent: Wednesday, January 25, 2023 5:55 PM
> > > On Wed, Jan 25, 2023 at 1:49 AM Yoshihiro Shimoda
> > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > From: Geert Uytterhoeven, Sent: Tuesday, January 24, 2023 11:35 PM
> > > > > On Mon, Jan 23, 2023 at 2:35 AM Yoshihiro Shimoda
> > > > > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > > > > Since R-Car Gen4 doens't have the main IPMMU IMSSTR register, but
> > > > > > each cache IPMMU has own module id. So, update descriptions of
> > > > > > renesas,ipmmu-main property for R-Car Gen4.
> > > > > >
> > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > > > > ---
> > > > > >  The old R-Car S4-8 datasheet had described IPMMU IMSSTR register, but
> > > > > >  the latest datasheet undocumented the register. So, update the propeties
> > > > > >  description. Note that the second argument is not used on the driver.
> > > > >
> > > > > DT describes hardware, not software policy.
> > > >
> > > > I think so.
> > > >
> > > > > >  So no behavior change.
> > > > >
> > > > > So where do we get the module id numbers to use, if they are no longer
> > > > > documented in the Hardware Manual?
> > > >
> > > > If so, we cannot get the module id numbers. So, should we use other
> > > > information which is completely fixed instead? I have some ideas:
> > > > 1) Just 0 (or other fixed value) if the IMSSTR register doesn't exist.
> > > > 2) Sequential numbers from register base offset.
> > > >    In R-Car S4: ipmmu_rt0 is the first node from register base offset,
> > > >    and ipmmu_rt1 is the second one.
> > > >    So, ipmmu_rt0 is 0, ipmmu_rt1 is 1, ipmmu_ds0 is 2 and ipmmu_hc is 3.
> > > > 3) Using base address upper 16-bits.
> > > >    In R-Car S4: ipmmu_rt0 is 0xee480000. So, the value is 0xee48.
> > > >
> > > > Perhaps, the option 1) is reasonable, I think. But, what do you think?
> > >
> > > I would not make up numbers, as that would cause confusion with SoCs
> > > where the numbers do match the hardware.
> >
> > I see.
> >
> > > As the driver doesn't use the module id number (it already loops
> > > over all domains, instead of checking IMSSTR, probably because of
> > > historical (R-Car Gen2) reasons?), what about dropping it from the
> > > property? I.e. add "minItems: 1", possibly only when compatible with
> > > renesas,rcar-gen4-ipmmu-vmsa?
> >
> > It looks a good idea to me.
> >
> > > BTW, the related IMCTR register is still documented, and the driver
> > > does enable the interrupt bit (IMCTR_INTEN), so I'm wondering how the
> > > hardware (documentation) people intend this to be used...
> > > Perhaps IMCTR_INTEN will be removed/undocumented, too?
> > > Or perhaps the removal/undocumentation of IMSSTR was a mistake?
> >
> > I don't think that IMCTR_INTEN will be removed/undocumented too because
> > the IMCTR register is related to IMSTR register, not IMSSTR register ;)
> >                                  ~~~~~               ~~~~~~
> > About undocumentation of IMSSTR, I found that accessing the register is
> > possible to cause a potential issue on both R-Car Gen3/4. That's why
> > the IMSSTR register is undocumented on R-Car Gen4. I'm not sure whether
> > R-Car Gen3 will be undocumented too in the future though, but at least,
> > we should not access the IMSSTR register on both R-Car Gen3/Gen4.
> > # I'm not sure, but that's why the driver doesn't check IMSSTR to avoid
> > # the potential issue??
> 
> IC
> 
> > So, to simplify the dt-bindings, just removing the second property without
> > any condition is better, I think. But, what do you think?
> 
> So just add "minItems: 1", and leave out the second value of the
> "renesas,ipmmu-main" property when adding support for new SoCs.
> I don't think there is an immediate need to remove the existing second
> value on already supported SoCs, as these values are not incorrect
> hardware description.

Thank you for your comment! I understood it.
So, I'll submit such a patch as v2 tomorrow.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds