diff mbox series

[v3,8/9] mips: generic: add fdt fixup for Realtek reference board

Message ID 20240627043317.3751996-9-chris.packham@alliedtelesis.co.nz (mailing list archive)
State Superseded
Headers show
Series mips: Support for RTL9302C | expand

Commit Message

Chris Packham June 27, 2024, 4:33 a.m. UTC 
The bootloader used on the Realtek RTL9302C boards is an ancient vendor
fork of U-Boot that doesn't understand device trees. So to run a modern
kernel it is necessary use one of the APPENDED_DTB options.

When appending the DTB the inintrd information, if present, needs to be
inserted into the /chosen device tree node. The bootloader provides the
initrd start/size via the firmware environment. Add a fdt fixup that
will update the device tree with the initrd information.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v3:
    - None
    Changes in v2:
    - update compatible string

 arch/mips/generic/Makefile        |  1 +
 arch/mips/generic/board-realtek.c | 81 +++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 arch/mips/generic/board-realtek.c

Comments

Marek Behún June 27, 2024, 11:19 a.m. UTC | #1 
On Thu, 27 Jun 2024 16:33:16 +1200
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> +static __init int realtek_add_initrd(void *fdt)
> +{
> +	int node, err;
> +	u32 start, size;
> +
> +	node = fdt_path_offset(fdt, "/chosen");
> +	if (node < 0) {
> +		pr_err("/chosen node not found\n");

linux/printk.h

> +static const struct of_device_id realtek_of_match[] __initconst = {
> +	{
> +		.compatible = "realtek,rtl9302",
> +	},

One line instead of three?

Marek
Rob Herring (Arm) June 27, 2024, 5:48 p.m. UTC | #2 
On Wed, Jun 26, 2024 at 10:33 PM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> The bootloader used on the Realtek RTL9302C boards is an ancient vendor
> fork of U-Boot that doesn't understand device trees. So to run a modern
> kernel it is necessary use one of the APPENDED_DTB options.
>
> When appending the DTB the inintrd information, if present, needs to be
> inserted into the /chosen device tree node. The bootloader provides the
> initrd start/size via the firmware environment. Add a fdt fixup that
> will update the device tree with the initrd information.

Is this really specific to this board/soc? I think there are lots of
MIPS boards in this state. The code to handle all the possible
combinations of bootloader handoff information and sources of DTB is
quite the mess. Just for DTB source you have bootloader DTB, appended
DTB, or built-in DTB (and there's even logic if you have multiple of
those). Contrast that to arm32 ('the zoo"), where you have 2 choices:
bootloader DTB or appended DTB with legacy bootloader parameters
transferred to DTB. All the uglyness is contained and the kernel boot
deals with 1 possibility. </rant>

>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v3:
>     - None
>     Changes in v2:
>     - update compatible string
>
>  arch/mips/generic/Makefile        |  1 +
>  arch/mips/generic/board-realtek.c | 81 +++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+)
>  create mode 100644 arch/mips/generic/board-realtek.c
>
> diff --git a/arch/mips/generic/Makefile b/arch/mips/generic/Makefile
> index 56011d738441..ea0e4ad5e600 100644
> --- a/arch/mips/generic/Makefile
> +++ b/arch/mips/generic/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEGACY_BOARD_SEAD3)      += board-sead3.o
>  obj-$(CONFIG_LEGACY_BOARD_OCELOT)      += board-ocelot.o
>  obj-$(CONFIG_MACH_INGENIC)                     += board-ingenic.o
>  obj-$(CONFIG_VIRT_BOARD_RANCHU)                += board-ranchu.o
> +obj-$(CONFIG_MACH_REALTEK_RTL)         += board-realtek.o
> diff --git a/arch/mips/generic/board-realtek.c b/arch/mips/generic/board-realtek.c
> new file mode 100644
> index 000000000000..cd83fbf1968c
> --- /dev/null
> +++ b/arch/mips/generic/board-realtek.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Kernel license by default is GPL-2.0-only. Why do something different?

> +/*
> + * Copyright (C) 2024 Allied Telesis
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/libfdt.h>
> +#include <linux/of_address.h>

You aren't using this header.

> +#include <linux/types.h>
> +
> +#include <asm/fw/fw.h>
> +#include <asm/machine.h>
> +
> +static __init int realtek_add_initrd(void *fdt)
> +{
> +       int node, err;
> +       u32 start, size;
> +
> +       node = fdt_path_offset(fdt, "/chosen");
> +       if (node < 0) {
> +               pr_err("/chosen node not found\n");
> +               return -ENOENT;
> +       }
> +
> +       start = fw_getenvl("initrd_start");
> +       size = fw_getenvl("initrd_size");
> +
> +       if (start == 0 && size == 0)
> +               return 0;
> +
> +       pr_info("Adding initrd info from environment\n");
> +
> +       err = fdt_setprop_u32(fdt, node, "linux,initrd-start", start);
> +       if (err) {
> +               pr_err("unable to set initrd-start: %d\n", err);
> +               return err;
> +       }
> +
> +       err = fdt_setprop_u32(fdt, node, "linux,initrd-end", start + size);
> +       if (err) {
> +               pr_err("unable to set initrd-end: %d\n", err);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct mips_fdt_fixup realtek_fdt_fixups[] __initconst = {
> +       { realtek_add_initrd, "add initrd" },
> +       {},
> +};
> +
> +static __init const void *realtek_fixup_fdt(const void *fdt, const void *match_data)
> +{
> +       static unsigned char fdt_buf[16 << 10] __initdata;
> +       int err;
> +
> +       if (fdt_check_header(fdt))
> +               panic("Corrupt DT");
> +
> +       fw_init_cmdline();
> +
> +       err = apply_mips_fdt_fixups(fdt_buf, sizeof(fdt_buf), fdt, realtek_fdt_fixups);
> +       if (err)
> +               panic("Unable to fixup FDT: %d", err);
> +
> +       return fdt_buf;
> +
> +}
> +
> +static const struct of_device_id realtek_of_match[] __initconst = {
> +       {
> +               .compatible = "realtek,rtl9302",
> +       },
> +       {}
> +};
> +
> +MIPS_MACHINE(realtek) = {
> +       .matches = realtek_of_match,
> +       .fixup_fdt = realtek_fixup_fdt,
> +};
> --
> 2.45.2
>
>
Chris Packham June 30, 2024, 10:39 p.m. UTC | #3 
On 28/06/24 05:48, Rob Herring wrote:
> On Wed, Jun 26, 2024 at 10:33 PM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
>> The bootloader used on the Realtek RTL9302C boards is an ancient vendor
>> fork of U-Boot that doesn't understand device trees. So to run a modern
>> kernel it is necessary use one of the APPENDED_DTB options.
>>
>> When appending the DTB the inintrd information, if present, needs to be
>> inserted into the /chosen device tree node. The bootloader provides the
>> initrd start/size via the firmware environment. Add a fdt fixup that
>> will update the device tree with the initrd information.
> Is this really specific to this board/soc?

Based on my sample size of 1 I can't say. I thought this was a quirk of 
the realtek SDK version of U-Boot because it's based on something old 
and dt unaware. I did think about putting this code in a more generic 
place but in the end I went with something less invasive figuring it 
could be expanded on later if needed.

> I think there are lots of MIPS boards in this state.

If anyone can confirm that I'd be happy to make the workaround kick in 
if the fw_env has an initrd_start in it. I'm not sure if there is any 
risk in that approach. I don't know how other mips boards are currently 
booting using the generic board support.

> The code to handle all the possible
> combinations of bootloader handoff information and sources of DTB is
> quite the mess. Just for DTB source you have bootloader DTB, appended
> DTB, or built-in DTB (and there's even logic if you have multiple of
> those). Contrast that to arm32 ('the zoo"), where you have 2 choices:
> bootloader DTB or appended DTB with legacy bootloader parameters
> transferred to DTB. All the uglyness is contained and the kernel boot
> deals with 1 possibility. </rant>
>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v3:
>>      - None
>>      Changes in v2:
>>      - update compatible string
>>
>>   arch/mips/generic/Makefile        |  1 +
>>   arch/mips/generic/board-realtek.c | 81 +++++++++++++++++++++++++++++++
>>   2 files changed, 82 insertions(+)
>>   create mode 100644 arch/mips/generic/board-realtek.c
>>
>> diff --git a/arch/mips/generic/Makefile b/arch/mips/generic/Makefile
>> index 56011d738441..ea0e4ad5e600 100644
>> --- a/arch/mips/generic/Makefile
>> +++ b/arch/mips/generic/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEGACY_BOARD_SEAD3)      += board-sead3.o
>>   obj-$(CONFIG_LEGACY_BOARD_OCELOT)      += board-ocelot.o
>>   obj-$(CONFIG_MACH_INGENIC)                     += board-ingenic.o
>>   obj-$(CONFIG_VIRT_BOARD_RANCHU)                += board-ranchu.o
>> +obj-$(CONFIG_MACH_REALTEK_RTL)         += board-realtek.o
>> diff --git a/arch/mips/generic/board-realtek.c b/arch/mips/generic/board-realtek.c
>> new file mode 100644
>> index 000000000000..cd83fbf1968c
>> --- /dev/null
>> +++ b/arch/mips/generic/board-realtek.c
>> @@ -0,0 +1,81 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
> Kernel license by default is GPL-2.0-only. Why do something different?

Copy and paste from the existing arch/mips/generic/board-*.c. I'm fine 
with making it GPL-2.0-only

>> +/*
>> + * Copyright (C) 2024 Allied Telesis
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/libfdt.h>
>> +#include <linux/of_address.h>
> You aren't using this header.
Ack.
>> +#include <linux/types.h>
>> +
>> +#include <asm/fw/fw.h>
>> +#include <asm/machine.h>
>> +
>> +static __init int realtek_add_initrd(void *fdt)
>> +{
>> +       int node, err;
>> +       u32 start, size;
>> +
>> +       node = fdt_path_offset(fdt, "/chosen");
>> +       if (node < 0) {
>> +               pr_err("/chosen node not found\n");
>> +               return -ENOENT;
>> +       }
>> +
>> +       start = fw_getenvl("initrd_start");
>> +       size = fw_getenvl("initrd_size");
>> +
>> +       if (start == 0 && size == 0)
>> +               return 0;
>> +
>> +       pr_info("Adding initrd info from environment\n");
>> +
>> +       err = fdt_setprop_u32(fdt, node, "linux,initrd-start", start);
>> +       if (err) {
>> +               pr_err("unable to set initrd-start: %d\n", err);
>> +               return err;
>> +       }
>> +
>> +       err = fdt_setprop_u32(fdt, node, "linux,initrd-end", start + size);
>> +       if (err) {
>> +               pr_err("unable to set initrd-end: %d\n", err);
>> +               return err;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct mips_fdt_fixup realtek_fdt_fixups[] __initconst = {
>> +       { realtek_add_initrd, "add initrd" },
>> +       {},
>> +};
>> +
>> +static __init const void *realtek_fixup_fdt(const void *fdt, const void *match_data)
>> +{
>> +       static unsigned char fdt_buf[16 << 10] __initdata;
>> +       int err;
>> +
>> +       if (fdt_check_header(fdt))
>> +               panic("Corrupt DT");
>> +
>> +       fw_init_cmdline();
>> +
>> +       err = apply_mips_fdt_fixups(fdt_buf, sizeof(fdt_buf), fdt, realtek_fdt_fixups);
>> +       if (err)
>> +               panic("Unable to fixup FDT: %d", err);
>> +
>> +       return fdt_buf;
>> +
>> +}
>> +
>> +static const struct of_device_id realtek_of_match[] __initconst = {
>> +       {
>> +               .compatible = "realtek,rtl9302",
>> +       },
>> +       {}
>> +};
>> +
>> +MIPS_MACHINE(realtek) = {
>> +       .matches = realtek_of_match,
>> +       .fixup_fdt = realtek_fixup_fdt,
>> +};
>> --
>> 2.45.2
>>
>>
diff mbox series

Patch

diff --git a/arch/mips/generic/Makefile b/arch/mips/generic/Makefile
index 56011d738441..ea0e4ad5e600 100644
--- a/arch/mips/generic/Makefile
+++ b/arch/mips/generic/Makefile
@@ -13,3 +13,4 @@  obj-$(CONFIG_LEGACY_BOARD_SEAD3)	+= board-sead3.o
 obj-$(CONFIG_LEGACY_BOARD_OCELOT)	+= board-ocelot.o
 obj-$(CONFIG_MACH_INGENIC)			+= board-ingenic.o
 obj-$(CONFIG_VIRT_BOARD_RANCHU)		+= board-ranchu.o
+obj-$(CONFIG_MACH_REALTEK_RTL)		+= board-realtek.o
diff --git a/arch/mips/generic/board-realtek.c b/arch/mips/generic/board-realtek.c
new file mode 100644
index 000000000000..cd83fbf1968c
--- /dev/null
+++ b/arch/mips/generic/board-realtek.c
@@ -0,0 +1,81 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2024 Allied Telesis
+ */
+
+#include <linux/errno.h>
+#include <linux/libfdt.h>
+#include <linux/of_address.h>
+#include <linux/types.h>
+
+#include <asm/fw/fw.h>
+#include <asm/machine.h>
+
+static __init int realtek_add_initrd(void *fdt)
+{
+	int node, err;
+	u32 start, size;
+
+	node = fdt_path_offset(fdt, "/chosen");
+	if (node < 0) {
+		pr_err("/chosen node not found\n");
+		return -ENOENT;
+	}
+
+	start = fw_getenvl("initrd_start");
+	size = fw_getenvl("initrd_size");
+
+	if (start == 0 && size == 0)
+		return 0;
+
+	pr_info("Adding initrd info from environment\n");
+
+	err = fdt_setprop_u32(fdt, node, "linux,initrd-start", start);
+	if (err) {
+		pr_err("unable to set initrd-start: %d\n", err);
+		return err;
+	}
+
+	err = fdt_setprop_u32(fdt, node, "linux,initrd-end", start + size);
+	if (err) {
+		pr_err("unable to set initrd-end: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static const struct mips_fdt_fixup realtek_fdt_fixups[] __initconst = {
+	{ realtek_add_initrd, "add initrd" },
+	{},
+};
+
+static __init const void *realtek_fixup_fdt(const void *fdt, const void *match_data)
+{
+	static unsigned char fdt_buf[16 << 10] __initdata;
+	int err;
+
+	if (fdt_check_header(fdt))
+		panic("Corrupt DT");
+
+	fw_init_cmdline();
+
+	err = apply_mips_fdt_fixups(fdt_buf, sizeof(fdt_buf), fdt, realtek_fdt_fixups);
+	if (err)
+		panic("Unable to fixup FDT: %d", err);
+
+	return fdt_buf;
+
+}
+
+static const struct of_device_id realtek_of_match[] __initconst = {
+	{
+		.compatible = "realtek,rtl9302",
+	},
+	{}
+};
+
+MIPS_MACHINE(realtek) = {
+	.matches = realtek_of_match,
+	.fixup_fdt = realtek_fixup_fdt,
+};