| Message ID | 1439785625-36183-1-git-send-email-B48286@freescale.com |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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
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
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
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