linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] spi: dt-bindings: spi-controller: Slave mode fixes
@ 2020-03-03  9:45 Geert Uytterhoeven
       [not found] ` <20200303094522.23180-1-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-03-03  9:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Maxime Ripard
  Cc: Yoshihiro Shimoda, linux-spi, devicetree, linux-renesas-soc,
	Geert Uytterhoeven

	Hi Mark, Rob, Maxime,

This patch series contains two fixes for the SPI controller DT bindings
regarding SPI controllers configured for slave mode.

Changes compared to v1[*]:
  - Use "enum: [0, 1]" instead of min/max limit,
  - use "- spi-slave" instead of "[ spi-slave ]".
  - New fix for spi-[rt]x-bus-width.

Thanks!

[*] https://lore.kernel.org/linux-devicetree/20200227130323.15327-1-geert+renesas@glider.be/

Geert Uytterhoeven (2):
  spi: dt-bindings: spi-controller: Fix #address-cells for slave mode
  spi: dt-bindings: spi-controller: Fix spi-[rt]x-bus-width for slave
    mode

 .../bindings/spi/spi-controller.yaml           | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

-- 
2.17.1

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/2] spi: dt-bindings: spi-controller: Fix #address-cells for slave mode
       [not found] ` <20200303094522.23180-1-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
@ 2020-03-03  9:45   ` Geert Uytterhoeven
       [not found]     ` <20200303094522.23180-2-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
  2020-03-03  9:45   ` [PATCH v2 2/2] spi: dt-bindings: spi-controller: Fix spi-[rt]x-bus-width " Geert Uytterhoeven
  1 sibling, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-03-03  9:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Maxime Ripard
  Cc: Yoshihiro Shimoda, linux-spi-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven

Currently, the DT bindings for an SPI controller specify that
"#address-cells" must be fixed to one.  However, that applies to an SPI
controller in master mode only.  When running in SPI slave mode,
"#address-cells" should be zero.

Fix this making the value of "#address-cells" dependent on the presence
of "spi-slave".

Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options")
Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
v2:
  - Use "enum: [0, 1]" instead of min/max limit,
  - use "- spi-slave" instead of "[ spi-slave ]".

As of dtc commit 403cc79f06a135ae ("checks: Update SPI bus check for
'spi-slave'") and Linux commit c2e7075ca8303631 ("scripts/dtc: Update to
upstream version v1.4.7-57-gf267e674d145"), dtc knows about SPI slave.

However, when using "#address-cells = <0>" with W=1:

    Warning (avoid_unnecessary_addr_size): /soc/spi@e6e10000: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property

Just removing #address-cells (using /delete-property/ in the board DTS)
to fix this warning causes:

    Warning (spi_bus_bridge): /soc/spi@e6e10000: incorrect #address-cells for SPI bus

as spi_bus_bridge() uses node_addr_cells(), which defaults to 2 (due to
dtc's ppc64 heritage? But node_size_cells() defaults to 1, not 2?).

How should this be fixed?
Should "#address-cells = <0>" be left out or not?
Should node_{addr,size}_cells() in dtc default to zero?

Thanks!
---
 .../devicetree/bindings/spi/spi-controller.yaml    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 1e0ca6ccf64bbd0a..401d41a97562dc8d 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -20,7 +20,7 @@ properties:
     pattern: "^spi(@.*|-[0-9a-f])*$"
 
   "#address-cells":
-    const: 1
+    enum: [0, 1]
 
   "#size-cells":
     const: 0
@@ -52,6 +52,18 @@ properties:
     description:
       The SPI controller acts as a slave, instead of a master.
 
+if:
+  required:
+    - spi-slave
+then:
+  properties:
+    "#address-cells":
+      const: 0
+else:
+  properties:
+    "#address-cells":
+      const: 1
+
 patternProperties:
   "^slave$":
     type: object
-- 
2.17.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] spi: dt-bindings: spi-controller: Fix spi-[rt]x-bus-width for slave mode
       [not found] ` <20200303094522.23180-1-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
  2020-03-03  9:45   ` [PATCH v2 1/2] spi: dt-bindings: spi-controller: Fix #address-cells for slave mode Geert Uytterhoeven
@ 2020-03-03  9:45   ` Geert Uytterhoeven
  1 sibling, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-03-03  9:45 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Maxime Ripard
  Cc: Yoshihiro Shimoda, linux-spi-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven

The descriptions for the spi-rx-bus-width and spi-tx-bus-width
properties refer to "MISO" and "MOSI", which are not explained in the
document.  While these abbreviations are fairly common when talking
about SPI, and thus may not need an explanation, they are not entirely
correct in this context, as the SPI controller may be used in slave mode
instead of master mode.

Fix this by replacing them by "read transfers" resp. "write transfers",
like is done for the spi-rx-delay-us and spi-tx-delay-us properties.

Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
v2:
  - New.

This issue was present in the .txt version of the bindings, too, so
technically it needs
Fixes: a8830cb19cfea04e ("spi: Document DT bindings for SPI controllers in slave mode")
but of course it won't apply to that version.
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 401d41a97562dc8d..a07819f4efeb9dfe 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -126,7 +126,7 @@ patternProperties:
           - enum: [ 1, 2, 4, 8 ]
           - default: 1
         description:
-          Bus width to the SPI bus used for MISO.
+          Bus width to the SPI bus used for read transfers.
 
       spi-rx-delay-us:
         description:
@@ -138,7 +138,7 @@ patternProperties:
           - enum: [ 1, 2, 4, 8 ]
           - default: 1
         description:
-          Bus width to the SPI bus used for MOSI.
+          Bus width to the SPI bus used for write transfers.
 
       spi-tx-delay-us:
         description:
-- 
2.17.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] spi: dt-bindings: spi-controller: Fix #address-cells for slave mode
       [not found]     ` <20200303094522.23180-2-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
@ 2020-03-03 21:05       ` Rob Herring
       [not found]         ` <CAL_JsqL+9Tcqm_bsorRwqvWZyJXAZmJhXb=EmJ+nZ44kCFp6Kg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2020-03-03 21:05 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Mark Brown, Maxime Ripard, Yoshihiro Shimoda, linux-spi,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On Tue, Mar 3, 2020 at 3:45 AM Geert Uytterhoeven
<geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org> wrote:
>
> Currently, the DT bindings for an SPI controller specify that
> "#address-cells" must be fixed to one.  However, that applies to an SPI
> controller in master mode only.  When running in SPI slave mode,
> "#address-cells" should be zero.
>
> Fix this making the value of "#address-cells" dependent on the presence
> of "spi-slave".
>
> Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options")
> Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> ---
> v2:
>   - Use "enum: [0, 1]" instead of min/max limit,
>   - use "- spi-slave" instead of "[ spi-slave ]".
>
> As of dtc commit 403cc79f06a135ae ("checks: Update SPI bus check for
> 'spi-slave'") and Linux commit c2e7075ca8303631 ("scripts/dtc: Update to
> upstream version v1.4.7-57-gf267e674d145"), dtc knows about SPI slave.
>
> However, when using "#address-cells = <0>" with W=1:
>
>     Warning (avoid_unnecessary_addr_size): /soc/spi@e6e10000: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property

What was the point in having #address-cells in the first place for
slaves? Seems like we should make it mutually exclusive with
'spi-slave'.

Rob

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] spi: dt-bindings: spi-controller: Fix #address-cells for slave mode
       [not found]         ` <CAL_JsqL+9Tcqm_bsorRwqvWZyJXAZmJhXb=EmJ+nZ44kCFp6Kg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2020-03-04 12:50           ` Geert Uytterhoeven
  2020-03-04 15:12             ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-03-04 12:50 UTC (permalink / raw)
  To: Rob Herring
  Cc: Geert Uytterhoeven, Mark Brown, Maxime Ripard, Yoshihiro Shimoda,
	linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

Hi Rob,

On Tue, Mar 3, 2020 at 10:05 PM Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 3, 2020 at 3:45 AM Geert Uytterhoeven
> <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org> wrote:
> > Currently, the DT bindings for an SPI controller specify that
> > "#address-cells" must be fixed to one.  However, that applies to an SPI
> > controller in master mode only.  When running in SPI slave mode,
> > "#address-cells" should be zero.
> >
> > Fix this making the value of "#address-cells" dependent on the presence
> > of "spi-slave".
> >
> > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options")
> > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> > ---
> > v2:
> >   - Use "enum: [0, 1]" instead of min/max limit,
> >   - use "- spi-slave" instead of "[ spi-slave ]".
> >
> > As of dtc commit 403cc79f06a135ae ("checks: Update SPI bus check for
> > 'spi-slave'") and Linux commit c2e7075ca8303631 ("scripts/dtc: Update to
> > upstream version v1.4.7-57-gf267e674d145"), dtc knows about SPI slave.
> >
> > However, when using "#address-cells = <0>" with W=1:
> >
> >     Warning (avoid_unnecessary_addr_size): /soc/spi@e6e10000: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property
>
> What was the point in having #address-cells in the first place for
> slaves?

I don't know, commit a8830cb19cfea04e ("spi: Document DT bindings for
SPI controllers in slave mode") doesn't require any #address-cells for
slave mode.

Perhaps because node_addr_cells() in dtc defaults to 2?
Or because of_bus_n_addr_cells() walks up the parent chain and thus
defaults to the first found parent value?

> Seems like we should make it mutually exclusive with 'spi-slave'.

Sounds like a good idea. How to express that in yaml?

Thanks!

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] spi: dt-bindings: spi-controller: Fix #address-cells for slave mode
  2020-03-04 12:50           ` Geert Uytterhoeven
@ 2020-03-04 15:12             ` Rob Herring
  0 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2020-03-04 15:12 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Geert Uytterhoeven, Mark Brown, Maxime Ripard, Yoshihiro Shimoda,
	linux-spi,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On Wed, Mar 4, 2020 at 6:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Tue, Mar 3, 2020 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Tue, Mar 3, 2020 at 3:45 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> > > Currently, the DT bindings for an SPI controller specify that
> > > "#address-cells" must be fixed to one.  However, that applies to an SPI
> > > controller in master mode only.  When running in SPI slave mode,
> > > "#address-cells" should be zero.
> > >
> > > Fix this making the value of "#address-cells" dependent on the presence
> > > of "spi-slave".
> > >
> > > Fixes: 0a1b929356830257 ("spi: Add YAML schemas for the generic SPI options")
> > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > v2:
> > >   - Use "enum: [0, 1]" instead of min/max limit,
> > >   - use "- spi-slave" instead of "[ spi-slave ]".
> > >
> > > As of dtc commit 403cc79f06a135ae ("checks: Update SPI bus check for
> > > 'spi-slave'") and Linux commit c2e7075ca8303631 ("scripts/dtc: Update to
> > > upstream version v1.4.7-57-gf267e674d145"), dtc knows about SPI slave.
> > >
> > > However, when using "#address-cells = <0>" with W=1:
> > >
> > >     Warning (avoid_unnecessary_addr_size): /soc/spi@e6e10000: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property
> >
> > What was the point in having #address-cells in the first place for
> > slaves?
>
> I don't know, commit a8830cb19cfea04e ("spi: Document DT bindings for
> SPI controllers in slave mode") doesn't require any #address-cells for
> slave mode.
>
> Perhaps because node_addr_cells() in dtc defaults to 2?
> Or because of_bus_n_addr_cells() walks up the parent chain and thus
> defaults to the first found parent value?
>
> > Seems like we should make it mutually exclusive with 'spi-slave'.
>
> Sounds like a good idea. How to express that in yaml?

oneOf:
  - required:
      - "#address-cells"
  - required:
      - spi-slave

Rob

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-03-04 15:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03  9:45 [PATCH v2 0/2] spi: dt-bindings: spi-controller: Slave mode fixes Geert Uytterhoeven
     [not found] ` <20200303094522.23180-1-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
2020-03-03  9:45   ` [PATCH v2 1/2] spi: dt-bindings: spi-controller: Fix #address-cells for slave mode Geert Uytterhoeven
     [not found]     ` <20200303094522.23180-2-geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
2020-03-03 21:05       ` Rob Herring
     [not found]         ` <CAL_JsqL+9Tcqm_bsorRwqvWZyJXAZmJhXb=EmJ+nZ44kCFp6Kg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-03-04 12:50           ` Geert Uytterhoeven
2020-03-04 15:12             ` Rob Herring
2020-03-03  9:45   ` [PATCH v2 2/2] spi: dt-bindings: spi-controller: Fix spi-[rt]x-bus-width " Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).