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

[Jernej Škrabec]

Dne nedelja, 27. november 2022 ob 20:22:35 CET je Samuel Holland napisal(a):
> On 11/27/22 11:41, Andre Przywara wrote:
> > On Fri, 25 Nov 2022 17:46:48 -0600
> > 
> > Samuel Holland <samuel@xxxxxxxxxxxx> 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. 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
> > 
> > So I compared all the reg and interrupts properties against the T113
> > manual, they match, as far as they are described there. The undocumented
> > rest matches what we already have in other SoCs.
> > 
> > I noticed two things, though, mentioned inline below:
> > 
> > [...]
> > 
> >> +		display_clocks: clock-controller@5000000 {
> > 
> > The clocks and the two mixers are not children of a bus node anymore,
> > IIUC correctly this was to manage the SRAM control. Is that now handled
> > differently, or does the D1 generation not require this anymore?
> 
> The D1 family uses the DSP SRAM for extra space during early boot --
> this applies even to D1s, where the DSP is fused off. Since the DE SRAM
> is not used for this purpose, the "SRAM C" aka "boot mode" bit in the
> SRAM controller is cleared by default, thus mapping the DE SRAM to the
> peripheral. So the DE works without touching the syscon registers.
> 
> However, if I set the SRAM C bit, the DE stops working. So having the
> bus node would make some sense. But I do not know any address for the
> SRAM -- there is no "boot" address, and the "peripheral" address may not
> even be accessible to the CPU. So it is not possible to represent this
> with a mmio-sram node like the binding requires.
> 
> So I suppose we should either change the binding to allow a child
> allwinner,sun50i-a64-sram-c node with no address (the driver should work
> fine with this); or leave out the bus, and people who go poking around
> in the syscon registers get to keep the pieces. :)

Just leave out the bus. I think this is just a leftover...

Acked-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>

Best regards,
Jernej

> 
> >> +			compatible = "allwinner,sun20i-d1-de2-clk",
> >> +				     "allwinner,sun50i-h5-de2-
clk";
> >> +			reg = <0x5000000 0x10000>;
> >> +			clocks = <&ccu CLK_BUS_DE>, <&ccu CLK_DE>;
> >> +			clock-names = "bus", "mod";
> >> +			resets = <&ccu RST_BUS_DE>;
> >> +			#clock-cells = <1>;
> >> +			#reset-cells = <1>;
> >> +		};
> >> +
> >> +		mixer0: mixer@5100000 {
> >> +			compatible = "allwinner,sun20i-d1-de2-
mixer-0";
> >> +			reg = <0x5100000 0x100000>;
> >> +			clocks = <&display_clocks CLK_BUS_MIXER0>,
> >> +				 <&display_clocks CLK_MIXER0>;
> >> +			clock-names = "bus", "mod";
> >> +			resets = <&display_clocks RST_MIXER0>;
> >> +
> >> +			ports {
> >> +				#address-cells = <1>;
> >> +				#size-cells = <0>;
> >> +
> >> +				mixer0_out: port@1 {
> >> +					reg = <1>;
> >> +
> >> +					
mixer0_out_tcon_top_mixer0: endpoint {
> >> +						remote-
endpoint = <&tcon_top_mixer0_in_mixer0>;
> >> +					};
> >> +				};
> >> +			};
> >> +		};
> >> +
> >> +		mixer1: mixer@5200000 {
> >> +			compatible = "allwinner,sun20i-d1-de2-
mixer-1";
> >> +			reg = <0x5200000 0x100000>;
> >> +			clocks = <&display_clocks CLK_BUS_MIXER1>,
> >> +				 <&display_clocks CLK_MIXER1>;
> >> +			clock-names = "bus", "mod";
> >> +			resets = <&display_clocks RST_MIXER1>;
> >> +
> >> +			ports {
> >> +				#address-cells = <1>;
> >> +				#size-cells = <0>;
> >> +
> >> +				mixer1_out: port@1 {
> >> +					reg = <1>;
> >> +
> >> +					
mixer1_out_tcon_top_mixer1: endpoint {
> >> +						remote-
endpoint = <&tcon_top_mixer1_in_mixer1>;
> >> +					};
> >> +				};
> >> +			};
> >> +		};
> >> +
> >> +		dsi: dsi@5450000 {
> >> +			compatible = "allwinner,sun20i-d1-mipi-dsi",
> >> +				     "allwinner,sun50i-a100-mipi-
dsi";
> >> +			reg = <0x5450000 0x1000>;
> >> +			interrupts = <SOC_PERIPHERAL_IRQ(92) 
IRQ_TYPE_LEVEL_HIGH>;
> >> +			clocks = <&ccu CLK_BUS_MIPI_DSI>,
> >> +				 <&tcon_top CLK_TCON_TOP_DSI>;
> >> +			clock-names = "bus", "mod";
> >> +			resets = <&ccu RST_BUS_MIPI_DSI>;
> >> +			phys = <&dphy>;
> >> +			phy-names = "dphy";
> >> +			status = "disabled";
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >> +
> >> +			port {
> >> +				dsi_in_tcon_lcd0: endpoint {
> >> +					remote-endpoint = 
<&tcon_lcd0_out_dsi>;
> >> +				};
> >> +			};
> >> +		};
> >> +
> >> +		dphy: phy@5451000 {
> >> +			compatible = "allwinner,sun20i-d1-mipi-
dphy",
> >> +				     "allwinner,sun50i-a100-mipi-
dphy";
> >> +			reg = <0x5451000 0x1000>;
> >> +			interrupts = <SOC_PERIPHERAL_IRQ(92) 
IRQ_TYPE_LEVEL_HIGH>;
> > 
> > This is the same interrupt number as for the DSI controller above. Is
> > that correct, and can the drivers handle that?
> 
> Yes, it is correct. Currently, neither driver uses the interrupt, so we
> will just need to keep the sharing in mind if/when that happens.
> 
> Regards,
> Samuel