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

[Geert Uytterhoeven]

Hi Mark,

On Wed, Apr 29, 2015 at 2:57 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
>> >> >> To preserve DT stability, we would like to add these properties to the
>> >> >> affected shmobile dtsi files.
>> >> >
>> >> > ... which means that they could be wrong, and will get in the way of
>> >> > stability rather than aiding it.
>> >>
>> >> We do know the GIC is part of the power domain, and has a controllable
>> >> clock (on the affected SoCs).
>> >
>> > ... my concern is that the data we place into the DT will be untested
>> > given that we don't have software relying on it. If said data is not
>> > correct, it is harmful to have, especially for such fundamental
>> > properties.
>>
>> Your statement challenges the viability of Stable DT Requirements, as we
>> can thus never write a DTS until the full software implementation has been
>> completed ;-)
>
> I appreciate this is difficult, but I disagree that it's impossible ;)
>
> If you don't want to do clock management currently, don't describe the
> clock controller, have some FW/loader pre-program the clocks, and list
> fixed-clocks in the DTB. This DTB should continue to work, but a new
> kernel alone won't give you fancy clock management. This is what we
> expect in terms of stable DTBs.
>
> When you add clock controller support, you need a different DT to
> describe the clock controller anyway. You can have it nuke the clock

Currently we have the clock controller in the DTS. It only lacks a few clocks
(like INTC-SYS -> GIC).

> configuration at boot time as a test that everything you need is
> described.

Don't worry, I have early boot code that disables all non-critical clocks.
This already caught many bugs (missing clock handling). It also caused a
bug, as I missed an interrupt storm due to a disabled clock ;-)

>> >> > 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).

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?

  - 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.

> 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.

Perhaps just

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

?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/