[PATCH v1 1/3] fdt: validate/fix cells count on mtdpart fixup
Marek Vasut
marex at denx.de
Mon Jan 16 16:59:54 PST 2023
On 1/16/23 19:00, Francesco Dolcini wrote:
> On Mon, Jan 16, 2023 at 06:54:44PM +0100, Marek Vasut wrote:
>> On 1/16/23 15:20, Francesco Dolcini wrote:
>>> On Sun, Jan 15, 2023 at 03:35:25PM +0100, Marek Vasut wrote:
>>>> On 1/13/23 19:45, Francesco Dolcini wrote:
>>>>> From: Francesco Dolcini <francesco.dolcini at toradex.com>
>>>>>
>>>>> Fixup #size-cells value when updating the MTD partitions, this is
>>>>> required to prevent issues in case the MTD parent set #size-cells to
>>>>> zero.
>>>>> This could happen for example in the legacy case in which the partitions
>>>>> are created as direct child of the mtd node and that specific node has
>>>>> no children. Recent clean-up on Linux device tree files created a boot
>>>>> regression on colibri-imx7.
>>>>>
>>>>> This fixup has the limitation to assume 32-bit (#size-cells=1)
>>>>> addressing, therefore it will not work with device bigger than 4GiB.
>>>>>
>>>>> This change also enforce #address-cells to be the same as #size-cells,
>>>>> this was already silently enforced by fdt_node_set_part_info(), now this
>>>>> is checked explicitly and partition fixup will just fail in such case.
>>>>>
>>>>> In general board should not generally need nor use this functionality
>>>>> and should be just deprecated, passing mtdparts= in the kernel command
>>>>> line is the preferred way according to Linux MTD subsystem maintainer.
>>>>>
>>>>> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>>>>> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>> Cc: Miquel Raynal <miquel.raynal at bootlin.com>
>>>>> Signed-off-by: Francesco Dolcini <francesco.dolcini at toradex.com>
>>>>> ---
>>>>> common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++----------
>>>>> 1 file changed, 35 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>>>>> index dbceec6f2dcc..3aee826e60cf 100644
>>>>> --- a/common/fdt_support.c
>>>>> +++ b/common/fdt_support.c
>>>>> @@ -877,27 +877,20 @@ static int fdt_del_partitions(void *blob, int parent_offset)
>>>>> return 0;
>>>>> }
>>>>> -static int fdt_node_set_part_info(void *blob, int parent_offset,
>>>>> +/* This expects #address-cells and #size-cells to have same value */
>>>>> +static int fdt_node_set_part_info(void *blob, int parent_offset, int sizecell,
>>>>> struct mtd_device *dev)
>>>>> {
>>>>> struct list_head *pentry;
>>>>> struct part_info *part;
>>>>> int off, ndepth = 0;
>>>>> int part_num, ret;
>>>>> - int sizecell;
>>>>> char buf[64];
>>>>> ret = fdt_del_partitions(blob, parent_offset);
>>>>> if (ret < 0)
>>>>> return ret;
>>>>> - /*
>>>>> - * Check if size/address is 1 or 2 cells.
>>>>> - * We assume #address-cells and #size-cells have same value.
>>>>> - */
>>>>> - sizecell = fdt_getprop_u32_default_node(blob, parent_offset,
>>>>> - 0, "#size-cells", 1);
>>>>> -
>>>>> /*
>>>>> * Check if it is nand {}; subnode, adjust
>>>>> * the offset in this case
>>>>> @@ -992,6 +985,31 @@ err_prop:
>>>>> return ret;
>>>>> }
>>>>> +static int fdt_mtdparts_cell_cnt(void *fdt, int off)
>>>>> +{
>>>>> + int sizecell, addrcell;
>>>>> +
>>>>> + sizecell = fdt_getprop_u32_default_node(fdt, off, 0, "#size-cells", 0);
>>>>> + if (sizecell != 1 && sizecell != 2) {
>>>>> + printf("%s: Invalid or missing #size-cells %d value, assuming 1\n",
>>>>> + __func__, sizecell);
>>>>> +
>>>>> + sizecell = 1;
>>>>> + if (fdt_setprop_u32(fdt, off, "#size-cells", sizecell))
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + addrcell = fdt_getprop_u32_default_node(fdt, off, 0,
>>>>> + "#address-cells", 0);
>>>>> + if (addrcell != sizecell) {
>>>>> + printf("%s: Invalid #address-cells %d != #size-cells %d, aborting\n",
>>>>> + __func__, addrcell, sizecell);
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + return sizecell;
>>>>> +}
>>>>> +
>>>>> /*
>>>>> * Update partitions in nor/nand nodes using info from
>>>>> * mtdparts environment variable. The nodes to update are
>>>>> @@ -1037,12 +1055,19 @@ void fdt_fixup_mtdparts(void *blob, const struct node_info *node_info,
>>>>> dev = device_find(node_info[i].type, idx++);
>>>>> if (dev) {
>>>>> + int cell;
>>>>> +
>>>>> parts = fdt_subnode_offset(blob, noff,
>>>>> "partitions");
>>>>> if (parts < 0)
>>>>> parts = noff;
>>>>> - if (fdt_node_set_part_info(blob, parts, dev))
>>>>> + cell = fdt_mtdparts_cell_cnt(blob, parts);
>>>>> + if (cell < 0)
>>>>> + return;
>>>>> +
>>>>> + if (fdt_node_set_part_info(blob, parts,
>>>>> + cell, dev))
>>>>> return; /* return on error */
>>>>> }
>>>>> }
>>>>
>>>> Can you please include the resulting gpmi node content with this fixup
>>>> applied in the commit message , so it can be validated ?
>>>
>>> I will add it to v2, I would wait a little bit more time to get
>>> additional feedback sending it however.
>>>
>>> In the meantime here the output, but nothing really changed!
>>> What this change is doing is just
>>> - setting #size-cells to <1> when it is invalid
>>> - skip generation at all when #size-cells != #address-cells. Former
>>> code was just generating a broken table without any error
>>> message.
>>>
>>> Here what is generated for colibri-imx7
>>>
>>> nand-controller at 33002000 {
>>> compatible = "fsl,imx7d-gpmi-nand";
>>>
>>> #address-cells = <0x01>;
>>> #size-cells = <0x01>;
>>>
>>> [...snip...]
>>>
>>> partition at 0 {
>>> label = "mx7-bcb";
>>> reg = <0x00 0x80000>;
>>> };
>>>
>>> partition at 400000 {
>>> label = "ubi";
>>> reg = <0x400000 0x1fc00000>;
>>> };
>>>
>>> partition at 80000 {
>>> read_only;
>>> label = "u-boot1";
>>> reg = <0x80000 0x180000>;
>>> };
>>>
>>> partition at 380000 {
>>> label = "u-boot-env";
>>> reg = <0x380000 0x80000>;
>>> };
>>>
>>> partition at 200000 {
>>> read_only;
>>> label = "u-boot2";
>>> reg = <0x200000 0x180000>;
>>> };
>>> };
>>
>> This is what I was afraid of, shouldn't this contain the partitions in
>> per-chipselect sub-node instead of directly in the GPMI node ?
>
> That does not exists in my source DTS, this function just look for a
> partitions node and update it when it exists.
> I do not have a nand chip, and I do not want to add.
I know. I wonder if the function should convert the bindings to latest
greatest, but I am starting to feel like hard-coding this kind of
complex logic into bootloader is not a great idea .
> The reason is what I wrote in my other email, if I would do something
> like older U-Boot's would ignore it and just generate the partitions
> as direct children of the nand-controller.
>
> Commit 36fee2f7621e ("common: fdt_support: add support for "partitions"
> subnode to fdt_fixup_mtdparts()") was introduced only in v2022.04.
There is the Linux BOOT_CONFIG , that might be some sort of alternative
to complex kernel command line, but that's a bit more work. I figured
I'd mention it here.
More information about the linux-mtd
mailing list