[RFC] spi: Switch call order of spi master setup and spi_set_cs

[RFC] spi: Switch call order of spi master setup and spi_set_cs

[Franklin S Cooper Jr]

Keystone 2 devices currently fail to boot in linux-next after the
below commit was applied:

spi: bitbang: switch to the generic implementation of transfer_one_message
commit: 0037686596832572bbca05ab168d9884d7d704c1

This patch allows Keystone 2 devices to boot again in linux-next.

Tested this patch on K2E evm and am437 starterkit which both have SPI
devices to insure regressions aren't seen.


Franklin S Cooper Jr (1):
  spi: Setup the master controller driver before setting the chipselect

 drivers/spi/spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.6.1

[RFC][PATCH] spi: Setup the master controller driver before setting the chipselect

[Franklin S Cooper Jr]

Some devices depend on the master controller driver setup function being
called before calling any chipselect functions.

Insure that this is done otherwise uninitialized structures may be
accessed causing a kernel panic.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/spi/spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 38006cc..9374d82 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2053,11 +2053,11 @@ int spi_setup(struct spi_device *spi)
 	if (!spi->max_speed_hz)
 		spi->max_speed_hz = spi->master->max_speed_hz;
 
-	spi_set_cs(spi, false);
-
 	if (spi->master->setup)
 		status = spi->master->setup(spi);
 
+	spi_set_cs(spi, false);
+
 	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
 			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
 			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
-- 
2.6.1

Re: [RFC] spi: Switch call order of spi master setup and spi_set_cs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Mon, Oct 12, 2015 at 04:01:10PM -0500, Franklin S Cooper Jr wrote:
> Keystone 2 devices currently fail to boot in linux-next after the
> below commit was applied:

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that
any important information is recorded in the changelog rather than being
lost.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC] spi: Switch call order of spi master setup and spi_set_cs

[Franklin S Cooper Jr.]

On 10/13/2015 07:10 AM, Mark Brown wrote:
> On Mon, Oct 12, 2015 at 04:01:10PM -0500, Franklin S Cooper Jr wrote:
>> Keystone 2 devices currently fail to boot in linux-next after the
>> below commit was applied:
> Please don't send cover letters for single patches, if there is anything
> that needs saying put it in the changelog of the patch or after the ---
> if it's administrative stuff.  This reduces mail volume and ensures that
> any important information is recorded in the changelog rather than being
> lost.
Sorry about that. Will do next time.

Re: [RFC][PATCH] spi: Setup the master controller driver before setting the chipselect

[Ivan T. Ivanov]

Adding Andy.


> On Oct 13, 2015, at 12:01 AM, Franklin S Cooper Jr <fcooper@ti.com> wrote:
> 
> Some devices depend on the master controller driver setup function being
> called before calling any chipselect functions.
> 
> Insure that this is done otherwise uninitialized structures may be
> accessed causing a kernel panic.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> drivers/spi/spi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 38006cc..9374d82 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2053,11 +2053,11 @@ int spi_setup(struct spi_device *spi)
> 	if (!spi->max_speed_hz)
> 		spi->max_speed_hz = spi->master->max_speed_hz;
> 
> -	spi_set_cs(spi, false);
> -
> 	if (spi->master->setup)
> 		status = spi->master->setup(spi);
> 
> +	spi_set_cs(spi, false);
> +
> 	dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> 			(int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> 			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> -- 
> 2.6.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH] spi: Setup the master controller driver before setting the chipselect

[Andy Shevchenko]

+Cc: Jarkko to see from spi-pxa2xx prospective

On Wed, Oct 14, 2015 at 12:47 PM, Ivan T. Ivanov <iivanov.xz@gmail.com> wrote:
> Adding Andy.
>
>
>> On Oct 13, 2015, at 12:01 AM, Franklin S Cooper Jr <fcooper@ti.com> wrote:
>>
>> Some devices depend on the master controller driver setup function being
>> called before calling any chipselect functions.
>>
>> Insure that this is done otherwise uninitialized structures may be
>> accessed causing a kernel panic.

As far as I understand my concern should be about spi-dw driver.

So, I have just tested yesterday's linux-next with and without
proposed patch. Works for me:
Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>> drivers/spi/spi.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 38006cc..9374d82 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -2053,11 +2053,11 @@ int spi_setup(struct spi_device *spi)
>>       if (!spi->max_speed_hz)
>>               spi->max_speed_hz = spi->master->max_speed_hz;
>>
>> -     spi_set_cs(spi, false);
>> -
>>       if (spi->master->setup)
>>               status = spi->master->setup(spi);
>>
>> +     spi_set_cs(spi, false);
>> +
>>       dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
>>                       (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>>                       (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
>> --
>> 2.6.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
With Best Regards,
Andy Shevchenko

Re: [RFC][PATCH] spi: Setup the master controller driver before setting the chipselect

[Heiner Kallweit]

On Wed, Oct 14, 2015 at 1:08 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> +Cc: Jarkko to see from spi-pxa2xx prospective
>
> On Wed, Oct 14, 2015 at 12:47 PM, Ivan T. Ivanov <iivanov.xz@gmail.com> wrote:
>> Adding Andy.
>>
>>
>>> On Oct 13, 2015, at 12:01 AM, Franklin S Cooper Jr <fcooper@ti.com> wrote:
>>>
>>> Some devices depend on the master controller driver setup function being
>>> called before calling any chipselect functions.
>>>
>>> Insure that this is done otherwise uninitialized structures may be
>>> accessed causing a kernel panic.
>
> As far as I understand my concern should be about spi-dw driver.
>
> So, I have just tested yesterday's linux-next with and without
> proposed patch. Works for me:
> Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
>>>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>> ---
>>> drivers/spi/spi.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>> index 38006cc..9374d82 100644
>>> --- a/drivers/spi/spi.c
>>> +++ b/drivers/spi/spi.c
>>> @@ -2053,11 +2053,11 @@ int spi_setup(struct spi_device *spi)
>>>       if (!spi->max_speed_hz)
>>>               spi->max_speed_hz = spi->master->max_speed_hz;
>>>
>>> -     spi_set_cs(spi, false);
>>> -
>>>       if (spi->master->setup)
>>>               status = spi->master->setup(spi);
>>>
>>> +     spi_set_cs(spi, false);
>>> +
>>>       dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
>>>                       (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>>>                       (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
>>> --
>>> 2.6.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
The recent change to the bitbang driver leads to the the set_cs hook
of spi_master being set
now for all drivers using the bitbang layer. This hook is called also
from spi_setup and therefore
one possible side effect is issues with bitbang drivers implementing
the chipselect hook of
spi_bitbang with a dependency on the master being set up before.
The proposed patch looks good to me.
There should be no impact on drivers not using bitbang.

Re: [RFC][PATCH] spi: Setup the master controller driver before setting the chipselect

[Franklin S Cooper Jr.]

On 10/14/2015 06:45 AM, Heiner Kallweit wrote:
> On Wed, Oct 14, 2015 at 1:08 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> +Cc: Jarkko to see from spi-pxa2xx prospective
>>
>> On Wed, Oct 14, 2015 at 12:47 PM, Ivan T. Ivanov <iivanov.xz@gmail.com> wrote:
>>> Adding Andy.
>>>
>>>
>>>> On Oct 13, 2015, at 12:01 AM, Franklin S Cooper Jr <fcooper@ti.com> wrote:
>>>>
>>>> Some devices depend on the master controller driver setup function being
>>>> called before calling any chipselect functions.
>>>>
>>>> Insure that this is done otherwise uninitialized structures may be
>>>> accessed causing a kernel panic.
>> As far as I understand my concern should be about spi-dw driver.
>>
>> So, I have just tested yesterday's linux-next with and without
>> proposed patch. Works for me:
>> Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>
>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>> ---
>>>> drivers/spi/spi.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>>> index 38006cc..9374d82 100644
>>>> --- a/drivers/spi/spi.c
>>>> +++ b/drivers/spi/spi.c
>>>> @@ -2053,11 +2053,11 @@ int spi_setup(struct spi_device *spi)
>>>>       if (!spi->max_speed_hz)
>>>>               spi->max_speed_hz = spi->master->max_speed_hz;
>>>>
>>>> -     spi_set_cs(spi, false);
>>>> -
>>>>       if (spi->master->setup)
>>>>               status = spi->master->setup(spi);
>>>>
>>>> +     spi_set_cs(spi, false);
>>>> +
>>>>       dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
>>>>                       (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
>>>>                       (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
>>>> --
>>>> 2.6.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-spi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> The recent change to the bitbang driver leads to the the set_cs hook
> of spi_master being set
> now for all drivers using the bitbang layer. This hook is called also
> from spi_setup and therefore
> one possible side effect is issues with bitbang drivers implementing
> the chipselect hook of
> spi_bitbang with a dependency on the master being set up before.
> The proposed patch looks good to me.
> There should be no impact on drivers not using bitbang.
Thank all. Since nothing obvious seems wrong with this patch I will resend this patch without the RFC.