Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Ben Hutchings]

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

On Tue, 2014-09-02 at 16:54 +0100, Ian Campbell wrote:
> Hello,
> 
> I recently tried to switch to using the DT based kirkwood stuff with
> v3.16.1 on a QNAP TS-419 for the Debian kernel. Most things seemed ok
> except that the flash driver was not autoloaded -- as well as spi-nor.ko
> I had to manually modprobe m25p80.ko. Once I loaded both the flash was
> detected.
> 
> I noticed that many platforms declare the flash chip as compatible =
> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
> changing that and it didn't help. In any case without spi-nor.ko being
> autoloaded I don't support m25p80.ko ever would be.

m25p80.c has:

static struct spi_driver m25p80_driver = {
	...
	.id_table	= spi_nor_ids,
	...
};

while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
separate modules, modpost can't read the ID table and add the device ID
aliases to m25p80.ko.

It looks like m25p80 has been split up in order that the new fsl-qspi
driver can share its code (bypassing the SPI core and its device
model!).  Why doesn't fsl-qspi implement an SPI controller device?  I
think it is totally unacceptable to regress m25p80 in this way for the
sake of a new driver.

> I could set these things to =m for the Debian kirkwood flavour, but
> since this is supposed to be a distro-style kernel.
> 
> I'm not entirely sure how module loading of this SPI stuff is supposed
> to fit together -- anyone got any hints?

Same as for any bus driver.

Ben.

-- 
Ben Hutchings
Experience is directly proportional to the value of equipment destroyed.
                                                         - Carolyn Scheppner

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Geert Uytterhoeven]

Hi Ben,

On Thu, Sep 4, 2014 at 4:11 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> I noticed that many platforms declare the flash chip as compatible =
>> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
>> changing that and it didn't help. In any case without spi-nor.ko being
>> autoloaded I don't support m25p80.ko ever would be.
>
> m25p80.c has:
>
> static struct spi_driver m25p80_driver = {
>         ...
>         .id_table       = spi_nor_ids,
>         ...
> };
>
> while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
> separate modules, modpost can't read the ID table and add the device ID
> aliases to m25p80.ko.

Woops. This indeed doesn't work.
Note that the MODULE_DEVICE_TABLE() is also gone from m25p80.c

So m25p80.c needs to contain the IDs, while spi-nor.c also needs the IDs
because it's a framework/library.

A quick solution would be to move the ID table to a header file, and include
that by both, and re-add the lost MODULE_DEVICE_TABLE() to m25p80.c.
That duplicates the data, though.

Hmm, for the built-in case, we can avoid the duplication by letting m25p80.c
refer to the table in spi-nor.c if !MODULE.
Does anyone see a better solution?

> It looks like m25p80 has been split up in order that the new fsl-qspi
> driver can share its code (bypassing the SPI core and its device
> model!).  Why doesn't fsl-qspi implement an SPI controller device?  I
> think it is totally unacceptable to regress m25p80 in this way for the
> sake of a new driver.

Fsl-qspi is not a generic SPI driver, but a dedicated SPI NOR driver.
It's programmed using high-level SPI NOR commands, instead of low-level
SPI commands.

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

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Brian Norris]

On Thu, Sep 04, 2014 at 09:02:04AM +0200, Geert Uytterhoeven wrote:
> On Thu, Sep 4, 2014 at 4:11 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> >> I noticed that many platforms declare the flash chip as compatible =
> >> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
> >> changing that and it didn't help. In any case without spi-nor.ko being
> >> autoloaded I don't support m25p80.ko ever would be.
> >
> > m25p80.c has:
> >
> > static struct spi_driver m25p80_driver = {
> >         ...
> >         .id_table       = spi_nor_ids,
> >         ...
> > };
> >
> > while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
> > separate modules, modpost can't read the ID table and add the device ID
> > aliases to m25p80.ko.
> 
> Woops. This indeed doesn't work.
> Note that the MODULE_DEVICE_TABLE() is also gone from m25p80.c
> 
> So m25p80.c needs to contain the IDs, while spi-nor.c also needs the IDs
> because it's a framework/library.

Actually, m25p80.c only needs the strings (i.e., the named aliases --
character data), and for the most part, spi-nor.c only needs the IDs (i.e.,
the identification bytes).

What's more, I don't think that any modern SPI NOR user really needs to
be specifying exactly which SPI device it is via a specific string. Our
driver code pretty much always re-detects the device by reading its
device ID. All the SPI NOR code needs to know is how to read its ID.

> A quick solution would be to move the ID table to a header file, and include
> that by both, and re-add the lost MODULE_DEVICE_TABLE() to m25p80.c.
> That duplicates the data, though.
> 
> Hmm, for the built-in case, we can avoid the duplication by letting m25p80.c
> refer to the table in spi-nor.c if !MODULE.
> Does anyone see a better solution?

A little bit of a shot in the dark, as I haven't fleshed this one out:

Would it work to just copy the SPI ID strings into m25p80.c, keep the
full table in spi-nor.c, stop adding SPI ID string (and DT) bindings to
the m25p80 table (force platforms to use "m25p80"-compatible probing, or
maybe something generic like "spi-nor-rdid", "spi-nor-sfdp", etc.) and
eventually kill the strings from spi-nor.c entirely? I really don't want
to propagate string-binding much further into the SPI NOR library, since
everything should be auto-detectable--partly just by an ID table as we
have now, but eventually we can use CFI or SFDP parameters provided by
relatively new SPI NOR chips.

I'm not sure if this is great for the short-term problem of fixing
module-autoloading. Perhaps we should do a short-term hack to duplicate
the SPI ID strings to m25p80.c by including them in a header (and
backport for 3.16.y stable?), but I'd like to disentangle this.

Brian

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Brian Norris]

+ Huang (his Freescale address is bouncing), Rafal

On Tue, Sep 9, 2014 at 11:00 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Thu, Sep 04, 2014 at 09:02:04AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Sep 4, 2014 at 4:11 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> >> I noticed that many platforms declare the flash chip as compatible =
>> >> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
>> >> changing that and it didn't help. In any case without spi-nor.ko being
>> >> autoloaded I don't support m25p80.ko ever would be.
>> >
>> > m25p80.c has:
>> >
>> > static struct spi_driver m25p80_driver = {
>> >         ...
>> >         .id_table       = spi_nor_ids,
>> >         ...
>> > };
>> >
>> > while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
>> > separate modules, modpost can't read the ID table and add the device ID
>> > aliases to m25p80.ko.
>>
>> Woops. This indeed doesn't work.
>> Note that the MODULE_DEVICE_TABLE() is also gone from m25p80.c
>>
>> So m25p80.c needs to contain the IDs, while spi-nor.c also needs the IDs
>> because it's a framework/library.
>
> Actually, m25p80.c only needs the strings (i.e., the named aliases --
> character data), and for the most part, spi-nor.c only needs the IDs (i.e.,
> the identification bytes).
>
> What's more, I don't think that any modern SPI NOR user really needs to
> be specifying exactly which SPI device it is via a specific string. Our
> driver code pretty much always re-detects the device by reading its
> device ID. All the SPI NOR code needs to know is how to read its ID.
>
>> A quick solution would be to move the ID table to a header file, and include
>> that by both, and re-add the lost MODULE_DEVICE_TABLE() to m25p80.c.
>> That duplicates the data, though.
>>
>> Hmm, for the built-in case, we can avoid the duplication by letting m25p80.c
>> refer to the table in spi-nor.c if !MODULE.
>> Does anyone see a better solution?
>
> A little bit of a shot in the dark, as I haven't fleshed this one out:
>
> Would it work to just copy the SPI ID strings into m25p80.c, keep the
> full table in spi-nor.c, stop adding SPI ID string (and DT) bindings to
> the m25p80 table (force platforms to use "m25p80"-compatible probing, or
> maybe something generic like "spi-nor-rdid", "spi-nor-sfdp", etc.) and
> eventually kill the strings from spi-nor.c entirely? I really don't want
> to propagate string-binding much further into the SPI NOR library, since
> everything should be auto-detectable--partly just by an ID table as we
> have now, but eventually we can use CFI or SFDP parameters provided by
> relatively new SPI NOR chips.
>
> I'm not sure if this is great for the short-term problem of fixing
> module-autoloading. Perhaps we should do a short-term hack to duplicate
> the SPI ID strings to m25p80.c by including them in a header (and
> backport for 3.16.y stable?), but I'd like to disentangle this.
>
> Brian

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Geert Uytterhoeven]

+ devicetree, as the names are used in DTS file.

On Wed, Sep 10, 2014 at 8:11 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> + Huang (his Freescale address is bouncing), Rafal
>
> On Tue, Sep 9, 2014 at 11:00 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
>> On Thu, Sep 04, 2014 at 09:02:04AM +0200, Geert Uytterhoeven wrote:
>>> On Thu, Sep 4, 2014 at 4:11 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
>>> >> I noticed that many platforms declare the flash chip as compatible =
>>> >> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
>>> >> changing that and it didn't help. In any case without spi-nor.ko being
>>> >> autoloaded I don't support m25p80.ko ever would be.
>>> >
>>> > m25p80.c has:
>>> >
>>> > static struct spi_driver m25p80_driver = {
>>> >         ...
>>> >         .id_table       = spi_nor_ids,
>>> >         ...
>>> > };
>>> >
>>> > while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
>>> > separate modules, modpost can't read the ID table and add the device ID
>>> > aliases to m25p80.ko.
>>>
>>> Woops. This indeed doesn't work.
>>> Note that the MODULE_DEVICE_TABLE() is also gone from m25p80.c
>>>
>>> So m25p80.c needs to contain the IDs, while spi-nor.c also needs the IDs
>>> because it's a framework/library.
>>
>> Actually, m25p80.c only needs the strings (i.e., the named aliases --
>> character data), and for the most part, spi-nor.c only needs the IDs (i.e.,
>> the identification bytes).
>>
>> What's more, I don't think that any modern SPI NOR user really needs to
>> be specifying exactly which SPI device it is via a specific string. Our
>> driver code pretty much always re-detects the device by reading its
>> device ID. All the SPI NOR code needs to know is how to read its ID.

But the proper name should be in the compatible property.

>>> A quick solution would be to move the ID table to a header file, and include
>>> that by both, and re-add the lost MODULE_DEVICE_TABLE() to m25p80.c.
>>> That duplicates the data, though.
>>>
>>> Hmm, for the built-in case, we can avoid the duplication by letting m25p80.c
>>> refer to the table in spi-nor.c if !MODULE.
>>> Does anyone see a better solution?
>>
>> A little bit of a shot in the dark, as I haven't fleshed this one out:
>>
>> Would it work to just copy the SPI ID strings into m25p80.c, keep the
>> full table in spi-nor.c, stop adding SPI ID string (and DT) bindings to
>> the m25p80 table (force platforms to use "m25p80"-compatible probing, or
>> maybe something generic like "spi-nor-rdid", "spi-nor-sfdp", etc.) and
>> eventually kill the strings from spi-nor.c entirely? I really don't want
>> to propagate string-binding much further into the SPI NOR library, since
>> everything should be auto-detectable--partly just by an ID table as we
>> have now, but eventually we can use CFI or SFDP parameters provided by
>> relatively new SPI NOR chips.

How does the SPI core know to use m25p80? From the ids, and the compatible
value. If you won't add new names, all devices should declare compatiblity
with "m25p80" (that's "spansion,m25p80", right?) as a fallback.
However, are all devices really compatible with m25p80?
I.e. does m25p80.c work will all devices, in the absense of support for the
real device? If not, the fallback must not be there, and new device names
must be added.

>> I'm not sure if this is great for the short-term problem of fixing
>> module-autoloading. Perhaps we should do a short-term hack to duplicate
>> the SPI ID strings to m25p80.c by including them in a header (and
>> backport for 3.16.y stable?), but I'd like to disentangle this.
>>
>> Brian

Autoloading is based on "spi:<name>". If new names aren't added, m25p80
won't be loaded for new SPI FLASHes, unless I'm missing something?

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

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Ben Hutchings]

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

On Tue, 2014-09-09 at 23:00 -0700, Brian Norris wrote:
> On Thu, Sep 04, 2014 at 09:02:04AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Sep 4, 2014 at 4:11 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > >> I noticed that many platforms declare the flash chip as compatible =
> > >> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
> > >> changing that and it didn't help. In any case without spi-nor.ko being
> > >> autoloaded I don't support m25p80.ko ever would be.
> > >
> > > m25p80.c has:
> > >
> > > static struct spi_driver m25p80_driver = {
> > >         ...
> > >         .id_table       = spi_nor_ids,
> > >         ...
> > > };
> > >
> > > while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
> > > separate modules, modpost can't read the ID table and add the device ID
> > > aliases to m25p80.ko.
> > 
> > Woops. This indeed doesn't work.
> > Note that the MODULE_DEVICE_TABLE() is also gone from m25p80.c
> > 
> > So m25p80.c needs to contain the IDs, while spi-nor.c also needs the IDs
> > because it's a framework/library.
> 
> Actually, m25p80.c only needs the strings (i.e., the named aliases --
> character data), and for the most part, spi-nor.c only needs the IDs (i.e.,
> the identification bytes).

Unfortunately, spi-nor.c also needs the strings to implement
spi_nor_match_id() (which is only used by fsl-quadspi.c) and to report
mismatches in spi_nor_scan().

But spi_nor_match_id(), spi_nor_scan() and spi_nor::read_id should work
with something like struct flash_info, not struct spi_device_id.

> What's more, I don't think that any modern SPI NOR user really needs to
> be specifying exactly which SPI device it is via a specific string. Our
> driver code pretty much always re-detects the device by reading its
> device ID. All the SPI NOR code needs to know is how to read its ID.
>
> > A quick solution would be to move the ID table to a header file, and include
> > that by both, and re-add the lost MODULE_DEVICE_TABLE() to m25p80.c.
> > That duplicates the data, though.
> > 
> > Hmm, for the built-in case, we can avoid the duplication by letting m25p80.c
> > refer to the table in spi-nor.c if !MODULE.
> > Does anyone see a better solution?
> 
> A little bit of a shot in the dark, as I haven't fleshed this one out:
> 
> Would it work to just copy the SPI ID strings into m25p80.c, keep the
> full table in spi-nor.c, stop adding SPI ID string (and DT) bindings to
> the m25p80 table (force platforms to use "m25p80"-compatible probing, or
> maybe something generic like "spi-nor-rdid", "spi-nor-sfdp", etc.) and
> eventually kill the strings from spi-nor.c entirely?

I think that a DT node is always supposed to include a compatible string
for the specific device.  It could also include a generic compatible
string for SPI NOR chips, but the *only* thing a driver can do with that
is to use the JEDEC ID command.  It can't even generically read a single
byte, since addresses may be either 3 or 4 bytes long!  So I think that
if a generic compatible string is defined, the DT binding must also
define the properties that spi-nor currently reads out of its static
table.

> I really don't want
> to propagate string-binding much further into the SPI NOR library, since
> everything should be auto-detectable--partly just by an ID table as we
> have now, but eventually we can use CFI or SFDP parameters provided by
> relatively new SPI NOR chips.
>
> I'm not sure if this is great for the short-term problem of fixing
> module-autoloading. Perhaps we should do a short-term hack to duplicate
> the SPI ID strings to m25p80.c by including them in a header (and
> backport for 3.16.y stable?), but I'd like to disentangle this.

I'll have a go at doing that.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: Autoloading of SPI/nor driver on kirkwood (qnap devices)

[Ben Hutchings]

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

[Re-sending with more recent address for Huang Shijie.]

On Tue, 2014-09-02 at 16:54 +0100, Ian Campbell wrote:
> Hello,
> 
> I recently tried to switch to using the DT based kirkwood stuff with
> v3.16.1 on a QNAP TS-419 for the Debian kernel. Most things seemed ok
> except that the flash driver was not autoloaded -- as well as spi-nor.ko
> I had to manually modprobe m25p80.ko. Once I loaded both the flash was
> detected.
> 
> I noticed that many platforms declare the flash chip as compatible =
> "st,m25pXXX" whereas the ts219.dtsi just said m25p128 but I tried
> changing that and it didn't help. In any case without spi-nor.ko being
> autoloaded I don't support m25p80.ko ever would be.

m25p80.c has:

static struct spi_driver m25p80_driver = {
	...
	.id_table	= spi_nor_ids,
	...
};

while spi_nor_ids is defined in spi-nor.c.  Since they end up as two
separate modules, modpost can't read the ID table and add the device ID
aliases to m25p80.ko.

It looks like m25p80 has been split up in order that the new fsl-qspi
driver can share its code (bypassing the SPI core and its device
model!).  Why doesn't fsl-qspi implement an SPI controller device?  I
think it is totally unacceptable to regress m25p80 in this way for the
sake of a new driver.

> I could set these things to =m for the Debian kirkwood flavour, but
> since this is supposed to be a distro-style kernel.
> 
> I'm not entirely sure how module loading of this SPI stuff is supposed
> to fit together -- anyone got any hints?

Same as for any bus driver.

Ben.

-- 
Ben Hutchings
Experience is directly proportional to the value of equipment destroyed.
                                                         - Carolyn Scheppner

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]