Uboot SPI uclass - question

Uboot SPI uclass - question

[Moshe, Yaniv]

Hi Simon,
Hope you're doing good.

My name is Yaniv, and I have a question regarding an old commit in U-boot:
commit 60e2809a848bccd3a8090d3f2237964670f2780c
Author: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>
Date:   Tue Feb 17 15:29:35 2015 -0700

    dm: spi: Avoid setting the speed with every transfer

    Only set the speed if it has changed from last time. Since the speed will
    be 0 when the device is probed it will always be changed on the first
    transfer after the device is probed.

    Signed-off-by: Simon Glass sjg at chromium.org<mailto:sjg@chromium.org>


As far as I understand it, each SPI slave has its own struct. So, when we will use the device for the first time, we will set the speed.
But, if we're jumping between different slaves, the slaves structs were already configured at the first time, and since then there won't be any future speed setting.

e.g:
access flash with max-freq = X - > speed set
access TPM with max-freq =Y -> speed set
access flash again, slave struct already configured, no change between speed and slave->speed, -> speed stay the same as for TPM.

Did I got it wrong? I'm trying to understand what am I missing...

Thanks,
-Yaniv Moshe

Uboot SPI uclass - question

[Simon Glass]

Hi Yaniv,

On Mon, 2 Nov 2020 at 11:14, Moshe, Yaniv <yanivmo@amazon.com> wrote:
>
> Hi Simon,
>
> Hope you?re doing good.
>
>
>
> My name is Yaniv, and I have a question regarding an old commit in U-boot:
>
> commit 60e2809a848bccd3a8090d3f2237964670f2780c
>
> Author: Simon Glass <sjg@chromium.org>
>
> Date:   Tue Feb 17 15:29:35 2015 -0700
>
>
>
>     dm: spi: Avoid setting the speed with every transfer
>
>
>
>     Only set the speed if it has changed from last time. Since the speed will
>
>     be 0 when the device is probed it will always be changed on the first
>
>     transfer after the device is probed.
>
>
>
>     Signed-off-by: Simon Glass sjg at chromium.org
>
>
>
>
>
> As far as I understand it, each SPI slave has its own struct. So, when we will use the device for the first time, we will set the speed.
>
> But, if we?re jumping between different slaves, the slaves structs were already configured at the first time, and since then there won?t be any future speed setting.
>
>
>
> e.g:
>
> access flash with max-freq = X - > speed set
>
> access TPM with max-freq =Y -> speed set
>
> access flash again, slave struct already configured, no change between speed and slave->speed, -> speed stay the same as for TPM.
>
>
>
> Did I got it wrong? I?m trying to understand what am I missing?

Yes I think you are right. I think we need to have a single speed for
the SPI bus and check against that instead. It could be stored in
dm_spi_bus perhaps?

Regards,
Simon

Uboot SPI uclass - question

[Rasmus Villemoes]

On 02/11/2020 19.14, Moshe, Yaniv wrote:
> Hi Simon,
> Hope you're doing good.
> 
> My name is Yaniv, and I have a question regarding an old commit in U-boot:
> commit 60e2809a848bccd3a8090d3f2237964670f2780c
> Author: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>
> Date:   Tue Feb 17 15:29:35 2015 -0700
> 
>     dm: spi: Avoid setting the speed with every transfer
> 
>     Only set the speed if it has changed from last time. Since the speed will
>     be 0 when the device is probed it will always be changed on the first
>     transfer after the device is probed.
> 
>     Signed-off-by: Simon Glass sjg at chromium.org<mailto:sjg@chromium.org>
> 
> 
> As far as I understand it, each SPI slave has its own struct. So, when we will use the device for the first time, we will set the speed.
> But, if we're jumping between different slaves, the slaves structs were already configured at the first time, and since then there won't be any future speed setting.
> 
> e.g:
> access flash with max-freq = X - > speed set
> access TPM with max-freq =Y -> speed set
> access flash again, slave struct already configured, no change between speed and slave->speed, -> speed stay the same as for TPM.
> 
> Did I got it wrong? I'm trying to understand what am I missing...

No, you're reading it the same way I did:

https://lists.denx.de/pipermail/u-boot/2019-December/393854.html

I never followed up because I didn't actually have two different spi
devices in play on the board I was working on at the time. But it's
something that really should be fixed.

Rasmus

Uboot SPI uclass - question

[Moshe, Yaniv]

Hi,

So, I was thinking about the same solution of working with one frequency for the SPI, but then I saw this
comment in the "include/spi.h":

/* TODO(sjg at chromium.org): Remove this and use max_hz from struct spi_slave */
struct dm_spi_bus {
	uint max_hz;
};

So, I thought that maybe someone already has some progress about this problem...
In my case, the easy W/A was to just ignore the if that check if frequency is the same in the slave struct, and instead, I'm updating the speed for every change in slave.

I do think that working with one speed variable for the SPI is the right solution.

-Yaniv



-----Original Message-----
From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> 
Sent: Sunday, November 22, 2020 11:22 PM
To: Moshe, Yaniv <yanivmo@amazon.com>; u-boot at lists.denx.de
Cc: sjg at chromium.org
Subject: RE: [EXTERNAL] Uboot SPI uclass - question

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On 02/11/2020 19.14, Moshe, Yaniv wrote:
> Hi Simon,
> Hope you're doing good.
>
> My name is Yaniv, and I have a question regarding an old commit in U-boot:
> commit 60e2809a848bccd3a8090d3f2237964670f2780c
> Author: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>
> Date:   Tue Feb 17 15:29:35 2015 -0700
>
>     dm: spi: Avoid setting the speed with every transfer
>
>     Only set the speed if it has changed from last time. Since the speed will
>     be 0 when the device is probed it will always be changed on the first
>     transfer after the device is probed.
>
>     Signed-off-by: Simon Glass 
> sjg at chromium.org<mailto:sjg@chromium.org>
>
>
> As far as I understand it, each SPI slave has its own struct. So, when we will use the device for the first time, we will set the speed.
> But, if we're jumping between different slaves, the slaves structs were already configured at the first time, and since then there won't be any future speed setting.
>
> e.g:
> access flash with max-freq = X - > speed set access TPM with max-freq 
> =Y -> speed set access flash again, slave struct already configured, 
> no change between speed and slave->speed, -> speed stay the same as for TPM.
>
> Did I got it wrong? I'm trying to understand what am I missing...

No, you're reading it the same way I did:

https://lists.denx.de/pipermail/u-boot/2019-December/393854.html

I never followed up because I didn't actually have two different spi devices in play on the board I was working on at the time. But it's something that really should be fixed.

Rasmus

Uboot SPI uclass - question

[Simon Glass]

Hi,

On Mon, 23 Nov 2020 at 00:22, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 02/11/2020 19.14, Moshe, Yaniv wrote:
> > Hi Simon,
> > Hope you're doing good.
> >
> > My name is Yaniv, and I have a question regarding an old commit in U-boot:
> > commit 60e2809a848bccd3a8090d3f2237964670f2780c
> > Author: Simon Glass <sjg at chromium.org<mailto:sjg@chromium.org>>
> > Date:   Tue Feb 17 15:29:35 2015 -0700
> >
> >     dm: spi: Avoid setting the speed with every transfer
> >
> >     Only set the speed if it has changed from last time. Since the speed will
> >     be 0 when the device is probed it will always be changed on the first
> >     transfer after the device is probed.
> >
> >     Signed-off-by: Simon Glass sjg at chromium.org<mailto:sjg@chromium.org>
> >
> >
> > As far as I understand it, each SPI slave has its own struct. So, when we will use the device for the first time, we will set the speed.
> > But, if we're jumping between different slaves, the slaves structs were already configured at the first time, and since then there won't be any future speed setting.
> >
> > e.g:
> > access flash with max-freq = X - > speed set
> > access TPM with max-freq =Y -> speed set
> > access flash again, slave struct already configured, no change between speed and slave->speed, -> speed stay the same as for TPM.
> >
> > Did I got it wrong? I'm trying to understand what am I missing...
>
> No, you're reading it the same way I did:
>
> https://lists.denx.de/pipermail/u-boot/2019-December/393854.html
>
> I never followed up because I didn't actually have two different spi
> devices in play on the board I was working on at the time. But it's
> something that really should be fixed.

It is easy enough to add a sandbox test for this I think.

Regards,
Simon