diff mbox series

[RFC,1/3] spi: Allow SPI controller override device buswidth

Message ID 1582903131-160033-2-git-send-email-john.garry@huawei.com
State New
Headers show
Series spi/HiSilicon v3xx: Support dual and quad mode through DMI quirks | expand

Commit Message

John Garry Feb. 28, 2020, 3:18 p.m. UTC 
Currently ACPI firmware description for a SPI device does not have any
method to describe the data buswidth on the board.

So even through the controller and device may support higher modes than
standard SPI, it cannot be assumed that the board does - as such, that
device is limited to standard SPI in such a circumstance.

As a workaround, allow the controller driver supply buswidth override bits,
which are used inform the core code that the controller driver knows the
buswidth supported on that board for that device.

A host controller driver might know this info from DMI tables, for example.

Signed-off-by: John Garry <john.garry@huawei.com>

---
 drivers/spi/spi.c       | 4 +++-
 include/linux/spi/spi.h | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.17.1


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

Comments

Sergei Shtylyov March 1, 2020, 10:04 a.m. UTC | #1 
Hello!

On 28.02.2020 18:18, John Garry wrote:

> Currently ACPI firmware description for a SPI device does not have any

> method to describe the data buswidth on the board.

> 

> So even through the controller and device may support higher modes than

           ^^^^^^^
    Though?

> standard SPI, it cannot be assumed that the board does - as such, that

> device is limited to standard SPI in such a circumstance.

> 

> As a workaround, allow the controller driver supply buswidth override bits,

> which are used inform the core code that the controller driver knows the

> buswidth supported on that board for that device.

> 

> A host controller driver might know this info from DMI tables, for example.

> 

> Signed-off-by: John Garry <john.garry@huawei.com>

> ---

>   drivers/spi/spi.c       | 4 +++-

>   include/linux/spi/spi.h | 3 +++

>   2 files changed, 6 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c

> index 38b4c78df506..292f26807b41 100644

> --- a/drivers/spi/spi.c

> +++ b/drivers/spi/spi.c

> @@ -510,6 +510,7 @@ struct spi_device *spi_alloc_device(struct spi_controller *ctlr)

>   	spi->dev.bus = &spi_bus_type;

>   	spi->dev.release = spidev_release;

>   	spi->cs_gpio = -ENOENT;

> +	spi->mode = ctlr->buswidth_override_bits;

>   

>   	spin_lock_init(&spi->statistics.lock);

>   

> @@ -2181,9 +2182,10 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,

>   		return AE_NO_MEMORY;

>   	}

>   

> +


    What for?

>   	ACPI_COMPANION_SET(&spi->dev, adev);

>   	spi->max_speed_hz	= lookup.max_speed_hz;

> -	spi->mode		= lookup.mode;

> +	spi->mode		|= lookup.mode;

>   	spi->irq		= lookup.irq;

>   	spi->bits_per_word	= lookup.bits_per_word;

>   	spi->chip_select	= lookup.chip_select;

[...]

MBR, Sergei

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
John Garry March 2, 2020, 9:30 a.m. UTC | #2 
On 01/03/2020 10:04, Sergei Shtylyov wrote:
> Hello!
> 

Hi Sergei,

> On 28.02.2020 18:18, John Garry wrote:
> 
>> Currently ACPI firmware description for a SPI device does not have any
>> method to describe the data buswidth on the board.
>>
>> So even through the controller and device may support higher modes than
>            ^^^^^^^
>     Though?
> 

right

>> standard SPI, it cannot be assumed that the board does - as such, that
>> device is limited to standard SPI in such a circumstance.
>>
>> As a workaround, allow the controller driver supply buswidth override 
>> bits,
>> which are used inform the core code that the controller driver knows the
>> buswidth supported on that board for that device.
>>
>> A host controller driver might know this info from DMI tables, for 
>> example.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/spi/spi.c       | 4 +++-
>>   include/linux/spi/spi.h | 3 +++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 38b4c78df506..292f26807b41 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -510,6 +510,7 @@ struct spi_device *spi_alloc_device(struct 
>> spi_controller *ctlr)
>>       spi->dev.bus = &spi_bus_type;
>>       spi->dev.release = spidev_release;
>>       spi->cs_gpio = -ENOENT;
>> +    spi->mode = ctlr->buswidth_override_bits;
>>       spin_lock_init(&spi->statistics.lock);
>> @@ -2181,9 +2182,10 @@ static acpi_status 
>> acpi_register_spi_device(struct spi_controller *ctlr,
>>           return AE_NO_MEMORY;
>>       }
>> +
> 
>     What for?

slipped through the net

> 
>>       ACPI_COMPANION_SET(&spi->dev, adev);
>>       spi->max_speed_hz    = lookup.max_speed_hz;
>> -    spi->mode        = lookup.mode;
>> +    spi->mode        |= lookup.mode;
>>       spi->irq        = lookup.irq;
>>       spi->bits_per_word    = lookup.bits_per_word;
>>       spi->chip_select    = lookup.chip_select;
> [...]

TBH, I did not think that this series would be applied since I tagged it 
as RFC, hence the typos which would have been caught. Indeed, this also 
exposes an issue with enabling quad SPI for a spansion SPI NOR part, 
which I need to debug now in the SPI NOR driver.

Hi Mark,

Do you want me to do anything about the above superfluous newline?

Thanks,
John

> 
> MBR, Sergei
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> .
Mark Brown March 2, 2020, 12:22 p.m. UTC | #3 
On Mon, Mar 02, 2020 at 09:30:08AM +0000, John Garry wrote:

> Do you want me to do anything about the above superfluous newline?


Whatever you prefer, I don't really care either way.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Geert Uytterhoeven March 2, 2020, 4:12 p.m. UTC | #4 
Hi John,

Thanks for your patch!

On Fri, Feb 28, 2020 at 4:23 PM John Garry <john.garry@huawei.com> wrote:
> Currently ACPI firmware description for a SPI device does not have any

> method to describe the data buswidth on the board.

>

> So even through the controller and device may support higher modes than

> standard SPI, it cannot be assumed that the board does - as such, that

> device is limited to standard SPI in such a circumstance.


Indeed.

> As a workaround, allow the controller driver supply buswidth override bits,

> which are used inform the core code that the controller driver knows the

> buswidth supported on that board for that device.


I feel this is a bit dangerous, and might bite us one day.

> A host controller driver might know this info from DMI tables, for example.


Can't acpi_register_spi_device() obtain that info from DMI tables,
to avoid contaminating the generic code?

> Signed-off-by: John Garry <john.garry@huawei.com>


> --- a/drivers/spi/spi.c

> +++ b/drivers/spi/spi.c

> @@ -510,6 +510,7 @@ struct spi_device *spi_alloc_device(struct spi_controller *ctlr)

>         spi->dev.bus = &spi_bus_type;

>         spi->dev.release = spidev_release;

>         spi->cs_gpio = -ENOENT;

> +       spi->mode = ctlr->buswidth_override_bits;


This could just be moved to acpi_register_spi_device(), right?

>

>         spin_lock_init(&spi->statistics.lock);

>

> @@ -2181,9 +2182,10 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,

>                 return AE_NO_MEMORY;

>         }

>

> +

>         ACPI_COMPANION_SET(&spi->dev, adev);

>         spi->max_speed_hz       = lookup.max_speed_hz;

> -       spi->mode               = lookup.mode;

> +       spi->mode               |= lookup.mode;

>         spi->irq                = lookup.irq;

>         spi->bits_per_word      = lookup.bits_per_word;

>         spi->chip_select        = lookup.chip_select;

> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h

> index 6d16ba01ff5a..600e3793303e 100644

> --- a/include/linux/spi/spi.h

> +++ b/include/linux/spi/spi.h

> @@ -481,6 +481,9 @@ struct spi_controller {

>         /* spi_device.mode flags understood by this controller driver */

>         u32                     mode_bits;

>

> +       /* spi_device.mode flags override flags for this controller */

> +       u32                     buswidth_override_bits;


And I'd be happy if we could avoid adding this here ;-)

> +

>         /* bitmask of supported bits_per_word for transfers */

>         u32                     bits_per_word_mask;

>  #define SPI_BPW_MASK(bits) BIT((bits) - 1)


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Mark Brown March 2, 2020, 4:33 p.m. UTC | #5 
On Mon, Mar 02, 2020 at 05:12:05PM +0100, Geert Uytterhoeven wrote:
> On Fri, Feb 28, 2020 at 4:23 PM John Garry <john.garry@huawei.com> wrote:


> > A host controller driver might know this info from DMI tables, for example.


> Can't acpi_register_spi_device() obtain that info from DMI tables,

> to avoid contaminating the generic code?


The DMI tables are going to boil down to per board quirks which we
*could* put in the core but you end up with a lot of them and chances
are that at some point we'll end up with device specific quirks which
don't fit so well in the core.  Handling stuff in the drivers is fairly
idiomatic.

Much ACPI, so standards :/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Geert Uytterhoeven March 2, 2020, 6:51 p.m. UTC | #6 
Hi John,

On Fri, Feb 28, 2020 at 4:23 PM John Garry <john.garry@huawei.com> wrote:
> Currently ACPI firmware description for a SPI device does not have any

> method to describe the data buswidth on the board.

>

> So even through the controller and device may support higher modes than

> standard SPI, it cannot be assumed that the board does - as such, that

> device is limited to standard SPI in such a circumstance.

>

> As a workaround, allow the controller driver supply buswidth override bits,

> which are used inform the core code that the controller driver knows the

> buswidth supported on that board for that device.


Just wondering: can't the controller just override this (e.g. in the .setuup()
callback) without having to touch the generic code?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
John Garry March 3, 2020, 9:42 a.m. UTC | #7 
Hi Geert,

> 

> On Fri, Feb 28, 2020 at 4:23 PM John Garry <john.garry@huawei.com> wrote:

>> Currently ACPI firmware description for a SPI device does not have any

>> method to describe the data buswidth on the board.

>>

>> So even through the controller and device may support higher modes than

>> standard SPI, it cannot be assumed that the board does - as such, that

>> device is limited to standard SPI in such a circumstance.

>>

>> As a workaround, allow the controller driver supply buswidth override bits,

>> which are used inform the core code that the controller driver knows the

>> buswidth supported on that board for that device.

> 

> Just wondering: can't the controller just override this (e.g. in the .setuup()

> callback) without having to touch the generic code?


I think that this is a good idea.

However, where we call .setup() in spi_setup() looks a little too late - 
at the point we call .setup(), most of the work on vetting/fixing the 
mode bits is complete. And I would not want the SPI controller driver 
just to disregard this work and overwrite the bits (in this way).

And if we wanted to move the .setup callback earlier, then I would be 
worried here with 2 things:
1. Some SPI controller drivers may rely on spi->mode being set finally 
when .setup() is called
2. We may need to undo the work of .setup() if we later error in 
spi_setup(), like for invalid mode bits

However, maybe another callback could be introduced, .early_setup().

Thanks,
John

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Mark Brown March 3, 2020, 12:43 p.m. UTC | #8 
On Tue, Mar 03, 2020 at 09:42:45AM +0000, John Garry wrote:

> However, maybe another callback could be introduced, .early_setup().


One thing I like about the explicit core code is that it makes it much
easier to see which drivers are doing the worrying thing, with just
overwriting things in a callback everything is very freeform and you
have to audit things individually.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
diff mbox series

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 38b4c78df506..292f26807b41 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -510,6 +510,7 @@  struct spi_device *spi_alloc_device(struct spi_controller *ctlr)
 	spi->dev.bus = &spi_bus_type;
 	spi->dev.release = spidev_release;
 	spi->cs_gpio = -ENOENT;
+	spi->mode = ctlr->buswidth_override_bits;
 
 	spin_lock_init(&spi->statistics.lock);
 
@@ -2181,9 +2182,10 @@  static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
 		return AE_NO_MEMORY;
 	}
 
+
 	ACPI_COMPANION_SET(&spi->dev, adev);
 	spi->max_speed_hz	= lookup.max_speed_hz;
-	spi->mode		= lookup.mode;
+	spi->mode		|= lookup.mode;
 	spi->irq		= lookup.irq;
 	spi->bits_per_word	= lookup.bits_per_word;
 	spi->chip_select	= lookup.chip_select;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 6d16ba01ff5a..600e3793303e 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -481,6 +481,9 @@  struct spi_controller {
 	/* spi_device.mode flags understood by this controller driver */
 	u32			mode_bits;
 
+	/* spi_device.mode flags override flags for this controller */
+	u32			buswidth_override_bits;
+
 	/* bitmask of supported bits_per_word for transfers */
 	u32			bits_per_word_mask;
 #define SPI_BPW_MASK(bits) BIT((bits) - 1)