Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi

[Daniel Palmer]

Hi Andreas,

On Thu, 11 Jun 2020 at 22:39, Andreas FÃrber <afaerber@xxxxxxx> wrote:

> Can you split this up into three parts for easier review?

Understood. Will do.

> 2019-2020? Check elsewhere.

The patches are originally from 2019. I'll update everything.

> > + * Author: Daniel Palmer <daniel@xxxxxxxxx>
> > + */
> > +
> > +#include "infinity.dtsi"
> > +
> > +/ {
> > +     memory {
> > +             device_type = "memory";
> > +             reg = <0x20000000 0x4000000>;
>
> The memory node needs to become memory@20000000 then.
>
> > +     };
>
> I take it, this RAM is integrated? Maybe add some explanation of what
> this file is

Yes. The memory is integrated and the size depends on the specific chips
so the memory node is at the chip level dtsi and not the board level.

> > +};
> > diff --git a/arch/arm/boot/dts/infinity.dtsi b/arch/arm/boot/dts/infinity.dtsi
> > new file mode 100644
> > index 000000000000..25d379028689
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/infinity.dtsi
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 thingy.jp.
> > + * Author: Daniel Palmer <daniel@xxxxxxxxx>
> > + */
> > +
> > +#include "mstar-v7.dtsi"
> > +
> > +/ {
> > +};
>
> What do you intend to add here? Is it really needed? (same below)

The specific nodes will go into there. This is mostly an artefact of splitting
the device trees out of a more developed tree.

> > new file mode 100644
> > index 000000000000..cf5f18a07835
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/infinity3.dtsi
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 thingy.jp.
> > + * Author: Daniel Palmer <daniel@xxxxxxxxx>
> > + */
> > +
> > +#include "infinity.dtsi"
> > +
> > +/ {
> > +};
>
> Don't you anticipate incompatibilities between infinity and infinity3,
> i.e., things you don't want to inherit? Seems a bit optimistic. You can
> of course overwrite properties, but deleting is more difficult.

In my tree with drivers for the rest of the hardware it hasn't been a problem.
So far I've found infinity, infinity2, infinity3, infinity5 and
infinity6 chips. The memory
map for them is almost identical and the changes are adding in more
copies of things
like DMA controllers, extra clock blocks etc.

Having infinity.dtsi as the base for the newer versions should avoid
having duplication
of nodes that aren't common on the mstar armv7 level but are common to
the infinity subtypes.

There are two good examples of this:
For infinity? the USB and SD controllers are at the same place for all
of the chips I've
found so far. Unfortunately, despite using the same IP block and the
same driver those
blocks are in different places in the mercury5. At the moment with all
of the infinity chips
pulling in infinity.dtsi those nodes are only in infinity.dtsi and
mercury5.dtsi.
If infinity?.dtsi are all stacked on top of the mstarv7.dtsi base then
there will be a number of
copies of the same nodes.

> > diff --git a/arch/arm/boot/dts/mercury5.dtsi b/arch/arm/boot/dts/mercury5.dtsi
> > new file mode 100644
> > index 000000000000..25d379028689
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/mercury5.dtsi
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 thingy.jp.
> > + * Author: Daniel Palmer <daniel@xxxxxxxxx>
> > + */
> > +
> > +#include "mstar-v7.dtsi"
> > +
> > +/ {
> > +};
> > diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
> > new file mode 100644
> > index 000000000000..0fccc4ca52a4
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/mstar-v7.dtsi
>
> So this is the only file starting with mstar. Have you thought about
> prefixing infinity/mercury, so that they're grouped together?

I have been thinking about that. I didn't see any other dts in arm that had
the vendor as a prefix though. With arm64 everything is in per vendor
subdirectories
to achieve the same thing.

> > +             };
> > +
> > +             pm_uart: uart@1f221000 {
> > +                     compatible = "ns16550a";
> > +                     reg = <0x1f221000 0x100>;
> > +                     reg-shift = <3>;
> > +                     clock-frequency = <172000000>;
> > +                     status = "disabled";
> > +             };
>
> If you have any decent manuals for these SoCs,

Everything so far has been reverse engineered from an old 3.18
kernel, poking with a multimeter etc. I wish I had a decent manual.

> I suggest to check whether there are any internal buses that you may
> want to model as simple-bus for grouping. In-tree examples include meson
> and recently merged rtd1195 - it affects the reg addresses and unit addresses via
> suitable ranges mappings.

There is a bridge that is between the CPU and the peripheral registers
that doesn't quite fit simple-bus (from what I remember it needs a clk).
My plan was to introduce the thin driver for that bus (it just consumes the clks
it needs so they don't get disabled at the moment) after introducing
the clk support.

Thanks,

Daniel