| Message ID | 1439505965-134748-2-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | e747dbe75e83345379455a78bb208ab7202229df |
| Headers | show |
On Friday, August 14, 2015 at 12:46:02 AM, Brian Norris wrote: > We don't really need the flash information from the device tree here. > Let's stick with autodetection here instead. Thanks for keeping me in the loop. > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > drivers/mtd/spi-nor/fsl-quadspi.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c > b/drivers/mtd/spi-nor/fsl-quadspi.c index d32b7e04ccca..2a17ec6269ff > 100644 > --- a/drivers/mtd/spi-nor/fsl-quadspi.c > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c > @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device > *pdev) > > /* iterate the subnodes. */ > for_each_available_child_of_node(dev->of_node, np) { > - char modalias[40]; > - > /* skip the holes */ > if (!q->has_second_chip) > i *= 2; > @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device > *pdev) nor->prepare = fsl_qspi_prep; > nor->unprepare = fsl_qspi_unprep; > > - ret = of_modalias_node(np, modalias, sizeof(modalias)); > - if (ret < 0) > - goto mutex_failed; > - > ret = of_property_read_u32(np, "spi-max-frequency", > &q->clk_rate); > if (ret < 0) > @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device > *pdev) /* set the chip address for READID */ > fsl_qspi_set_base_addr(q, nor); > > - ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD); > + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); This is something I don't quite understand. So we have a SPI NOR controller, which as it's own struct device instance. This controller can have multiple SPI NORs on it. Each SPI NOR has it's own struct spi_nor instance and struct mtd_info instance, right? But, all of the SPI NORs share the same struct device as the controller. Do I understand that correctly ? Does that make sense to NOT allocate a new struct device for each of the SPI NORs ? Best regards, Marek Vasut
On Fri, Aug 14, 2015 at 01:09:14AM +0200, Marek Vasut wrote: > This is something I don't quite understand. So we have a SPI NOR controller, > which as it's own struct device instance. Right. > This controller can have multiple SPI NORs on it. Each SPI NOR has it's own > struct spi_nor instance and struct mtd_info instance, right? Right. > But, all of the SPI NORs share the same struct device as the controller. Do > I understand that correctly ? Mostly yes. But your next statement doesn't quite follow in my mind, so maybe you've missed something. There is typically a single platform device (and associated struct device) that represents the fsl-quadspi controller. (Pedantic side point: each MTD actually creates one or more struct device objects; one for the master MTD, and one for each partition that might be created. But I don't think you're asking about these struct device's.) But none of this is super-relevant to this patch; I'm not talking about struct devices (think kobjects, Linux driver model), I'm dealing with struct device_node (think device tree, of_*() APIs, etc.). Now, each flash connected to the controller has its own device_node. All this patch is saying is that we don't need to know much about that node; as long as it responds to the READ ID command properly, spi_nor_scan() can autodetect it. > Does that make sense to NOT allocate a new > struct device for each of the SPI NORs ? I don't quite see how this question relates. Are you suggesting that we should be using fewer *device_node* structs? That would involve rewriting the device tree, I believe. Or perhaps I've misunderstood your train of thought. Brian
On Friday, August 14, 2015 at 01:24:47 AM, Brian Norris wrote: > On Fri, Aug 14, 2015 at 01:09:14AM +0200, Marek Vasut wrote: > > This is something I don't quite understand. So we have a SPI NOR > > controller, which as it's own struct device instance. > > Right. > > > This controller can have multiple SPI NORs on it. Each SPI NOR has it's > > own struct spi_nor instance and struct mtd_info instance, right? > > Right. > > > But, all of the SPI NORs share the same struct device as the controller. > > Do I understand that correctly ? > > Mostly yes. But your next statement doesn't quite follow in my mind, so > maybe you've missed something. I think maybe, just maybe, I should've asked this in an entirely separate thread, sorry. > There is typically a single platform device (and associated struct > device) that represents the fsl-quadspi controller. > > (Pedantic side point: each MTD actually creates one or more struct > device objects; one for the master MTD, and one for each partition that > might be created. But I don't think you're asking about these struct > device's.) Right. > But none of this is super-relevant to this patch; I'm not talking about > struct devices (think kobjects, Linux driver model), I'm dealing with > struct device_node (think device tree, of_*() APIs, etc.). > > Now, each flash connected to the controller has its own device_node. All > this patch is saying is that we don't need to know much about that node; > as long as it responds to the READ ID command properly, spi_nor_scan() > can autodetect it. Right. > > Does that make sense to NOT allocate a new > > struct device for each of the SPI NORs ? > > I don't quite see how this question relates. Are you suggesting that we > should be using fewer *device_node* structs? That would involve > rewriting the device tree, I believe. > > Or perhaps I've misunderstood your train of thought. Sorry, I should've asked about this in a separate thread. Best regards, Marek Vasut
On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote: > On Fri, Aug 14, 2015 at 01:09:14AM +0200, Marek Vasut wrote: >> This is something I don't quite understand. So we have a SPI NOR controller, >> which as it's own struct device instance. > > Right. > >> This controller can have multiple SPI NORs on it. Each SPI NOR has it's own >> struct spi_nor instance and struct mtd_info instance, right? > > Right. > >> But, all of the SPI NORs share the same struct device as the controller. Do >> I understand that correctly ? > > Mostly yes. But your next statement doesn't quite follow in my mind, so > maybe you've missed something. > > There is typically a single platform device (and associated struct > device) that represents the fsl-quadspi controller. > > (Pedantic side point: each MTD actually creates one or more struct > device objects; one for the master MTD, and one for each partition that > might be created. But I don't think you're asking about these struct > device's.) > > But none of this is super-relevant to this patch; I'm not talking about > struct devices (think kobjects, Linux driver model), I'm dealing with > struct device_node (think device tree, of_*() APIs, etc.). > > Now, each flash connected to the controller has its own device_node. All > this patch is saying is that we don't need to know much about that node; > as long as it responds to the READ ID command properly, spi_nor_scan() > can autodetect it. > And if there was suppor for a flash chip that does not respond to READ ID (or uses a different opcode for it) this patch would break it, right? Thanks Michal
On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote: > On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote: > > Now, each flash connected to the controller has its own device_node. All > > this patch is saying is that we don't need to know much about that node; > > as long as it responds to the READ ID command properly, spi_nor_scan() > > can autodetect it. > > > > And if there was suppor for a flash chip that does not respond to READ > ID (or uses a different opcode for it) this patch would break it, > right? For the latter: this already doesn't support chips that use different opcodes. For the former: we're only talking about the "*-nonjedec" and similar, right? I'm not confident those were supported well by this driver in the first place. (And "*-nonjedec" should really die; if it's needed, support should be added by design, not by accident.) Perhaps Huang can comment. Brian
Seems Huang has changed the e-mail address. On Mon, Aug 17, 2015 at 9:56 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote: >> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote: >> > Now, each flash connected to the controller has its own device_node. All >> > this patch is saying is that we don't need to know much about that node; >> > as long as it responds to the READ ID command properly, spi_nor_scan() >> > can autodetect it. >> > >> >> And if there was suppor for a flash chip that does not respond to READ >> ID (or uses a different opcode for it) this patch would break it, >> right? > > For the latter: this already doesn't support chips that use different > opcodes. > > For the former: we're only talking about the "*-nonjedec" and similar, > right? I'm not confident those were supported well by this driver in the > first place. (And "*-nonjedec" should really die; if it's needed, > support should be added by design, not by accident.) > > Perhaps Huang can comment. > > Brian > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Mon, Aug 17, 2015 at 10:15:50PM -0500, Han Xu wrote: Hi Brian & Han, > Seems Huang has changed the e-mail address. > > On Mon, Aug 17, 2015 at 9:56 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote: > >> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote: > >> > Now, each flash connected to the controller has its own device_node. All > >> > this patch is saying is that we don't need to know much about that node; > >> > as long as it responds to the READ ID command properly, spi_nor_scan() > >> > can autodetect it. > >> > > >> > >> And if there was suppor for a flash chip that does not respond to READ > >> ID (or uses a different opcode for it) this patch would break it, > >> right? Do not worry about the quadspi controller :) The controller only uses the best NOR flash (or maybe the best NOR flash), I remember that the NOR flash can support the QUAD DDR read which is not supported not. So it will not break the driver. Btw: @Han Xu : I think you can test this patch. > > > > For the latter: this already doesn't support chips that use different > > opcodes. > > > > For the former: we're only talking about the "*-nonjedec" and similar, > > right? I'm not confident those were supported well by this driver in the > > first place. (And "*-nonjedec" should really die; if it's needed, > > support should be added by design, not by accident.) > > > > Perhaps Huang can comment. I think this patch is okay. But please wait for Han's test result. thanks Huang Shijie
On Tue, Aug 18, 2015 at 04:09:27PM +0800, Huang Shijie wrote: > On Mon, Aug 17, 2015 at 10:15:50PM -0500, Han Xu wrote: > > Seems Huang has changed the e-mail address. Thanks, noted. > > On Mon, Aug 17, 2015 at 9:56 PM, Brian Norris > > <computersforpeace@gmail.com> wrote: > > > On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote: > > >> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote: > > >> > Now, each flash connected to the controller has its own device_node. All > > >> > this patch is saying is that we don't need to know much about that node; > > >> > as long as it responds to the READ ID command properly, spi_nor_scan() > > >> > can autodetect it. > > >> > > > >> > > >> And if there was suppor for a flash chip that does not respond to READ > > >> ID (or uses a different opcode for it) this patch would break it, > > >> right? > > Do not worry about the quadspi controller :) > > The controller only uses the best NOR flash (or maybe the best NOR flash), I remember > that the NOR flash can support the QUAD DDR read which is not supported not. > > So it will not break the driver. > > Btw: @Han Xu : I think you can test this patch. > > > > > > > For the latter: this already doesn't support chips that use different > > > opcodes. > > > > > > For the former: we're only talking about the "*-nonjedec" and similar, > > > right? I'm not confident those were supported well by this driver in the > > > first place. (And "*-nonjedec" should really die; if it's needed, > > > support should be added by design, not by accident.) > > > > > > Perhaps Huang can comment. > > I think this patch is okay. But please wait for Han's test result. OK. Pushed patches 2-4 to l2-mtd.git/next. Will wait on this one.
On Thu, Aug 13, 2015 at 5:46 PM, Brian Norris <computersforpeace@gmail.com> wrote: > We don't really need the flash information from the device tree here. > Let's stick with autodetection here instead. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > drivers/mtd/spi-nor/fsl-quadspi.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c > index d32b7e04ccca..2a17ec6269ff 100644 > --- a/drivers/mtd/spi-nor/fsl-quadspi.c > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c > @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device *pdev) > > /* iterate the subnodes. */ > for_each_available_child_of_node(dev->of_node, np) { > - char modalias[40]; > - > /* skip the holes */ > if (!q->has_second_chip) > i *= 2; > @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device *pdev) > nor->prepare = fsl_qspi_prep; > nor->unprepare = fsl_qspi_unprep; > > - ret = of_modalias_node(np, modalias, sizeof(modalias)); > - if (ret < 0) > - goto mutex_failed; > - > ret = of_property_read_u32(np, "spi-max-frequency", > &q->clk_rate); > if (ret < 0) > @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) > /* set the chip address for READID */ > fsl_qspi_set_base_addr(q, nor); > > - ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD); > + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); > if (ret) > goto mutex_failed; > Acked-by: Han xu <han.xu@freescale.com> Tested-by: Han xu <han.xu@freescale.com> > -- > 2.5.0.276.gf5e568e > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Tue, Sep 01, 2015 at 06:40:17PM -0500, Han Xu wrote: > On Thu, Aug 13, 2015 at 5:46 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > We don't really need the flash information from the device tree here. > > Let's stick with autodetection here instead. > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > --- > > drivers/mtd/spi-nor/fsl-quadspi.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c > > index d32b7e04ccca..2a17ec6269ff 100644 > > --- a/drivers/mtd/spi-nor/fsl-quadspi.c > > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c > > @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device *pdev) > > > > /* iterate the subnodes. */ > > for_each_available_child_of_node(dev->of_node, np) { > > - char modalias[40]; > > - > > /* skip the holes */ > > if (!q->has_second_chip) > > i *= 2; > > @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device *pdev) > > nor->prepare = fsl_qspi_prep; > > nor->unprepare = fsl_qspi_unprep; > > > > - ret = of_modalias_node(np, modalias, sizeof(modalias)); > > - if (ret < 0) > > - goto mutex_failed; > > - > > ret = of_property_read_u32(np, "spi-max-frequency", > > &q->clk_rate); > > if (ret < 0) > > @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) > > /* set the chip address for READID */ > > fsl_qspi_set_base_addr(q, nor); > > > > - ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD); > > + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); > > if (ret) > > goto mutex_failed; > > > > Acked-by: Han xu <han.xu@freescale.com> > Tested-by: Han xu <han.xu@freescale.com> Thanks, applied to l2-mtd.git/next
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c index d32b7e04ccca..2a17ec6269ff 100644 --- a/drivers/mtd/spi-nor/fsl-quadspi.c +++ b/drivers/mtd/spi-nor/fsl-quadspi.c @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device *pdev) /* iterate the subnodes. */ for_each_available_child_of_node(dev->of_node, np) { - char modalias[40]; - /* skip the holes */ if (!q->has_second_chip) i *= 2; @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device *pdev) nor->prepare = fsl_qspi_prep; nor->unprepare = fsl_qspi_unprep; - ret = of_modalias_node(np, modalias, sizeof(modalias)); - if (ret < 0) - goto mutex_failed; - ret = of_property_read_u32(np, "spi-max-frequency", &q->clk_rate); if (ret < 0) @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device *pdev) /* set the chip address for READID */ fsl_qspi_set_base_addr(q, nor); - ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD); + ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD); if (ret) goto mutex_failed;
We don't really need the flash information from the device tree here. Let's stick with autodetection here instead. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/spi-nor/fsl-quadspi.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)