[v6,1/3] dt-bindings: Add beaglecc1352

  Add DT bindings for BeaglePlay CC1352 co-processor.

The BeaglePlay has a CC1352 co-processor. This co-processor is connected
to the main AM62 (running Linux) over UART. In the BeagleConnect
Technology, CC1352 is responsible for handling 6LoWPAN communication
with beagleconnect freedom nodes as well as their discovery

This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus
driver.

Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
---
 .../devicetree/bindings/net/ti,cc1352p7.yaml  | 48 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
  

On 02/10/2023 20:24, Ayush Singh wrote:
> Add DT bindings for BeaglePlay CC1352 co-processor.
> 

Thank you for your patch. There is something to discuss/improve.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For example:
dt-bindings: net:


> The BeaglePlay has a CC1352 co-processor. This co-processor is connected
> to the main AM62 (running Linux) over UART. In the BeagleConnect
> Technology, CC1352 is responsible for handling 6LoWPAN communication
> with beagleconnect freedom nodes as well as their discovery
> 
> This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus

A nit: I pointed you to the documentation explaining not to use "This
commit adds". It's v6 and the wording is back. Instead drop both
sentences - they are pointless in this context. First one repeats
previous text, second describes driver, but we do not talk here about
drivers.

> driver.
> 
> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
> ---
>  .../devicetree/bindings/net/ti,cc1352p7.yaml  | 48 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> new file mode 100644
> index 000000000000..57bc2c43e5b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments Simplelink CC1352P7 wireless MCU
> +
> +description:
> +  The cc1352p7 mcu can be connected via SPI or UART.

If over SPI, then the binding is incomplete. This is fine for now, I guess.

> +
> +maintainers:
> +  - Ayush Singh <ayushdevel1325@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: ti,cc1352p7
> +
> +  clocks:
> +    maxItems: 2

Instead please list the items like:

clocks:
  items:
    - description: High-Foo-bar
    - description: Low-Foo-bar

> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  power-gpios:
> +    maxItems: 3
> +    description:
> +      The device has three power rails that are exposed on external pins VDDS,
> +      VDDR and DCOUPL.

No, power rails are not GPIOs. You need supplies, so:
vdds-supply: true
vddr-supply: true
dcoupl-supply: true

Look also at:
Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml
which explicitly allows only one powerdown GPIO.

> +

Best regards,
Krzysztof
  

On 23:54-20231002, Ayush Singh wrote:
> Add DT bindings for BeaglePlay CC1352 co-processor.
> 
> The BeaglePlay has a CC1352 co-processor. This co-processor is connected
> to the main AM62 (running Linux) over UART. In the BeagleConnect
> Technology, CC1352 is responsible for handling 6LoWPAN communication
> with beagleconnect freedom nodes as well as their discovery
> 
> This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus
> driver.
> 
> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
> ---
>  .../devicetree/bindings/net/ti,cc1352p7.yaml  | 48 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> new file mode 100644
> index 000000000000..57bc2c43e5b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments Simplelink CC1352P7 wireless MCU
> +
> +description:
> +  The cc1352p7 mcu can be connected via SPI or UART.
> +
> +maintainers:
> +  - Ayush Singh <ayushdevel1325@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: ti,cc1352p7
> +
> +  clocks:
> +    maxItems: 2

I would suggest clock-names and description for it.

> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  power-gpios:
> +    maxItems: 3
> +    description:
> +      The device has three power rails that are exposed on external pins VDDS,
> +      VDDR and DCOUPL.

Shouldn't these be regulators? The power rails are input to the MCU,
correct?
The properties should be something like:
vdds-supply
vddr-supply
dcoupl-supply ? (not sure what dcoupl is, but description should provide
		that info).

the gpio controls for those can be modelled by regulator-gpio ?

> +
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    serial {
> +      mcu {
> +        compatible = "ti,cc1352p7";
> +        clocks = <&sclk_hf 0>, <&sclk_lf 25>;
> +        reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>;
> +        power-gpios = <&pio 1 GPIO_ACTIVE_HIGH>, <&pio 2 GPIO_ACTIVE_HIGH>, <&pio 3 GPIO_ACTIVE_HIGH>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 37b9626ee654..5467669d7963 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8969,6 +8969,12 @@ F:	drivers/staging/greybus/sdio.c
>  F:	drivers/staging/greybus/spi.c
>  F:	drivers/staging/greybus/spilib.c
>  
> +GREYBUS BEAGLEPLAY DRIVERS
> +M:	Ayush Singh <ayushdevel1325@gmail.com>
> +L:	greybus-dev@lists.linaro.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> +
>  GREYBUS SUBSYSTEM
>  M:	Johan Hovold <johan@kernel.org>
>  M:	Alex Elder <elder@kernel.org>
> -- 
> 2.41.0
>
  

>> driver.
>>
>> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
>> ---
>>   .../devicetree/bindings/net/ti,cc1352p7.yaml  | 48 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 +++
>>   2 files changed, 54 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
>> new file mode 100644
>> index 000000000000..57bc2c43e5b1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
>> @@ -0,0 +1,48 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments Simplelink CC1352P7 wireless MCU
>> +
>> +description:
>> +  The cc1352p7 mcu can be connected via SPI or UART.
> If over SPI, then the binding is incomplete. This is fine for now, I guess.
>
> Best regards,
> Krzysztof

Well, I added the line about SPI because the data sheet states that 
CC1352P7 can be connected over SPI or UART when used as wireless MCU. 
But yes, I do not have much knowledge about SPI itself, so the bindings 
might be incomplete for SPI usage. Should I remove it or leave it be?


Sincerely,

Ayush Singh
  

>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  power-gpios:
>> +    maxItems: 3
>> +    description:
>> +      The device has three power rails that are exposed on external pins VDDS,
>> +      VDDR and DCOUPL.
> Shouldn't these be regulators? The power rails are input to the MCU,
> correct?
> The properties should be something like:
> vdds-supply
> vddr-supply
> dcoupl-supply ? (not sure what dcoupl is, but description should provide
> 		that info).
>
> the gpio controls for those can be modelled by regulator-gpio ?

I picked up power lines from "CC13xx/CC26xx Hardware Configuration and 
PCB Design Considerations Application Report" present under "8.14 
Network Processor" of CC1352P7 data sheet.

But now looking closer, it doesn't seem like DCOUPL can be supplied 
externally for CC1352P7 and thus should probably be removed.

Also, it seems like for CC1352P7, VDDR must always be supplied 
internally (The data sheet states: "Internal supply, must be powered 
from the internal DC/DC converter or the internal LDO"). Thus, it should 
be safe to remove VDDR as well.


That means only VDDS needs to be present for power line.


CC13xx/CC26xx Hardware Configuration and PCB Design Considerations 
Application Report: https://www.ti.com/lit/pdf/swra640

CC1352P7 Data sheet: https://www.ti.com/lit/gpn/CC1352P7


Sincerely,

Ayush Singh
  

On 18:17-20231003, Ayush Singh wrote:
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  power-gpios:
> > > +    maxItems: 3
> > > +    description:
> > > +      The device has three power rails that are exposed on external pins VDDS,
> > > +      VDDR and DCOUPL.
> > Shouldn't these be regulators? The power rails are input to the MCU,
> > correct?
> > The properties should be something like:
> > vdds-supply
> > vddr-supply
> > dcoupl-supply ? (not sure what dcoupl is, but description should provide
> > 		that info).
> > 
> > the gpio controls for those can be modelled by regulator-gpio ?
> 
> I picked up power lines from "CC13xx/CC26xx Hardware Configuration and PCB
> Design Considerations Application Report" present under "8.14 Network
> Processor" of CC1352P7 data sheet.
> 
> But now looking closer, it doesn't seem like DCOUPL can be supplied
> externally for CC1352P7 and thus should probably be removed.
> 
> Also, it seems like for CC1352P7, VDDR must always be supplied internally
> (The data sheet states: "Internal supply, must be powered from the internal
> DC/DC converter or the internal LDO"). Thus, it should be safe to remove
> VDDR as well.
From Figure 3-1. CC1312R 7x7 RF Part Schematic Overview (app report you
point out below)
Typical usage is vdds-supply supplying:
VDDS (pin 44)
VDDS2 (pin 13)
VDDS3 (pin 22)
VDDS_DCDC (pin 34)

And DCDC_SW (pin 33) supplies vddr supplying:
VDDR(pin 45)
VDDR_RF (Pin 48)

> 
> 
> That means only VDDS needs to be present for power line.

I agree that that would be the typical supply model. So, just
vdds-supply

> 
> 
> CC13xx/CC26xx Hardware Configuration and PCB Design Considerations
> Application Report: https://www.ti.com/lit/pdf/swra640
> 
> CC1352P7 Data sheet: https://www.ti.com/lit/gpn/CC1352P7
  

On 17:39-20231003, Ayush Singh wrote:
> > > driver.
> > > 
> > > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
> > > ---
> > >   .../devicetree/bindings/net/ti,cc1352p7.yaml  | 48 +++++++++++++++++++
> > >   MAINTAINERS                                   |  6 +++
> > >   2 files changed, 54 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> > > new file mode 100644
> > > index 000000000000..57bc2c43e5b1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> > > @@ -0,0 +1,48 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Texas Instruments Simplelink CC1352P7 wireless MCU
> > > +
> > > +description:
> > > +  The cc1352p7 mcu can be connected via SPI or UART.
> > If over SPI, then the binding is incomplete. This is fine for now, I guess.
> > 
> > Best regards,
> > Krzysztof
> 
> Well, I added the line about SPI because the data sheet states that CC1352P7
> can be connected over SPI or UART when used as wireless MCU. But yes, I do
> not have much knowledge about SPI itself, so the bindings might be
> incomplete for SPI usage. Should I remove it or leave it be?

I'd suggest to leave it for now, we can expand as there is a need.