| Message ID | 1553771784-3364-1-git-send-email-naga.sureshkumar.relli@xilinx.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for Zynq QSPI controller driver | expand |
On Thu, 28 Mar 2019 16:46:24 +0530 Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote: > Call spi_mem_default_supports_op() first, before calling controller > specific ctlr->supports_op(). > With this, controller drivers can drop checking the buswidths again. No, this was done on purpose, in case the controller does not want the default check to be applied (say it does not need bus-width props to be defined and has another way to check if a device can be accessed in dual, quad or octal mode). Just call spi_mem_default_supports_op() from your driver ->supports_op() hook if needed. > > Suggested-by: Vignesh Raghavendra <vigneshr@ti.com> > Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> > --- > Details can be found at https://lkml.org/lkml/2019/3/1/183 > --- > drivers/spi/spi-mem.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 5217a56..56aa158 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -189,11 +189,14 @@ static bool spi_mem_internal_supports_op(struct spi_mem *mem, > const struct spi_mem_op *op) > { > struct spi_controller *ctlr = mem->spi->controller; > + bool ret; > + > + ret = spi_mem_default_supports_op(mem, op); > > if (ctlr->mem_ops && ctlr->mem_ops->supports_op) > - return ctlr->mem_ops->supports_op(mem, op); > + ret = ctlr->mem_ops->supports_op(mem, op); > > - return spi_mem_default_supports_op(mem, op); > + return ret; > } > > /**
Hi Boris, On 29/03/19 1:25 AM, Boris Brezillon wrote: > On Thu, 28 Mar 2019 16:46:24 +0530 > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote: > >> Call spi_mem_default_supports_op() first, before calling controller >> specific ctlr->supports_op(). >> With this, controller drivers can drop checking the buswidths again. > > No, this was done on purpose, in case the controller does not want the > default check to be applied (say it does not need bus-width props to > be defined and has another way to check if a device can be accessed in > dual, quad or octal mode). Sorry, I don't understand here. Based on capabilities declared in spi_device->mode, m25p80 driver will claim appropriate SNOR_HWCAPS_*. SPI NOR layer chooses opcodes based on that for which m25p80 layer populates buswidths. So, I don't really expect any mismatch in spi_mem_default_supports_op() in the case you mentioned. Or did I miss something? Maybe something SPI NAND specific? Regards Vignesh > Just call spi_mem_default_supports_op() from your driver > ->supports_op() hook if needed. > >> >> Suggested-by: Vignesh Raghavendra <vigneshr@ti.com> >> Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> >> --- >> Details can be found at https://lkml.org/lkml/2019/3/1/183 >> --- >> drivers/spi/spi-mem.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >> index 5217a56..56aa158 100644 >> --- a/drivers/spi/spi-mem.c >> +++ b/drivers/spi/spi-mem.c >> @@ -189,11 +189,14 @@ static bool spi_mem_internal_supports_op(struct spi_mem *mem, >> const struct spi_mem_op *op) >> { >> struct spi_controller *ctlr = mem->spi->controller; >> + bool ret; >> + >> + ret = spi_mem_default_supports_op(mem, op); >> >> if (ctlr->mem_ops && ctlr->mem_ops->supports_op) >> - return ctlr->mem_ops->supports_op(mem, op); >> + ret = ctlr->mem_ops->supports_op(mem, op); >> >> - return spi_mem_default_supports_op(mem, op); >> + return ret; >> } >> >> /** >
On Fri, 29 Mar 2019 13:50:26 +0530 Vignesh Raghavendra <vigneshr@ti.com> wrote: > Hi Boris, > > On 29/03/19 1:25 AM, Boris Brezillon wrote: > > On Thu, 28 Mar 2019 16:46:24 +0530 > > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote: > > > >> Call spi_mem_default_supports_op() first, before calling controller > >> specific ctlr->supports_op(). > >> With this, controller drivers can drop checking the buswidths again. > > > > No, this was done on purpose, in case the controller does not want the > > default check to be applied (say it does not need bus-width props to > > be defined and has another way to check if a device can be accessed in > > dual, quad or octal mode). > > Sorry, I don't understand here. > Based on capabilities declared in spi_device->mode, m25p80 driver will > claim appropriate SNOR_HWCAPS_*. SPI NOR layer chooses opcodes based > on that for which m25p80 layer populates buswidths. Well, that test in m25p80 should go away and be replaced by a proper spi_mem_supports_op() iteration on all modes reported as supported by the *chip* (I think that's what I did in my series merging m25p80 code into the spi-nor core). But that's not really related to the problem we're discussing here. > > So, I don't really expect any mismatch in spi_mem_default_supports_op() > in the case you mentioned. Or did I miss something? Maybe something SPI > NAND specific? Nothing NAND specific, just something controller specific and how we want to deal with buswidth detection. Most memory devices expose their caps in some way (be it ID-based detection or using some kind of caps/parameters table), so they know what they're capable of. SPI controllers know what they're capable of, of course. The only part that remains unknown for buswidth negotiation is how things are wired on the board. I keep thinking that defining buswidth in the DT (using spi-{tx,rx}-bus-width) should only be done if there are board-related limiting factors (some IO pins not routed). If you look at the code, SPI_{TX,RX}_{DUAL,QUAD,OCTAL} flags are only set if the spi-{tx,rx}-bus-width props are defined. The idea behind making spi_mem_default_supports_op() optional is to let new drivers implement a new scheme where missing spi-{tx,rx}-bus-width does not necessarily mean "use regular SPI mode".
On 29/03/19 2:23 PM, Boris Brezillon wrote: > On Fri, 29 Mar 2019 13:50:26 +0530 > Vignesh Raghavendra <vigneshr@ti.com> wrote: > >> Hi Boris, >> >> On 29/03/19 1:25 AM, Boris Brezillon wrote: >>> On Thu, 28 Mar 2019 16:46:24 +0530 >>> Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote: >>> >>>> Call spi_mem_default_supports_op() first, before calling controller >>>> specific ctlr->supports_op(). >>>> With this, controller drivers can drop checking the buswidths again. >>> >>> No, this was done on purpose, in case the controller does not want the >>> default check to be applied (say it does not need bus-width props to >>> be defined and has another way to check if a device can be accessed in >>> dual, quad or octal mode). >> >> Sorry, I don't understand here. >> Based on capabilities declared in spi_device->mode, m25p80 driver will >> claim appropriate SNOR_HWCAPS_*. SPI NOR layer chooses opcodes based >> on that for which m25p80 layer populates buswidths. > > Well, that test in m25p80 should go away and be replaced by a proper > spi_mem_supports_op() iteration on all modes reported as supported by > the *chip* (I think that's what I did in my series merging m25p80 code > into the spi-nor core). But that's not really related to the problem > we're discussing here. > I see that now. >> >> So, I don't really expect any mismatch in spi_mem_default_supports_op() >> in the case you mentioned. Or did I miss something? Maybe something SPI >> NAND specific? > > Nothing NAND specific, just something controller specific and how we > want to deal with buswidth detection. Most memory devices expose their > caps in some way (be it ID-based detection or using some kind of > caps/parameters table), so they know what they're capable of. SPI > controllers know what they're capable of, of course. The only part that > remains unknown for buswidth negotiation is how things are wired on the > board. I keep thinking that defining buswidth in the DT (using > spi-{tx,rx}-bus-width) should only be done if there are board-related > limiting factors (some IO pins not routed). > If you look at the code, SPI_{TX,RX}_{DUAL,QUAD,OCTAL} flags are only > set if the spi-{tx,rx}-bus-width props are defined. > > The idea behind making spi_mem_default_supports_op() optional is to let > new drivers implement a new scheme where missing > spi-{tx,rx}-bus-width does not necessarily mean "use regular SPI mode". > Ok, thanks for explanation.
Hi Naga Sureshkumar On 29/03/19 1:25 AM, Boris Brezillon wrote: > On Thu, 28 Mar 2019 16:46:24 +0530 > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote: > >> Call spi_mem_default_supports_op() first, before calling controller >> specific ctlr->supports_op(). >> With this, controller drivers can drop checking the buswidths again. > > No, this was done on purpose, in case the controller does not want the > default check to be applied (say it does not need bus-width props to > be defined and has another way to check if a device can be accessed in > dual, quad or octal mode). > Just call spi_mem_default_supports_op() from your driver > ->supports_op() hook if needed. > Could you rework to follow Boris's suggestion here? >> >> Suggested-by: Vignesh Raghavendra <vigneshr@ti.com> >> Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> >> --- >> Details can be found at https://lkml.org/lkml/2019/3/1/183 >> --- >> drivers/spi/spi-mem.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >> index 5217a56..56aa158 100644 >> --- a/drivers/spi/spi-mem.c >> +++ b/drivers/spi/spi-mem.c >> @@ -189,11 +189,14 @@ static bool spi_mem_internal_supports_op(struct spi_mem *mem, >> const struct spi_mem_op *op) >> { >> struct spi_controller *ctlr = mem->spi->controller; >> + bool ret; >> + >> + ret = spi_mem_default_supports_op(mem, op); >> >> if (ctlr->mem_ops && ctlr->mem_ops->supports_op) >> - return ctlr->mem_ops->supports_op(mem, op); >> + ret = ctlr->mem_ops->supports_op(mem, op); >> >> - return spi_mem_default_supports_op(mem, op); >> + return ret; >> } >> >> /** >
Hi Vignesh, > -----Original Message----- > From: linux-spi-owner@vger.kernel.org <linux-spi-owner@vger.kernel.org> On Behalf Of > Vignesh Raghavendra > Sent: Friday, March 29, 2019 4:32 PM > To: Boris Brezillon <boris.brezillon@collabora.com>; Naga Sureshkumar Relli > <nagasure@xilinx.com> > Cc: broonie@kernel.org; bbrezillon@kernel.org; linux-spi@vger.kernel.org; > dwmw2@infradead.org; marek.vasut@gmail.com; richard@nod.at; linux- > mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>; > nagasuresh12@gmail.com > Subject: Re: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first > > Hi Naga Sureshkumar > > On 29/03/19 1:25 AM, Boris Brezillon wrote: > > On Thu, 28 Mar 2019 16:46:24 +0530 > > Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> wrote: > > > >> Call spi_mem_default_supports_op() first, before calling controller > >> specific ctlr->supports_op(). > >> With this, controller drivers can drop checking the buswidths again. > > > > No, this was done on purpose, in case the controller does not want the > > default check to be applied (say it does not need bus-width props to > > be defined and has another way to check if a device can be accessed in > > dual, quad or octal mode). > > Just call spi_mem_default_supports_op() from your driver > > ->supports_op() hook if needed. > > > > Could you rework to follow Boris's suggestion here? Sure. I will update. Thanks, Naga Sureshkumar Relli > > >> > >> Suggested-by: Vignesh Raghavendra <vigneshr@ti.com> > >> Signed-off-by: Naga Sureshkumar Relli > >> <naga.sureshkumar.relli@xilinx.com> > >> --- > >> Details can be found at https://lkml.org/lkml/2019/3/1/183 > >> --- > >> drivers/spi/spi-mem.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index > >> 5217a56..56aa158 100644 > >> --- a/drivers/spi/spi-mem.c > >> +++ b/drivers/spi/spi-mem.c > >> @@ -189,11 +189,14 @@ static bool spi_mem_internal_supports_op(struct spi_mem > *mem, > >> const struct spi_mem_op *op) > >> { > >> struct spi_controller *ctlr = mem->spi->controller; > >> + bool ret; > >> + > >> + ret = spi_mem_default_supports_op(mem, op); > >> > >> if (ctlr->mem_ops && ctlr->mem_ops->supports_op) > >> - return ctlr->mem_ops->supports_op(mem, op); > >> + ret = ctlr->mem_ops->supports_op(mem, op); > >> > >> - return spi_mem_default_supports_op(mem, op); > >> + return ret; > >> } > >> > >> /** > > > > -- > Regards > Vignesh
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 5217a56..56aa158 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -189,11 +189,14 @@ static bool spi_mem_internal_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) { struct spi_controller *ctlr = mem->spi->controller; + bool ret; + + ret = spi_mem_default_supports_op(mem, op); if (ctlr->mem_ops && ctlr->mem_ops->supports_op) - return ctlr->mem_ops->supports_op(mem, op); + ret = ctlr->mem_ops->supports_op(mem, op); - return spi_mem_default_supports_op(mem, op); + return ret; } /**
Call spi_mem_default_supports_op() first, before calling controller specific ctlr->supports_op(). With this, controller drivers can drop checking the buswidths again. Suggested-by: Vignesh Raghavendra <vigneshr@ti.com> Signed-off-by: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com> --- Details can be found at https://lkml.org/lkml/2019/3/1/183 --- drivers/spi/spi-mem.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)