[PATCH] of: Add Google Juniper to excluded default cells list

[PATCH] of: Add Google Juniper to excluded default cells list

[Krzysztof Kozlowski]

Google Juniper platforms have a very old bootloader which populates
/firmware node without proper address/size-cells leading to warnings:

  Missing '#address-cells' in /firmware
  WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
  Modules linked in:
  CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
  Hardware name: Google juniper sku16 board (DT)
  ...
  Missing '#size-cells' in /firmware
  WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
  Modules linked in:
  CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
  Tainted: [W]=WARN
  Hardware name: Google juniper sku16 board (DT)

The platform won't receive updated bootloader/firmware so add it to
excluded platform list to silence the warning.

Reported-by: Sasha Levin <sashal@kernel.org>
Closes: https://lore.kernel.org/all/Z0NUdoG17EwuCigT@sashalap/
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chen-Yu Tsai <wenst@chromium.org>
Cc: Conor Dooley <conor@kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/of/base.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a8b0c42bdc8e..13f0b2877ee0 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -56,6 +56,16 @@ DEFINE_MUTEX(of_mutex);
  */
 DEFINE_RAW_SPINLOCK(devtree_lock);
 
+/*
+ * List of machines running old firmware without explicit #address-cells and
+ * #size-cells values for parent nodes, which are most likely not going get any
+ * update.
+ */
+static const char * const excluded_default_cells_compats[] = {
+	"google,juniper",
+	NULL
+};
+
 bool of_node_name_eq(const struct device_node *np, const char *name)
 {
 	const char *node_name;
@@ -91,6 +101,17 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
 	IS_ENABLED(CONFIG_SPARC) \
 )
 
+static bool excluded_default_cells_machines(void)
+{
+	/* Do not repeat the machine checks for every bus */
+	static int excluded_machine = -1;
+
+	if (excluded_machine < 0)
+		excluded_machine = of_machine_compatible_match(excluded_default_cells_compats);
+
+	return !!excluded_machine;
+}
+
 int of_bus_n_addr_cells(struct device_node *np)
 {
 	u32 cells;
@@ -103,7 +124,7 @@ int of_bus_n_addr_cells(struct device_node *np)
 		 * is deprecated. Any platforms which hit this warning should
 		 * be added to the excluded list.
 		 */
-		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
+		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS && !excluded_default_cells_machines(),
 			  "Missing '#address-cells' in %pOF\n", np);
 	}
 	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
@@ -125,12 +146,13 @@ int of_bus_n_size_cells(struct device_node *np)
 	for (; np; np = np->parent) {
 		if (!of_property_read_u32(np, "#size-cells", &cells))
 			return cells;
+
 		/*
 		 * Default root value and walking parent nodes for "#size-cells"
 		 * is deprecated. Any platforms which hit this warning should
 		 * be added to the excluded list.
 		 */
-		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
+		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS && !excluded_default_cells_machines(),
 			  "Missing '#size-cells' in %pOF\n", np);
 	}
 	return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
-- 
2.43.0

Re: [PATCH] of: Add Google Juniper to excluded default cells list

[Krzysztof Kozlowski]

On 25/11/2024 12:31, Krzysztof Kozlowski wrote:
> Google Juniper platforms have a very old bootloader which populates
> /firmware node without proper address/size-cells leading to warnings:
> 
>   Missing '#address-cells' in /firmware
>   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
>   Modules linked in:
>   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>   Hardware name: Google juniper sku16 board (DT)
>   ...
>   Missing '#size-cells' in /firmware
>   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
>   Modules linked in:
>   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>   Tainted: [W]=WARN
>   Hardware name: Google juniper sku16 board (DT)
> 
> The platform won't receive updated bootloader/firmware so add it to
> excluded platform list to silence the warning.
> 
> Reported-by: Sasha Levin <sashal@kernel.org>
> Closes: https://lore.kernel.org/all/Z0NUdoG17EwuCigT@sashalap/
> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Chen-Yu Tsai <wenst@chromium.org>
> Cc: Conor Dooley <conor@kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  drivers/of/base.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index a8b0c42bdc8e..13f0b2877ee0 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -56,6 +56,16 @@ DEFINE_MUTEX(of_mutex);
>   */
>  DEFINE_RAW_SPINLOCK(devtree_lock);
>  
> +/*
> + * List of machines running old firmware without explicit #address-cells and
> + * #size-cells values for parent nodes, which are most likely not going get any
> + * update.
> + */
> +static const char * const excluded_default_cells_compats[] = {
> +	"google,juniper",
> +	NULL
> +};
> +
>  bool of_node_name_eq(const struct device_node *np, const char *name)
>  {
>  	const char *node_name;
> @@ -91,6 +101,17 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
>  	IS_ENABLED(CONFIG_SPARC) \
>  )
>  
> +static bool excluded_default_cells_machines(void)
> +{
> +	/* Do not repeat the machine checks for every bus */
> +	static int excluded_machine = -1;
> +
> +	if (excluded_machine < 0)
> +		excluded_machine = of_machine_compatible_match(excluded_default_cells_compats);
> +
> +	return !!excluded_machine;
> +}
> +
>  int of_bus_n_addr_cells(struct device_node *np)
>  {
>  	u32 cells;
> @@ -103,7 +124,7 @@ int of_bus_n_addr_cells(struct device_node *np)
>  		 * is deprecated. Any platforms which hit this warning should
>  		 * be added to the excluded list.
>  		 */
> -		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS && !excluded_default_cells_machines(),
>  			  "Missing '#address-cells' in %pOF\n", np);
>  	}
>  	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
> @@ -125,12 +146,13 @@ int of_bus_n_size_cells(struct device_node *np)
>  	for (; np; np = np->parent) {
>  		if (!of_property_read_u32(np, "#size-cells", &cells))
>  			return cells;
> +


This was not intentional, I'll fix it in v2.

Obviously this code is not really SMP aware, but even with store tearing
I don't think it will be issue. Worst case the
of_machine_compatible_match() will be called more than one, which is not
fatal and might not justify atomics or locks.

Best regards,
Krzysztof

Re: [PATCH] of: Add Google Juniper to excluded default cells list

[AngeloGioacchino Del Regno]

Il 25/11/24 12:38, Krzysztof Kozlowski ha scritto:
> On 25/11/2024 12:31, Krzysztof Kozlowski wrote:
>> Google Juniper platforms have a very old bootloader which populates
>> /firmware node without proper address/size-cells leading to warnings:
>>
>>    Missing '#address-cells' in /firmware
>>    WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
>>    Modules linked in:
>>    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>>    Hardware name: Google juniper sku16 board (DT)
>>    ...
>>    Missing '#size-cells' in /firmware
>>    WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
>>    Modules linked in:
>>    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>>    Tainted: [W]=WARN
>>    Hardware name: Google juniper sku16 board (DT)
>>
>> The platform won't receive updated bootloader/firmware so add it to
>> excluded platform list to silence the warning.
>>
>> Reported-by: Sasha Levin <sashal@kernel.org>
>> Closes: https://lore.kernel.org/all/Z0NUdoG17EwuCigT@sashalap/
>> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> Cc: Matthias Brugger <matthias.bgg@gmail.com>
>> Cc: Chen-Yu Tsai <wenst@chromium.org>
>> Cc: Conor Dooley <conor@kernel.org>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   drivers/of/base.c | 26 ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index a8b0c42bdc8e..13f0b2877ee0 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -56,6 +56,16 @@ DEFINE_MUTEX(of_mutex);
>>    */
>>   DEFINE_RAW_SPINLOCK(devtree_lock);
>>   
>> +/*
>> + * List of machines running old firmware without explicit #address-cells and
>> + * #size-cells values for parent nodes, which are most likely not going get any
>> + * update.
>> + */
>> +static const char * const excluded_default_cells_compats[] = {
>> +	"google,juniper",
>> +	NULL
>> +};
>> +
>>   bool of_node_name_eq(const struct device_node *np, const char *name)
>>   {
>>   	const char *node_name;
>> @@ -91,6 +101,17 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
>>   	IS_ENABLED(CONFIG_SPARC) \
>>   )
>>   
>> +static bool excluded_default_cells_machines(void)
>> +{
>> +	/* Do not repeat the machine checks for every bus */
>> +	static int excluded_machine = -1;
>> +
>> +	if (excluded_machine < 0)
>> +		excluded_machine = of_machine_compatible_match(excluded_default_cells_compats);
>> +
>> +	return !!excluded_machine;
>> +}
>> +
>>   int of_bus_n_addr_cells(struct device_node *np)
>>   {
>>   	u32 cells;
>> @@ -103,7 +124,7 @@ int of_bus_n_addr_cells(struct device_node *np)
>>   		 * is deprecated. Any platforms which hit this warning should
>>   		 * be added to the excluded list.
>>   		 */
>> -		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
>> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS && !excluded_default_cells_machines(),
>>   			  "Missing '#address-cells' in %pOF\n", np);
>>   	}
>>   	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>> @@ -125,12 +146,13 @@ int of_bus_n_size_cells(struct device_node *np)
>>   	for (; np; np = np->parent) {
>>   		if (!of_property_read_u32(np, "#size-cells", &cells))
>>   			return cells;
>> +
> 
> 
> This was not intentional, I'll fix it in v2.
> 
> Obviously this code is not really SMP aware, but even with store tearing
> I don't think it will be issue. Worst case the
> of_machine_compatible_match() will be called more than one, which is not
> fatal and might not justify atomics or locks.
> 
> Best regards,
> Krzysztof


For v2, feel free to add my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH] of: Add Google Juniper to excluded default cells list

[Rob Herring]

On Mon, Nov 25, 2024 at 5:32 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Google Juniper platforms have a very old bootloader which populates
> /firmware node without proper address/size-cells leading to warnings:
>
>   Missing '#address-cells' in /firmware
>   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
>   Modules linked in:
>   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>   Hardware name: Google juniper sku16 board (DT)
>   ...
>   Missing '#size-cells' in /firmware
>   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
>   Modules linked in:
>   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
>   Tainted: [W]=WARN
>   Hardware name: Google juniper sku16 board (DT)
>
> The platform won't receive updated bootloader/firmware so add it to
> excluded platform list to silence the warning.

I'm interested to know what needs these in /firmware. /firmware is
supposed to be for things without an MMIO interface. An alternative
solution is to add the properties. That doesn't require
CONFIG_OF_DYNAMIC and is often the approach powerpc uses.

I'm also wondering if ranges is also missing?

Rob

Re: [PATCH] of: Add Google Juniper to excluded default cells list

[Chen-Yu Tsai]

On Mon, Nov 25, 2024 at 11:46 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Nov 25, 2024 at 5:32 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > Google Juniper platforms have a very old bootloader which populates
> > /firmware node without proper address/size-cells leading to warnings:
> >
> >   Missing '#address-cells' in /firmware
> >   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
> >   Modules linked in:
> >   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
> >   Hardware name: Google juniper sku16 board (DT)
> >   ...
> >   Missing '#size-cells' in /firmware
> >   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
> >   Modules linked in:
> >   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
> >   Tainted: [W]=WARN
> >   Hardware name: Google juniper sku16 board (DT)
> >
> > The platform won't receive updated bootloader/firmware so add it to
> > excluded platform list to silence the warning.

Unfortunately this isn't the only platform affected. AFAIK basically any
ARM-based Chromebook out there has the same issue.

> I'm interested to know what needs these in /firmware. /firmware is
> supposed to be for things without an MMIO interface. An alternative
> solution is to add the properties. That doesn't require
> CONFIG_OF_DYNAMIC and is often the approach powerpc uses.

The regs point to a memory region that contains bits left by coreboot,
the coreboot table and the CBMEM region. The latter encompasses the
former, and also includes things such as the coreboot boot log.
These are covered by the binding firmware/coreboot.txt and supported
by drivers under drivers/firmware/google/.

> I'm also wondering if ranges is also missing?

It has an empty "ranges" property. I dug through our repository, and it
looks like the code was added [1] back in the ARMv7 days for the Nyan
(Tegra K1) series of Chromebooks. The code was never updated to use
64-bit addresses.


ChenYu

[1] https://crrev.com/c/203693

Re: [PATCH] of: Add Google Juniper to excluded default cells list

[Rob Herring]

On Mon, Nov 25, 2024 at 09:46:10AM -0600, Rob Herring wrote:
> On Mon, Nov 25, 2024 at 5:32 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > Google Juniper platforms have a very old bootloader which populates
> > /firmware node without proper address/size-cells leading to warnings:
> >
> >   Missing '#address-cells' in /firmware
> >   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x90/0xf0
> >   Modules linked in:
> >   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
> >   Hardware name: Google juniper sku16 board (DT)
> >   ...
> >   Missing '#size-cells' in /firmware
> >   WARNING: CPU: 0 PID: 1 at drivers/of/base.c:133 of_bus_n_size_cells+0x90/0xf0
> >   Modules linked in:
> >   CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.12.0 #1 933ab9971ff4d5dc58cb378a96f64c7f72e3454d
> >   Tainted: [W]=WARN
> >   Hardware name: Google juniper sku16 board (DT)
> >
> > The platform won't receive updated bootloader/firmware so add it to
> > excluded platform list to silence the warning.
> 
> I'm interested to know what needs these in /firmware. /firmware is
> supposed to be for things without an MMIO interface. An alternative
> solution is to add the properties. That doesn't require
> CONFIG_OF_DYNAMIC and is often the approach powerpc uses.

Here's the fixup based approach I had in mind. The main possible issue I 
see is it doesn't run until of_core_init() which happens at driver core 
init. 

static void dup_parent_cells(struct device_node *np, const char *cellname)
{
	struct device_node __free(device_node) *parent = of_get_parent(np);
	struct property *prop;

	if (of_property_present(np, cellname))
		return;

	prop = of_find_property(parent, cellname, NULL);
	if (prop)
		prop = __of_prop_dup(prop, GFP_KERNEL);
	if (!prop)
		return;
	of_add_property(np, prop);
}

/*
 * Some Google Chromebooks have MMIO addresses in firmware nodes and fail to
 * populate /firmware node with #address-cells and #size-cells.
 */
static void fixup_firmware_cells(void)
{
	struct device_node __free(device_node) *np = of_find_node_by_path("/firmware");

	if (!np || !of_property_present(np, "ranges"))
		return;

	dup_parent_cells(np, "#size-cells");
	dup_parent_cells(np, "#address-cells");
}

void of_apply_fixups(void)
{
	fixup_firmware_cells();
}