From: Sven Van Asbroeck <[email protected]>
This driver makes sure the underlying SPI bus is set to "mode 0"
by assigning SPI_MODE_0 to spi->mode. This overwrites all other
SPI mode flags.
In some circumstances, this can break the underlying SPI bus driver.
For example, if SPI_CS_HIGH is set on the SPI bus, the driver
will clear that flag, which results in a chip-select polarity issue.
Fix by changing only the SPI_MODE_N bits, i.e. SPI_CPHA and SPI_CPOL.
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
Tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git # bff6f1db91e3
To: Andrew Lunn <[email protected]>
To: Heiner Kallweit <[email protected]>
To: Jakub Kicinski <[email protected]>
Cc: Russell King <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
drivers/net/phy/spi_ks8995.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c
index 4b198399bfa2..3c6c87a09b03 100644
--- a/drivers/net/phy/spi_ks8995.c
+++ b/drivers/net/phy/spi_ks8995.c
@@ -491,7 +491,9 @@ static int ks8995_probe(struct spi_device *spi)
spi_set_drvdata(spi, ks);
- spi->mode = SPI_MODE_0;
+ /* use SPI_MODE_0 without changing any other mode flags */
+ spi->mode &= ~(SPI_CPHA | SPI_CPOL);
+ spi->mode |= SPI_MODE_0;
spi->bits_per_word = 8;
err = spi_setup(spi);
if (err) {
--
2.17.1
> +++ b/drivers/net/phy/spi_ks8995.c
> @@ -491,7 +491,9 @@ static int ks8995_probe(struct spi_device *spi)
>
> spi_set_drvdata(spi, ks);
>
> - spi->mode = SPI_MODE_0;
> + /* use SPI_MODE_0 without changing any other mode flags */
> + spi->mode &= ~(SPI_CPHA | SPI_CPOL);
> + spi->mode |= SPI_MODE_0;
> spi->bits_per_word = 8;
Did you check to see if there is a help to set just the mode without
changing any of the other bits?
Andrew
On Mon, Nov 9, 2020 at 2:49 PM Andrew Lunn <[email protected]> wrote:
>
> Did you check to see if there is a help to set just the mode without
> changing any of the other bits?
Absolutely, but it doesn't exist, AFAIK.
It would be great if client spi drivers would use a helper function like
that. The spi bus driver on my platform (imx ecspi) was recently changed
upstream, which means SPI_CS_HIGH is now always set, irrespective
of the actual chip select polarity. This change breaks every single spi
driver which "plows over" the spi->mode flags. And there are quite
a few...
Sven
On Mon, Nov 09, 2020 at 02:56:38PM -0500, Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 2:49 PM Andrew Lunn <[email protected]> wrote:
> >
> > Did you check to see if there is a help to set just the mode without
> > changing any of the other bits?
>
> Absolutely, but it doesn't exist, AFAIK.
> It would be great if client spi drivers would use a helper function like
> that.
Then you should consider adding it, and cross post the SPI list.
Andrew
On Mon, Nov 9, 2020 at 3:26 PM Andrew Lunn <[email protected]> wrote:
>
> Then you should consider adding it, and cross post the SPI list.
>
Good idea. I will give that a try.
On Mon, 9 Nov 2020 14:31:17 -0500 Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <[email protected]>
>
> This driver makes sure the underlying SPI bus is set to "mode 0"
> by assigning SPI_MODE_0 to spi->mode. This overwrites all other
> SPI mode flags.
>
> In some circumstances, this can break the underlying SPI bus driver.
> For example, if SPI_CS_HIGH is set on the SPI bus, the driver
> will clear that flag, which results in a chip-select polarity issue.
>
> Fix by changing only the SPI_MODE_N bits, i.e. SPI_CPHA and SPI_CPOL.
>
> Signed-off-by: Sven Van Asbroeck <[email protected]>
This is a fix right? You seem to be targeting net-next and there is no
Fixes tag but it sounds like a bug.
On Mon, Nov 9, 2020 at 4:09 PM Jakub Kicinski <[email protected]> wrote:
>
> This is a fix right? You seem to be targeting net-next and there is no
> Fixes tag but it sounds like a bug.
I'm not sure. The original code used to work for me, until the spi bus
driver I'm using to communicate to this chip was changed to always
require SPI_CS_HIGH. The current ks8995 driver will now plow over
this flag, and spi communication breaks.
Is this a bug? If so, what should its Fixes commit be? The spi commit
upstream that enables SPI_CS_HIGH on my platform?
On Mon, 9 Nov 2020 16:20:46 -0500 Sven Van Asbroeck wrote:
> Is this a bug? If so, what should its Fixes commit be? The spi commit
> upstream that enables SPI_CS_HIGH on my platform?
I'd put two fixes tags one for the spi commit and one for the commit
which introduced the assignment in the client driver.
On Mon, Nov 9, 2020 at 4:24 PM Jakub Kicinski <[email protected]> wrote:
>
> the commit
> which introduced the assignment in the client driver.
That's the commit which adds the initial driver to the tree, back in 2011.
Should I use that for Fixes?
On Mon, 9 Nov 2020 16:29:19 -0500 Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 4:24 PM Jakub Kicinski <[email protected]> wrote:
> >
> > the commit
> > which introduced the assignment in the client driver.
>
> That's the commit which adds the initial driver to the tree, back in 2011.
> Should I use that for Fixes?
Yup
On Mon, Nov 9, 2020 at 5:04 PM Jakub Kicinski <[email protected]> wrote:
>
> Yup
Just a minute. Earlier in the thread, Andrew Lunn is suggesting I
create a new spi helper function, and cross-post to the spi group(s).
That doesn't sound like a minimal fix, should it go to net or net-next?
Thanks,
Sven
On Mon, 9 Nov 2020 17:19:48 -0500 Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 5:04 PM Jakub Kicinski <[email protected]> wrote:
> >
> > Yup
>
> Just a minute. Earlier in the thread, Andrew Lunn is suggesting I
> create a new spi helper function, and cross-post to the spi group(s).
>
> That doesn't sound like a minimal fix, should it go to net or net-next?
Is it only broken for you in linux-next or just in the current 5.10
release?
On Mon, Nov 9, 2020 at 5:23 PM Jakub Kicinski <[email protected]> wrote:
>
> Is it only broken for you in linux-next or just in the current 5.10
> release?
It's broken for me in the current 5.10 release. That means it should
go to net, not net-next, correct?
On Mon, 9 Nov 2020 17:27:28 -0500 Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 5:23 PM Jakub Kicinski <[email protected]> wrote:
> >
> > Is it only broken for you in linux-next or just in the current 5.10
> > release?
>
> It's broken for me in the current 5.10 release. That means it should
> go to net, not net-next, correct?
Yes, most certainly. Especially with 5.10 being LTS.
You can send a minimal fix (perhaps what you got already?), and follow
up with the helper as suggested by Andrew after ~a week when net is
merged into net-next.
But please at least repost for net and CC Mark and the SPI list for
input.
On Mon, Nov 9, 2020 at 5:36 PM Jakub Kicinski <[email protected]> wrote:
>
> Yes, most certainly. Especially with 5.10 being LTS.
>
> You can send a minimal fix (perhaps what you got already?), and follow
> up with the helper as suggested by Andrew after ~a week when net is
> merged into net-next.
>
What I already posted (as v1) should be the minimal fix.
Can we proceed on that basis? I'll follow up with the helper
after the net -> net-next merge, as you suggested.
On Mon, 9 Nov 2020 17:39:22 -0500 Sven Van Asbroeck wrote:
> What I already posted (as v1) should be the minimal fix.
> Can we proceed on that basis? I'll follow up with the helper
> after the net -> net-next merge, as you suggested.
Well, you cut off the relevant part of my email where I said:
But please at least repost for net and CC Mark and the SPI list
for input.
Maybe Mark has a different idea on how client drivers should behave.
Also please obviously CC the author of the patch who introduced
the breakage.
On Mon, Nov 9, 2020 at 5:50 PM Jakub Kicinski <[email protected]> wrote:
>
> But please at least repost for net and CC Mark and the SPI list
> for input.
>
> Maybe Mark has a different idea on how client drivers should behave.
>
> Also please obviously CC the author of the patch who introduced
> the breakage.
I believe they're aware: I tried to propose a patch to fix this in the
spi core, but it looks like it was rejected - with feedback: "fix the
client drivers instead"
https://patchwork.kernel.org/project/spi-devel-general/patch/[email protected]/#23747737
I will respin this minimal fix as v2, add Fixes tags and Cc the
people involved, as you suggested.