| Message ID | A765B125120D1346A63912DDE6D8B6315E62CB@NTXXIAMBX02.xacn.micron.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tuesday, October 14, 2014 at 03:26:49 AM, Bean Huo 霍斌斌 (beanhuo) wrote: [...] > - dev_warn(dev, "found %s, expected %s\n", > - jid->name, id->name); > - id = jid; > - info = (void *)jid->driver_data; > + struct flash_info *tmpinfo; > + > + tmpinfo = (void *)jid->driver_data; > + if (tmpinfo->jedec_id != info->jedec_id || > + (info->ext_id != 0 && > + tmpinfo->ext_id != info->ext_id)) { > + dev_warn(dev, "found %s, expected %s\n", > + jid->name, id->name); > + id = jid; > + info = (void *)jid->driver_data; > + } Won't $info contain an undefined value in case the newly added condition isn't met ? The old code initialized $info to a certain value always, the new code does not do that. Best regards, Marek Vasut
>Won't $info contain an undefined value in case the newly added condition isn't met ? The old code initialized $info to a certain value always, the new code does not do that.
Hi,Marek Vasut
Thanks.the $info has been defined before as below:
info = (void *)id->driver_data;
Unless id has not been initialized before.If having the following situation,
current linux codes will not properly match platform device data:
For example:
const struct spi_device_id spi_nor_ids[] = {
......
......
......
{ "n25q064", INFO(0x20ba17, 0, 64 * 1024, 128, 0) },
{ "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256, 0) },//former right platform point will be overwrote by this data info.
{ "n25q128a13", INFO(0x20bb18, 0x1234, 64 * 1024, 512, SECT_4K) },//this is the right platform data,I want to match this data info.
{ "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
{ "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
......
......
......
};
EXPORT_SYMBOL_GPL(spi_nor_ids);
Maybe my patch should take into account that ID is NULL,
I want to change my patch following, could you please give me some suggestions:
struct flash_info *tmpinfo;
tmpinfo = (void *)jid->driver_data;
if (id == NULL || tmpinfo->jedec_id != info->jedec_id ||
(info->ext_id != 0 && tmpinfo->ext_id != info->ext_id)) {
dev_warn(dev, "found %s, expected %s\n",
jid->name, id->name);
id = jid;
info = (void *)jid->driver_data;
}
On 14 October 2014 03:26, Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote: > This patch used to modify the method of spi_nor_scan overwrite platform ID point. > > If type of platform data match with the name of spi_nor_ids set, > and JEDEC ID also match with INFO ID of spi_nor_ids set,spi device > ID point(this is before probed according to device name) shouldn't be > overwrote.Otherwise,this point will be overwrote by new spi_nor_ids > set point that probed according to JEDEC ID. There are a lot of changes happening/requested around this code. I also proposed some patch touching this code, see https://patchwork.ozlabs.org/patch/377917/ Right now there is a slow ongoing work on fixing some m25p80 regression, which also may touch the code you change. So I'll suggest postponing this patch until we get a regression fixed and then work on this hacky code again.
>There are a lot of changes happening/requested around this code. I also proposed some patch touching this code, see https://patchwork.ozlabs.org/patch/377917/ >Right now there is a slow ongoing work on fixing some m25p80 regression, which also may touch the code you change. So I'll suggest postponing this patch until we get a regression fixed and then work on this hacky code again. Thanks for your concerns.But for the same deivce id with the different extended id and the same device name with the different device id issue, in your patch,this doesn't take into account. I have encountered this situation. for example: first condition (for the same deivce id with the different extended id): { "n25q064", INFO(0x20ba17, 0, 64 * 1024, 128, 0) }, { "n25ml064", INFO(0x20ba17, 0x1234, 64 * 1024, 256, 0) },//right device data { "n25q128a13", INFO(0x20ba18, 0, 64 * 1024, 256, 0) }, { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) }, { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) }, second condition(the same device id with the different device name): { "n25tl28", INFO(0x20ba17, 0, 64 * 1024, 128, 0) }, { "n25ml028", INFO(0x20ba17, 0, 64 * 1024, 256, 0) },//right device data { "n25q128a13", INFO(0x20ba18, 0, 64 * 1024, 256, 0) }, { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) }, { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
On 16 October 2014 10:00, Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote: >>There are a lot of changes happening/requested around this code. I also proposed some patch touching this code, see https://patchwork.ozlabs.org/patch/377917/ > >>Right now there is a slow ongoing work on fixing some m25p80 regression, which also may touch the code you change. So I'll suggest postponing this patch until we get a regression fixed and then work on this hacky code again. > > > Thanks for your concerns.But for the same deivce id with the different extended id and > the same device name with the different device id issue, in your patch,this doesn't take > into account. I have encountered this situation. for example: I really wasn't looking into details yet and I'm aware my patch does something else. I just say we should first fix the regression and then base next patches on top of that regression fix. I'm not NACKing your changes :)
>I really wasn't looking into details yet and I'm aware my patch does something else. I just say we should first fix the regression and then base next patches on top of that regression fix. I'm not NACKing your changes :)
I will take into account for your patch,and will cover all these isse in my next version patch.
By the way ,I still don't understand why you specify id NULL in you patch:
+ if (id) {
+ info = (void *)id->driver_data;
+ if (info->jedec_id) {
+ dev_warn(dev,
+ "passed SPI device ID (%s) contains JEDEC, ignoring it, driver should be fixed!\n",
+ id->name);
+ id = NULL;
}
}
+ if (!id) {
+ id = nor->read_id(nor);
+ if (IS_ERR(id))
+ return PTR_ERR(id);
+ }
+ info = (void *)id->driver_data;
+
On 16 October 2014 10:41, bpqw <bpqw@micron.com> wrote:
> By the way ,I still don't understand why you specify id NULL in you patch:
I guess I needed it for my Broadcom SPI driver, see:
mtd: bcm53xxspiflash: new driver for SPI flahes on Broadcom ARM SoCs
https://patchwork.ozlabs.org/patch/381902/
But I'll need to re-think that and update my patch.
On 16 October 2014 02:52, bpqw <bpqw@micron.com> wrote: > For example: > > const struct spi_device_id spi_nor_ids[] = { > ...... > ...... > ...... > { "n25q064", INFO(0x20ba17, 0, 64 * 1024, 128, 0) }, > { "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256, 0) },//former right platform point will be overwrote by this data info. > { "n25q128a13", INFO(0x20bb18, 0x1234, 64 * 1024, 512, SECT_4K) },//this is the right platform data,I want to match this data info. > { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) }, > { "n25q512a", INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) }, Your initial commit was really missing some example indeed. It was trying to understand it for 10 minutes with no luck (until seeing above case) ;) I see problem in spi-nor that your patch reveals: ISSUE: spi-nor accepts any entry from spi_nor_ids with no ext_id specified as long as the jedec_id matches I guess reason behind this is to support chips with the same parameters but different extended IDs. That simplifies the driver and support for extra chips but may also cause problems like in your case. So AFAIU your patch tries to force using specific flash info, because JEDEC based detection fails. In general this is what we try to avoid. We want to auto-detect flash chips and to relieve platform code / DT. Could we drop this patch and modify spi-nor to detect your chip automatically? That would be much cleaner solution to me.
Hi,rafal Maybe this patch is not very reasonable.But for fix this case,I will develop a new patch that is just used to add extended ID for micron spi nor in the spi_nor_ids[]. Thanks for your review my patch.
On 27 October 2014 09:44, bpqw <bpqw@micron.com> wrote: > Maybe this patch is not very reasonable.But for fix this case,I will develop a new patch that > is just used to add extended ID for micron spi nor in the spi_nor_ids[]. Great, I hope it will work on top of the [PATCH] mtd: spi-nor: prefer more specific entries from chips database , good luck :)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index b5ad6be..3bb8077 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -963,16 +963,23 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, return PTR_ERR(jid); } else if (jid != id) { /* - * JEDEC knows better, so overwrite platform ID. We - * can't trust partitions any longer, but we'll let - * mtd apply them anyway, since some partitions may be - * marked read-only, and we don't want to lose that - * information, even if it's not 100% accurate. + * If type of platform data match with the name of + * spi_nor_ids set,and JEDEC ID also match with + * INFO ID of spi_nor_ids set,shouldn't overwrite + * spi device info point.Otherwise,will overwrite + * it. */ - dev_warn(dev, "found %s, expected %s\n", - jid->name, id->name); - id = jid; - info = (void *)jid->driver_data; + struct flash_info *tmpinfo; + + tmpinfo = (void *)jid->driver_data; + if (tmpinfo->jedec_id != info->jedec_id || + (info->ext_id != 0 && + tmpinfo->ext_id != info->ext_id)) { + dev_warn(dev, "found %s, expected %s\n", + jid->name, id->name); + id = jid; + info = (void *)jid->driver_data; + } } }
This patch used to modify the method of spi_nor_scan overwrite platform ID point. If type of platform data match with the name of spi_nor_ids set, and JEDEC ID also match with INFO ID of spi_nor_ids set,spi device ID point(this is before probed according to device name) shouldn't be overwrote.Otherwise,this point will be overwrote by new spi_nor_ids set point that probed according to JEDEC ID. Signed-off-by: bean huo <beanhuo@micron.com> --- v1-v2: add extended ID match judgment. drivers/mtd/spi-nor/spi-nor.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)