Re: [PATCH v2 04/12] riscv: dts: allwinner: Add the D1/D1s SoC devicetree

[Icenowy Zheng]

在 2022-11-26星期六的 16:03 +0000，Conor Dooley写道：
> On Fri, Nov 25, 2022 at 05:46:48PM -0600, Samuel Holland wrote:
> > D1 (aka D1-H), D1s (aka F133), R528, and T113 are a family of SoCs
> > based
> > on a single die, or at a pair of dies derived from the same design.
> > 
> > D1 and D1s contain a single T-HEAD Xuantie C906 CPU, whereas R528
> > and
> > T113 contain a pair of Cortex-A7's.
> 
> Is this "additionally contain" or a case of the D1 is the R528 but
> with
> s/arm/riscv/? It's the latter, right?

Technically they're the same die, but the CPU cores are selectively
enabled, and at least what Allwinner says is that D1 contains only RV
and R528 contains only ARM.

> 
> > D1 and R528 are the full version of
> > the chip with a BGA package, whereas D1s and T113 are low-pin-count
> > QFP
> > variants.
> > 
> > Because the original design supported both ARM and RISC-V CPUs,
> > some
> > peripherals are duplicated. In addition, all variants except D1s
> > contain
> > a HiFi 4 DSP with its own set of peripherals.
> > 
> > The devicetrees are organized to minimize duplication:
> >  - Common perhiperals are described in sunxi-d1s-t113.dtsi
> >  - DSP-related peripherals are described in sunxi-d1-t113.dtsi
> >  - RISC-V specific hardware is described in sun20i-d1s.dtsi
> >  - Functionality unique to the D1 variant is described in sun20i-
> > d1.dtsi
> > 
> > The SOC_PERIPHERAL_IRQ macro handles the different #interrupt-cells
> > values between the ARM (GIC) and RISC-V (PLIC) versions of the SoC.
> 
> Modulo the warnings I replied to the cover with & one minor comment
> below:
> Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> 
> > Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> > ---
> > 
> > Changes in v2:
> >  - Split into separate files for sharing with D1s/R528/T113
> >  - Use SOC_PERIPHERAL_IRQ macro for interrupts
> >  - Rename osc24M to dcxo and move the frequency to the board DTs
> >  - Drop analog LDOs due to the missing binding
> >  - Correct tcon_top DSI clock reference
> >  - Add DMIC, DSI controller, and DPHY (bindings are in linux-next)
> >  - Add CPU OPP table
> > 
> >  arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi  |  66 ++
> >  arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi |  76 ++
> >  .../boot/dts/allwinner/sunxi-d1-t113.dtsi     |  15 +
> >  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    | 844
> > ++++++++++++++++++
> >  4 files changed, 1001 insertions(+)
> >  create mode 100644 arch/riscv/boot/dts/allwinner/sun20i-d1.dtsi
> >  create mode 100644 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
> >  create mode 100644 arch/riscv/boot/dts/allwinner/sunxi-d1-
> > t113.dtsi
> >  create mode 100644 arch/riscv/boot/dts/allwinner/sunxi-d1s-
> > t113.dtsi
> 
> 
> > diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> > b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> > new file mode 100644
> > index 000000000000..c8815cbf0b46
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> > @@ -0,0 +1,844 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
> > +// Copyright (C) 2021-2022 Samuel Holland <samuel@xxxxxxxxxxxx>
> > +
> > +#include <dt-bindings/clock/sun6i-rtc.h>
> > +#include <dt-bindings/clock/sun8i-de2.h>
> > +#include <dt-bindings/clock/sun8i-tcon-top.h>
> > +#include <dt-bindings/clock/sun20i-d1-ccu.h>
> > +#include <dt-bindings/clock/sun20i-d1-r-ccu.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/reset/sun8i-de2.h>
> > +#include <dt-bindings/reset/sun20i-d1-ccu.h>
> > +#include <dt-bindings/reset/sun20i-d1-r-ccu.h>
> > +
> > +/ {
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       dcxo: dcxo-clk {
> > +               compatible = "fixed-clock";
> > +               /* This value must be overridden by the board */
> > +               clock-frequency = <0>;
> 
> Since this is a "must", can you drop the clock-frequency = <0> here
> so
> that if someone doesn't override it in their board dt-validate
> complains?
> 
> Thanks,
> Conor.
> 
> > +               clock-output-names = "dcxo";
> > +               #clock-cells = <0>;
> > +       };
> > +
>  
>