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

[Samuel Holland <samuel@sholland.org>]

On 11/27/22 11:41, Andre Przywara wrote:
> On Fri, 25 Nov 2022 17:46:48 -0600
> Samuel Holland <samuel@sholland.org> 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. :)

>> +			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