[PATCH V2] mtd: call of_platform_populate() for MTD partitions

[PATCH V2] mtd: call of_platform_populate() for MTD partitions

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Until this change MTD subsystem supported handling partitions only with
MTD partitions parsers. That's a specific / limited API designed around
partitions.

Some MTD partitions may however require different handling. They may
contain specific data that needs to be parsed and somehow extracted. For
that purpose MTD subsystem should allow binding of standard platform
drivers.

An example can be U-Boot (sub)partition with environment variables.
There exist a "u-boot,env" DT binding for MTD (sub)partition that
requires an NVMEM driver.

Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Fix case for # CONFIG_MTD_PARTITIONED_MASTER is not set
    master->dev can't be used blindly as it may point to unregistered
    device and cause WARNINGs
---
 drivers/mtd/mtdpart.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 357661b62c94..4971fa69d076 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -17,6 +17,7 @@
 #include <linux/mtd/partitions.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 
 #include "mtdcore.h"
 
@@ -577,10 +578,16 @@ static int mtd_part_of_parse(struct mtd_info *master,
 	struct mtd_part_parser *parser;
 	struct device_node *np;
 	struct property *prop;
+	struct device *dev;
 	const char *compat;
 	const char *fixed = "fixed-partitions";
 	int ret, err = 0;
 
+	if (mtd_is_partition(master) || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
+		dev = &master->dev;
+	else
+		dev = master->dev.parent;
+
 	np = mtd_get_of_node(master);
 	if (mtd_is_partition(master))
 		of_node_get(np);
@@ -593,6 +600,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 			continue;
 		ret = mtd_part_do_parse(parser, master, pparts, NULL);
 		if (ret > 0) {
+			of_platform_populate(np, NULL, NULL, dev);
 			of_node_put(np);
 			return ret;
 		}
@@ -600,6 +608,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
 		if (ret < 0 && !err)
 			err = ret;
 	}
+	of_platform_populate(np, NULL, NULL, dev);
 	of_node_put(np);
 
 	/*
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2] mtd: call of_platform_populate() for MTD partitions

[Miquel Raynal]

Hi Rafał,

zajec5@gmail.com wrote on Wed,  4 May 2022 21:44:48 +0200:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Until this change MTD subsystem supported handling partitions only with
> MTD partitions parsers. That's a specific / limited API designed around
> partitions.
> 
> Some MTD partitions may however require different handling. They may
> contain specific data that needs to be parsed and somehow extracted. For
> that purpose MTD subsystem should allow binding of standard platform
> drivers.
> 
> An example can be U-Boot (sub)partition with environment variables.
> There exist a "u-boot,env" DT binding for MTD (sub)partition that
> requires an NVMEM driver.
> 
> Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Fix case for # CONFIG_MTD_PARTITIONED_MASTER is not set
>     master->dev can't be used blindly as it may point to unregistered
>     device and cause WARNINGs
> ---
>  drivers/mtd/mtdpart.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 357661b62c94..4971fa69d076 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -17,6 +17,7 @@
>  #include <linux/mtd/partitions.h>
>  #include <linux/err.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  
>  #include "mtdcore.h"
>  
> @@ -577,10 +578,16 @@ static int mtd_part_of_parse(struct mtd_info *master,
>  	struct mtd_part_parser *parser;
>  	struct device_node *np;
>  	struct property *prop;
> +	struct device *dev;
>  	const char *compat;
>  	const char *fixed = "fixed-partitions";
>  	int ret, err = 0;
>  
> +	if (mtd_is_partition(master) || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))

Are you sure about this condition? Isn't accessing master->dev.parent
going to fail if !IS_ENABLED(PARTITIONED_MASTER) ?

I'm not 100% sure my remark is correct but I fail to get the logic
here.

> +		dev = &master->dev;
> +	else
> +		dev = master->dev.parent;
> +
>  	np = mtd_get_of_node(master);
>  	if (mtd_is_partition(master))
>  		of_node_get(np);
> @@ -593,6 +600,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
>  			continue;
>  		ret = mtd_part_do_parse(parser, master, pparts, NULL);
>  		if (ret > 0) {
> +			of_platform_populate(np, NULL, NULL, dev);
>  			of_node_put(np);
>  			return ret;
>  		}
> @@ -600,6 +608,7 @@ static int mtd_part_of_parse(struct mtd_info *master,
>  		if (ret < 0 && !err)
>  			err = ret;
>  	}
> +	of_platform_populate(np, NULL, NULL, dev);
>  	of_node_put(np);
>  
>  	/*


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2] mtd: call of_platform_populate() for MTD partitions

[Rafał Miłecki]

On 9.05.2022 16:17, Miquel Raynal wrote:
> zajec5@gmail.com wrote on Wed,  4 May 2022 21:44:48 +0200:
> 
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Until this change MTD subsystem supported handling partitions only with
>> MTD partitions parsers. That's a specific / limited API designed around
>> partitions.
>>
>> Some MTD partitions may however require different handling. They may
>> contain specific data that needs to be parsed and somehow extracted. For
>> that purpose MTD subsystem should allow binding of standard platform
>> drivers.
>>
>> An example can be U-Boot (sub)partition with environment variables.
>> There exist a "u-boot,env" DT binding for MTD (sub)partition that
>> requires an NVMEM driver.
>>
>> Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Fix case for # CONFIG_MTD_PARTITIONED_MASTER is not set
>>      master->dev can't be used blindly as it may point to unregistered
>>      device and cause WARNINGs
>> ---
>>   drivers/mtd/mtdpart.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> index 357661b62c94..4971fa69d076 100644
>> --- a/drivers/mtd/mtdpart.c
>> +++ b/drivers/mtd/mtdpart.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/mtd/partitions.h>
>>   #include <linux/err.h>
>>   #include <linux/of.h>
>> +#include <linux/of_platform.h>
>>   
>>   #include "mtdcore.h"
>>   
>> @@ -577,10 +578,16 @@ static int mtd_part_of_parse(struct mtd_info *master,
>>   	struct mtd_part_parser *parser;
>>   	struct device_node *np;
>>   	struct property *prop;
>> +	struct device *dev;
>>   	const char *compat;
>>   	const char *fixed = "fixed-partitions";
>>   	int ret, err = 0;
>>   
>> +	if (mtd_is_partition(master) || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> 
> Are you sure about this condition? Isn't accessing master->dev.parent
> going to fail if !IS_ENABLED(PARTITIONED_MASTER) ?
> 
> I'm not 100% sure my remark is correct but I fail to get the logic
> here.

It seems to work as expected, I tested it using device with following
layout:

2 fixed-partitions partitions found on MTD device brcmnand.0
Creating 2 MTD partitions on "brcmnand.0":
0x000000000000-0x000000100000 : "loader"
1 fixed-partitions partitions found on MTD device loader
Creating 1 MTD partitions on "loader":
0x000000040000-0x000000044008 : "u-boot-env"
0x000000100000-0x00001ff00000 : "image"

brcmnand.0
├── loader
│   └── u-boot-env
└── image



# CONFIG_MTD_PARTITIONED_MASTER is not set

┌────────┬────────────┬────────────────────────────────────────┐
│ Device │    Name    │           struct device *dev           │
├────────┼────────────┼────────────────────────────────────────┤
│ -      │ brcmnand.0 │ bcm63138_nand ff801800.nand-controller │
│ mtd0   │ loader     │ mtd mtd0                               │
│ mtd1   │ u-boot-env │ mtd mtd1                               │
│ mtd2   │ image      │ mtd mtd2                               │
└────────┴────────────┴────────────────────────────────────────┘



CONFIG_MTD_PARTITIONED_MASTER=y

┌────────┬────────────┬────────────────────┐
│ Device │    Name    │ struct device *dev │
├────────┼────────────┼────────────────────┤
│ mtd0   │ brcmnand.0 │ mtd0               │
│ mtd1   │ loader     │ mtd mtd1           │
│ mtd2   │ u-boot-env │ mtd mtd2           │
│ mtd3   │ image      │ mtd mtd3           │
└────────┴────────────┴────────────────────┘


As you can see the only tricky case is *master* mtd *without*
CONFIG_MTD_PARTITIONED_MASTER. In that case we don't register its device
and so a parent (NAND controller) has to be used instead. My condition
seems to handle that correctly.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH V2] mtd: call of_platform_populate() for MTD partitions

[Miquel Raynal]

Hi Rafał,

zajec5@gmail.com wrote on Tue, 10 May 2022 07:56:02 +0200:

> On 9.05.2022 16:17, Miquel Raynal wrote:
> > zajec5@gmail.com wrote on Wed,  4 May 2022 21:44:48 +0200:
> >   
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> Until this change MTD subsystem supported handling partitions only with
> >> MTD partitions parsers. That's a specific / limited API designed around
> >> partitions.
> >>
> >> Some MTD partitions may however require different handling. They may
> >> contain specific data that needs to be parsed and somehow extracted. For
> >> that purpose MTD subsystem should allow binding of standard platform
> >> drivers.
> >>
> >> An example can be U-Boot (sub)partition with environment variables.
> >> There exist a "u-boot,env" DT binding for MTD (sub)partition that
> >> requires an NVMEM driver.
> >>
> >> Ref: 5db1c2dbc04c ("dt-bindings: nvmem: add U-Boot environment variables binding")
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >> V2: Fix case for # CONFIG_MTD_PARTITIONED_MASTER is not set
> >>      master->dev can't be used blindly as it may point to unregistered
> >>      device and cause WARNINGs
> >> ---
> >>   drivers/mtd/mtdpart.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> >> index 357661b62c94..4971fa69d076 100644
> >> --- a/drivers/mtd/mtdpart.c
> >> +++ b/drivers/mtd/mtdpart.c
> >> @@ -17,6 +17,7 @@
> >>   #include <linux/mtd/partitions.h>
> >>   #include <linux/err.h>
> >>   #include <linux/of.h>
> >> +#include <linux/of_platform.h>  
> >>   >>   #include "mtdcore.h"
> >>   >> @@ -577,10 +578,16 @@ static int mtd_part_of_parse(struct mtd_info *master,  
> >>   	struct mtd_part_parser *parser;
> >>   	struct device_node *np;
> >>   	struct property *prop;
> >> +	struct device *dev;
> >>   	const char *compat;
> >>   	const char *fixed = "fixed-partitions";
> >>   	int ret, err = 0;  
> >>   >> +	if (mtd_is_partition(master) || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))  
> > 
> > Are you sure about this condition? Isn't accessing master->dev.parent
> > going to fail if !IS_ENABLED(PARTITIONED_MASTER) ?
> > 
> > I'm not 100% sure my remark is correct but I fail to get the logic
> > here.  
> 
> It seems to work as expected, I tested it using device with following
> layout:
> 
> 2 fixed-partitions partitions found on MTD device brcmnand.0
> Creating 2 MTD partitions on "brcmnand.0":
> 0x000000000000-0x000000100000 : "loader"
> 1 fixed-partitions partitions found on MTD device loader
> Creating 1 MTD partitions on "loader":
> 0x000000040000-0x000000044008 : "u-boot-env"
> 0x000000100000-0x00001ff00000 : "image"
> 
> brcmnand.0
> ├── loader
> │   └── u-boot-env
> └── image
> 
> 
> 
> # CONFIG_MTD_PARTITIONED_MASTER is not set
> 
> ┌────────┬────────────┬────────────────────────────────────────┐
> │ Device │    Name    │           struct device *dev           │
> ├────────┼────────────┼────────────────────────────────────────┤
> │ -      │ brcmnand.0 │ bcm63138_nand ff801800.nand-controller │
> │ mtd0   │ loader     │ mtd mtd0                               │
> │ mtd1   │ u-boot-env │ mtd mtd1                               │
> │ mtd2   │ image      │ mtd mtd2                               │
> └────────┴────────────┴────────────────────────────────────────┘
> 
> 
> 
> CONFIG_MTD_PARTITIONED_MASTER=y
> 
> ┌────────┬────────────┬────────────────────┐
> │ Device │    Name    │ struct device *dev │
> ├────────┼────────────┼────────────────────┤
> │ mtd0   │ brcmnand.0 │ mtd0               │
> │ mtd1   │ loader     │ mtd mtd1           │
> │ mtd2   │ u-boot-env │ mtd mtd2           │
> │ mtd3   │ image      │ mtd mtd3           │
> └────────┴────────────┴────────────────────┘
> 
> 
> As you can see the only tricky case is *master* mtd *without*
> CONFIG_MTD_PARTITIONED_MASTER. In that case we don't register its device
> and so a parent (NAND controller) has to be used instead. My condition
> seems to handle that correctly.

I think I get it now. I know it's a lot of not- conditions but I feel
the logic could be simpler to understand this way:

dev = mtd->dev;
/* Use the top device structure (the controller) if the top level MTD
parent is not registered */
if (!IS_ENABLED(PARTITIONED_MASTER) && !mtd_is_partition(mtd))

What do you think?

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/