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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> > > > ---
> > > > 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@xxxxxxxxxxxxxx
>
> 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
