Hi Geert
Thank you for your review
> > + a720_0: cpu@0 {
> > + compatible = "arm,cortex-a720";
>
> "arm,cortex-a720ae"?
OK, will add a720ae support. But I'm not 100% familiar with ARM Cortex,
I assumes A720AE feature itself is same as A720.
> > + reg = <0x0 0x0>;
> > + device_type = "cpu";
> > + next-level-cache = <&L1_CA720_0>;
> > + enable-method = "psci";
>
> Please drop this line, as there is no psci node yet.
OK, will remove
> > +
> > + L1_CA720_0: controller-0 {
>
> cache-controller (lots of places).
Oops, cpu0 part lost "cache-". Will add
> > + compatible = "cache";
> > + cache-unified;
>
> The L1 cache is not unified, according to the documentation?
>
> > + cache-level = <1>;
>
> dtschema/schemas/cache-controller.yaml says valid values are 2..32,
> so I think you just have to ignore the L1 cache.
> I.e. drop it from DTS, and let the CPU's next-level-cache point to
> the L2 cache.
Thanks. Will fix
> > + L2_CA720_0: controller-1 {
> > + compatible = "cache";
> > + cache-unified;
> > + cache-level = <2>;
> > + next-level-cache = <&L3_CA720_0>;
> > + };
>
> Shouldn't this node be located outside the cpu@0 node?
Sorry, this is my bad. L2 is per core.
I will remove cache-unified, and keep it on each CPU.
> > + /*
> > + * In the early phase, there is no clock control support,
> > + * so assume that the clocks are enabled by default.
> > + * Therefore, dummy clocks are used.
> > + */
> > + dummy_clk_sgasyncd4: dummy-clk-sgasyncd4 {
>
> Perhaps use clock-*, for consistency with extal(r)?
> However, please note that so far no Renesas DTS files use "clock-*"
> as a node name, but "*-clk" is in active use. Krzysztof?
Yeah, "*-clk" is more used to see.
> > + /* External SCIF clock - to be overridden by boards that provide it */
> > + scif_clk: clock-scif {
> > + compatible = "fixed-clock";
> > + #clock-cells = <0>;
> > + /* clock-frequency must be set on board */
>
> This clock is optional, and thus may be left unpopulated on a board,
> so please (re-)add "clock-frequency = <0>;".
OK, will do
> > + /* The Arm GIC-700AE - View 1 */
>
> s/700/720/
Oops, thanks. Will fix
> > + gic: interrupt-controller@39000000 {
> > + compatible = "arm,gic-v3";
>
> The documentation states it is compliant with GICv4.1?
I'm not familiar with GIC. And I think there is no v4 support on Linux yet ?
If my understanding was correct, GICv4 have GICv3 compatible.
We can use v3 driver so far, and can be replaced to v4 driver if it was
supported in Linux?
> > + #interrupt-cells = <3>;
> > + #address-cells = <0>;
> > + interrupt-controller;
> > + redistributor-stride = <0x0 0x40000>;
> > + #redistributor-regions = <32>;
> > + reg = <0 0x39000000 0 0x20000>, // GICD
>
> The base address is 0x38000000, according to the docs?
It is indicated in very deep place in datasheet. I will indicate
detail in v2.
> > + <0 0x397C0000 0 0x40000>, // GICR Core29
> > + <0 0x39800000 0 0x40000>, // GICR Core30
> > + <0 0x39840000 0 0x40000>; // GICR Core31
>
> No GICC, GICH, and GICV?
will be added later ?
> > + scif0: serial@c0700000 {
> > + compatible = "renesas,rcar-gen5-scif", "renesas,scif";
>
> Missing "renesas,scif-r8a78000" (everywhere)
> ("make dtbs_check" would have told you).
Grr, thank you for pointing it. will fix
> > + reg = <0 0xc0700000 0 0x40>;
> > + interrupts = <GIC_SPI 4074 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&dummy_clk_sgasyncd4>, <&dummy_clk_sgasyncd4>, <&scif_clk>;
> > + clock-names = "fck", "brg_int", "scif_clk";
>
> "fck" on SCIF should be (derived from) SGASYNCD16 (66.666 MHz).
(snip)
> "fck" on HSCIF should be (derived from) SGASYNCD8 (133.33 MHz).
In the early phase, there is no clock control support, so assume that
the clocks are enabled by default. Therefore, dummy clocks are used.
But indeed the naming seems strange. Will use just "dummy-clk".
> According to the DT bindings, "power-domains" and "resets" are missing.
Unfortunately, can't use for now. It needs SCP support but is under
development. How should I do in this case ? Maybe use dummy device,
but can we use it ??
> > + hscif0: serial@c0710000 {
> > + compatible = "renesas,rcar-gen5-hscif", "renesas,hscif";
>
> Missing "renesas,hscif-r8a78000".
Grr, thank you for pointing it. will fix
Thank you for your help !!
Best regards
---
Kuninori Morimoto
