[PATCH V6] mtd: m25p80: Modify the name of mtd_info

[PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Zhiqiang Hou]

From: Hou Zhiqiang <B48286@freescale.com>

Set the mtd_info's name to a fixed one, so spi flash layouts can
be specified by "mtdparts=..." in kernel cmdline, because the
cmdlinepart's parser will match the name of mtd_info with the name
given in cmdline.

So far, if DT is used, the mtd_info's name will be set to the name
of spi->dev. It includes spi_master->bus_num, and the bus_num may
be dynamically allocated. So, replace the component bus_num with
the physical address of spi controller.

Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
---
V6:
	Removed the sentence return error number when allocating the
	mtd_info's name failed.
V5:
	Rebase this patch on the latest l2-mtd tree.
V4:
	add check no-NULL for pointer of master's device node, and if it failed
	to get physcial address of the master, then name the mtd_info by the
	name of spi->dev.
V3:
	Fix a bug, matching unsigned long long with "%llx".
V2:
	1. Fix some code style issue.
	2. Cast physical address to unsigned long long.

 drivers/mtd/devices/m25p80.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d313f948b..35d11c3 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -26,6 +26,7 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/flash.h>
 #include <linux/mtd/spi-nor.h>
+#include <linux/of_address.h>
 
 #define	MAX_CMD_SIZE		6
 struct m25p {
@@ -183,6 +184,8 @@ static int m25p_probe(struct spi_device *spi)
 	struct spi_nor *nor;
 	enum read_mode mode = SPI_NOR_NORMAL;
 	char *flash_name = NULL;
+	struct resource res;
+	struct device_node *mnp = spi->master->dev.of_node;
 	int ret;
 
 	data = dev_get_platdata(&spi->dev);
@@ -215,6 +218,16 @@ static int m25p_probe(struct spi_device *spi)
 
 	if (data && data->name)
 		flash->mtd.name = data->name;
+	else if (mnp) {
+		ret = of_address_to_resource(mnp, 0, &res);
+		if (!ret)
+			flash->mtd.name = kasprintf(GFP_KERNEL, "spi%llx.%d",
+					(unsigned long long)res.start,
+					spi->chip_select);
+		else
+			dev_dbg(&spi->dev,
+				"Failed to get spi master resource\n");
+	}
 
 	/* For some (historical?) reason many platforms provide two different
 	 * names in flash_platform_data: "name" and "type". Quite often name is
-- 
2.1.0.27.g96db324

Re: [PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Jonas Gorski]

Hi,

On Mon, Aug 17, 2015 at 6:27 AM, Zhiqiang Hou <B48286@freescale.com> wrote:
> From: Hou Zhiqiang <B48286@freescale.com>
>
> Set the mtd_info's name to a fixed one, so spi flash layouts can
> be specified by "mtdparts=..." in kernel cmdline, because the
> cmdlinepart's parser will match the name of mtd_info with the name
> given in cmdline.
>
> So far, if DT is used, the mtd_info's name will be set to the name
> of spi->dev. It includes spi_master->bus_num, and the bus_num may
> be dynamically allocated. So, replace the component bus_num with
> the physical address of spi controller.

You can easily enforce fixed bus numers in linux using aliases in the
DT, this is supported
by the spi core since v3.9 or so.

Also won't this change break it for everyone relying on the old naming
in their commandline mtdparts?

>
> Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
> ---
> V6:
>         Removed the sentence return error number when allocating the
>         mtd_info's name failed.
> V5:
>         Rebase this patch on the latest l2-mtd tree.
> V4:
>         add check no-NULL for pointer of master's device node, and if it failed
>         to get physcial address of the master, then name the mtd_info by the
>         name of spi->dev.
> V3:
>         Fix a bug, matching unsigned long long with "%llx".
> V2:
>         1. Fix some code style issue.
>         2. Cast physical address to unsigned long long.
>
>  drivers/mtd/devices/m25p80.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index d313f948b..35d11c3 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -26,6 +26,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/spi/flash.h>
>  #include <linux/mtd/spi-nor.h>
> +#include <linux/of_address.h>
>
>  #define        MAX_CMD_SIZE            6
>  struct m25p {
> @@ -183,6 +184,8 @@ static int m25p_probe(struct spi_device *spi)
>         struct spi_nor *nor;
>         enum read_mode mode = SPI_NOR_NORMAL;
>         char *flash_name = NULL;
> +       struct resource res;
> +       struct device_node *mnp = spi->master->dev.of_node;
>         int ret;
>
>         data = dev_get_platdata(&spi->dev);
> @@ -215,6 +218,16 @@ static int m25p_probe(struct spi_device *spi)
>
>         if (data && data->name)
>                 flash->mtd.name = data->name;
> +       else if (mnp) {

Style nitpick: you need do add { } to the first if as well. checkpatch
should have complained.

> +               ret = of_address_to_resource(mnp, 0, &res);
> +               if (!ret)
> +                       flash->mtd.name = kasprintf(GFP_KERNEL, "spi%llx.%d",
> +                                       (unsigned long long)res.start,
> +                                       spi->chip_select);
> +               else
> +                       dev_dbg(&spi->dev,
> +                               "Failed to get spi master resource\n");


Regards
Jonas

Re: [PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Michal Suchanek]

On 17 August 2015 at 18:27, Jonas Gorski <jogo@openwrt.org> wrote:
> Hi,
>
> On Mon, Aug 17, 2015 at 6:27 AM, Zhiqiang Hou <B48286@freescale.com> wrote:
>> From: Hou Zhiqiang <B48286@freescale.com>
>>
>> Set the mtd_info's name to a fixed one, so spi flash layouts can
>> be specified by "mtdparts=..." in kernel cmdline, because the
>> cmdlinepart's parser will match the name of mtd_info with the name
>> given in cmdline.
>>
>> So far, if DT is used, the mtd_info's name will be set to the name
>> of spi->dev. It includes spi_master->bus_num, and the bus_num may
>> be dynamically allocated. So, replace the component bus_num with
>> the physical address of spi controller.
>
> You can easily enforce fixed bus numers in linux using aliases in the
> DT, this is supported
> by the spi core since v3.9 or so.

WIth the bonus that you can move your boot partitions to another mtd
device with a one line change in dt.

It might be nice to add a warning when partitions are created and the
alias is not present.

Thanks

Michal

Re: [PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Brian Norris]

On Mon, Aug 17, 2015 at 06:27:36PM +0200, Jonas Gorski wrote:
> On Mon, Aug 17, 2015 at 6:27 AM, Zhiqiang Hou <B48286@freescale.com> wrote:
> > From: Hou Zhiqiang <B48286@freescale.com>
> >
> > Set the mtd_info's name to a fixed one, so spi flash layouts can
> > be specified by "mtdparts=..." in kernel cmdline, because the
> > cmdlinepart's parser will match the name of mtd_info with the name
> > given in cmdline.
> >
> > So far, if DT is used, the mtd_info's name will be set to the name
> > of spi->dev. It includes spi_master->bus_num, and the bus_num may
> > be dynamically allocated. So, replace the component bus_num with
> > the physical address of spi controller.
> 
> You can easily enforce fixed bus numers in linux using aliases in the
> DT, this is supported
> by the spi core since v3.9 or so.

Interesting. Thanks for the suggestion. I haven't verified it myself,
but if this is a workable solution, then I'd much prefer that to
fiddling with the name here. So, tentative NAK.

> Also won't this change break it for everyone relying on the old naming
> in their commandline mtdparts?

Yes, and that's been my comment on the first several versions. I didn't
have time to bother repeating it on these latter revisions.

Brian

Re: [PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Jonas Gorski]

On Tue, Aug 18, 2015 at 3:30 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Mon, Aug 17, 2015 at 06:27:36PM +0200, Jonas Gorski wrote:
>> On Mon, Aug 17, 2015 at 6:27 AM, Zhiqiang Hou <B48286@freescale.com> wrote:
>> > From: Hou Zhiqiang <B48286@freescale.com>
>> >
>> > Set the mtd_info's name to a fixed one, so spi flash layouts can
>> > be specified by "mtdparts=..." in kernel cmdline, because the
>> > cmdlinepart's parser will match the name of mtd_info with the name
>> > given in cmdline.
>> >
>> > So far, if DT is used, the mtd_info's name will be set to the name
>> > of spi->dev. It includes spi_master->bus_num, and the bus_num may
>> > be dynamically allocated. So, replace the component bus_num with
>> > the physical address of spi controller.
>>
>> You can easily enforce fixed bus numers in linux using aliases in the
>> DT, this is supported
>> by the spi core since v3.9 or so.
>
> Interesting. Thanks for the suggestion. I haven't verified it myself,
> but if this is a workable solution, then I'd much prefer that to
> fiddling with the name here. So, tentative NAK.

Should have added that to my reply, it's this part:

http://lxr.free-electrons.com/source/drivers/spi/spi.c#L1536

int spi_register_master(struct spi_master *master)
{
        ...
        if ((master->bus_num < 0) && master->dev.of_node)
                master->bus_num = of_alias_get_id(master->dev.of_node, "spi");


so just using

aliases {
    spi123 = &spi;
};

should be enough to get "stable" names.

Jonas

RE: [PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Hou Zhiqiang]

Hi Brian,

> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: 2015年8月18日 9:30
> To: Jonas Gorski
> Cc: Hou Zhiqiang-B48286; MTD Maling List; David Woodhouse; Hu Mingkai-
> B21284; Rafa?? Mi??ecki; mike@steroidmicros.com
> Subject: Re: [PATCH V6] mtd: m25p80: Modify the name of mtd_info
> 
> On Mon, Aug 17, 2015 at 06:27:36PM +0200, Jonas Gorski wrote:
> > On Mon, Aug 17, 2015 at 6:27 AM, Zhiqiang Hou <B48286@freescale.com>
> wrote:
> > > From: Hou Zhiqiang <B48286@freescale.com>
> > >
> > > Set the mtd_info's name to a fixed one, so spi flash layouts can be
> > > specified by "mtdparts=..." in kernel cmdline, because the
> > > cmdlinepart's parser will match the name of mtd_info with the name
> > > given in cmdline.
> > >
> > > So far, if DT is used, the mtd_info's name will be set to the name
> > > of spi->dev. It includes spi_master->bus_num, and the bus_num may be
> > > dynamically allocated. So, replace the component bus_num with the
> > > physical address of spi controller.
> >
> > You can easily enforce fixed bus numers in linux using aliases in the
> > DT, this is supported by the spi core since v3.9 or so.
> 
> Interesting. Thanks for the suggestion. I haven't verified it myself, but
> if this is a workable solution, then I'd much prefer that to fiddling
> with the name here. So, tentative NAK.
> 
> > Also won't this change break it for everyone relying on the old naming
> > in their commandline mtdparts?
> 
> Yes, and that's been my comment on the first several versions. I didn't
> have time to bother repeating it on these latter revisions.

I had tried to add the 'bus_num' property to DT and handle in spi controller,
after a serious discussion, that patch was rejected due to the DT should describe
the hardware features and get a conclusion that we'd better reform the mtd_info's
name using phy-address of spi controller to consist with NOR and Nand flash.

The aliases id is a good choice to enforce fixed the bus numbers and it won't break
others' cmdline mtdparts.

Thanks,
Zhiqiang

RE: [PATCH V6] mtd: m25p80: Modify the name of mtd_info

[Hou Zhiqiang]

Hi Jonas,

> -----Original Message-----
> From: Jonas Gorski [mailto:jogo@openwrt.org]
> Sent: 2015年8月18日 0:28
> To: Hou Zhiqiang-B48286
> Cc: MTD Maling List; Brian Norris; David Woodhouse; Hu Mingkai-B21284;
> Rafał Miłecki; mike@steroidmicros.com
> Subject: Re: [PATCH V6] mtd: m25p80: Modify the name of mtd_info
> 
> Hi,
> 
> On Mon, Aug 17, 2015 at 6:27 AM, Zhiqiang Hou <B48286@freescale.com>
> wrote:
> > From: Hou Zhiqiang <B48286@freescale.com>
> >
> > Set the mtd_info's name to a fixed one, so spi flash layouts can be
> > specified by "mtdparts=..." in kernel cmdline, because the
> > cmdlinepart's parser will match the name of mtd_info with the name
> > given in cmdline.
> >
> > So far, if DT is used, the mtd_info's name will be set to the name of
> > spi->dev. It includes spi_master->bus_num, and the bus_num may be
> > dynamically allocated. So, replace the component bus_num with the
> > physical address of spi controller.
> 
> You can easily enforce fixed bus numers in linux using aliases in the DT,
> this is supported by the spi core since v3.9 or so.
> 

Thanks for the suggestion.

> Also won't this change break it for everyone relying on the old naming in
> their commandline mtdparts?
> 
> >
> > Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
> > ---
> > V6:
> >         Removed the sentence return error number when allocating the
> >         mtd_info's name failed.
> > V5:
> >         Rebase this patch on the latest l2-mtd tree.
> > V4:
> >         add check no-NULL for pointer of master's device node, and if
> it failed
> >         to get physcial address of the master, then name the mtd_info
> by the
> >         name of spi->dev.
> > V3:
> >         Fix a bug, matching unsigned long long with "%llx".
> > V2:
> >         1. Fix some code style issue.
> >         2. Cast physical address to unsigned long long.
> >
> >  drivers/mtd/devices/m25p80.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/mtd/devices/m25p80.c
> > b/drivers/mtd/devices/m25p80.c index d313f948b..35d11c3 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/spi/spi.h>
> >  #include <linux/spi/flash.h>
> >  #include <linux/mtd/spi-nor.h>
> > +#include <linux/of_address.h>
> >
> >  #define        MAX_CMD_SIZE            6
> >  struct m25p {
> > @@ -183,6 +184,8 @@ static int m25p_probe(struct spi_device *spi)
> >         struct spi_nor *nor;
> >         enum read_mode mode = SPI_NOR_NORMAL;
> >         char *flash_name = NULL;
> > +       struct resource res;
> > +       struct device_node *mnp = spi->master->dev.of_node;
> >         int ret;
> >
> >         data = dev_get_platdata(&spi->dev); @@ -215,6 +218,16 @@
> > static int m25p_probe(struct spi_device *spi)
> >
> >         if (data && data->name)
> >                 flash->mtd.name = data->name;
> > +       else if (mnp) {
> 
> Style nitpick: you need do add { } to the first if as well. checkpatch
> should have complained.
> 
> > +               ret = of_address_to_resource(mnp, 0, &res);
> > +               if (!ret)
> > +                       flash->mtd.name = kasprintf(GFP_KERNEL,
> "spi%llx.%d",
> > +                                       (unsigned long long)res.start,
> > +                                       spi->chip_select);
> > +               else
> > +                       dev_dbg(&spi->dev,
> > +                               "Failed to get spi master
> > + resource\n");
> 
> 
> Regards
> Jonas