[LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Naga Sureshkumar Relli]

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(-)

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;
 }
 
 /**
-- 
2.7.4

Re: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Boris Brezillon]

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;
>  }
>  
>  /**

Re: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Vignesh Raghavendra]

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;
>>  }
>>  
>>  /**
> 

Re: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Boris Brezillon]

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".

Re: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Vignesh Raghavendra]

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.


-- 
Regards
Vignesh

Re: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Vignesh Raghavendra]

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;
>>  }
>>  
>>  /**
> 

-- 
Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [LINUX PATCH 2/3] spi: spi-mem: call spi_mem_default_supports_op() first

[Naga Sureshkumar Relli]

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