[PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[Ben Whitten]

Add basic documentation in YAML format for the sx130x series concentrators
from Semtech.
Required is; the location on the SPI bus, the reset gpio and the node for
downstream IQ radios, typically sx125x.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
new file mode 100644
index 000000000000..ad263bc4e60d
--- /dev/null
+++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/lora/semtech,sx130x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech LoRa concentrator
+
+maintainers:
+  - Andreas Färber <afaerber@suse.de>
+  - Ben Whitten <ben.whitten@gmail.com>
+
+description: |
+  Semtech LoRa concentrator sx130x digital baseband chip is capable of
+  demodulating LoRa signals on 8 channels simultaneously.
+
+  It is typically paired with two sx125x IQ radios controlled over an
+  SPI directly from the concentrator.
+
+  The concentrator itself it controlled over SPI.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - semtech,sx1301
+        - semtech,sx1308
+
+  reg:
+    maxItems: 1
+    description: The chip select on the SPI bus.
+
+  reset-gpios:
+    maxItems: 1
+    description: A connection of the reset gpio line.
+
+  spi-max-frequency:
+    maximum: 10000000
+    default: 8000000
+    description: The frequency of the SPI communication to the concentrator,
+      in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
+      on a number of cards.
+
+  radio-spi:
+    description: The concentrator has two radios connected which are contained
+      within the following node.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    required:
+      - '#address-cells'
+      - '#size-cells'
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - radio-spi
+
+examples:
+  - |
+    concentrator0: lora@0 {
+      compatible = "semtech,sx1301";
+      reg = <0>;
+      reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
+      spi-max-frequency = <8000000>;
+
+      radio-spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        radio0: lora@0 {
+          compatible = "semtech,sx1257";
+          reg = <0>;
+        };
+
+        radio1: lora@1 {
+          compatible = "semtech,sx1257";
+          reg = <1>;
+        };
+      };
+    };
-- 
2.17.1

[PATCH lora-next 2/4] dt-bindings: lora: sx125x: add basic documentation

[Ben Whitten]

The sx125x family are IQ radio transceivers from Semtech configured over
SPI, they are typically connected to an sx130x series concentrator however
may be connected to a host directly.

Required properties include the radio number of the host or concentrator
bus.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 .../bindings/lora/semtech,sx125x.yaml         | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx125x.yaml

diff --git a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
new file mode 100644
index 000000000000..5eadec860b70
--- /dev/null
+++ b/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/lora/semtech,sx125x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech IQ modulator/de-modulator transeiver
+
+maintainers:
+  - Andreas Färber <afaerber@suse.de>
+  - Ben Whitten <ben.whitten@gmail.com>
+
+description: |
+  The sx125x family are highly integrated RF front-end to digital I and Q
+  modulator/demodulator Multi-PHY mode transceiver capable of supporting
+  multiple constant and non-constant envelope modulation schemes.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - semtech,sx1255
+        - semtech,sx1257
+        - semtech,sx1258
+
+  reg:
+    maxItems: 1
+    description: The chip select on the SPI bus or radio number in concentrator.
+
+  spi-max-frequency:
+    maximum: 10000000
+    default: 8000000
+    description: The frequency of the SPI communication to the radio,
+      in Hz. Maximum SPI frequency is 10MHz.
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    radio0: lora@0 {
+      compatible = "semtech,sx1257";
+      reg = <0>;
+    };
-- 
2.17.1

Re: [PATCH lora-next 2/4] dt-bindings: lora: sx125x: add basic documentation

[Andreas Färber]

Am 08.01.19 um 09:41 schrieb Ben Whitten:
> The sx125x family are IQ radio transceivers from Semtech configured over
> SPI, they are typically connected to an sx130x series concentrator however
> may be connected to a host directly.

"SX125x" and "SX130x"

> 
> Required properties include the radio number of the host or concentrator
> bus.
> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  .../bindings/lora/semtech,sx125x.yaml         | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> new file mode 100644
> index 000000000000..5eadec860b70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/lora/semtech,sx125x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech IQ modulator/de-modulator transeiver
> +
> +maintainers:
> +  - Andreas Färber <afaerber@suse.de>
> +  - Ben Whitten <ben.whitten@gmail.com>
> +
> +description: |
> +  The sx125x family are highly integrated RF front-end to digital I and Q
> +  modulator/demodulator Multi-PHY mode transceiver capable of supporting
> +  multiple constant and non-constant envelope modulation schemes.

"SX125x"

"are ... transceivers"

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - semtech,sx1255
> +        - semtech,sx1257
> +        - semtech,sx1258

Do we need all three? Probably yes for the concentrator to make decisions?

> +
> +  reg:
> +    maxItems: 1
> +    description: The chip select on the SPI bus or radio number in concentrator.

", with radio A = 0 and radio B = 1."

> +
> +  spi-max-frequency:
> +    maximum: 10000000
> +    default: 8000000
> +    description: The frequency of the SPI communication to the radio,
> +      in Hz. Maximum SPI frequency is 10MHz.

If on the concentrator's SPI bus then this is unused. You don't mark it
required below, so maybe add a textual note?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    radio0: lora@0 {
> +      compatible = "semtech,sx1257";
> +      reg = <0>;
> +    };

If you compare my sx127x implementation, should we prepare a
"radio-frequency" (or "radio-center-frequency") property for the
antenna, to preconfigure the driver during probe? Contents would then be
<868000000>, for example.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH lora-next 2/4] dt-bindings: lora: sx125x: add basic documentation

[Ben Whitten]

Hi Andreas,

> Am 08.01.19 um 09:41 schrieb Ben Whitten:
> > The sx125x family are IQ radio transceivers from Semtech configured over
> > SPI, they are typically connected to an sx130x series concentrator however
> > may be connected to a host directly.
> 
> "SX125x" and "SX130x"
> 
> >
> > Required properties include the radio number of the host or concentrator
> > bus.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> > ---
> >  .../bindings/lora/semtech,sx125x.yaml         | 45 +++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> b/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> > new file mode 100644
> > index 000000000000..5eadec860b70
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> > @@ -0,0 +1,45 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/lora/semtech,sx125x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Semtech IQ modulator/de-modulator transeiver
> > +
> > +maintainers:
> > +  - Andreas Färber <afaerber@suse.de>
> > +  - Ben Whitten <ben.whitten@gmail.com>
> > +
> > +description: |
> > +  The sx125x family are highly integrated RF front-end to digital I and Q
> > +  modulator/demodulator Multi-PHY mode transceiver capable of
> supporting
> > +  multiple constant and non-constant envelope modulation schemes.
> 
> "SX125x"
> 
> "are ... transceivers"
> 
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - semtech,sx1255
> > +        - semtech,sx1257
> > +        - semtech,sx1258
> 
> Do we need all three? Probably yes for the concentrator to make decisions?

We can probe the device version from the concentrator or a host, but if I recall
from a previous review we should not rely on the version register for making
decisions.
The version register of the SX1257 has been missed from the public datasheet
(although exists) and the existing concentrator HAL does not make decisions
based on the version register value.
Better to be explicit in the DT and let the driver use that.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: The chip select on the SPI bus or radio number in
> concentrator.
> 
> ", with radio A = 0 and radio B = 1."
> 
> > +
> > +  spi-max-frequency:
> > +    maximum: 10000000
> > +    default: 8000000
> > +    description: The frequency of the SPI communication to the radio,
> > +      in Hz. Maximum SPI frequency is 10MHz.
> 
> If on the concentrator's SPI bus then this is unused. You don't mark it
> required below, so maybe add a textual note?

As with SX130x comment, this should be covered in common SPI bindings
anyone connecting directly to spi will inherit that, and then anyone using
concentrator will not require it so we can drop this.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    radio0: lora@0 {
> > +      compatible = "semtech,sx1257";
> > +      reg = <0>;
> > +    };
> 
> If you compare my sx127x implementation, should we prepare a
> "radio-frequency" (or "radio-center-frequency") property for the
> antenna, to preconfigure the driver during probe? Contents would then be
> <868000000>, for example.

I don’t know the policy of configuration in DT.
Is the value a hardware property of the antenna? Could we call it
"antenna-frequency" or "bandwidth" and give it a high low point?
Whilst considering antenna properties we should include a gain value as
that affects the radiated power at a given transmit power, food for though
when we come to regulatory db.

Thanks,
Ben Whitten

[PATCH lora-next 3/4] dt-bindings: lora: sx125x: add clock bindings

[Ben Whitten]

The sx125x consumes a 32MHz clock and if it is coupled with a sx130x
concentrator may also provide a gated version of this 32MHz for the
concentrator.

In the example we connect to output 0 of "txco" clock source. The radio
also provides a single clock output, hence "#clock-cells = <0>", named
"clk32m" for consumption by the sx130x concentrator.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 .../{ => net}/lora/semtech,sx125x.yaml        | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 rename Documentation/devicetree/bindings/{ => net}/lora/semtech,sx125x.yaml (67%)

diff --git a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml b/Documentation/devicetree/bindings/net/lora/semtech,sx125x.yaml
similarity index 67%
rename from Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
rename to Documentation/devicetree/bindings/net/lora/semtech,sx125x.yaml
index 5eadec860b70..c2fb4ac06341 100644
--- a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
+++ b/Documentation/devicetree/bindings/net/lora/semtech,sx125x.yaml
@@ -33,13 +33,40 @@ properties:
     description: The frequency of the SPI communication to the radio,
       in Hz. Maximum SPI frequency is 10MHz.
 
+  clocks:
+    maxItems: 1
+    description: 32MHz clock provider
+
+  clock-names:
+    items:
+      - const: txco
+
+  clock-output-names:
+    items:
+      - const: clk32m
+    description: 32MHz output clock name
+
+  '#clock-cells':
+    const: 0
+
 required:
   - compatible
   - reg
 
 examples:
   - |
+    tcxo: dummy32m {
+      compatible = "fixed-clock";
+      clock-frequency = <32000000>;
+      clock-output-names = "tcxo";
+      #clock-cells = <0>;
+    };
+
     radio0: lora@0 {
       compatible = "semtech,sx1257";
       reg = <0>;
+      clocks = <&tcxo 0>;
+      clock-names = "tcxo";
+      clock-output-names = "clk32m";
+      #clock-cells = <0>;
     };
-- 
2.17.1

Re: [PATCH lora-next 3/4] dt-bindings: lora: sx125x: add clock bindings

[Andreas Färber]

Am 08.01.19 um 09:41 schrieb Ben Whitten:
> The sx125x consumes a 32MHz clock and if it is coupled with a sx130x
> concentrator may also provide a gated version of this 32MHz for the
> concentrator.

"SX125x", "SX130x", "32 MHz"

> 
> In the example we connect to output 0 of "txco" clock source. The radio
> also provides a single clock output, hence "#clock-cells = <0>", named
> "clk32m" for consumption by the sx130x concentrator.

No, as pointed out before, output 0 with #clock-cells = <0> is wrong!
Either it's 1 and then you can use a single-cell index, or it's 0, in
which case that's the next (invalid) clock.

> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  .../{ => net}/lora/semtech,sx125x.yaml        | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  rename Documentation/devicetree/bindings/{ => net}/lora/semtech,sx125x.yaml (67%)
> 
> diff --git a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml b/Documentation/devicetree/bindings/net/lora/semtech,sx125x.yaml
> similarity index 67%
> rename from Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> rename to Documentation/devicetree/bindings/net/lora/semtech,sx125x.yaml

In wrong patch?

> index 5eadec860b70..c2fb4ac06341 100644
> --- a/Documentation/devicetree/bindings/lora/semtech,sx125x.yaml
> +++ b/Documentation/devicetree/bindings/net/lora/semtech,sx125x.yaml
> @@ -33,13 +33,40 @@ properties:
>      description: The frequency of the SPI communication to the radio,
>        in Hz. Maximum SPI frequency is 10MHz.
>  
> +  clocks:
> +    maxItems: 1
> +    description: 32MHz clock provider
> +
> +  clock-names:
> +    items:
> +      - const: txco

The name TCXO and 32 MHz appear to come from the SX1301 manual's
reference example?

In the SX1257 manual I see XTA/XTB connected to an XTAL, and CLK_IN.
Both CLK_IN and CLK_OUT are documented as 36 MHz (page 8). In table 2-4
(page 11) however FXOSC is 32 to 36 MHz, typically 36 MHz; FCLK_IN also
has a range of 32 to 36 MHz there. So I guess we should be having two
named clocks here and be more permissive in the description?

While getting into the hairy details of clock names, we should also
prepare optional properties for supply voltages, in case they come from
some PMIC that needs a regulator drifer. By my count there's 7 inputs:
VBAT1/2/3, VR_PA, VR_ANA1/2, VR_DIG. Do we need a named -supply property
for each?

> +
> +  clock-output-names:
> +    items:
> +      - const: clk32m
> +    description: 32MHz output clock name
> +
> +  '#clock-cells':
> +    const: 0
> +
>  required:
>    - compatible
>    - reg
>  
>  examples:
>    - |
> +    tcxo: dummy32m {
> +      compatible = "fixed-clock";
> +      clock-frequency = <32000000>;
> +      clock-output-names = "tcxo";
> +      #clock-cells = <0>;
> +    };
> +
>      radio0: lora@0 {
>        compatible = "semtech,sx1257";
>        reg = <0>;
> +      clocks = <&tcxo 0>;

just <&tcxo>

> +      clock-names = "tcxo";
> +      clock-output-names = "clk32m";
> +      #clock-cells = <0>;
>      };
> 

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[PATCH lora-next 4/4] dt-bindings: lora: sx130x: add clock bindings

[Ben Whitten]

The sx130x family consumes two clocks, a 32MHz clock provided by a
connected IQ transceiver, and a 133MHz high speed clock.

In the example we connect the concentrator to output 0 of a fixed clock
providing the 133MHz high speed clock, and we connect to output 0 of a
connected transceiver 32MHz clock.

The connected radios are both fed from output 0 of a fixed 32MHz clock,
with only one being the clock source back with one output to the
sx130x concentrator.

Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
---
 .../{ => net}/lora/semtech,sx130x.yaml        | 39 ++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)
 rename Documentation/devicetree/bindings/{ => net}/lora/semtech,sx130x.yaml (62%)

diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/net/lora/semtech,sx130x.yaml
similarity index 62%
rename from Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
rename to Documentation/devicetree/bindings/net/lora/semtech,sx130x.yaml
index ad263bc4e60d..23a096ca2912 100644
--- a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
+++ b/Documentation/devicetree/bindings/net/lora/semtech,sx130x.yaml
@@ -15,7 +15,8 @@ description: |
   demodulating LoRa signals on 8 channels simultaneously.
 
   It is typically paired with two sx125x IQ radios controlled over an
-  SPI directly from the concentrator.
+  SPI directly from the concentrator. One of the radios will provide
+  a 32MHz clock back into the concentrator.
 
   The concentrator itself it controlled over SPI.
 
@@ -41,6 +42,20 @@ properties:
       in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
       on a number of cards.
 
+  clocks:
+    maxItems: 2
+    items:
+      - description: 32MHz clock provider
+      - description: 133MHz high speed clock provider
+    description: The chip requires two clock inputs; A 32MHz clock at CMOS
+      level which is provided from a connected radio.
+      And a 133MHz high speed clock at CMOS level provided by an oscillator.
+
+  clock-names:
+    items:
+      - const: clk32m
+      - const: clkhs
+
   radio-spi:
     description: The concentrator has two radios connected which are contained
       within the following node.
@@ -64,11 +79,27 @@ required:
 
 examples:
   - |
+    tcxo: dummy32m {
+      compatible = "fixed-clock";
+      clock-frequency = <32000000>;
+      clock-output-names = "tcxo";
+      #clock-cells = <0>;
+    };
+
+    clkhs: dummy133m {
+      compatible = "fixed-clock";
+      clock-frequency = <133000000>;
+      clock-output-names = "clkhs";
+      #clock-cells = <0>;
+    };
+
     concentrator0: lora@0 {
       compatible = "semtech,sx1301";
       reg = <0>;
       reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
       spi-max-frequency = <8000000>;
+      clocks = <&radio1 0>, <&clkhs 0>;
+      clock-names = "clk32m", "clkhs";
 
       radio-spi {
         #address-cells = <1>;
@@ -77,11 +108,17 @@ examples:
         radio0: lora@0 {
           compatible = "semtech,sx1257";
           reg = <0>;
+          clocks = <&tcxo 0>;
+          clock-names = "tcxo";
         };
 
         radio1: lora@1 {
           compatible = "semtech,sx1257";
           reg = <1>;
+          clocks = <&tcxo 0>;
+          clock-names = "tcxo";
+          clock-output-names = "clk32m";
+          #clock-cells = <0>;
         };
       };
     };
-- 
2.17.1

Re: [PATCH lora-next 4/4] dt-bindings: lora: sx130x: add clock bindings

[Andreas Färber]

Am 08.01.19 um 09:41 schrieb Ben Whitten:
> The sx130x family consumes two clocks, a 32MHz clock provided by a
> connected IQ transceiver, and a 133MHz high speed clock.
> 
> In the example we connect the concentrator to output 0 of a fixed clock
> providing the 133MHz high speed clock, and we connect to output 0 of a
> connected transceiver 32MHz clock.
> 
> The connected radios are both fed from output 0 of a fixed 32MHz clock,
> with only one being the clock source back with one output to the
> sx130x concentrator.

No, no, no... See below.

> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  .../{ => net}/lora/semtech,sx130x.yaml        | 39 ++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>  rename Documentation/devicetree/bindings/{ => net}/lora/semtech,sx130x.yaml (62%)
> 
> diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/net/lora/semtech,sx130x.yaml
> similarity index 62%
> rename from Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> rename to Documentation/devicetree/bindings/net/lora/semtech,sx130x.yaml
> index ad263bc4e60d..23a096ca2912 100644
> --- a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> +++ b/Documentation/devicetree/bindings/net/lora/semtech,sx130x.yaml
> @@ -15,7 +15,8 @@ description: |
>    demodulating LoRa signals on 8 channels simultaneously.
>  
>    It is typically paired with two sx125x IQ radios controlled over an
> -  SPI directly from the concentrator.
> +  SPI directly from the concentrator. One of the radios will provide
> +  a 32MHz clock back into the concentrator.

The SX125x does not seem to document the frequency of the output clock,
so I assume it's pass-through of whatever went in, and 32 MHz seems
correct for the SX130x input side.

>  
>    The concentrator itself it controlled over SPI.
>  
> @@ -41,6 +42,20 @@ properties:
>        in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
>        on a number of cards.
>  
> +  clocks:
> +    maxItems: 2
> +    items:
> +      - description: 32MHz clock provider
> +      - description: 133MHz high speed clock provider
> +    description: The chip requires two clock inputs; A 32MHz clock at CMOS
> +      level which is provided from a connected radio.
> +      And a 133MHz high speed clock at CMOS level provided by an oscillator.
> +
> +  clock-names:
> +    items:
> +      - const: clk32m

The pin is called CLK32M; in the specs there's XTAL32F/XTAL32T.

> +      - const: clkhs

The pin is called CLKHS; in the specs it's HSC_F.

So I assume both names are good, but wanted to double-check.

> +
>    radio-spi:
>      description: The concentrator has two radios connected which are contained
>        within the following node.
> @@ -64,11 +79,27 @@ required:
>  
>  examples:
>    - |
> +    tcxo: dummy32m {
> +      compatible = "fixed-clock";
> +      clock-frequency = <32000000>;
> +      clock-output-names = "tcxo";
> +      #clock-cells = <0>;
> +    };
> +
> +    clkhs: dummy133m {
> +      compatible = "fixed-clock";
> +      clock-frequency = <133000000>;
> +      clock-output-names = "clkhs";
> +      #clock-cells = <0>;
> +    };
> +
>      concentrator0: lora@0 {
>        compatible = "semtech,sx1301";
>        reg = <0>;
>        reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
>        spi-max-frequency = <8000000>;
> +      clocks = <&radio1 0>, <&clkhs 0>;

Once again, this is wrong use of #clock-cells: <&radio1>, <&clkhs>
Otherwise you have four clocks, and your "clkhs" will be <0>.

> +      clock-names = "clk32m", "clkhs";
>  
>        radio-spi {
>          #address-cells = <1>;
> @@ -77,11 +108,17 @@ examples:
>          radio0: lora@0 {
>            compatible = "semtech,sx1257";
>            reg = <0>;
> +          clocks = <&tcxo 0>;

Ditto, <&tcxo>.

> +          clock-names = "tcxo";
>          };
>  
>          radio1: lora@1 {
>            compatible = "semtech,sx1257";
>            reg = <1>;
> +          clocks = <&tcxo 0>;

<&tcxo>

> +          clock-names = "tcxo";
> +          clock-output-names = "clk32m";
> +          #clock-cells = <0>;
>          };
>        };
>      };

This example may need to be updated to use a fixed-clock instead of the
radio'S output, due to unsolved clk provider implementation problems
that will render the example unusable.

Same as for the radio, we should also prepare optional regulator inputs.
There's at least 1.8 V and 3.3 V supplies here.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[Andreas Färber]

Hi Ben,

Am 08.01.19 um 09:41 schrieb Ben Whitten:
> Add basic documentation in YAML format for the sx130x series concentrators
> from Semtech.
> Required is; the location on the SPI bus, the reset gpio and the node for
> downstream IQ radios, typically sx125x.
> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

Patch 3/4 moves this to net/lora/, which I think is more appropriate.

> 
> diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> new file mode 100644
> index 000000000000..ad263bc4e60d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/lora/semtech,sx130x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech LoRa concentrator
> +
> +maintainers:
> +  - Andreas Färber <afaerber@suse.de>
> +  - Ben Whitten <ben.whitten@gmail.com>
> +
> +description: |
> +  Semtech LoRa concentrator sx130x digital baseband chip is capable of

SX130x or SX1301/SX1308, to distinguish from driver name sx130x and to
avoid ambiguities of which x is a wildcard.

> +  demodulating LoRa signals on 8 channels simultaneously.
> +
> +  It is typically paired with two sx125x IQ radios controlled over an

Ditto, SX125x

> +  SPI directly from the concentrator.
> +
> +  The concentrator itself it controlled over SPI.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - semtech,sx1301
> +        - semtech,sx1308

We should only list both if we expect differences between the two
models. Otherwise SX1308 can reuse the SX1301 compatible. If we want to
mark it up just in case then rearranging the above to be a sequence of
"semtech,sx1308", "semtech,sx1301" would be an alternative.

> +
> +  reg:
> +    maxItems: 1
> +    description: The chip select on the SPI bus.

Is this mandatory now or not with maxItems?

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: A connection of the reset gpio line.

This needs to be optional, which I think the maxItems syntax says unlike
the commit message?
On an mPCIe card you won't have such a GPIO, for instance. We do a Soft
Reset, so it's not functionally mandatory.

> +
> +  spi-max-frequency:
> +    maximum: 10000000
> +    default: 8000000
> +    description: The frequency of the SPI communication to the concentrator,
> +      in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
> +      on a number of cards.

Do we really need to describe this here? It should be covered in the
common SPI bindings, and only applies to SPI bus, not USB picoGW.

> +
> +  radio-spi:
> +    description: The concentrator has two radios connected which are contained
> +      within the following node.

"can have"

> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    required:
> +      - '#address-cells'
> +      - '#size-cells'

I'm pretty sure that Rob would like to have a compatible here, even if
unneeded in our Linux driver?

BTW if someone has better naming suggestions than "radio-spi"... I just
wanted to avoid having it in the main node directly, in case we need
other sub-nodes, too.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios

Must be optional.

> +  - radio-spi

Should be optional. (Driver needs it today, but that's another problem.)

> +
> +examples:
> +  - |
> +    concentrator0: lora@0 {
> +      compatible = "semtech,sx1301";
> +      reg = <0>;
> +      reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
> +      spi-max-frequency = <8000000>;
> +
> +      radio-spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        radio0: lora@0 {
> +          compatible = "semtech,sx1257";
> +          reg = <0>;
> +        };
> +
> +        radio1: lora@1 {
> +          compatible = "semtech,sx1257";
> +          reg = <1>;
> +        };
> +      };
> +    };

Thanks for looking into this,

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[Ben Whitten]

Hi Andreas,

> Am 08.01.19 um 09:41 schrieb Ben Whitten:
> > Add basic documentation in YAML format for the sx130x series concentrators
> > from Semtech.
> > Required is; the location on the SPI bus, the reset gpio and the node for
> > downstream IQ radios, typically sx125x.
> >
> > Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> > ---
> >  .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> 
> Patch 3/4 moves this to net/lora/, which I think is more appropriate.

Agreed, I think it was a change merged into the wrong commits by mistake

> >
> > diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> > new file mode 100644
> > index 000000000000..ad263bc4e60d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/lora/semtech,sx130x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Semtech LoRa concentrator
> > +
> > +maintainers:
> > +  - Andreas Färber <afaerber@suse.de>
> > +  - Ben Whitten <ben.whitten@gmail.com>
> > +
> > +description: |
> > +  Semtech LoRa concentrator sx130x digital baseband chip is capable of
> 
> SX130x or SX1301/SX1308, to distinguish from driver name sx130x and to
> avoid ambiguities of which x is a wildcard.
> 
> > +  demodulating LoRa signals on 8 channels simultaneously.
> > +
> > +  It is typically paired with two sx125x IQ radios controlled over an
> 
> Ditto, SX125x
> 
> > +  SPI directly from the concentrator.
> > +
> > +  The concentrator itself it controlled over SPI.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +        - semtech,sx1301
> > +        - semtech,sx1308
> 
> We should only list both if we expect differences between the two
> models. Otherwise SX1308 can reuse the SX1301 compatible. If we want to
> mark it up just in case then rearranging the above to be a sequence of
> "semtech,sx1308", "semtech,sx1301" would be an alternative.

It was my understanding that we should name each device that is compatible,
avoiding wildcard 'x' in compatible names. This allows the device tree to be
more accurate to the hardware that it is describing.

I do not expect there to be much difference, but there may be some that I 
am unaware of.

Not sure I follow here, do you wish for the order to be flipped if we do want
to state every device? I see that example-schema does indeed have entries
in reverse.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: The chip select on the SPI bus.
> 
> Is this mandatory now or not with maxItems?

min/maxItems is implied if you have a list but for our chipselect we have no
list. I followed child-node-example.yaml in yaml-bindings and
trivial-devices.yaml in being explicit and stating it be one element and making
reg required.

> 
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +    description: A connection of the reset gpio line.
> 
> This needs to be optional, which I think the maxItems syntax says unlike
> the commit message?
> On an mPCIe card you won't have such a GPIO, for instance. We do a Soft
> Reset, so it's not functionally mandatory.

I'll drop this from required and from the commit message.

> > +
> > +  spi-max-frequency:
> > +    maximum: 10000000
> > +    default: 8000000
> > +    description: The frequency of the SPI communication to the concentrator,
> > +      in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
> > +      on a number of cards.
> 
> Do we really need to describe this here? It should be covered in the
> common SPI bindings, and only applies to SPI bus, not USB picoGW.

True, I'll drop this.

> 
> > +
> > +  radio-spi:
> > +    description: The concentrator has two radios connected which are
> contained
> > +      within the following node.
> 
> "can have"
> 
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    required:
> > +      - '#address-cells'
> > +      - '#size-cells'
> 
> I'm pretty sure that Rob would like to have a compatible here, even if
> unneeded in our Linux driver?
> 
> BTW if someone has better naming suggestions than "radio-spi"... I just
> wanted to avoid having it in the main node directly, in case we need
> other sub-nodes, too.

Just like ahb and apb it makes sense to separate the bus from the device
An alternative could be "transceiver-bus" or "radio-bus" as the fact its
spi is masked away anyway.
What would it's compatible string be, match the node name?
Would the sx130x_radio bus type match against it?

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reset-gpios
> 
> Must be optional.
> 
> > +  - radio-spi
> 
> Should be optional. (Driver needs it today, but that's another problem.)
> 
> > +
> > +examples:
> > +  - |
> > +    concentrator0: lora@0 {
> > +      compatible = "semtech,sx1301";
> > +      reg = <0>;
> > +      reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
> > +      spi-max-frequency = <8000000>;
> > +
> > +      radio-spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        radio0: lora@0 {
> > +          compatible = "semtech,sx1257";
> > +          reg = <0>;
> > +        };
> > +
> > +        radio1: lora@1 {
> > +          compatible = "semtech,sx1257";
> > +          reg = <1>;
> > +        };
> > +      };
> > +    };
> 
> Thanks for looking into this,
> 
> Andreas
> 

Thanks,
Ben Whitten

Re: [PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[Rob Herring]

On Tue, Jan 08, 2019 at 05:41:29PM +0900, Ben Whitten wrote:
> Add basic documentation in YAML format for the sx130x series concentrators
> from Semtech.
> Required is; the location on the SPI bus, the reset gpio and the node for
> downstream IQ radios, typically sx125x.
> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

A schema binding. Yay!

> 
> diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> new file mode 100644
> index 000000000000..ad263bc4e60d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/lora/semtech,sx130x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech LoRa concentrator
> +
> +maintainers:
> +  - Andreas Färber <afaerber@suse.de>
> +  - Ben Whitten <ben.whitten@gmail.com>
> +
> +description: |
> +  Semtech LoRa concentrator sx130x digital baseband chip is capable of
> +  demodulating LoRa signals on 8 channels simultaneously.
> +
> +  It is typically paired with two sx125x IQ radios controlled over an
> +  SPI directly from the concentrator.
> +
> +  The concentrator itself it controlled over SPI.

s/it/is/

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - semtech,sx1301
> +        - semtech,sx1308
> +
> +  reg:
> +    maxItems: 1
> +    description: The chip select on the SPI bus.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: A connection of the reset gpio line.
> +
> +  spi-max-frequency:
> +    maximum: 10000000
> +    default: 8000000
> +    description: The frequency of the SPI communication to the concentrator,
> +      in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
> +      on a number of cards.
> +
> +  radio-spi:

child nodes should have 'type: object'

> +    description: The concentrator has two radios connected which are contained
> +      within the following node.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0

You need the grandchildren here too to define 'reg' should be 0-8. 
You'll need to use 'patternProperties' since there's a unit-address.

You could maybe get rid of the radio-spi node. You don't need it unless 
you wanted to add other types of child nodes.

> +
> +    required:
> +      - '#address-cells'
> +      - '#size-cells'
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - radio-spi
> +
> +examples:
> +  - |
> +    concentrator0: lora@0 {
> +      compatible = "semtech,sx1301";
> +      reg = <0>;
> +      reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
> +      spi-max-frequency = <8000000>;
> +
> +      radio-spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        radio0: lora@0 {
> +          compatible = "semtech,sx1257";
> +          reg = <0>;
> +        };
> +
> +        radio1: lora@1 {
> +          compatible = "semtech,sx1257";
> +          reg = <1>;
> +        };
> +      };
> +    };
> -- 
> 2.17.1
> 

Re: [PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[Rob Herring]

On Mon, Jan 21, 2019 at 01:14:20PM -0600, Rob Herring wrote:
> On Tue, Jan 08, 2019 at 05:41:29PM +0900, Ben Whitten wrote:
> > Add basic documentation in YAML format for the sx130x series concentrators
> > from Semtech.
> > Required is; the location on the SPI bus, the reset gpio and the node for
> > downstream IQ radios, typically sx125x.
> > 
> > Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> > ---
> >  .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> 
> A schema binding. Yay!
> 
> > 
> > diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> > new file mode 100644
> > index 000000000000..ad263bc4e60d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: GPL-2.0

Also, I'd like new bindings to be dual GPL-2.0 and BSD-2-Clause if you 
don't mind. This will make it easier to move the schema out of the 
kernel if/when we ever do that.

Rob

Re: [PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

[Rob Herring]

On Tue, Jan 08, 2019 at 05:41:29PM +0900, Ben Whitten wrote:
> Add basic documentation in YAML format for the sx130x series concentrators
> from Semtech.
> Required is; the location on the SPI bus, the reset gpio and the node for
> downstream IQ radios, typically sx125x.
> 
> Signed-off-by: Ben Whitten <ben.whitten@gmail.com>
> ---
>  .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> new file mode 100644
> index 000000000000..ad263bc4e60d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/lora/semtech,sx130x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech LoRa concentrator
> +
> +maintainers:
> +  - Andreas Färber <afaerber@suse.de>
> +  - Ben Whitten <ben.whitten@gmail.com>
> +
> +description: |
> +  Semtech LoRa concentrator sx130x digital baseband chip is capable of
> +  demodulating LoRa signals on 8 channels simultaneously.
> +
> +  It is typically paired with two sx125x IQ radios controlled over an
> +  SPI directly from the concentrator.
> +
> +  The concentrator itself it controlled over SPI.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - semtech,sx1301
> +        - semtech,sx1308
> +
> +  reg:
> +    maxItems: 1
> +    description: The chip select on the SPI bus.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: A connection of the reset gpio line.
> +
> +  spi-max-frequency:
> +    maximum: 10000000
> +    default: 8000000
> +    description: The frequency of the SPI communication to the concentrator,
> +      in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
> +      on a number of cards.
> +
> +  radio-spi:
> +    description: The concentrator has two radios connected which are contained
> +      within the following node.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    required:
> +      - '#address-cells'
> +      - '#size-cells'
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - radio-spi
> +
> +examples:
> +  - |

One more comment. Make sure you "build" the schema with 
'dt_binding_check' target. That builds the examples.

It is also checked by a CI job and the status is added to patchwork[1]. 
(Though it didn't work correctly for this one and I just fixed it.)

> +    concentrator0: lora@0 {

This example doesn't build because there's an expectation of the top 
level nodes being MMIO if there's a reg prop. You just need to wrap this 
in a parent node representing a spi controller:

spi {
  #address-cells = <1>;
  #size-cells = <0>;

  ...

};

> +      compatible = "semtech,sx1301";
> +      reg = <0>;
> +      reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
> +      spi-max-frequency = <8000000>;
> +
> +      radio-spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        radio0: lora@0 {
> +          compatible = "semtech,sx1257";
> +          reg = <0>;
> +        };
> +
> +        radio1: lora@1 {
> +          compatible = "semtech,sx1257";
> +          reg = <1>;
> +        };
> +      };
> +    };

[1] https://patchwork.ozlabs.org/patch/1021773/