Re: [PATCH] ARM: gic: Document Power and Clock Domain optional properties

[Mark Rutland <mark.rutland@arm.com>]

Hi Geert,

> >> >> > I'm also concerned that the carving up of clock inputs, power domains,
> >> >> > and other physical details is implementation-specific. I imagine that
> >> >> > pretty much every user that will care about this is using GIC-400, so
> >> >> > could we make this specific to GIC-400?
> >> >>
> >> >> I have no idea which GIC version is being used.
> >> >
> >> > This is unfortunate.
> >> >
> >> >> This is for Renesas R-Mobile APE6 (r8a73a4) and various R-Car Gen2.
> >> >> According to the DTS, they're compatible with "arm,cortex-a15-gic" and
> >> >> "arm,cortex-a7-gic", and work with that value.
> >> >
> >> > Who put the DT together in the first place?
> >>
> >> Magnus (added to CC).
> >>
> >> > If it's a multi-cluster SoC then we know that we're not using any
> >> > built-in distributor...
> >>
> >> R-Mobile APE6 has 2 clusters (4xCA15 and 4xCA7).
> >> R-Car Gen2 has 1 or 2 clusters (2/4xCA15 and/or 2/4xCA7).
> >
> > It looks like we should be able to read the GICD_IIDR to figure out what
> > imlpementation is used. Could you see what GICD_IIDR reports on those
> > platforms? There's a patch at the end of the email to do so.
> 
> Thanks!
> 
>   - R-Mobile APE6 (r8a73a4):        GICD_IIDR reports 0x0200043b
>   - R-Car Gen M2-W (r8a7791):       GICD_IIDR reports 0x0200043b
> 
>         ProductID = 0x02 (GIC-400)
>         Variant = 0x0
>         Revision = 0x0
>         Implementer = 0x43b (ARM)
> 
> So both are GIC-400 (in the mean time I found a reference to GIC-400 in
> the APE6 docs).

Ok. We already support the "arm,gic-400" compatible string, so these
could be moved over, and given a CLK clock input.

> I also ran it on a few other SoCs:
> 
>   - SH-Mobile AG5 (sh73a0):         GICD_IIDR reports 0x0102043b
> 
>         Implementation version = 0x01 (Cortex-A9 MPCore)
>         Revision number = 0x020
>         Implementer = 0x43b (ARM)
> 
>     Which GIC is that?

That seems to be a GIC internal to the A9 MPCore, so "arm,cortex-a9-gic"
is fine for this.

It seems to take separate PERIPHCLK and PERIPHCLKEN clock inputs, though
it's not clear to be from the TRM whether PERIPHCLKEN is generated
internally from PERIPHCLK, or if it needs to be generated by an external
clock controller.

>   - R-Mobile A1 (r8a7740):          GICD_IIDR reports 0x0000043b
> 
>         ProductID = 0x00 (GIC-500 or Cortex-A15 MPCore or PL390)
>         Variant = 0x0
>         Revision = 0x0
>         Implementer = 0x43b (ARM)
> 
>     Seems like 0x0000043b can mean multiple things. Docs say PL390 r0p0.

We might want to add an "arm,pl390" compatible string for this. The
PL390 seems to take a single "gclk" clock input.

> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 7b315e3..02c8bb4 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -959,6 +959,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >         irq_hw_number_t hwirq_base;
> >         struct gic_chip_data *gic;
> >         int gic_irqs, irq_base, i;
> > +       unsigned long iidr;
> >
> >         BUG_ON(gic_nr >= MAX_GIC_NR);
> >
> > @@ -996,6 +997,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >                 gic_set_base_accessor(gic, gic_get_common_base);
> >         }
> >
> > +       iidr = readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR);
> > +       pr_info("GICD_IIDR reports 0x%08lx\n", (unsigned long)iidr);
> 
> No need for the cast.

For arm64 there is ;)

> Perhaps just
> 
>         pr_debug("GICD_IIDR reports 0x%08lx\n",
>                  readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IIDR));

I could fold the read in, but with the cast it was either overly long or
weirdly formatted.

Thanks,
Mark.