[PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Jan Kundrát]

Commit b28b9149b37f added support for using an additional GPIO pin for
Chip Select. According to that commit and to my understanding of a
semi-public datahseet, it seems that the SPI hardware really insists on
driving *some* HW CS signal whenever a SPI transaction is in progress.

The old code effectively forced the HW CS pin to be CS0. That means that
there could not be anything connected to the real CS0 signal when one
uses a GPIO CS. That assumption does not hold on e.g. Solidrun Clearfog
where some SOM models have a built-in SPI flash on SPI1, CS0.

Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>
---
 .../devicetree/bindings/spi/spi-orion.txt          | 22 ++++++++++++++++++++++
 drivers/spi/spi-orion.c                            | 10 +++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt
index 8434a65fc12a..c7027af04fb2 100644
--- a/Documentation/devicetree/bindings/spi/spi-orion.txt
+++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
@@ -28,6 +28,11 @@ Optional properties:
 - clock-names : names of used clocks, mandatory if the second clock is
 		used, the name must be "core", and "axi" (the latter
 		is only for Armada 7K/8K).
+- orion-spi,cs-gpio-hw-cs : Because the SoC always wants to drive some HW Chip
+			    Select pin, it is necessary to specify which one
+			    should be used when Linux drives an additional
+			    GPIO as a software-controlled CS. Defaults to HW
+			    CS 0.
 
 
 Example:
@@ -74,6 +79,23 @@ are used in the default indirect (PIO) mode):
 			 <MBUS_ID(0x01, 0x5e) 0 0 0xf1100000 0x10000>,	/* SPI0-DEV1 */
 			 <MBUS_ID(0x01, 0x9a) 0 0 0xf1110000 0x10000>;	/* SPI1-DEV2 */
 
+
+Example with a GPIO CS to be driven by software in addition to HW CS3. This
+can be used when the board or the pinmux does not allow connections to HW CS
+pins, for example:
+
+	spi0: spi@10600 {
+		/* ... */
+		cs-gpios = <0>, <&gpio0 22 GPIO_ACTIVE_HIGH>, <0>;
+		orion-spi,cs-gpio-hw-cs = <3>;
+
+		something@1 {
+			reg = <1>;
+			/* this device should be now connected to a custom GPIO CS */
+		};
+	}
+
+
 For further information on the MBus bindings, please see the MBus
 DT documentation:
 Documentation/devicetree/bindings/bus/mvebu-mbus.txt
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index 482a0cf3b7aa..b1b0537b4450 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -96,6 +96,7 @@ struct orion_spi {
 	struct clk              *clk;
 	struct clk              *axi_clk;
 	const struct orion_spi_dev *devdata;
+	int			cs_hw_gpio;
 
 	struct orion_direct_acc	direct_access[ORION_NUM_CHIPSELECTS];
 };
@@ -324,13 +325,13 @@ static void orion_spi_set_cs(struct spi_device *spi, bool enable)
 	struct orion_spi *orion_spi;
 	int cs;
 
+	orion_spi = spi_master_get_devdata(spi->master);
+
 	if (gpio_is_valid(spi->cs_gpio))
-		cs = 0;
+		cs = orion_spi->cs_hw_gpio;
 	else
 		cs = spi->chip_select;
 
-	orion_spi = spi_master_get_devdata(spi->master);
-
 	orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG, ORION_SPI_CS_MASK);
 	orion_spi_setbits(orion_spi, ORION_SPI_IF_CTRL_REG,
 				ORION_SPI_CS(cs));
@@ -645,6 +646,9 @@ static int orion_spi_probe(struct platform_device *pdev)
 
 	tclk_hz = clk_get_rate(spi->clk);
 
+	if (!pdev->dev.of_node || of_property_read_u32(pdev->dev.of_node, "orion-spi,cs-gpio-hw-cs", &spi->cs_hw_gpio))
+		spi->cs_hw_gpio = 0;
+
 	/*
 	 * With old device tree, armada-370-spi could be used with
 	 * Armada XP, however for this SoC the maximum frequency is
-- 
2.14.3


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Geert Uytterhoeven]

Hi Jan,

On Tue, Jan 23, 2018 at 8:56 PM, Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org> wrote:
> Commit b28b9149b37f added support for using an additional GPIO pin for
> Chip Select. According to that commit and to my understanding of a
> semi-public datahseet, it seems that the SPI hardware really insists on
> driving *some* HW CS signal whenever a SPI transaction is in progress.
>
> The old code effectively forced the HW CS pin to be CS0. That means that
> there could not be anything connected to the real CS0 signal when one
> uses a GPIO CS. That assumption does not hold on e.g. Solidrun Clearfog
> where some SOM models have a built-in SPI flash on SPI1, CS0.
>
> Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>

Thanks for your patch!

> --- a/Documentation/devicetree/bindings/spi/spi-orion.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
> @@ -28,6 +28,11 @@ Optional properties:
>  - clock-names : names of used clocks, mandatory if the second clock is
>                 used, the name must be "core", and "axi" (the latter
>                 is only for Armada 7K/8K).
> +- orion-spi,cs-gpio-hw-cs : Because the SoC always wants to drive some HW Chip
> +                           Select pin, it is necessary to specify which one
> +                           should be used when Linux drives an additional
> +                           GPIO as a software-controlled CS. Defaults to HW
> +                           CS 0.

Can't you just deduce an unused HW CS from the cs-gpios property?

Renesas MSIOF also requires driving a native chip select, cfr. commit
b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration").

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Andy Shevchenko]

On Tue, Jan 23, 2018 at 9:56 PM, Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org> wrote:
>
> Commit b28b9149b37f added support for using an additional GPIO pin for
> Chip Select. According to that commit and to my understanding of a
> semi-public datahseet, it seems that the SPI hardware really insists on
> driving *some* HW CS signal whenever a SPI transaction is in progress.
>
> The old code effectively forced the HW CS pin to be CS0. That means that
> there could not be anything connected to the real CS0 signal when one
> uses a GPIO CS. That assumption does not hold on e.g. Solidrun Clearfog
> where some SOM models have a built-in SPI flash on SPI1, CS0.
Like Geert said I also highly recommend to use existing properties.

Better if you, in the meantime, can switch the driver to use GPIO
descriptors, while core still use legacy numbers.
It would make life easier in the future to switch SPI core to GPIO
descriptors completely.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Jan Kundrát]

On středa 24. ledna 2018 14:41:27 CET, Geert Uytterhoeven wrote:
> Can't you just deduce an unused HW CS from the cs-gpios property?

Thanks for a review. Yes, I probably can. I just noticed that my local use 
of spi-orion's "direct mode" is probably the root cause of some weirdness 
that I'm seeing (such as the CS GPIO pin going up in the middle of a 
two-byte transaction, and other nasty issues that just go away once one 
adds a dev_info call...).

> Renesas MSIOF also requires driving a native chip select, cfr. commit
> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration").

I'll take a look. It could be a candidate for some shared code, I suppose.

Cheers,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Mark Brown]

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

On Wed, Jan 24, 2018 at 04:19:41PM +0200, Andy Shevchenko wrote:

> It would make life easier in the future to switch SPI core to GPIO
> descriptors completely.

Linus has patches for that which I'm expecting very soon after the merge
window.

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

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Jan Kundrát]

On středa 24. ledna 2018 16:03:15 CET, Jan Kundrát wrote:
>> Renesas MSIOF also requires driving a native chip select, cfr. commit
>> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration").
>
> I'll take a look. It could be a candidate for some shared code, I suppose.

Hi Gert, if I take your code from that commit almost as-is, probing of my 
SPI controller fails with errno -16 (that's EBUSY, right?). The GPIO pin 
that I want to use is defined as a gpio-hog in the DT.

Is my DT correct? Should I define the GPIO pin as an output hog? IS your 
code supposed to deal with this?

With kind regards,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Geert Uytterhoeven]

Hi Jan,

On Wed, Jan 24, 2018 at 6:56 PM, Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org> wrote:
> On středa 24. ledna 2018 16:03:15 CET, Jan Kundrát wrote:
>>> Renesas MSIOF also requires driving a native chip select, cfr. commit
>>> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration").
>>
>> I'll take a look. It could be a candidate for some shared code, I suppose.
>
> Hi Gert, if I take your code from that commit almost as-is, probing of my
> SPI controller fails with errno -16 (that's EBUSY, right?). The GPIO pin
> that I want to use is defined as a gpio-hog in the DT.
>
> Is my DT correct? Should I define the GPIO pin as an output hog? IS your
> code supposed to deal with this?

Using a hog indeed marks it busy, as the hog is a user.

Why do you use a hog in the first place? Shouldn't the SPI driver configure
the GPIO, according to cs-gpios?

Before the advent of DT, the platform code was supposed to take care of that,
so this may be a too-literal conversion from platform code to DT.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Chris Packham]

Hi Jan, Geert,

On 25/01/18 04:03, Jan Kundrát wrote:
> On středa 24. ledna 2018 14:41:27 CET, Geert Uytterhoeven wrote:
>> Can't you just deduce an unused HW CS from the cs-gpios property?
> 
> Thanks for a review. Yes, I probably can. I just noticed that my local use
> of spi-orion's "direct mode" is probably the root cause of some weirdness
> that I'm seeing (such as the CS GPIO pin going up in the middle of a
> two-byte transaction, and other nasty issues that just go away once one
> adds a dev_info call...).
> 
>> Renesas MSIOF also requires driving a native chip select, cfr. commit
>> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration").
> 
> I'll take a look. It could be a candidate for some shared code, I suppose.

How would this work? If I understand the sh-msiof driver it picks an 
unused native CS. In the case of b28b9149b37f the gpios supplement the 
native CS they do not replace it (the hardware I was using has a GPIO 
which controls a gate attached to CS0).

My specific board isn't upstream but here's the snippet from its dts

&spi0 {
         status = "okay";
         cs-gpios = <0
                     &gpio0 17 GPIO_ACTIVE_LOW>;

         spi-flash@0 {
		/* This is on CS0 when GPIO 17 is high */
         };

         spi-sram@1 {
                 /* This is on CS0 when GPIO 17 is low */
	};
};

I'm not sure what else I could do. I can't claim the GPIO twice. If I 
could I could probably use spi-cs-high to handle the high/low toggle.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Geert Uytterhoeven]

Hi Chris,

On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham
<Chris.Packham-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org> wrote:
> On 25/01/18 04:03, Jan Kundrát wrote:
>> On středa 24. ledna 2018 14:41:27 CET, Geert Uytterhoeven wrote:
>>> Can't you just deduce an unused HW CS from the cs-gpios property?
>>
>> Thanks for a review. Yes, I probably can. I just noticed that my local use
>> of spi-orion's "direct mode" is probably the root cause of some weirdness
>> that I'm seeing (such as the CS GPIO pin going up in the middle of a
>> two-byte transaction, and other nasty issues that just go away once one
>> adds a dev_info call...).
>>
>>> Renesas MSIOF also requires driving a native chip select, cfr. commit
>>> b8761434bdec32fa ("spi: sh-msiof: Implement cs-gpios configuration").
>>
>> I'll take a look. It could be a candidate for some shared code, I suppose.
>
> How would this work? If I understand the sh-msiof driver it picks an
> unused native CS.

Correct. The hardware always has to drive one of the 3 available native chip
selects.

> In the case of b28b9149b37f the gpios supplement the
> native CS they do not replace it (the hardware I was using has a GPIO
> which controls a gate attached to CS0).

However, Jan wrote:
> semi-public datahseet, it seems that the SPI hardware really insists on
> driving *some* HW CS signal whenever a SPI transaction is in progress.

If that is correct, it behaves like MSIOF, so you must leave one native chip
select unused. If all native chip selects are in use, one of them must be
replaced by a GPIO chip select (which could be the same physical pin,
depending on pinmux capabilities).

> My specific board isn't upstream but here's the snippet from its dts
>
> &spi0 {
>          status = "okay";
>          cs-gpios = <0
>                      &gpio0 17 GPIO_ACTIVE_LOW>;
>
>          spi-flash@0 {
>                 /* This is on CS0 when GPIO 17 is high */
>          };
>
>          spi-sram@1 {
>                  /* This is on CS0 when GPIO 17 is low */
>         };
> };
>
> I'm not sure what else I could do. I can't claim the GPIO twice. If I
> could I could probably use spi-cs-high to handle the high/low toggle.

So this uses GPIO17 to drive a mux to connect CS0 to either the first or
second device?

But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you
rely on CS0 still being driven when @1 is selected, right?
That indeed won't work when an unused native chip select is driven when
using cs-gpio.

IMHO, this needs the mux to be described properly in DT.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Mark Brown]

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

")
Fcc: +sent-mail

On Thu, Jan 25, 2018 at 09:51:06AM +0100, Geert Uytterhoeven wrote:
> On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham

> However, Jan wrote:
> > semi-public datahseet, it seems that the SPI hardware really insists on
> > driving *some* HW CS signal whenever a SPI transaction is in progress.

> If that is correct, it behaves like MSIOF, so you must leave one native chip
> select unused. If all native chip selects are in use, one of them must be
> replaced by a GPIO chip select (which could be the same physical pin,
> depending on pinmux capabilities).

Using the same pin is the ideal thing obviously, or if not then
something that we know isn't routed out of the SoC (which may or may not
be possible with a given chip design).

> >          spi-flash@0 {
> >                 /* This is on CS0 when GPIO 17 is high */
> >          };
> >
> >          spi-sram@1 {
> >                  /* This is on CS0 when GPIO 17 is low */
> >         };
> > };

> > I'm not sure what else I could do. I can't claim the GPIO twice. If I
> > could I could probably use spi-cs-high to handle the high/low toggle.

> So this uses GPIO17 to drive a mux to connect CS0 to either the first or
> second device?

Interesting, coincidentally there was someone else sending a patch for
muxing the other day which looked like having some support for chip
select muxing would be the most sensible implementation.  I've copied in
Ben who had initially approached this with a mux for the full bus which
I'm not so convincd about.

> But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you
> rely on CS0 still being driven when @1 is selected, right?
> That indeed won't work when an unused native chip select is driven when
> using cs-gpio.

> IMHO, this needs the mux to be described properly in DT.

Yes.

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

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Chris Packham]

Hi Mark, Geert,

On 26/01/18 00:57, Mark Brown wrote:
> ")
> Fcc: +sent-mail
> 
> On Thu, Jan 25, 2018 at 09:51:06AM +0100, Geert Uytterhoeven wrote:
>> On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham
> 
>> However, Jan wrote:
>>> semi-public datahseet, it seems that the SPI hardware really insists on
>>> driving *some* HW CS signal whenever a SPI transaction is in progress.
> 
>> If that is correct, it behaves like MSIOF, so you must leave one native chip
>> select unused. If all native chip selects are in use, one of them must be
>> replaced by a GPIO chip select (which could be the same physical pin,
>> depending on pinmux capabilities).
> 
> Using the same pin is the ideal thing obviously, or if not then
> something that we know isn't routed out of the SoC (which may or may not
> be possible with a given chip design).
> 
>>>           spi-flash@0 {
>>>                  /* This is on CS0 when GPIO 17 is high */
>>>           };
>>>
>>>           spi-sram@1 {
>>>                   /* This is on CS0 when GPIO 17 is low */
>>>          };
>>> };
> 
>>> I'm not sure what else I could do. I can't claim the GPIO twice. If I
>>> could I could probably use spi-cs-high to handle the high/low toggle.
> 
>> So this uses GPIO17 to drive a mux to connect CS0 to either the first or
>> second device?

Correct. GPIO17 directs CS0 with the addition of a logic gate.

> Interesting, coincidentally there was someone else sending a patch for
> muxing the other day which looked like having some support for chip
> select muxing would be the most sensible implementation.  I've copied in
> Ben who had initially approached this with a mux for the full bus which
> I'm not so convincd about.
> 
>> But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you
>> rely on CS0 still being driven when @1 is selected, right?
>> That indeed won't work when an unused native chip select is driven when
>> using cs-gpio.

Yes that's my problem.

> 
>> IMHO, this needs the mux to be described properly in DT.
> 
> Yes.

I'm more than happy to update the DT on this product. But I've no idea 
what that update would look like.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Geert Uytterhoeven]

Hi Chris,

On Thu, Jan 25, 2018 at 10:18 PM, Chris Packham
<Chris.Packham-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org> wrote:
> On 26/01/18 00:57, Mark Brown wrote:
>> ")
>> Fcc: +sent-mail
>>
>> On Thu, Jan 25, 2018 at 09:51:06AM +0100, Geert Uytterhoeven wrote:
>>> On Wed, Jan 24, 2018 at 10:29 PM, Chris Packham
>>
>>> However, Jan wrote:
>>>> semi-public datahseet, it seems that the SPI hardware really insists on
>>>> driving *some* HW CS signal whenever a SPI transaction is in progress.
>>
>>> If that is correct, it behaves like MSIOF, so you must leave one native chip
>>> select unused. If all native chip selects are in use, one of them must be
>>> replaced by a GPIO chip select (which could be the same physical pin,
>>> depending on pinmux capabilities).
>>
>> Using the same pin is the ideal thing obviously, or if not then
>> something that we know isn't routed out of the SoC (which may or may not
>> be possible with a given chip design).
>>
>>>>           spi-flash@0 {
>>>>                  /* This is on CS0 when GPIO 17 is high */
>>>>           };
>>>>
>>>>           spi-sram@1 {
>>>>                   /* This is on CS0 when GPIO 17 is low */
>>>>          };
>>>> };
>>
>>>> I'm not sure what else I could do. I can't claim the GPIO twice. If I
>>>> could I could probably use spi-cs-high to handle the high/low toggle.
>>
>>> So this uses GPIO17 to drive a mux to connect CS0 to either the first or
>>> second device?
>
> Correct. GPIO17 directs CS0 with the addition of a logic gate.
>
>> Interesting, coincidentally there was someone else sending a patch for
>> muxing the other day which looked like having some support for chip
>> select muxing would be the most sensible implementation.  I've copied in
>> Ben who had initially approached this with a mux for the full bus which
>> I'm not so convincd about.
>>
>>> But you describe this as @0 being driven by CS0, and @1 by GPIO17, and you
>>> rely on CS0 still being driven when @1 is selected, right?
>>> That indeed won't work when an unused native chip select is driven when
>>> using cs-gpio.
>
> Yes that's my problem.
>
>>
>>> IMHO, this needs the mux to be described properly in DT.
>>
>> Yes.
>
> I'm more than happy to update the DT on this product. But I've no idea
> what that update would look like.

(Un)fortunately a mux driver is WIP, as Mark mentioned above.
See https://lkml.org/lkml/2018/1/22/859

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Mark Brown]

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

On Thu, Jan 25, 2018 at 10:27:24PM +0100, Geert Uytterhoeven wrote:
> On Thu, Jan 25, 2018 at 10:18 PM, Chris Packham

> > I'm more than happy to update the DT on this product. But I've no idea
> > what that update would look like.

> (Un)fortunately a mux driver is WIP, as Mark mentioned above.
> See https://lkml.org/lkml/2018/1/22/859

From the discussion there I think the case that Ben had was for a
controller on the SPI bus rather than a mux on the CS so you're probably
going to have to do this yourself.  I've been thinking about what we
might do in DT terms but I've not been getting too many ideas, we may
need to talk it over with the DT people.

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

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Jan Kundrát]

On středa 24. ledna 2018 19:17:35 CET, Geert Uytterhoeven wrote:
> Using a hog indeed marks it busy, as the hog is a user.
>
> Why do you use a hog in the first place? Shouldn't the SPI driver configure
> the GPIO, according to cs-gpios?

That doesn't happen with spi-orion.c. If I remove my gpio-hog for the CS 
GPIO pin from the DT, then my device doesn't appear to be selected. Also, I 
can run gpio-hammer targetting my CS pin just fine -- and that seems 
dangerous.

I don't see any "claim" of a GPIO neither in spi.c nor spi-orion.c. I did 
some very hacky grepping [1], and it seems that other drivers probably 
handle this on their own. The only other match is spi-lantiq-ssc.c which 
doesn't however touch the CS GPIO.

Shouldn't this be really handled by the SPI core? (I don't think I'm 
volunteering with patches as I couldn't really test these.)

Should I submit a patch which adapts spi-orion.c to essentially follow how 
e.g spi-imx.c allocates a GPIO pin for CS?

Cheers,
Jan

[1] `git grep --files-with-match cs_gpio drivers/spi/*.c | xargs -n1 git 
grep -E --files-without-match 'gpio_direction_output|gpio_request' | cat`
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] spi: orion: Rework GPIO CS handling

[Jan Kundrát]

- Claim the GPIO from the driver, not via DT bindings or through the
  platform code
- Find an unused HW CS signal because Orion needs to drive one for each
  SPI transaction

The spi-orion.c was the only driver which supported (or cared about) the
CS GPIO, while it wasn't actually requesting it. This change means that
the DT bindings should stop hogging the GPIO CS pins because it's now
being handled by the driver.

Signed-off-by: Jan Kundrát <jan.kundrat-xoNZbkdr4U2lVyrhU4qvOw@public.gmane.org>
---
Changes v1..v2:
  A complete rework of "spi: orion: Allow specifying which HW CS to use with a GPIO CS"
  (https://patchwork.kernel.org/patch/10182583/)
---
 drivers/spi/spi-orion.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index deca63e82ff6..b341235d2947 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -96,6 +96,7 @@ struct orion_spi {
 	struct clk              *clk;
 	struct clk              *axi_clk;
 	const struct orion_spi_dev *devdata;
+	int			unused_hw_gpio;
 
 	struct orion_direct_acc	direct_access[ORION_NUM_CHIPSELECTS];
 };
@@ -324,13 +325,13 @@ static void orion_spi_set_cs(struct spi_device *spi, bool enable)
 	struct orion_spi *orion_spi;
 	int cs;
 
+	orion_spi = spi_master_get_devdata(spi->master);
+
 	if (gpio_is_valid(spi->cs_gpio))
-		cs = 0;
+		cs = orion_spi->unused_hw_gpio;
 	else
 		cs = spi->chip_select;
 
-	orion_spi = spi_master_get_devdata(spi->master);
-
 	orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG, ORION_SPI_CS_MASK);
 	orion_spi_setbits(orion_spi, ORION_SPI_IF_CTRL_REG,
 				ORION_SPI_CS(cs));
@@ -498,6 +499,9 @@ static int orion_spi_transfer_one(struct spi_master *master,
 
 static int orion_spi_setup(struct spi_device *spi)
 {
+	if (gpio_is_valid(spi->cs_gpio)) {
+		gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
+	}
 	return orion_spi_setup_transfer(spi, NULL);
 }
 
@@ -620,6 +624,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 
 	spi = spi_master_get_devdata(master);
 	spi->master = master;
+	spi->unused_hw_gpio = -1;
 
 	of_id = of_match_device(orion_spi_of_match_table, &pdev->dev);
 	devdata = (of_id) ? of_id->data : &orion_spi_dev_data;
@@ -731,8 +736,44 @@ static int orion_spi_probe(struct platform_device *pdev)
 	if (status < 0)
 		goto out_rel_pm;
 
+	if (master->cs_gpios) {
+		int i;
+		for (i = 0; i < master->num_chipselect; ++i) {
+			char *gpio_name;
+
+			if (!gpio_is_valid(master->cs_gpios[i])) {
+				continue;
+			}
+
+			gpio_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+					"%s-CS%d", dev_name(&pdev->dev), i);
+			if (!gpio_name) {
+				status = -ENOMEM;
+				goto out_rel_master;
+			}
+
+			status = devm_gpio_request(&pdev->dev,
+					master->cs_gpios[i], gpio_name);
+			if (status) {
+				dev_err(&pdev->dev,
+					"Can't request GPIO for CS %d\n",
+					master->cs_gpios[i]);
+				goto out_rel_master;
+			}
+			if (spi->unused_hw_gpio == -1) {
+				dev_info(&pdev->dev,
+					"Selected unused HW CS#%d "
+					"for any GPIO CSes\n", i);
+				spi->unused_hw_gpio = i;
+			}
+		}
+	}
+
+
 	return status;
 
+out_rel_master:
+	spi_unregister_master(master);
 out_rel_pm:
 	pm_runtime_disable(&pdev->dev);
 out_rel_axi_clk:
-- 
2.14.3


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Trent Piepho]

On Tue, 2018-01-23 at 20:56 +0100, Jan Kundrát wrote:
> Commit b28b9149b37f added support for using an additional GPIO pin for
> Chip Select. According to that commit and to my understanding of a
> semi-public datahseet, it seems that the SPI hardware really insists on
> driving *some* HW CS signal whenever a SPI transaction is in progress.

It's pretty common for SPI hardware to have this limitation.

Another way to fix it, besides default to 0 or to select an unused
chipselect, is to use the cs number of the slave modulus the number of
native chip selects.  They are or were some other drivers that do this.
The idea is that CS numbers don't need to be sequential and there isn't
much of a cost of an unused chip select (one word in device tree
property).

So say you want two GPIO CS devices and they should make use of native
CS 1, because that pin has no conflicts for this usage, and you also
want a device on native chip select 0 that's not using a GPIO CS.  The
spi master has four native chip selects.  Set up the DT like this:

cs-gpios = <0>, <&gpio 42>, <0>, <0>, <0>, <&gpio 43>;
flash@0 { reg = <0>; } /* on native CS 0 */
slave@1 { reg = <1>; } /* on GPIO 42, also native CS 1 */
slave@5 { reg = <5>; } /* on GPIO 43, also native CS 1 */

This allows assigning which native CS to use.  It could pix muxing
limitations don't allow just any CS to be chosen.  If there's no reason
to care, then selecting automatically is easier to use.

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Jan Kundrát]

On úterý 30. ledna 2018 2:13:37 CET, Trent Piepho wrote:
> Another way to fix it, besides default to 0 or to select an unused
> chipselect, is to use the cs number of the slave modulus the number of
> native chip selects.  They are or were some other drivers that do this.
> The idea is that CS numbers don't need to be sequential and there isn't
> much of a cost of an unused chip select (one word in device tree
> property).

Hi Trent, are you talking about an integer modulo here? I don't think that 
modulo is a correct approach. If my HW has two CS and I use the following 
DT:

	cs-gpios = <0>, <&gpio0 1>, <&gpio0 2>, <0>;

...then the logical CS2 (at gpio0 pin 2) conflicts with the native CS0.

Also, the spi-orion driver currently hard-codes that each and every model 
is supposed to have exactly eight HW CS pins. That's not true for my SoC 
(Marvell Armada 388, the Solidrun Clearfog Base board); the 88F6828 CPU 
only has four SPI CS signals.

What my patch does is simply picking the *first* unused CS and going with 
that one because that looks like the easiest and also safest option.

Cheers,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Mark Brown]

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

On Tue, Jan 30, 2018 at 09:03:10AM +0100, Jan Kundrát wrote:
> On úterý 30. ledna 2018 2:13:37 CET, Trent Piepho wrote:

> > Another way to fix it, besides default to 0 or to select an unused
> > chipselect, is to use the cs number of the slave modulus the number of
> > native chip selects.  They are or were some other drivers that do this.

> Hi Trent, are you talking about an integer modulo here? I don't think that
> modulo is a correct approach. If my HW has two CS and I use the following
> DT:

> 	cs-gpios = <0>, <&gpio0 1>, <&gpio0 2>, <0>;

> ...then the logical CS2 (at gpio0 pin 2) conflicts with the native CS0.

You can't physically design working hardware for a system with this
limitation that doesn't have a free chip select somehow - if the
hardware requires that it controls an internal chip select line then
using a GPIO chip select is going to require that there's some internal
chip select line that's getting controlled so you need one that's not
connected to an actual device that can be used for this purpose.

> Also, the spi-orion driver currently hard-codes that each and every model is
> supposed to have exactly eight HW CS pins. That's not true for my SoC
> (Marvell Armada 388, the Solidrun Clearfog Base board); the 88F6828 CPU only
> has four SPI CS signals.

> What my patch does is simply picking the *first* unused CS and going with
> that one because that looks like the easiest and also safest option.

I would expect that you want to have this selected by the board designer
(unless there's something convenient like a chip select present in the
controller but not routed out of the IP you can guarantee will never be
used).  That will avoid issues if for example the DT is still not
complete.

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

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Jan Kundrát]

On úterý 30. ledna 2018 16:54:09 CET, Mark Brown wrote:
> You can't physically design working hardware for a system with this
> limitation that doesn't have a free chip select somehow - if the
> hardware requires that it controls an internal chip select line then
> using a GPIO chip select is going to require that there's some internal
> chip select line that's getting controlled so you need one that's not
> connected to an actual device that can be used for this purpose.

Hi Mark,
yes, I understand that. Sorry if I confused things by trying to explain why 
a modulo-algorithm won't work.

I have an unused CS lane. It's just that the spi-orion driver currently 
hardcodes HW CS#0, and that one is not free on my board.

The latest version of my patch [1] finds the first *GPIO* CS number. As you 
pointed out, this allocation means that a HW CS signal with the same number 
is definitely not connected to any other slave's CS. My patch therefore 
drives this "unused" HW CS whenever a GPIO CS is needed. This is similar to 
what e.g. spi-sh-msiof.c is doing now.

Should I perhaps improve the patch description? I'm open to all comments 
here.

I see that it might be a bit confusing that I also changed spi-orion.c so 
that it claims/requests/registers/allocates the GPIO for CS. Without that 
patch , spi-orion is different from other SPI masters; I have to manually 
create a gpio-hog for my CS GPIO. I'll be happy to rework my changes into 
two separate patches.

>> Also, the spi-orion driver currently hard-codes that each and 
>> every model is
>> supposed to have exactly eight HW CS pins. That's not true for my SoC
>> (Marvell Armada 388, the Solidrun Clearfog Base board); the 
>> 88F6828 CPU only
>> has four SPI CS signals.
>
>> What my patch does is simply picking the *first* unused CS and going with
>> that one because that looks like the easiest and also safest option.
>
> I would expect that you want to have this selected by the board designer
> (unless there's something convenient like a chip select present in the
> controller but not routed out of the IP you can guarantee will never be
> used).  That will avoid issues if for example the DT is still not
> complete.

That's the approach that I used in my first version of this patch [2], but 
Geert suggested to follow some other SPI master's code and implement 
autodiscovery. I don't really care. Which approach is better from your 
point of view?

I understand the preference to specify the "fake" HW CS manually. My board 
has a flash at CS#0, and it is marked as disabled in a default DT. But 
perhaps that's a special case. Your call, anyway.

With kind regards,
Jan

[1] https://patchwork.kernel.org/patch/10187175/
[2] https://patchwork.kernel.org/patch/10182583/
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Trent Piepho]

On Tue, 2018-01-30 at 09:03 +0100, Jan Kundrát wrote:
> On úterý 30. ledna 2018 2:13:37 CET, Trent Piepho wrote:
> > Another way to fix it, besides default to 0 or to select an unused
> > chipselect, is to use the cs number of the slave modulus the number of
> > native chip selects.  They are or were some other drivers that do this.
> > The idea is that CS numbers don't need to be sequential and there isn't
> > much of a cost of an unused chip select (one word in device tree
> > property).
> 
> Hi Trent, are you talking about an integer modulo here? I don't think that 
> modulo is a correct approach. If my HW has two CS and I use the following 
> DT:
> 
> 	cs-gpios = <0>, <&gpio0 1>, <&gpio0 2>, <0>;
> 
> ...then the logical CS2 (at gpio0 pin 2) conflicts with the native CS0.

What you'd do is use this:

    cs-gpios = <0>, <&gpio0 1>, <0>, <&gpio0 2>;
Put the slaves on cs 0, 1, and 3.  The slaves on 1 and 3 use native CS1
with their gpios.

> Also, the spi-orion driver currently hard-codes that each and every model 
> is supposed to have exactly eight HW CS pins. That's not true for my SoC 
> (Marvell Armada 388, the Solidrun Clearfog Base board); the 88F6828 CPU 
> only has four SPI CS signals.

Yes, you need to know how many CS the driver will consider the device
to have.  That is an additional difficulty.

> 
> What my patch does is simply picking the *first* unused CS and going with 
> that one because that looks like the easiest and also safest option.

Yes, I agree the easiest to use.  Don't need to know how many HW CS the
driver will think there are.  Don't need to manually place your gpios
cs in the list to avoid native cs conflicts.

But you do lose some flexibility.  What if you don't want the first
unused CS?  Probably rather obscure to have that issue.

Re: [PATCH] spi: orion: Allow specifying which HW CS to use with a GPIO CS

[Trent Piepho]

On Tue, 2018-01-30 at 17:09 +0100, Jan Kundrát wrote:
> On úterý 30. ledna 2018 16:54:09 CET, Mark Brown wrote:
> > > What my patch does is simply picking the *first* unused CS and going with
> > > that one because that looks like the easiest and also safest option.
> > 
> > I would expect that you want to have this selected by the board designer
> > (unless there's something convenient like a chip select present in the
> > controller but not routed out of the IP you can guarantee will never be
> > used).  That will avoid issues if for example the DT is still not
> > complete.
> 
> That's the approach that I used in my first version of this patch [2], but 
> Geert suggested to follow some other SPI master's code and implement 
> autodiscovery. I don't really care. Which approach is better from your 
> point of view?

Auto selecting the first unused is certainly nice for the DT
constructor if it works.  Also seems less fragile to me.

But what about possible future support for dynamic creation of spi
devices?  There's already a patch for that out there and there is also
DT fragment loading that could be merged.

There might not be a device on HW CS0 when the driver loads, but then
one is added later, which will be too late since the HW CS was chosen
at driver probe time.

That issue can probably be fixed by choosing the HW CS at spi transfer
setup time.

Another problem is that I don't believe there is any way for the master
to know about what SPI slaves exist.  In Jan's patch, the assumption is
that a non-gpio CS is used.  But this is not strictly correct, as is
does not distinguish between an unused CS and in-use HW CS.  Both those
cases are "not gpio".

But perhaps this is ok.  I think the instructions to the DT author
should be:
    If you will have GPIO chip selects, you must choose one HW CS line
    to be consumed and used by all GPIO CS based slave(s).  Choose by
    placing the first GPIO CS at the position of the HW CS that should
    be consumed by that GPIO CS and any remaining ones.  E.g., if you
    first to have three GPIO CS, and want them to consume HW CS 1, the
    first GPIO CS *must* be placed at CS 1, the remaining two may be at
    any subsequent CS.

Re: [PATCH v2] spi: orion: Rework GPIO CS handling

[Mark Brown]

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

On Fri, Jan 26, 2018 at 11:56:10PM +0100, Jan Kundrát wrote:

> +			if (status) {
> +				dev_err(&pdev->dev,
> +					"Can't request GPIO for CS %d\n",
> +					master->cs_gpios[i]);
> +				goto out_rel_master;
> +			}
> +			if (spi->unused_hw_gpio == -1) {
> +				dev_info(&pdev->dev,
> +					"Selected unused HW CS#%d "
> +					"for any GPIO CSes\n", i);

Don't split log messages over multiple lines, it gets in the way of
anyone searching for them in the code.  Please send a followup patch
fixing this.

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