RE: [PATCH v3 2/2] arm64: dts: realtek: Add RTD1319 SoC and Realtek PymParticle EVB

["James Tai [戴志峰]" <james.tai@realtek.com>]

Hi Andreas,

> Am 04.02.20 um 15:52 schrieb James Tai:
> > diff --git a/arch/arm64/boot/dts/realtek/rtd1319-pymparticle.dts
> > b/arch/arm64/boot/dts/realtek/rtd1319-pymparticle.dts
> > new file mode 100644
> > index 000000000000..2a36d220fef6
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/realtek/rtd1319-pymparticle.dts
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> > +/*
> > + * Copyright (c) 2019 Realtek Semiconductor Corp.
> 
> 2019-2020? (also elsewhere)
> 
Yes. It should be changed to "2019-2020".

> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "rtd1319.dtsi"
> > +
> > +/ {
> > +	compatible = "realtek,pymparticle", "realtek,rtd1319";
> > +	model = "Realtek PymParticle EVB";
> > +
> > +	memory@2e000 {
> > +		device_type = "memory";
> > +		reg = <0x2e000 0x3ffd2000>; /* boot ROM to 1 GiB or 2 GiB */
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:460800n8";
> > +	};
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +	};
> > +};
> > +
> > +/* debug console (J1) */
> > +&uart0 {
> > +	status = "okay";
> > +};
> > +
> > +/* M.2 slot (CON8) */
> 
> Also J14 and CON2 (unless the board is mislabeled?).
> 
> /* J14 and M.2 slots (CON2, CON8) */ ?
> 
Yes. It should be changed to "M.2 slots (CON2, CON8)".

> > +&uart1 {
> > +	status = "disabled";
> > +};
> > +
> > +/* GPIO connector (T1) */
> > +&uart2 {
> > +	status = "disabled";
> > +};
> > diff --git a/arch/arm64/boot/dts/realtek/rtd1319.dtsi
> > b/arch/arm64/boot/dts/realtek/rtd1319.dtsi
> > new file mode 100644
> > index 000000000000..1dcee00009cd
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/realtek/rtd1319.dtsi
> > @@ -0,0 +1,12 @@
> > +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> > +/*
> > + * Realtek RTD1319 SoC
> > + *
> > + * Copyright (c) 2019 Realtek Semiconductor Corp.
> > + */
> > +
> > +#include "rtd13xx.dtsi"
> > +
> > +/ {
> > +	compatible = "realtek,rtd1319";
> > +};
> > diff --git a/arch/arm64/boot/dts/realtek/rtd13xx.dtsi
> > b/arch/arm64/boot/dts/realtek/rtd13xx.dtsi
> > new file mode 100644
> > index 000000000000..f6d73f18345d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/realtek/rtd13xx.dtsi
> > @@ -0,0 +1,213 @@
> > +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> > +/*
> > + * Realtek RTD13xx SoC family
> > + *
> > + * Copyright (c) 2019 Realtek Semiconductor Corp.
> > + */
> > +
> > +/memreserve/	0x0000000000000000 0x000000000002e000; /* Boot ROM
> */
> 
> Can you check whether your U-Boot and LK respectively need this memreserve
> entry, here and for previous SoCs? Because for RTD16xx we don't seem to have
> any memreserve entries at all. We do have it in rtd139x.dtsi, rtd129x.dtsi and
> rtd1195.dtsi.
>
I've checked that the boot code doesn't need this memreserve entry.
Therefore, I will remove it.

> Unrelated: Since we're carving out the 2e000 or so from /memory node and
> mapping ranges for /soc, I've been wondering whether we should represent
> the Boot ROM as node somehow. But since it's a ROM with (I assume) binary
> code only, I didn't see any need to have it accessible as mtd-rom device, so it's
> way down my to-do list to research how other mainline platforms might model
> their boot ROMs... (maybe your team has time, or someone reading happens
> to know?)
> 
I'll add it to my to-do list.

> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Docum
> entation/devicetree/bindings/mtd/mtd-physmap.txt
> 
> > +/memreserve/	0x000000000002e000 0x0000000000100000; /* Boot
> loader */
> 
> Is this a) correctly sized (not 0xd2000?) and b) still needed? I thought the
> documented sub-0x100000 memory corruption were fixed in newer BSPs?
> 
We're in the process of re-planning the memory layout,
so that address will move to new address.

> > +/memreserve/	0x000000000f400000 0x0000000000500000; /* Video FW
> */
> > +/memreserve/	0x000000000f900000 0x0000000000500000; /* Audio FW
> */
> > +/memreserve/	0x0000000010000000 0x0000000000014000; /* Audio FW
> RAM */
> [snip]
> 
> Are these needed for the bootloader not to overwrite preloaded firmware, or
> could these become /mem-reserve sub-nodes instead?
> 
Yes. These could become /mem-reserve sub-nodes instead.

> Long-term I'm assuming we would move the responsibility for loading these to
> the new kernel drivers (so that the bootloader doesn't need to take care
> anymore) and ship the needed blobs in linux-firmware.git?
> 
> Or is the video FW needed by the bootloader itself for HDMI/DP output?
>
I agree with you. The video FW can be loaded into memory through this mechanism.
But the audio FW needed by the bootloader itself for HDMI/DP output. 
Therefore, the audio FW can't be loaded into memory through it.

Thank you.

Regards,
James