Re: [RFC PATCH 5/6] arm64: dts: ti: Add Support for AM654 SoC

[Nishanth Menon]

On 14:05-20180605, Rob Herring wrote:
> On Tue, Jun 5, 2018 at 1:05 AM, Nishanth Menon <nm@xxxxxx> wrote:
[...]

> > +       soc0: soc0 {
> > +               compatible = "simple-bus";
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> 
> Really need 64-bit addresses and sizes? Use ranges to limit the
> address space if possible.

Done -> overall the addresses are really in the 64bit addresses, but
used bus segments and ranges to reduce to 32bit maps where possible.

OSPI, PCIE, FSS (Flash subsystem) , CPTS are some of the ones that
probably will need some level of cleanups when those are introduced
later.

Unfortunately, there is a lot of interleaved addressing between bus
segments themselves, I have tried to keep the ranges as clean as
reasonably possible. I also tried to use 1-1 map for children nodes to
maintain some level of sanity as we add more device nodes. There might
be a few exceptions, but overall the ranges currently map 1-1 physical
32bit address - OSPI, CPTS, FSS will however have to have a different
mapping.

See [1]
> 
> > +
> > +               a53_timer0: timer-cl0-cpu0 {
> > +                       compatible = "arm,armv8-timer";
> > +                       interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW>, /* cntpsirq */
> > +                                    <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW>, /* cntpnsirq */
> > +                                    <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW>, /* cntvirq */
> > +                                    <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>; /* cnthpirq */
> > +               };
> > +
> > +               pmu: pmu {
> > +                       compatible = "arm,armv8-pmuv3";
> > +                       /* Recommendation from GIC500 TRM Table A.3 */
> > +                       interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
> > +               };
> 
> These 2 nodes aren't on the bus, so move them up a level.

Thanks. oversight on my end. I have fixed it (see [1])
> 
> > +
> > +               gic: interrupt-controller@1800000 {
> > +                       compatible = "arm,gic-v3";
> 
> gic-500?

Yes, GIC500.

> 
> > +                       #address-cells = <2>;
> > +                       #size-cells = <2>;
> > +                       ranges;
> > +                       #interrupt-cells = <3>;
> > +                       interrupt-controller;
> > +                       /*
> > +                        * NOTE: we are NOT gicv2 backward compat, so no GICC,
> > +                        * GICH or GICV
> 
> The compatible should imply this.

GIC500 at SoC design instantiation takes a parameter
"are_option" -> this is set to no-compatibility for V2 for AM6. This is indeed
discovered by the driver, However,
Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
just notes that GICC, GICH, GICV as optional.. With backward
compatibility disabled, these are'nt even present.

I have dropped the comment, was helpful for me when I was first
adding support for GIC500, It is pretty common knowledge now for other
ARMV8 developers, so no point in retaining newbie info as comment.

[...]
> > diff --git a/arch/arm64/boot/dts/ti/k3-am654.dtsi b/arch/arm64/boot/dts/ti/k3-am654.dtsi
> > new file mode 100644
> > index 000000000000..d9b70081daba
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/ti/k3-am654.dtsi
> > @@ -0,0 +1,117 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree Source for AM6 SoC family in Quad core configuration
> > + *
> > + * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/
> > + */
> > +
> > +#include "k3-am6.dtsi"
> > +
> > +/ {
> > +       cpus: cpus {
> 
> Really need a label?

Thanks. Dropped.

> 
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +               cpu-map {
> 
> IIRC, this goes at the top level.

Documentation/devicetree/bindings/arm/topology.txt
States to keep in cpus node. Quote:

| The ARM CPU topology is defined within the cpu-map node, which is a direct
| child of the cpus node and provides a container where the actual topology
| nodes are listed.

Retained as is to stay in sync with binding.

> > +               cpu0: cpu@0 {
> > +                       compatible = "arm,cortex-a53","arm,armv8";
> 
> space between compatibles.

Oops. Fixed. thanks.

> 
> > +                       reg = <0x000>;
> > +                       device_type = "cpu";
> > +                       enable-method = "psci";
> 
> > +                       i-cache-size = <0x8000>;
> > +                       i-cache-line-size = <64>;
> > +                       i-cache-sets = <256>;
> > +                       d-cache-size = <0x8000>;
> > +                       d-cache-line-size = <64>;
> > +                       d-cache-sets = <128>;
> 
> All this should be discoverable.

Unfortunately no.
Previously CCSIDR_EL1 was a good place to lookup this data.
But as Sudeep pointed me offline:
commit a8d4636f96ad ("arm64: cacheinfo: Remove CCSIDR-based cache information probing")
and commit 9a802431c527 ("arm64: cacheinfo: add support to override cache levels via device tree")
had already provided options to override cache information from the device tree.

This is what I am using.
Quote from commit:
The architecture explicitly states:

| You cannot make any inference about the actual sizes of caches based
| on these parameters.

[...]
> > +
> > +&soc0 {
> > +       L2_0: l2-cache0 {
> > +               compatible = "cache";
> 
> Is this documented?

Just what Documentation/devicetree/bindings/arm/cpu-capacity.txt states
as an example. We dont seem to have anything similar to
Documentation/devicetree/bindings/arm/l2c2x0.txt in armv8 as per the
comments in the document at least.

> 
> > +               cache-level = <2>;
> > +               cache-size = <0x80000>;
> > +               cache-line-size = <64>;
> > +               cache-sets = <512>;
> 
> Discoverable?

Same comment as L1 cache details.

> 
> > +               next-level-cache = <&msmc_l3>;
> > +       };
> > +
> > +       L2_1: l2-cache1 {
> > +               compatible = "cache";
> > +               cache-level = <2>;
> > +               cache-size = <0x80000>;
> > +               cache-line-size = <64>;
> > +               cache-sets = <512>;
> > +               next-level-cache = <&msmc_l3>;
> > +       };
> > +
> > +       msmc_l3: l3-cache0 {
> > +               compatible = "cache";
> 
> Is this something TI specific or follows the (ARM) architecture?

ARM Architecture permits an L3 Cache. TI implements it differently (we
dont use CCI), instead an L3 Cache is always active in our
implementation - backing memory is configurable and is designed to be
completely transparent to s/w running on ARMv8.

Description here is to meet the hardware description of cache topology
to be accurate.

[...]
> > diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
> > index 92770d84a288..be4570baad96 100644
> > --- a/drivers/soc/ti/Kconfig
> > +++ b/drivers/soc/ti/Kconfig
> > @@ -1,3 +1,17 @@
> > +# 64-bit ARM SoCs from TI
> > +if ARM64
> > +
> > +if ARCH_K3
> > +
> > +config ARCH_K3_AM6_SOC
> 
> This should be in another patch (or dropped?).

Agreed and split off into another patch.

[1]
Here is how the patch looks now (after incorporating changes suggested
by Tony as well) - Thoughts please:
https://github.com/nmenon/linux-2.6-playground/tree/upstream/next-20180604/k3-5-defconfig/arch/arm64/boot/dts/ti
has everything consolidated.

8<--