Aspeed SPI driver upstreaming

Aspeed SPI driver upstreaming

[Patrick Williams]

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

Cedric, Joel,

There is currently the aspeed-smc driver[1], which is upstreamed, but only
supports spi-nor devices.  There also a more generic spi-aspeed
driver[2], which might only exist in Facebook kernel trees, that
supports all spi devices but it doesn't do the calibration work.

I made some changes to the spi-aspeed driver recently in order to get it
to somewhat support TPM 2.0 devices (*).  The spi-aspeed driver also
already supported generic spi-nor MTD devices, but just at a slower
speed than aspeed-smc due to missing the calibration routines.

Tao mentioned to me that there was a discussion at one of the F2F events
in 2019 about combining those two drivers and getting them upstreamed,
but that the hang-up was getting upstream mtd and spi subsystems to
agree on how to handle calibration routines in the spi subsystem?  I
can't seem to find anything about this on the LKML.  Do either of you
know where that discussion went and what the current state / plans of
upstreamming a generic Aspeed SPI driver are?

[1] https://github.com/openbmc/linux/blob/dev-5.3/drivers/mtd/spi-nor/aspeed-smc.c
[2] https://github.com/facebook/openbmc-linux/blob/dev-5.0/drivers/spi/spi-aspeed.c

(*) The Aspeed SPI master is half-duplex and the TPM SPI spec effectively
    requires full duplex hardware.  I did some workarounds to get it to work
    with one particular part and need to work with the vendor and upstream
    to figure out the best way to reliably handle half-duplex SPI masters.
-- 
Patrick Williams

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

Re: Aspeed SPI driver upstreaming

[Cédric Le Goater]

Hello,

On 1/7/20 12:27 AM, Patrick Williams wrote:
> Cedric, Joel,
> 
> There is currently the aspeed-smc driver[1], which is upstreamed, but only
> supports spi-nor devices.  

Upstream lacks read training.

> There also a more generic spi-aspeed
> driver[2], which might only exist in Facebook kernel trees, that
> supports all spi devices but it doesn't do the calibration work.
> 
> I made some changes to the spi-aspeed driver recently in order to get it
> to somewhat support TPM 2.0 devices (*).  The spi-aspeed driver also
> already supported generic spi-nor MTD devices, but just at a slower
> speed than aspeed-smc due to missing the calibration routines.

Yes. That is the problem.

> Tao mentioned to me that there was a discussion at one of the F2F events
> in 2019 about combining those two drivers and getting them upstreamed,
> but that the hang-up was getting upstream mtd and spi subsystems to
> agree on how to handle calibration routines in the spi subsystem?  I
> can't seem to find anything about this on the LKML.  Do either of you
> know where that discussion went and what the current state / plans of
> upstreamming a generic Aspeed SPI driver are?

Regarding the SMC driver, the maintainers are requesting a rewrite 
of the driver using the spimem layer, but we lack handlers to do 
the read training and compute the timing register value.

This is the first thing to address on the todo list. When available,
it shouldn't take too long to upstream the driver. Some more info 
here :

   https://www.spinics.net/lists/linux-mtd/msg09417.html

Cheers,

C.

> 
> [1] https://github.com/openbmc/linux/blob/dev-5.3/drivers/mtd/spi-nor/aspeed-smc.c
> [2] https://github.com/facebook/openbmc-linux/blob/dev-5.0/drivers/spi/spi-aspeed.c
> 
> (*) The Aspeed SPI master is half-duplex and the TPM SPI spec effectively
>     requires full duplex hardware.  I did some workarounds to get it to work
>     with one particular part and need to work with the vendor and upstream
>     to figure out the best way to reliably handle half-duplex SPI masters.
> 

Re: Aspeed SPI driver upstreaming

[Patrick Williams]

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

Thanks for the reply Cédric.

On Tue, Jan 07, 2020 at 09:34:03AM +0100, Cédric Le Goater wrote:
> Regarding the SMC driver, the maintainers are requesting a rewrite 
> of the driver using the spimem layer, but we lack handlers to do 
> the read training and compute the timing register value.
> 
> This is the first thing to address on the todo list. When available,
> it shouldn't take too long to upstream the driver. Some more info 
> here :
> 
>    https://www.spinics.net/lists/linux-mtd/msg09417.html
> 

It looks like this patch set is still the MTD-only implementation, which
is useful for SPI-NOR chips but not useful for non-flash devices such as
TPMs.  Is there any work or thought into how we could do a generic SPI
controller and then layer the MTD above it?

We have some system designs where we have both a NOR device and a TPM on
the same SPI bus.  What we're currently doing is using the
(non-upstream) aspeed-spi driver which lets us use both the TPM and
MTD/SPI-NOR driver, but since it doesn't have the calibration routines
the SPI-NOR runs at a slower speed than optimal.

I'd really like to get a generic SPI controller driver upstreamed, even if it
doesn't have the calibration (the SPI-NOR device in this case is not as
performance critical as the BMC's own NOR devices).  Is there any path
to combining the features of the aspeed-smc and aspeed-spi into a single
driver, or do you think we should start with them as separate and get
aspeed-spi upstreamed as an alternative to aspeed-smc?

Also, do you have any soft timeline on the follow-ups to this patch set?

-- 
Patrick Williams

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

Re: Aspeed SPI driver upstreaming

[Cédric Le Goater]

On 1/9/20 5:43 PM, Patrick Williams wrote:
> Thanks for the reply Cédric.
> 
> On Tue, Jan 07, 2020 at 09:34:03AM +0100, Cédric Le Goater wrote:
>> Regarding the SMC driver, the maintainers are requesting a rewrite 
>> of the driver using the spimem layer, but we lack handlers to do 
>> the read training and compute the timing register value.
>>
>> This is the first thing to address on the todo list. When available,
>> it shouldn't take too long to upstream the driver. Some more info 
>> here :
>>
>>    https://www.spinics.net/lists/linux-mtd/msg09417.html
>>
> 
> It looks like this patch set is still the MTD-only implementation, which
> is useful for SPI-NOR chips but not useful for non-flash devices such as
> TPMs.  Is there any work or thought into how we could do a generic SPI
> controller and then layer the MTD above it?

Not on my side. I always worked with flash support in mind.

> We have some system designs where we have both a NOR device and a TPM on
> the same SPI bus.  What we're currently doing is using the
> (non-upstream) aspeed-spi driver which lets us use both the TPM and
> MTD/SPI-NOR driver, but since it doesn't have the calibration routines
> the SPI-NOR runs at a slower speed than optimal.
> 
> I'd really like to get a generic SPI controller driver upstreamed, even if it
> doesn't have the calibration (the SPI-NOR device in this case is not as
> performance critical as the BMC's own NOR devices).  Is there any path
> to combining the features of the aspeed-smc and aspeed-spi into a single
> driver, or do you think we should start with them as separate and get
> aspeed-spi upstreamed as an alternative to aspeed-smc?

Yes, we should and this is requested by the MTD community which wants
to deprecate SPI-NOR.

> Also, do you have any soft timeline on the follow-ups to this patch set?

AST2600 was done on my spare time this year. I have been working on the 
other side of the system (PowerPC XIVE) these last two/three years and 
should continue to do so.

If a new Aspeed flash driver should emerge, I think some else should take 
ownership. It will be better for maintenance

Cheers,

C.
 

Re: Aspeed SPI driver upstreaming

[Brad Bishop]

Hi Patrick

On Thu, Jan 09, 2020 at 10:43:17AM -0600, Patrick Williams wrote:
>It looks like this patch set is still the MTD-only implementation, which
>is useful for SPI-NOR chips but not useful for non-flash devices such as
>TPMs.  Is there any work or thought into how we could do a generic SPI
>controller and then layer the MTD above it?

I wonder if it is "just" a matter of implementing one of the low level 
controller methods described here:
https://www.kernel.org/doc/html/latest/spi/spi-summary.html?highlight=spi#spi-master-methods
along side the spi-mem callbacks...

>We have some system designs where we have both a NOR device and a TPM on
>the same SPI bus.  What we're currently doing is using the
>(non-upstream) aspeed-spi driver which lets us use both the TPM and
>MTD/SPI-NOR driver, but since it doesn't have the calibration routines
>the SPI-NOR runs at a slower speed than optimal.

Are you still using the aspeed-spi driver?  Have you had any issues with 
using it?

Thanks,
Brad

Re: Aspeed SPI driver upstreaming

[Patrick Williams]

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

On Mon, May 16, 2022 at 02:18:24PM -0400, Brad Bishop wrote:
> Hi Patrick
> 
> On Thu, Jan 09, 2020 at 10:43:17AM -0600, Patrick Williams wrote:
> >It looks like this patch set is still the MTD-only implementation, which
> >is useful for SPI-NOR chips but not useful for non-flash devices such as
> >TPMs.  Is there any work or thought into how we could do a generic SPI
> >controller and then layer the MTD above it?
> 
> I wonder if it is "just" a matter of implementing one of the low level 
> controller methods described here:
> https://www.kernel.org/doc/html/latest/spi/spi-summary.html?highlight=spi#spi-master-methods
> along side the spi-mem callbacks...

I don't know the details here.  There was some dispute between the MTD
maintainers and work that others were doing in this area that has made
it difficult for us to get additional patches in until someone refactors
the Aspeed driver how upstream wants.  (last I was aware)

> >We have some system designs where we have both a NOR device and a TPM on
> >the same SPI bus.  What we're currently doing is using the
> >(non-upstream) aspeed-spi driver which lets us use both the TPM and
> >MTD/SPI-NOR driver, but since it doesn't have the calibration routines
> >the SPI-NOR runs at a slower speed than optimal.
> 
> Are you still using the aspeed-spi driver?  Have you had any issues with 
> using it?

I am not using the aspeed-spi driver in these conditions.  After
initially setting this up we did some testing with the TPM driver and
realized that it wasn't working.  It turns out that the Aspeed hardware
is incapable of bi-directional transactions (bytes going out MOSI and in
MISO at the same time), which is required by the TCG TPM protocol.
We've ended up having to use the GPIO-SPI bitbang driver for talking
with SPI-based TPMs.

-- 
Patrick Williams

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

Re: Aspeed SPI driver upstreaming

[Brad Bishop]

Thanks for the reply Patrick.

On Fri, May 20, 2022 at 04:10:51PM -0500, Patrick Williams wrote:
>I don't know the details here.  There was some dispute between the MTD
>maintainers and work that others were doing in this area that has made
>it difficult for us to get additional patches in until someone refactors
>the Aspeed driver how upstream wants.  (last I was aware)

Right, this work has just recently been done: 
https://lore.kernel.org/lkml/20220503060634.122722-1-clg@kaod.org/ 
(thanks Cedric, IBM, and anyone else that helped!)

>I am not using the aspeed-spi driver in these conditions.  After
>initially setting this up we did some testing with the TPM driver and
>realized that it wasn't working.  It turns out that the Aspeed hardware
>is incapable of bi-directional transactions (bytes going out MOSI and in
>MISO at the same time), which is required by the TCG TPM protocol.
>We've ended up having to use the GPIO-SPI bitbang driver for talking
>with SPI-based TPMs.

Good to know!  Thanks for the information,

Brad

Re: Aspeed SPI driver upstreaming

[Cédric Le Goater]

Hello,

On 5/24/22 02:10, Brad Bishop wrote:
> Thanks for the reply Patrick.
> 
> On Fri, May 20, 2022 at 04:10:51PM -0500, Patrick Williams wrote:
>> I don't know the details here.  There was some dispute between the MTD
>> maintainers and work that others were doing in this area that has made
>> it difficult for us to get additional patches in until someone refactors
>> the Aspeed driver how upstream wants.  (last I was aware)
> 
> Right, this work has just recently been done: https://lore.kernel.org/lkml/20220503060634.122722-1-clg@kaod.org/ (thanks Cedric, IBM, and anyone else that helped!)


The u-boot SPI flash driver is also going upstream :

   http://patchwork.ozlabs.org/project/uboot/list/?series=301700

and having some Tested-by would help.

Thanks,

C.

> 
>> I am not using the aspeed-spi driver in these conditions.  After
>> initially setting this up we did some testing with the TPM driver and
>> realized that it wasn't working.  It turns out that the Aspeed hardware
>> is incapable of bi-directional transactions (bytes going out MOSI and in
>> MISO at the same time), which is required by the TCG TPM protocol.
>> We've ended up having to use the GPIO-SPI bitbang driver for talking
>> with SPI-based TPMs.
> 
> Good to know!  Thanks for the information,
> 
> Brad