[PATCH v2 00/13] Make atmel serial driver aware of GCLK

[PATCH v2 00/13] Make atmel serial driver aware of GCLK

[Sergiu Moga]

This series of patches introduces the GCLK as a potential clock source for
the baudrate generator of UART on sama5d2 SoCs. Unlike the serial mode of
the USART offered by FLEXCOM, the UART does not provide a fractional part
that can be added to the clock divisor to obtain a more accurate result,
which greatly decreases the flexibility available for producing a higher
variety of baudrates. Now, with the last patch of the series, the driver
will check for a GCLK in the DT. If provided, whenever `atmel_set_termios`
is called, unless there is a fractional part, the driver will compare the
error rate between the desired baudrate and the actual baudrate obtained
through each of the available clock sources and will choose the clock source
with the lowest error rate. While at it, convert the DT binding
for UART/USART to json-schema, update the FLEXCOM binding to reference the
new UART/USART binding (while differentiating between the SPI of USART and the
SPI of FLEXCOM) and do some small DT related fixups.

The DT bindings related patches of this patch series depend on this patch
series converting atmel-flexcom bindings to json-schema:
https://lore.kernel.org/all/20220708115619.254073-1-kavyasree.kotagiri@microchip.com/

v1 -> v2:
- [PATCH 3] dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref
    binding:
	- use full schema paths

- [PATCH 5] dt-bindings: serial: atmel,at91-usart: convert to json-schema
	- only do what the commit says, split the addition of other compatibles
	(PATCH 6) and properties (PATCH 13) in other patches
	- remove unnecessary "|"'s
	- mention header in `atmel,usart-mode`'s description
	- place `if:` under `allOf:`
	- respect order of spi0's DT properties: compatible, then reg then the
	reset of properties

- two new baudrate clock source related patches:
  [PATCH 9] tty: serial: atmel: Add definition for GCLK as baudrate source clock
			+
  [PATCH 10] tty: serial: atmel: Define BRSRCCK bitmask of UART IP's Mode
    Register:
	- v1's bitfield definition of GCLK was wrong, so add two more patches:
		- one for the definition of GCLK of USART IP's
		- one for the definition of BRSRCCK bitmask and its bitfields
		for UART IP's

- a new cleanup related patch that introduces a new struct atmel_uart_port field:
  [PATCH 11] tty: serial: atmel: Only divide Clock Divisor if the IP is USART:
  	- this ensures a division by 8 which is unnecessary and unappliable to
	UART IP's is only done for USART IP's

- four new patches regarding DT fixes and a SPI binding update that I came
upon:
  [PATCH 1] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties
  [PATCH 2] ARM: dts: at91: sama7g5: Swap rx and tx for spi11
  [PATCH 4] ARM: dts: at91: sam9x60ek: Add DBGU compatibles to uart1
  [PATCH 6] dt-bindings: serial: atmel,at91-usart: Highlight SAM9X60 incremental

- [PATCH 12] tty: serial: atmel: Make the driver aware of the existence of GCLK
	- take into account the different placement of the baudrate clock source
	into the IP's Mode Register (USART vs UART)
	- don't check for atmel_port->gclk != NULL
	- use clk_round_rate instead of clk_set_rate + clk_get_rate
	- remove clk_disable_unprepare from the end of the probe method

Sergiu Moga (13):
  spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties
  ARM: dts: at91: sama7g5: Swap rx and tx for spi11
  dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref
    binding
  ARM: dts: at91: sam9x60ek: Add DBGU compatibles to uart1
  dt-bindings: serial: atmel,at91-usart: convert to json-schema
  dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to
    SAM9x60
  dt-bindings: mfd: atmel,sama5d2-flexcom: Add USART child node ref
    binding
  tty: serial: atmel: Define GCLK as USART baudrate source clock
  tty: serial: atmel: Define BRSRCCK bitmask of UART IP's Mode Register
  tty: serial: atmel: Only divide Clock Divisor if the IP is USART
  clk: at91: sama5d2: Add Generic Clocks for UART/USART
  tty: serial: atmel: Make the driver aware of the existence of GCLK
  dt-bindings: serial: atmel,at91-usart: Add gclk as a possible USART
    clock

 .../bindings/mfd/atmel,sama5d2-flexcom.yaml   |  19 +-
 .../devicetree/bindings/mfd/atmel-usart.txt   |  98 ---------
 .../bindings/serial/atmel,at91-usart.yaml     | 191 ++++++++++++++++++
 .../bindings/spi/atmel,at91rm9200-spi.yaml    |  10 +
 arch/arm/boot/dts/at91-sam9x60ek.dts          |   2 +-
 arch/arm/boot/dts/sama7g5.dtsi                |   6 +-
 drivers/clk/at91/sama5d2.c                    |  10 +
 drivers/tty/serial/atmel_serial.c             |  65 +++++-
 drivers/tty/serial/atmel_serial.h             |   4 +
 9 files changed, 295 insertions(+), 110 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
 create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 01/13] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties

[Sergiu Moga]

The DT nodes of the SPI IP's may contain DMA related properties so
make sure that the binding is able to properly validate those as
well by making it aware of these optional properties.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- Nothing, this patch was not here before


 .../devicetree/bindings/spi/atmel,at91rm9200-spi.yaml  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/atmel,at91rm9200-spi.yaml b/Documentation/devicetree/bindings/spi/atmel,at91rm9200-spi.yaml
index d85d54024b2e..4dd973e341e6 100644
--- a/Documentation/devicetree/bindings/spi/atmel,at91rm9200-spi.yaml
+++ b/Documentation/devicetree/bindings/spi/atmel,at91rm9200-spi.yaml
@@ -34,6 +34,16 @@ properties:
   clocks:
     maxItems: 1
 
+  dmas:
+    items:
+      - description: TX DMA Channel
+      - description: RX DMA Channel
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
   atmel,fifo-size:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: |
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/13] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 357 bytes --]

On Tue, Sep 06, 2022 at 04:55:00PM +0300, Sergiu Moga wrote:
> The DT nodes of the SPI IP's may contain DMA related properties so
> make sure that the binding is able to properly validate those as
> well by making it aware of these optional properties.

Acked-by: Mark Brown <broonie@kernel.org>

though it looks like perhaps this could just go separately?

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/13] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties

[Rob Herring]

On Tue, 06 Sep 2022 16:55:00 +0300, Sergiu Moga wrote:
> The DT nodes of the SPI IP's may contain DMA related properties so
> make sure that the binding is able to properly validate those as
> well by making it aware of these optional properties.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> v1 -> v2:
> - Nothing, this patch was not here before
> 
> 
>  .../devicetree/bindings/spi/atmel,at91rm9200-spi.yaml  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/


spi@400: dma-names:0: 'tx' was expected
	arch/arm/boot/dts/at91-sama7g5ek.dtb

spi@400: dma-names:1: 'rx' was expected
	arch/arm/boot/dts/at91-sama7g5ek.dtb

spi@400: Unevaluated properties are not allowed ('dma-names' was unexpected)
	arch/arm/boot/dts/at91-sama7g5ek.dtb


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/13] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties

[Sergiu.Moga]

On 07.09.2022 00:41, Rob Herring wrote:
> 
> On Tue, 06 Sep 2022 16:55:00 +0300, Sergiu Moga wrote:
>> The DT nodes of the SPI IP's may contain DMA related properties so
>> make sure that the binding is able to properly validate those as
>> well by making it aware of these optional properties.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
>>
>>
>>   .../devicetree/bindings/spi/atmel,at91rm9200-spi.yaml  | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/patch/
> 
> 
> spi@400: dma-names:0: 'tx' was expected
>          arch/arm/boot/dts/at91-sama7g5ek.dtb
> 
> spi@400: dma-names:1: 'rx' was expected
>          arch/arm/boot/dts/at91-sama7g5ek.dtb
> 
> spi@400: Unevaluated properties are not allowed ('dma-names' was unexpected)
>          arch/arm/boot/dts/at91-sama7g5ek.dtb
> 

Hi,

This should be solved by the next patch of this series. I guess this 
yaml property addition should have come after the DTS fix patch.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 01/13] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties

[Krzysztof Kozlowski]

On 06/09/2022 15:55, Sergiu Moga wrote:
> The DT nodes of the SPI IP's may contain DMA related properties so
> make sure that the binding is able to properly validate those as
> well by making it aware of these optional properties.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 02/13] ARM: dts: at91: sama7g5: Swap rx and tx for spi11

[Sergiu Moga]

Swap the rx and tx of the DMA related DT properties of the spi11 node
in order to maintain consistency across Microchip/Atmel SoC files.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- Nothing, this patch was not here before


 arch/arm/boot/dts/sama7g5.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
index bb6d71e6dfeb..249f9c640b6c 100644
--- a/arch/arm/boot/dts/sama7g5.dtsi
+++ b/arch/arm/boot/dts/sama7g5.dtsi
@@ -866,9 +866,9 @@ spi11: spi@400 {
 				#address-cells = <1>;
 				#size-cells = <0>;
 				atmel,fifo-size = <32>;
-				dmas = <&dma0 AT91_XDMAC_DT_PERID(27)>,
-					    <&dma0 AT91_XDMAC_DT_PERID(28)>;
-				dma-names = "rx", "tx";
+				dmas = <&dma0 AT91_XDMAC_DT_PERID(28)>,
+					    <&dma0 AT91_XDMAC_DT_PERID(27)>;
+				dma-names = "tx", "rx";
 				status = "disabled";
 			};
 		};
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 03/13] dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref binding

[Sergiu Moga]

Another functionality of FLEXCOM is that of SPI. In order for
the proper validation of the SPI children nodes through the binding
to occur, the proper binding for SPI must be referenced.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- use full schema paths


 .../devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml       | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
index 568da7cb630c..0db0f2728b65 100644
--- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
+++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
@@ -78,10 +78,9 @@ patternProperties:
       of USART bindings.
 
   "^spi@[0-9a-f]+$":
-    type: object
+    $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
     description:
-      Child node describing SPI. See ../spi/spi_atmel.txt for details
-      of SPI bindings.
+      Child node describing SPI.
 
   "^i2c@[0-9a-f]+$":
     $ref: ../i2c/atmel,at91sam-i2c.yaml
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 03/13] dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref binding

[Krzysztof Kozlowski]

On 06/09/2022 15:55, Sergiu Moga wrote:
> Another functionality of FLEXCOM is that of SPI. In order for
> the proper validation of the SPI children nodes through the binding
> to occur, the proper binding for SPI must be referenced.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 04/13] ARM: dts: at91: sam9x60ek: Add DBGU compatibles to uart1

[Sergiu Moga]

Maintain consistency among the compatibles of the serial nodes of
sam9x60ek and highlight the incremental characteristic of its serial
IP's by making sure that all serial nodes contain both the sam9x60
and sam9260 usart/dbgu compatibles.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---



v1 -> v2:
- Nothing, this patch was not here before



 arch/arm/boot/dts/at91-sam9x60ek.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/at91-sam9x60ek.dts b/arch/arm/boot/dts/at91-sam9x60ek.dts
index 81c38e101f58..49827e63508d 100644
--- a/arch/arm/boot/dts/at91-sam9x60ek.dts
+++ b/arch/arm/boot/dts/at91-sam9x60ek.dts
@@ -264,7 +264,7 @@ &flx5 {
 	status = "okay";
 
 	uart1: serial@200 {
-		compatible = "microchip,sam9x60-usart", "atmel,at91sam9260-usart";
+		compatible = "microchip,sam9x60-dbgu", "microchip,sam9x60-usart", "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
 		reg = <0x200 0x200>;
 		interrupts = <14 IRQ_TYPE_LEVEL_HIGH 7>;
 		dmas = <&dma0
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

[Sergiu Moga]

Convert at91 USART DT Binding for Atmel/Microchip SoCs to
json-schema format. Furthermore, move this binding to the
serial directory, since binding directories match hardware,
unlike the driver subsystems which match Linux convention.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---



v1 -> v2:
- only do what the commit says, split the addition of other compatibles and
properties in other patches
- remove unnecessary "|"'s
- mention header in `atmel,usart-mode`'s description
- place `if:` under `allOf:`
- respect order of spi0's DT properties: compatible, then reg then the reset of properties





 .../devicetree/bindings/mfd/atmel-usart.txt   |  98 ----------
 .../bindings/serial/atmel,at91-usart.yaml     | 183 ++++++++++++++++++
 2 files changed, 183 insertions(+), 98 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
 create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml

diff --git a/Documentation/devicetree/bindings/mfd/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt
deleted file mode 100644
index a09133066aff..000000000000
--- a/Documentation/devicetree/bindings/mfd/atmel-usart.txt
+++ /dev/null
@@ -1,98 +0,0 @@
-* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
-
-Required properties for USART:
-- compatible: Should be one of the following:
-	- "atmel,at91rm9200-usart"
-	- "atmel,at91sam9260-usart"
-	- "microchip,sam9x60-usart"
-	- "atmel,at91rm9200-dbgu", "atmel,at91rm9200-usart"
-	- "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart"
-	- "microchip,sam9x60-dbgu", "microchip,sam9x60-usart"
-- reg: Should contain registers location and length
-- interrupts: Should contain interrupt
-- clock-names: tuple listing input clock names.
-	Required elements: "usart"
-- clocks: phandles to input clocks.
-
-Required properties for USART in SPI mode:
-- #size-cells      : Must be <0>
-- #address-cells   : Must be <1>
-- cs-gpios: chipselects (internal cs not supported)
-- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h)
-
-Optional properties in serial and SPI mode:
-- dma bindings for dma transfer:
-	- dmas: DMA specifier, consisting of a phandle to DMA controller node,
-		memory peripheral interface and USART DMA channel ID, FIFO configuration.
-		The order of DMA channels is fixed. The first DMA channel must be TX
-		associated channel and the second one must be RX associated channel.
-		Refer to dma.txt and atmel-dma.txt for details.
-	- dma-names: "tx" for TX channel.
-		     "rx" for RX channel.
-		     The order of dma-names is also fixed. The first name must be "tx"
-		     and the second one must be "rx" as in the examples below.
-
-Optional properties in serial mode:
-- atmel,use-dma-rx: use of PDC or DMA for receiving data
-- atmel,use-dma-tx: use of PDC or DMA for transmitting data
-- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively.
-  It will use specified PIO instead of the peripheral function pin for the USART feature.
-  If unsure, don't specify this property.
-- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO
-  capable USARTs.
-- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt
-
-<chip> compatible description:
-- at91rm9200:  legacy USART support
-- at91sam9260: generic USART implementation for SAM9 SoCs
-
-Example:
-- use PDC:
-	usart0: serial@fff8c000 {
-		compatible = "atmel,at91sam9260-usart";
-		reg = <0xfff8c000 0x4000>;
-		interrupts = <7>;
-		clocks = <&usart0_clk>;
-		clock-names = "usart";
-		atmel,use-dma-rx;
-		atmel,use-dma-tx;
-		rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>;
-		cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>;
-		dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>;
-		dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>;
-		dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>;
-		rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>;
-	};
-
-- use DMA:
-	usart0: serial@f001c000 {
-		compatible = "atmel,at91sam9260-usart";
-		reg = <0xf001c000 0x100>;
-		interrupts = <12 4 5>;
-		clocks = <&usart0_clk>;
-		clock-names = "usart";
-		atmel,use-dma-rx;
-		atmel,use-dma-tx;
-		dmas = <&dma0 2 0x3>,
-		       <&dma0 2 0x204>;
-		dma-names = "tx", "rx";
-		atmel,fifo-size = <32>;
-	};
-
-- SPI mode:
-	#include <dt-bindings/mfd/at91-usart.h>
-
-	spi0: spi@f001c000 {
-		#address-cells = <1>;
-		#size-cells = <0>;
-		compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart";
-		atmel,usart-mode = <AT91_USART_MODE_SPI>;
-		reg = <0xf001c000 0x100>;
-		interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
-		clocks = <&usart0_clk>;
-		clock-names = "usart";
-		dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
-		       <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
-		dma-names = "tx", "rx";
-		cs-gpios = <&pioB 3 0>;
-	};
diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
new file mode 100644
index 000000000000..b25535b7a4d2
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
@@ -0,0 +1,183 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/atmel,at91-usart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
+
+maintainers:
+  - Richard Genoud <richard.genoud@gmail.com>
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - atmel,at91rm9200-usart
+          - atmel,at91sam9260-usart
+          - microchip,sam9x60-usart
+      - items:
+          - const: atmel,at91rm9200-dbgu
+          - const: atmel,at91rm9200-usart
+      - items:
+          - const: atmel,at91sam9260-dbgu
+          - const: atmel,at91sam9260-usart
+      - items:
+          - const: microchip,sam9x60-dbgu
+          - const: microchip,sam9x60-usart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clock-names:
+    const: usart
+
+  clocks:
+    maxItems: 1
+
+  dmas:
+    items:
+      - description: TX DMA Channel
+      - description: RX DMA Channel
+
+  dma-names:
+    items:
+      - const: tx
+      - const: rx
+
+  atmel,usart-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Must be either <AT91_USART_MODE_SPI> for SPI or
+      <AT91_USART_MODE_SERIAL> for USART (found in dt-bindings/mfd/at91-usart.h).
+    enum: [ 0, 1 ]
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clock-names
+  - clocks
+
+allOf:
+  - if:
+      properties:
+        $nodename:
+          pattern: "^serial@[0-9a-f]+$"
+    then:
+      allOf:
+        - $ref: /schemas/serial/serial.yaml#
+        - $ref: /schemas/serial/rs485.yaml#
+
+      properties:
+        atmel,use-dma-rx:
+          type: boolean
+          description: use of PDC or DMA for receiving data
+
+        atmel,use-dma-tx:
+          type: boolean
+          description: use of PDC or DMA for transmitting data
+
+        atmel,fifo-size:
+          $ref: /schemas/types.yaml#/definitions/uint32
+          description:
+            Maximum number of data the RX and TX FIFOs can store for FIFO
+            capable USARTS.
+          enum: [ 16, 32 ]
+
+    else:
+      if:
+        properties:
+          $nodename:
+            pattern: "^spi@[0-9a-f]+$"
+      then:
+        allOf:
+          - $ref: /schemas/spi/spi-controller.yaml#
+
+        properties:
+          atmel,usart-mode:
+            const: 1
+
+          "#size-cells":
+            const: 0
+
+          "#address-cells":
+            const: 1
+
+        required:
+          - atmel,usart-mode
+          - "#size-cells"
+          - "#address-cells"
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/mfd/at91-usart.h>
+    #include <dt-bindings/dma/at91.h>
+
+    /* use PDC */
+    usart0: serial@fff8c000 {
+        compatible = "atmel,at91sam9260-usart";
+        reg = <0xfff8c000 0x4000>;
+        interrupts = <7>;
+        clocks = <&usart0_clk>;
+        clock-names = "usart";
+        atmel,use-dma-rx;
+        atmel,use-dma-tx;
+        rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>;
+        cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>;
+        dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>;
+        dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>;
+        dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>;
+        rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>;
+    };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/mfd/at91-usart.h>
+    #include <dt-bindings/dma/at91.h>
+
+    /* use DMA */
+    usart1: serial@f001c000 {
+        compatible = "atmel,at91sam9260-usart";
+        reg = <0xf001c000 0x100>;
+        interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
+        clocks = <&usart0_clk>;
+        clock-names = "usart";
+        atmel,use-dma-rx;
+        atmel,use-dma-tx;
+        dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
+               <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
+        dma-names = "tx", "rx";
+        atmel,fifo-size = <32>;
+    };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/mfd/at91-usart.h>
+    #include <dt-bindings/dma/at91.h>
+
+    /* SPI mode */
+    spi0: spi@f001c000 {
+        compatible = "atmel,at91sam9260-usart";
+        reg = <0xf001c000 0x100>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        atmel,usart-mode = <AT91_USART_MODE_SPI>;
+        interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
+        clocks = <&usart0_clk>;
+        clock-names = "usart";
+        dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
+               <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
+        dma-names = "tx", "rx";
+        cs-gpios = <&pioB 3 GPIO_ACTIVE_HIGH>;
+    };
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

[Krzysztof Kozlowski]

On 06/09/2022 15:55, Sergiu Moga wrote:
> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
> json-schema format. Furthermore, move this binding to the
> serial directory, since binding directories match hardware,
> unlike the driver subsystems which match Linux convention.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> 
> v1 -> v2:
> - only do what the commit says, split the addition of other compatibles and
> properties in other patches
> - remove unnecessary "|"'s
> - mention header in `atmel,usart-mode`'s description
> - place `if:` under `allOf:`
> - respect order of spi0's DT properties: compatible, then reg then the reset of properties
> 
> 
> 
> 
> 
>  .../devicetree/bindings/mfd/atmel-usart.txt   |  98 ----------
>  .../bindings/serial/atmel,at91-usart.yaml     | 183 ++++++++++++++++++
>  2 files changed, 183 insertions(+), 98 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>  create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt
> deleted file mode 100644
> index a09133066aff..000000000000
> --- a/Documentation/devicetree/bindings/mfd/atmel-usart.txt
> +++ /dev/null
> @@ -1,98 +0,0 @@
> -* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
> -
> -Required properties for USART:
> -- compatible: Should be one of the following:
> -	- "atmel,at91rm9200-usart"
> -	- "atmel,at91sam9260-usart"
> -	- "microchip,sam9x60-usart"
> -	- "atmel,at91rm9200-dbgu", "atmel,at91rm9200-usart"
> -	- "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart"
> -	- "microchip,sam9x60-dbgu", "microchip,sam9x60-usart"
> -- reg: Should contain registers location and length
> -- interrupts: Should contain interrupt
> -- clock-names: tuple listing input clock names.
> -	Required elements: "usart"
> -- clocks: phandles to input clocks.
> -
> -Required properties for USART in SPI mode:
> -- #size-cells      : Must be <0>
> -- #address-cells   : Must be <1>
> -- cs-gpios: chipselects (internal cs not supported)
> -- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h)
> -
> -Optional properties in serial and SPI mode:
> -- dma bindings for dma transfer:
> -	- dmas: DMA specifier, consisting of a phandle to DMA controller node,
> -		memory peripheral interface and USART DMA channel ID, FIFO configuration.
> -		The order of DMA channels is fixed. The first DMA channel must be TX
> -		associated channel and the second one must be RX associated channel.
> -		Refer to dma.txt and atmel-dma.txt for details.
> -	- dma-names: "tx" for TX channel.
> -		     "rx" for RX channel.
> -		     The order of dma-names is also fixed. The first name must be "tx"
> -		     and the second one must be "rx" as in the examples below.
> -
> -Optional properties in serial mode:
> -- atmel,use-dma-rx: use of PDC or DMA for receiving data
> -- atmel,use-dma-tx: use of PDC or DMA for transmitting data
> -- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively.
> -  It will use specified PIO instead of the peripheral function pin for the USART feature.
> -  If unsure, don't specify this property.
> -- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO
> -  capable USARTs.
> -- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt
> -
> -<chip> compatible description:
> -- at91rm9200:  legacy USART support
> -- at91sam9260: generic USART implementation for SAM9 SoCs
> -
> -Example:
> -- use PDC:
> -	usart0: serial@fff8c000 {
> -		compatible = "atmel,at91sam9260-usart";
> -		reg = <0xfff8c000 0x4000>;
> -		interrupts = <7>;
> -		clocks = <&usart0_clk>;
> -		clock-names = "usart";
> -		atmel,use-dma-rx;
> -		atmel,use-dma-tx;
> -		rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>;
> -		cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>;
> -		dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>;
> -		dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>;
> -		dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>;
> -		rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>;
> -	};
> -
> -- use DMA:
> -	usart0: serial@f001c000 {
> -		compatible = "atmel,at91sam9260-usart";
> -		reg = <0xf001c000 0x100>;
> -		interrupts = <12 4 5>;
> -		clocks = <&usart0_clk>;
> -		clock-names = "usart";
> -		atmel,use-dma-rx;
> -		atmel,use-dma-tx;
> -		dmas = <&dma0 2 0x3>,
> -		       <&dma0 2 0x204>;
> -		dma-names = "tx", "rx";
> -		atmel,fifo-size = <32>;
> -	};
> -
> -- SPI mode:
> -	#include <dt-bindings/mfd/at91-usart.h>
> -
> -	spi0: spi@f001c000 {
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -		compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart";
> -		atmel,usart-mode = <AT91_USART_MODE_SPI>;
> -		reg = <0xf001c000 0x100>;
> -		interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
> -		clocks = <&usart0_clk>;
> -		clock-names = "usart";
> -		dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
> -		       <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
> -		dma-names = "tx", "rx";
> -		cs-gpios = <&pioB 3 0>;
> -	};
> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> new file mode 100644
> index 000000000000..b25535b7a4d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> @@ -0,0 +1,183 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/atmel,at91-usart.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
> +
> +maintainers:
> +  - Richard Genoud <richard.genoud@gmail.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - atmel,at91rm9200-usart
> +          - atmel,at91sam9260-usart
> +          - microchip,sam9x60-usart
> +      - items:
> +          - const: atmel,at91rm9200-dbgu
> +          - const: atmel,at91rm9200-usart
> +      - items:
> +          - const: atmel,at91sam9260-dbgu
> +          - const: atmel,at91sam9260-usart
> +      - items:
> +          - const: microchip,sam9x60-dbgu
> +          - const: microchip,sam9x60-usart
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: usart
> +
> +  clocks:
> +    maxItems: 1
> +
> +  dmas:
> +    items:
> +      - description: TX DMA Channel
> +      - description: RX DMA Channel
> +
> +  dma-names:
> +    items:
> +      - const: tx
> +      - const: rx
> +
> +  atmel,usart-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Must be either <AT91_USART_MODE_SPI> for SPI or
> +      <AT91_USART_MODE_SERIAL> for USART (found in dt-bindings/mfd/at91-usart.h).
> +    enum: [ 0, 1 ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-names
> +  - clocks
> +
> +allOf:
> +  - if:
> +      properties:
> +        $nodename:
> +          pattern: "^serial@[0-9a-f]+$"

You should rather check value of atmel,usart-mode, because now you won't
properly match device nodes called "foobar". Since usart-mode has only
two possible values, this will nicely simplify you if-else.


> +    then:
> +      allOf:
> +        - $ref: /schemas/serial/serial.yaml#
> +        - $ref: /schemas/serial/rs485.yaml#
> +
> +      properties:
> +        atmel,use-dma-rx:
> +          type: boolean
> +          description: use of PDC or DMA for receiving data
> +
> +        atmel,use-dma-tx:
> +          type: boolean
> +          description: use of PDC or DMA for transmitting data
> +
> +        atmel,fifo-size:
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          description:
> +            Maximum number of data the RX and TX FIFOs can store for FIFO
> +            capable USARTS.
> +          enum: [ 16, 32 ]

I did not mention it last time, but I think it should follow generic
practice, so define all properties top-level and disallow them for other
type. This allows you to simply use additionalProperties:false at the end.

> +
> +    else:
> +      if:
> +        properties:
> +          $nodename:
> +            pattern: "^spi@[0-9a-f]+$"
> +      then:
> +        allOf:
> +          - $ref: /schemas/spi/spi-controller.yaml#
> +
> +        properties:
> +          atmel,usart-mode:
> +            const: 1
> +
> +          "#size-cells":
> +            const: 0
> +
> +          "#address-cells":
> +            const: 1

The same - top level and disallow them for uart.

> +
> +        required:
> +          - atmel,usart-mode
> +          - "#size-cells"
> +          - "#address-cells"

End else in this branch is what?

> +
> +unevaluatedProperties: false
> +

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

[Sergiu.Moga]

On 08.09.2022 15:29, Krzysztof Kozlowski wrote:
> On 06/09/2022 15:55, Sergiu Moga wrote:
>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>> json-schema format. Furthermore, move this binding to the
>> serial directory, since binding directories match hardware,
>> unlike the driver subsystems which match Linux convention.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>>
>> v1 -> v2:
>> - only do what the commit says, split the addition of other compatibles and
>> properties in other patches
>> - remove unnecessary "|"'s
>> - mention header in `atmel,usart-mode`'s description
>> - place `if:` under `allOf:`
>> - respect order of spi0's DT properties: compatible, then reg then the reset of properties
>>
>>
>>
>>
>>
>>   .../devicetree/bindings/mfd/atmel-usart.txt   |  98 ----------
>>   .../bindings/serial/atmel,at91-usart.yaml     | 183 ++++++++++++++++++
>>   2 files changed, 183 insertions(+), 98 deletions(-)
>>   delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>   create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt
>> deleted file mode 100644
>> index a09133066aff..000000000000
>> --- a/Documentation/devicetree/bindings/mfd/atmel-usart.txt
>> +++ /dev/null
>> @@ -1,98 +0,0 @@
>> -* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>> -
>> -Required properties for USART:
>> -- compatible: Should be one of the following:
>> -     - "atmel,at91rm9200-usart"
>> -     - "atmel,at91sam9260-usart"
>> -     - "microchip,sam9x60-usart"
>> -     - "atmel,at91rm9200-dbgu", "atmel,at91rm9200-usart"
>> -     - "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart"
>> -     - "microchip,sam9x60-dbgu", "microchip,sam9x60-usart"
>> -- reg: Should contain registers location and length
>> -- interrupts: Should contain interrupt
>> -- clock-names: tuple listing input clock names.
>> -     Required elements: "usart"
>> -- clocks: phandles to input clocks.
>> -
>> -Required properties for USART in SPI mode:
>> -- #size-cells      : Must be <0>
>> -- #address-cells   : Must be <1>
>> -- cs-gpios: chipselects (internal cs not supported)
>> -- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h)
>> -
>> -Optional properties in serial and SPI mode:
>> -- dma bindings for dma transfer:
>> -     - dmas: DMA specifier, consisting of a phandle to DMA controller node,
>> -             memory peripheral interface and USART DMA channel ID, FIFO configuration.
>> -             The order of DMA channels is fixed. The first DMA channel must be TX
>> -             associated channel and the second one must be RX associated channel.
>> -             Refer to dma.txt and atmel-dma.txt for details.
>> -     - dma-names: "tx" for TX channel.
>> -                  "rx" for RX channel.
>> -                  The order of dma-names is also fixed. The first name must be "tx"
>> -                  and the second one must be "rx" as in the examples below.
>> -
>> -Optional properties in serial mode:
>> -- atmel,use-dma-rx: use of PDC or DMA for receiving data
>> -- atmel,use-dma-tx: use of PDC or DMA for transmitting data
>> -- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively.
>> -  It will use specified PIO instead of the peripheral function pin for the USART feature.
>> -  If unsure, don't specify this property.
>> -- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO
>> -  capable USARTs.
>> -- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt
>> -
>> -<chip> compatible description:
>> -- at91rm9200:  legacy USART support
>> -- at91sam9260: generic USART implementation for SAM9 SoCs
>> -
>> -Example:
>> -- use PDC:
>> -     usart0: serial@fff8c000 {
>> -             compatible = "atmel,at91sam9260-usart";
>> -             reg = <0xfff8c000 0x4000>;
>> -             interrupts = <7>;
>> -             clocks = <&usart0_clk>;
>> -             clock-names = "usart";
>> -             atmel,use-dma-rx;
>> -             atmel,use-dma-tx;
>> -             rts-gpios = <&pioD 15 GPIO_ACTIVE_LOW>;
>> -             cts-gpios = <&pioD 16 GPIO_ACTIVE_LOW>;
>> -             dtr-gpios = <&pioD 17 GPIO_ACTIVE_LOW>;
>> -             dsr-gpios = <&pioD 18 GPIO_ACTIVE_LOW>;
>> -             dcd-gpios = <&pioD 20 GPIO_ACTIVE_LOW>;
>> -             rng-gpios = <&pioD 19 GPIO_ACTIVE_LOW>;
>> -     };
>> -
>> -- use DMA:
>> -     usart0: serial@f001c000 {
>> -             compatible = "atmel,at91sam9260-usart";
>> -             reg = <0xf001c000 0x100>;
>> -             interrupts = <12 4 5>;
>> -             clocks = <&usart0_clk>;
>> -             clock-names = "usart";
>> -             atmel,use-dma-rx;
>> -             atmel,use-dma-tx;
>> -             dmas = <&dma0 2 0x3>,
>> -                    <&dma0 2 0x204>;
>> -             dma-names = "tx", "rx";
>> -             atmel,fifo-size = <32>;
>> -     };
>> -
>> -- SPI mode:
>> -     #include <dt-bindings/mfd/at91-usart.h>
>> -
>> -     spi0: spi@f001c000 {
>> -             #address-cells = <1>;
>> -             #size-cells = <0>;
>> -             compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart";
>> -             atmel,usart-mode = <AT91_USART_MODE_SPI>;
>> -             reg = <0xf001c000 0x100>;
>> -             interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
>> -             clocks = <&usart0_clk>;
>> -             clock-names = "usart";
>> -             dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(3)>,
>> -                    <&dma0 2 (AT91_DMA_CFG_PER_ID(4) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
>> -             dma-names = "tx", "rx";
>> -             cs-gpios = <&pioB 3 0>;
>> -     };
>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> new file mode 100644
>> index 000000000000..b25535b7a4d2
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> @@ -0,0 +1,183 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2022 Microchip Technology, Inc. and its subsidiaries
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/serial/atmel,at91-usart.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>> +
>> +maintainers:
>> +  - Richard Genoud <richard.genoud@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - enum:
>> +          - atmel,at91rm9200-usart
>> +          - atmel,at91sam9260-usart
>> +          - microchip,sam9x60-usart
>> +      - items:
>> +          - const: atmel,at91rm9200-dbgu
>> +          - const: atmel,at91rm9200-usart
>> +      - items:
>> +          - const: atmel,at91sam9260-dbgu
>> +          - const: atmel,at91sam9260-usart
>> +      - items:
>> +          - const: microchip,sam9x60-dbgu
>> +          - const: microchip,sam9x60-usart
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: usart
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  dmas:
>> +    items:
>> +      - description: TX DMA Channel
>> +      - description: RX DMA Channel
>> +
>> +  dma-names:
>> +    items:
>> +      - const: tx
>> +      - const: rx
>> +
>> +  atmel,usart-mode:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Must be either <AT91_USART_MODE_SPI> for SPI or
>> +      <AT91_USART_MODE_SERIAL> for USART (found in dt-bindings/mfd/at91-usart.h).
>> +    enum: [ 0, 1 ]
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clock-names
>> +  - clocks
>> +
>> +allOf:
>> +  - if:
>> +      properties:
>> +        $nodename:
>> +          pattern: "^serial@[0-9a-f]+$"
> 
> You should rather check value of atmel,usart-mode, because now you won't
> properly match device nodes called "foobar". Since usart-mode has only
> two possible values, this will nicely simplify you if-else.
> 
> 


I did think of that but the previous binding specifies that 
atmel,usart-mode is required only for the SPI mode and it is optional 
for the USART mode. That is why I went for the node's regex since I 
thought it is something that both nodes would have.


>> +    then:
>> +      allOf:
>> +        - $ref: /schemas/serial/serial.yaml#
>> +        - $ref: /schemas/serial/rs485.yaml#
>> +
>> +      properties:
>> +        atmel,use-dma-rx:
>> +          type: boolean
>> +          description: use of PDC or DMA for receiving data
>> +
>> +        atmel,use-dma-tx:
>> +          type: boolean
>> +          description: use of PDC or DMA for transmitting data
>> +
>> +        atmel,fifo-size:
>> +          $ref: /schemas/types.yaml#/definitions/uint32
>> +          description:
>> +            Maximum number of data the RX and TX FIFOs can store for FIFO
>> +            capable USARTS.
>> +          enum: [ 16, 32 ]
> 
> I did not mention it last time, but I think it should follow generic
> practice, so define all properties top-level and disallow them for other
> type. This allows you to simply use additionalProperties:false at the end.
> 


What would be a good example binding in this case?


>> +
>> +    else:
>> +      if:
>> +        properties:
>> +          $nodename:
>> +            pattern: "^spi@[0-9a-f]+$"
>> +      then:
>> +        allOf:
>> +          - $ref: /schemas/spi/spi-controller.yaml#
>> +
>> +        properties:
>> +          atmel,usart-mode:
>> +            const: 1
>> +
>> +          "#size-cells":
>> +            const: 0
>> +
>> +          "#address-cells":
>> +            const: 1
> 
> The same - top level and disallow them for uart.
> 


These values of #size-cells and #address-cells are only meant for the 
SPI so I guess I would still have to specify their mandatory const 
values here.


>> +
>> +        required:
>> +          - atmel,usart-mode
>> +          - "#size-cells"
>> +          - "#address-cells"
> 
> End else in this branch is what?
> 


You are right, I will remove the useless if: after else:


>> +
>> +unevaluatedProperties: false
>> +
> 
> Best regards,
> Krzysztof


Regards,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

[Krzysztof Kozlowski]

On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote:
> On 08.09.2022 15:29, Krzysztof Kozlowski wrote:

>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clock-names
>>> +  - clocks
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        $nodename:
>>> +          pattern: "^serial@[0-9a-f]+$"
>>
>> You should rather check value of atmel,usart-mode, because now you won't
>> properly match device nodes called "foobar". Since usart-mode has only
>> two possible values, this will nicely simplify you if-else.
>>
>>
> 
> 
> I did think of that but the previous binding specifies that 
> atmel,usart-mode is required only for the SPI mode and it is optional 
> for the USART mode. That is why I went for the node's regex since I 
> thought it is something that both nodes would have.

I think it should be explicit - you configure node either to this or
that, so the property should be always present. The node name should not
be responsible for it, even though we want node names to match certain
patterns.

> 
> 
>>> +    then:
>>> +      allOf:
>>> +        - $ref: /schemas/serial/serial.yaml#
>>> +        - $ref: /schemas/serial/rs485.yaml#
>>> +
>>> +      properties:
>>> +        atmel,use-dma-rx:
>>> +          type: boolean
>>> +          description: use of PDC or DMA for receiving data
>>> +
>>> +        atmel,use-dma-tx:
>>> +          type: boolean
>>> +          description: use of PDC or DMA for transmitting data
>>> +
>>> +        atmel,fifo-size:
>>> +          $ref: /schemas/types.yaml#/definitions/uint32
>>> +          description:
>>> +            Maximum number of data the RX and TX FIFOs can store for FIFO
>>> +            capable USARTS.
>>> +          enum: [ 16, 32 ]
>>
>> I did not mention it last time, but I think it should follow generic
>> practice, so define all properties top-level and disallow them for other
>> type. This allows you to simply use additionalProperties:false at the end.
>>
> 
> 
> What would be a good example binding in this case?

The example binding.

https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212

> 
> 
>>> +
>>> +    else:
>>> +      if:
>>> +        properties:
>>> +          $nodename:
>>> +            pattern: "^spi@[0-9a-f]+$"
>>> +      then:
>>> +        allOf:
>>> +          - $ref: /schemas/spi/spi-controller.yaml#
>>> +
>>> +        properties:
>>> +          atmel,usart-mode:
>>> +            const: 1
>>> +
>>> +          "#size-cells":
>>> +            const: 0
>>> +
>>> +          "#address-cells":
>>> +            const: 1
>>
>> The same - top level and disallow them for uart.
>>
> 
> 
> These values of #size-cells and #address-cells are only meant for the 
> SPI so I guess I would still have to specify their mandatory const 
> values here.

Sure, ok.

> 
> 
>>> +
>>> +        required:
>>> +          - atmel,usart-mode
>>> +          - "#size-cells"
>>> +          - "#address-cells"
>>
>> End else in this branch is what?
>>
> 
> 
> You are right, I will remove the useless if: after else:

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

[Sergiu.Moga]

On 08.09.2022 18:10, Krzysztof Kozlowski wrote:
> On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote:
>> On 08.09.2022 15:29, Krzysztof Kozlowski wrote:
> 
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - interrupts
>>>> +  - clock-names
>>>> +  - clocks
>>>> +
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        $nodename:
>>>> +          pattern: "^serial@[0-9a-f]+$"
>>>
>>> You should rather check value of atmel,usart-mode, because now you won't
>>> properly match device nodes called "foobar". Since usart-mode has only
>>> two possible values, this will nicely simplify you if-else.
>>>
>>>
>>
>>
>> I did think of that but the previous binding specifies that
>> atmel,usart-mode is required only for the SPI mode and it is optional
>> for the USART mode. That is why I went for the node's regex since I
>> thought it is something that both nodes would have.
> 
> I think it should be explicit - you configure node either to this or
> that, so the property should be always present.



No DT of ours has that property atm, since they are all on USART mode by 
default. If I were to make it required. all nodes would fail so I would 
have to add it to each of them.




> The node name should not
> be responsible for it, even though we want node names to match certain
> patterns.
> 


Does checkig for the node's pattern not make it better then? Since it 
imposes an additional check? If it would not have a conventional 
pattern, it would fail through unevaluatedProperies:false at the end, 
since it would have properties that were contained inside a branch that 
the validation of the node would not have gone through since it contains 
a pattern that does not match the conditions of that branch.


>>
>>
>>>> +    then:
>>>> +      allOf:
>>>> +        - $ref: /schemas/serial/serial.yaml#
>>>> +        - $ref: /schemas/serial/rs485.yaml#
>>>> +
>>>> +      properties:
>>>> +        atmel,use-dma-rx:
>>>> +          type: boolean
>>>> +          description: use of PDC or DMA for receiving data
>>>> +
>>>> +        atmel,use-dma-tx:
>>>> +          type: boolean
>>>> +          description: use of PDC or DMA for transmitting data
>>>> +
>>>> +        atmel,fifo-size:
>>>> +          $ref: /schemas/types.yaml#/definitions/uint32
>>>> +          description:
>>>> +            Maximum number of data the RX and TX FIFOs can store for FIFO
>>>> +            capable USARTS.
>>>> +          enum: [ 16, 32 ]
>>>
>>> I did not mention it last time, but I think it should follow generic
>>> practice, so define all properties top-level and disallow them for other
>>> type. This allows you to simply use additionalProperties:false at the end.
>>>
>>
>>
>> What would be a good example binding in this case?
> 
> The example binding.
> 
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
> 


Ah, I understand now. I did not get what you meant by "disallow", I 
guess it's just a "property-name: false".
Thanks!


>>
>>
>>>> +
>>>> +    else:
>>>> +      if:
>>>> +        properties:
>>>> +          $nodename:
>>>> +            pattern: "^spi@[0-9a-f]+$"
>>>> +      then:
>>>> +        allOf:
>>>> +          - $ref: /schemas/spi/spi-controller.yaml#
>>>> +
>>>> +        properties:
>>>> +          atmel,usart-mode:
>>>> +            const: 1
>>>> +
>>>> +          "#size-cells":
>>>> +            const: 0
>>>> +
>>>> +          "#address-cells":
>>>> +            const: 1
>>>
>>> The same - top level and disallow them for uart.
>>>
>>
>>
>> These values of #size-cells and #address-cells are only meant for the
>> SPI so I guess I would still have to specify their mandatory const
>> values here.
> 
> Sure, ok.
> 
>>
>>
>>>> +
>>>> +        required:
>>>> +          - atmel,usart-mode
>>>> +          - "#size-cells"
>>>> +          - "#address-cells"
>>>
>>> End else in this branch is what?
>>>
>>
>>
>> You are right, I will remove the useless if: after else:
> 
> Best regards,
> Krzysztof


Regards,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 05/13] dt-bindings: serial: atmel,at91-usart: convert to json-schema

[Krzysztof Kozlowski]

On 08/09/2022 17:27, Sergiu.Moga@microchip.com wrote:
> On 08.09.2022 18:10, Krzysztof Kozlowski wrote:
>> On 08/09/2022 17:06, Sergiu.Moga@microchip.com wrote:
>>> On 08.09.2022 15:29, Krzysztof Kozlowski wrote:
>>
>>>>> +required:
>>>>> +  - compatible
>>>>> +  - reg
>>>>> +  - interrupts
>>>>> +  - clock-names
>>>>> +  - clocks
>>>>> +
>>>>> +allOf:
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        $nodename:
>>>>> +          pattern: "^serial@[0-9a-f]+$"
>>>>
>>>> You should rather check value of atmel,usart-mode, because now you won't
>>>> properly match device nodes called "foobar". Since usart-mode has only
>>>> two possible values, this will nicely simplify you if-else.
>>>>
>>>>
>>>
>>>
>>> I did think of that but the previous binding specifies that
>>> atmel,usart-mode is required only for the SPI mode and it is optional
>>> for the USART mode. That is why I went for the node's regex since I
>>> thought it is something that both nodes would have.
>>
>> I think it should be explicit - you configure node either to this or
>> that, so the property should be always present.
> 
> 
> 
> No DT of ours has that property atm, since they are all on USART mode by 
> default. If I were to make it required. all nodes would fail so I would 
> have to add it to each of them.

Which is a problem because...?

Have in mind that bindings can be changed. ABI here won't be broken.

> 
> 
> 
> 
>> The node name should not
>> be responsible for it, even though we want node names to match certain
>> patterns.
>>
> 
> 
> Does checkig for the node's pattern not make it better then? Since it 
> imposes an additional check? 

Not really, because if it is "foobar" your schema would not be applied
correctly.

> If it would not have a conventional 
> pattern, it would fail through unevaluatedProperies:false at the end, 
> since it would have properties that were contained inside a branch that 
> the validation of the node would not have gone through since it contains 
> a pattern that does not match the conditions of that branch.

Not for properties which are for example missing...


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Sergiu Moga]

Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
in order to highlight the incremental characteristics of the SAM9X60
serial IP.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- Nothing, this patch was not here before


 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
index b25535b7a4d2..4d80006963c7 100644
--- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
+++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
@@ -26,6 +26,8 @@ properties:
       - items:
           - const: microchip,sam9x60-dbgu
           - const: microchip,sam9x60-usart
+          - const: atmel,at91sam9260-dbgu
+          - const: atmel,at91sam9260-usart
 
   reg:
     maxItems: 1
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Krzysztof Kozlowski]

On 06/09/2022 15:55, Sergiu Moga wrote:
> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
> in order to highlight the incremental characteristics of the SAM9X60
> serial IP.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> v1 -> v2:
> - Nothing, this patch was not here before
> 
> 
>  Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> index b25535b7a4d2..4d80006963c7 100644
> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> @@ -26,6 +26,8 @@ properties:
>        - items:
>            - const: microchip,sam9x60-dbgu
>            - const: microchip,sam9x60-usart
> +          - const: atmel,at91sam9260-dbgu
> +          - const: atmel,at91sam9260-usart

This is weird. You say in commit msg to "highlight the incremental
characteristics" but you basically change here existing compatibles.
This is not enum, but a list.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Sergiu.Moga]

On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
> On 06/09/2022 15:55, Sergiu Moga wrote:
>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>> in order to highlight the incremental characteristics of the SAM9X60
>> serial IP.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
>>
>>
>>   Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> index b25535b7a4d2..4d80006963c7 100644
>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> @@ -26,6 +26,8 @@ properties:
>>         - items:
>>             - const: microchip,sam9x60-dbgu
>>             - const: microchip,sam9x60-usart
>> +          - const: atmel,at91sam9260-dbgu
>> +          - const: atmel,at91sam9260-usart
> 
> This is weird. You say in commit msg to "highlight the incremental
> characteristics" but you basically change here existing compatibles.


Does "show that they are incremental IP's" sound better then?


> This is not enum, but a list.
> 


What do you mean by this? I know it is a list, I specified so in the 
commit message.


> 
> Best regards,
> Krzysztof


Regards,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Rob Herring]

On Thu, Sep 08, 2022 at 03:15:44PM +0000, Sergiu.Moga@microchip.com wrote:
> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
> > On 06/09/2022 15:55, Sergiu Moga wrote:
> >> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
> >> in order to highlight the incremental characteristics of the SAM9X60
> >> serial IP.
> >>
> >> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> >> ---
> >>
> >>
> >> v1 -> v2:
> >> - Nothing, this patch was not here before
> >>
> >>
> >>   Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> index b25535b7a4d2..4d80006963c7 100644
> >> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> @@ -26,6 +26,8 @@ properties:
> >>         - items:
> >>             - const: microchip,sam9x60-dbgu
> >>             - const: microchip,sam9x60-usart
> >> +          - const: atmel,at91sam9260-dbgu
> >> +          - const: atmel,at91sam9260-usart
> > 
> > This is weird. You say in commit msg to "highlight the incremental
> > characteristics" but you basically change here existing compatibles.
> 
> 
> Does "show that they are incremental IP's" sound better then?
> 
> 
> > This is not enum, but a list.
> > 
> 
> 
> What do you mean by this? I know it is a list, I specified so in the 
> commit message.

You are saying that compatible must be exactly the 4 strings above in 
the order listed. You need another entry with another 'items' list.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Sergiu.Moga]

On 09.09.2022 04:36, Rob Herring wrote:
> On Thu, Sep 08, 2022 at 03:15:44PM +0000, Sergiu.Moga@microchip.com wrote:
>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>> serial IP.
>>>>
>>>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>>>> ---
>>>>
>>>>
>>>> v1 -> v2:
>>>> - Nothing, this patch was not here before
>>>>
>>>>
>>>>    Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>> index b25535b7a4d2..4d80006963c7 100644
>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>> @@ -26,6 +26,8 @@ properties:
>>>>          - items:
>>>>              - const: microchip,sam9x60-dbgu
>>>>              - const: microchip,sam9x60-usart
>>>> +          - const: atmel,at91sam9260-dbgu
>>>> +          - const: atmel,at91sam9260-usart
>>>
>>> This is weird. You say in commit msg to "highlight the incremental
>>> characteristics" but you basically change here existing compatibles.
>>
>>
>> Does "show that they are incremental IP's" sound better then?
>>
>>
>>> This is not enum, but a list.
>>>
>>
>>
>> What do you mean by this? I know it is a list, I specified so in the
>> commit message.
> 
> You are saying that compatible must be exactly the 4 strings above in
> the order listed. You need another entry with another 'items' list.
> 
> Rob


That is what was intended though: a list of the 4 compatibles in that 
exact order. The 4th patch of this series also ensures that all 9x60 
nodes have that exact list of 4 compatibles.

Regards,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Krzysztof Kozlowski]

On 12/09/2022 09:45, Sergiu.Moga@microchip.com wrote:
> On 09.09.2022 04:36, Rob Herring wrote:
>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, Sergiu.Moga@microchip.com wrote:
>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>> serial IP.
>>>>>
>>>>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>>>>> ---
>>>>>
>>>>>
>>>>> v1 -> v2:
>>>>> - Nothing, this patch was not here before
>>>>>
>>>>>
>>>>>    Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>          - items:
>>>>>              - const: microchip,sam9x60-dbgu
>>>>>              - const: microchip,sam9x60-usart
>>>>> +          - const: atmel,at91sam9260-dbgu
>>>>> +          - const: atmel,at91sam9260-usart
>>>>
>>>> This is weird. You say in commit msg to "highlight the incremental
>>>> characteristics" but you basically change here existing compatibles.
>>>
>>>
>>> Does "show that they are incremental IP's" sound better then?
>>>
>>>
>>>> This is not enum, but a list.
>>>>
>>>
>>>
>>> What do you mean by this? I know it is a list, I specified so in the
>>> commit message.
>>
>> You are saying that compatible must be exactly the 4 strings above in
>> the order listed. You need another entry with another 'items' list.
>>
>> Rob
> 
> 
> That is what was intended though: a list of the 4 compatibles in that 
> exact order. The 4th patch of this series also ensures that all 9x60 
> nodes have that exact list of 4 compatibles.

The commit msg suggest otherwise - two options, because it is
incremental... But this one is not really incremental - you require this
one, only one, configuration. It's in general fine, but commit msg
should reflect what you are really intend to do here and why you are
doing it.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Sergiu.Moga]

On 12.09.2022 13:44, Krzysztof Kozlowski wrote:
> On 12/09/2022 09:45, Sergiu.Moga@microchip.com wrote:
>> On 09.09.2022 04:36, Rob Herring wrote:
>>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, Sergiu.Moga@microchip.com wrote:
>>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>>> serial IP.
>>>>>>
>>>>>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>>>>>> ---
>>>>>>
>>>>>>
>>>>>> v1 -> v2:
>>>>>> - Nothing, this patch was not here before
>>>>>>
>>>>>>
>>>>>>     Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>>           - items:
>>>>>>               - const: microchip,sam9x60-dbgu
>>>>>>               - const: microchip,sam9x60-usart
>>>>>> +          - const: atmel,at91sam9260-dbgu
>>>>>> +          - const: atmel,at91sam9260-usart
>>>>>
>>>>> This is weird. You say in commit msg to "highlight the incremental
>>>>> characteristics" but you basically change here existing compatibles.
>>>>
>>>>
>>>> Does "show that they are incremental IP's" sound better then?
>>>>
>>>>
>>>>> This is not enum, but a list.
>>>>>
>>>>
>>>>
>>>> What do you mean by this? I know it is a list, I specified so in the
>>>> commit message.
>>>
>>> You are saying that compatible must be exactly the 4 strings above in
>>> the order listed. You need another entry with another 'items' list.
>>>
>>> Rob
>>
>>
>> That is what was intended though: a list of the 4 compatibles in that
>> exact order. The 4th patch of this series also ensures that all 9x60
>> nodes have that exact list of 4 compatibles.
> 
> The commit msg suggest otherwise - two options, because it is
> incremental... But this one is not really incremental - you require this
> one, only one, configuration. It's in general fine, but commit msg
> should reflect what you are really intend to do here and why you are
> doing it.
> 
> 
> Best regards,
> Krzysztof


My apologies, I still do not understand what is wrong with the commit 
message. My intention was to ensure that every 9x60 usart compatible is 
followed by the 9260 compatibles because the 9x60 serial IP is an 
improvement over the 9260 one. Would you prefer it to be just the first 
part of the commit message: `Add the AT91SAM9260 serial compatibles to 
the list of SAM9X60 compatibles`? That way it would really only be what 
the commit does.

Thank you,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Krzysztof Kozlowski]

On 12/09/2022 15:09, Sergiu.Moga@microchip.com wrote:
> On 12.09.2022 13:44, Krzysztof Kozlowski wrote:
>> On 12/09/2022 09:45, Sergiu.Moga@microchip.com wrote:
>>> On 09.09.2022 04:36, Rob Herring wrote:
>>>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, Sergiu.Moga@microchip.com wrote:
>>>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>>>> serial IP.
>>>>>>>
>>>>>>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>> v1 -> v2:
>>>>>>> - Nothing, this patch was not here before
>>>>>>>
>>>>>>>
>>>>>>>     Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>>>     1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>>>           - items:
>>>>>>>               - const: microchip,sam9x60-dbgu
>>>>>>>               - const: microchip,sam9x60-usart
>>>>>>> +          - const: atmel,at91sam9260-dbgu
>>>>>>> +          - const: atmel,at91sam9260-usart
>>>>>>
>>>>>> This is weird. You say in commit msg to "highlight the incremental
>>>>>> characteristics" but you basically change here existing compatibles.
>>>>>
>>>>>
>>>>> Does "show that they are incremental IP's" sound better then?
>>>>>
>>>>>
>>>>>> This is not enum, but a list.
>>>>>>
>>>>>
>>>>>
>>>>> What do you mean by this? I know it is a list, I specified so in the
>>>>> commit message.
>>>>
>>>> You are saying that compatible must be exactly the 4 strings above in
>>>> the order listed. You need another entry with another 'items' list.
>>>>
>>>> Rob
>>>
>>>
>>> That is what was intended though: a list of the 4 compatibles in that
>>> exact order. The 4th patch of this series also ensures that all 9x60
>>> nodes have that exact list of 4 compatibles.
>>
>> The commit msg suggest otherwise - two options, because it is
>> incremental... But this one is not really incremental - you require this
>> one, only one, configuration. It's in general fine, but commit msg
>> should reflect what you are really intend to do here and why you are
>> doing it.
>>
>>
>> Best regards,
>> Krzysztof
> 
> 
> My apologies, I still do not understand what is wrong with the commit 
> message. My intention was to ensure that every 9x60 usart compatible is 
> followed by the 9260 compatibles because the 9x60 serial IP is an 
> improvement over the 9260 one. Would you prefer it to be just the first 
> part of the commit message: `Add the AT91SAM9260 serial compatibles to 
> the list of SAM9X60 compatibles`? That way it would really only be what 
> the commit does.

Let me rephrase it:

What your commit is doing is requiring additional fallback compatibles.
Therefore the commit msg should answer - why do you require additional
fallback compatibles?

Incremental characteristics sound to me optional. I can increment
sam9x60 with something or I can skip it. But you are not doing it...
sam9x60 was already there and now you require a fallback.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Sergiu.Moga]

On 13.09.2022 11:58, Krzysztof Kozlowski wrote:
> On 12/09/2022 15:09, Sergiu.Moga@microchip.com wrote:
>> On 12.09.2022 13:44, Krzysztof Kozlowski wrote:
>>> On 12/09/2022 09:45, Sergiu.Moga@microchip.com wrote:
>>>> On 09.09.2022 04:36, Rob Herring wrote:
>>>>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, Sergiu.Moga@microchip.com wrote:
>>>>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>>>>> serial IP.
>>>>>>>>
>>>>>>>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>
>>>>>>>> v1 -> v2:
>>>>>>>> - Nothing, this patch was not here before
>>>>>>>>
>>>>>>>>
>>>>>>>>      Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>>>>      1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>>>>            - items:
>>>>>>>>                - const: microchip,sam9x60-dbgu
>>>>>>>>                - const: microchip,sam9x60-usart
>>>>>>>> +          - const: atmel,at91sam9260-dbgu
>>>>>>>> +          - const: atmel,at91sam9260-usart
>>>>>>>
>>>>>>> This is weird. You say in commit msg to "highlight the incremental
>>>>>>> characteristics" but you basically change here existing compatibles.
>>>>>>
>>>>>>
>>>>>> Does "show that they are incremental IP's" sound better then?
>>>>>>
>>>>>>
>>>>>>> This is not enum, but a list.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> What do you mean by this? I know it is a list, I specified so in the
>>>>>> commit message.
>>>>>
>>>>> You are saying that compatible must be exactly the 4 strings above in
>>>>> the order listed. You need another entry with another 'items' list.
>>>>>
>>>>> Rob
>>>>
>>>>
>>>> That is what was intended though: a list of the 4 compatibles in that
>>>> exact order. The 4th patch of this series also ensures that all 9x60
>>>> nodes have that exact list of 4 compatibles.
>>>
>>> The commit msg suggest otherwise - two options, because it is
>>> incremental... But this one is not really incremental - you require this
>>> one, only one, configuration. It's in general fine, but commit msg
>>> should reflect what you are really intend to do here and why you are
>>> doing it.
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>
>>
>> My apologies, I still do not understand what is wrong with the commit
>> message. My intention was to ensure that every 9x60 usart compatible is
>> followed by the 9260 compatibles because the 9x60 serial IP is an
>> improvement over the 9260 one. Would you prefer it to be just the first
>> part of the commit message: `Add the AT91SAM9260 serial compatibles to
>> the list of SAM9X60 compatibles`? That way it would really only be what
>> the commit does.
> 
> Let me rephrase it:
> 
> What your commit is doing is requiring additional fallback compatibles.
> Therefore the commit msg should answer - why do you require additional
> fallback compatibles?
> 


The additional fallback compatibles are required because the driver in 
question only knows about the atmel,at91sam9260-usart compatible. 
Furthermore, it is also a better representation of the fact that the 
serial IP of 9x60 is an improvement over the serial IP of 9260 (it 
contains more hardware features not yet implemented in the driver).


> Incremental characteristics sound to me optional. I can increment
> sam9x60 with something or I can skip it. But you are not doing it...
> sam9x60 was already there and now you require a fallback.
> 
> Best regards,
> Krzysztof

So, what is your opinion on the following commit message:

"Fix sam9x60 compatible list by adding the sam9260 compatibles as 
fallback, since the atmel_serial driver only knows of the latter's 
compatible. The atmel_serial driver only has knowledge of the sam9260 
compatible because it does not have the sam9x60's serial IP specific 
features implemented yet and adding an empty compatible without adding 
support specific to that compatible would be misleading. Thus prefer the 
fallback mechanism in the detriment of adding an empty compatible in the 
driver."

Thanks,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Krzysztof Kozlowski]

On 13/09/2022 11:19, Sergiu.Moga@microchip.com wrote:
>>
>> Let me rephrase it:
>>
>> What your commit is doing is requiring additional fallback compatibles.
>> Therefore the commit msg should answer - why do you require additional
>> fallback compatibles?
>>
> 
> 
> The additional fallback compatibles are required because the driver in 
> question only knows about the atmel,at91sam9260-usart compatible. 
> Furthermore, it is also a better representation of the fact that the 
> serial IP of 9x60 is an improvement over the serial IP of 9260 (it 
> contains more hardware features not yet implemented in the driver).
> 
> 
>> Incremental characteristics sound to me optional. I can increment
>> sam9x60 with something or I can skip it. But you are not doing it...
>> sam9x60 was already there and now you require a fallback.
>>
>> Best regards,
>> Krzysztof
> 
> So, what is your opinion on the following commit message:
> 
> "Fix sam9x60 compatible list by adding the sam9260 compatibles as 
> fallback, since the atmel_serial driver only knows of the latter's 
> compatible. The atmel_serial driver only has knowledge of the sam9260 
> compatible because it does not have the sam9x60's serial IP specific 
> features implemented yet and adding an empty compatible without adding 
> support specific to that compatible would be misleading. Thus prefer the 
> fallback mechanism in the detriment of adding an empty compatible in the 
> driver."

It's fine. Also could work:

"Require sam9260 fallback compatible for sam9x60, because sam9x60 is
fully compatible with sam9260 and Linux driver requires the latter."

If it fixes any observable issue like lack of driver binding to DTS, you
can also mention that.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/13] dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to SAM9x60

[Sergiu.Moga]

On 13.09.2022 12:30, Krzysztof Kozlowski wrote:
> On 13/09/2022 11:19, Sergiu.Moga@microchip.com wrote:
>>>
>>> Let me rephrase it:
>>>
>>> What your commit is doing is requiring additional fallback compatibles.
>>> Therefore the commit msg should answer - why do you require additional
>>> fallback compatibles?
>>>
>>
>>
>> The additional fallback compatibles are required because the driver in
>> question only knows about the atmel,at91sam9260-usart compatible.
>> Furthermore, it is also a better representation of the fact that the
>> serial IP of 9x60 is an improvement over the serial IP of 9260 (it
>> contains more hardware features not yet implemented in the driver).
>>
>>
>>> Incremental characteristics sound to me optional. I can increment
>>> sam9x60 with something or I can skip it. But you are not doing it...
>>> sam9x60 was already there and now you require a fallback.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> So, what is your opinion on the following commit message:
>>
>> "Fix sam9x60 compatible list by adding the sam9260 compatibles as
>> fallback, since the atmel_serial driver only knows of the latter's
>> compatible. The atmel_serial driver only has knowledge of the sam9260
>> compatible because it does not have the sam9x60's serial IP specific
>> features implemented yet and adding an empty compatible without adding
>> support specific to that compatible would be misleading. Thus prefer the
>> fallback mechanism in the detriment of adding an empty compatible in the
>> driver."
> 
> It's fine. Also could work:
> 
> "Require sam9260 fallback compatible for sam9x60, because sam9x60 is
> fully compatible with sam9260 and Linux driver requires the latter."
> 


This version looks better indeed. Sums it all up and is only 2 lines :). 
Thank you very much for the suggestion it is greatly appeciated.


> If it fixes any observable issue like lack of driver binding to DTS, you
> can also mention that.
> 
> Best regards,
> Krzysztof


Thanks,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 07/13] dt-bindings: mfd: atmel,sama5d2-flexcom: Add USART child node ref binding

[Sergiu Moga]

FLEXCOM, among other functionalities, has the ability to offer the USART
serial communication protocol. To have the FLEXCOM binding properly
validate its USART children nodes, we must reference the correct binding.
To differentiate between the SPI of FLEXCOM and the SPI of USART in SPI
mode, use the clock-names property, since the latter's respective
property is supposed to contain the "usart" string.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- Nothing



 .../bindings/mfd/atmel,sama5d2-flexcom.yaml      | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
index 0db0f2728b65..b5fb509f07ec 100644
--- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
+++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
@@ -72,13 +72,21 @@ properties:
 
 patternProperties:
   "^serial@[0-9a-f]+$":
-    type: object
+    $ref: /schemas/serial/atmel,at91-usart.yaml
     description:
-      Child node describing USART. See atmel-usart.txt for details
-      of USART bindings.
+      Child node describing USART.
 
   "^spi@[0-9a-f]+$":
-    $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
+    allOf:
+      - if:
+          properties:
+            clock-names:
+              contains:
+                const: usart
+        then:
+          $ref: /schemas/serial/atmel,at91-usart.yaml
+        else:
+          $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
     description:
       Child node describing SPI.
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 07/13] dt-bindings: mfd: atmel,sama5d2-flexcom: Add USART child node ref binding

[Krzysztof Kozlowski]

On 06/09/2022 15:55, Sergiu Moga wrote:
> FLEXCOM, among other functionalities, has the ability to offer the USART
> serial communication protocol. To have the FLEXCOM binding properly
> validate its USART children nodes, we must reference the correct binding.
> To differentiate between the SPI of FLEXCOM and the SPI of USART in SPI
> mode, use the clock-names property, since the latter's respective
> property is supposed to contain the "usart" string.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> v1 -> v2:
> - Nothing
> 
> 
> 
>  .../bindings/mfd/atmel,sama5d2-flexcom.yaml      | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
> index 0db0f2728b65..b5fb509f07ec 100644
> --- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
> +++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
> @@ -72,13 +72,21 @@ properties:
>  
>  patternProperties:
>    "^serial@[0-9a-f]+$":
> -    type: object
> +    $ref: /schemas/serial/atmel,at91-usart.yaml
>      description:
> -      Child node describing USART. See atmel-usart.txt for details
> -      of USART bindings.
> +      Child node describing USART.
>  
>    "^spi@[0-9a-f]+$":
> -    $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
> +    allOf:
> +      - if:
> +          properties:
> +            clock-names:
> +              contains:
> +                const: usart

Devices are not different because they have or have not clock. Devices
are different... because they are simply different models, so this
should be different compatible.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 08/13] tty: serial: atmel: Define GCLK as USART baudrate source clock

[Sergiu Moga]

Define the bit that represents the choice of having GCLK as a baudrate
source clock inside the USCLKS bitmask of the Mode Register of
USART IP's.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- Nothing, this patch was not here before



 drivers/tty/serial/atmel_serial.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index 0d8a0f9cc5c3..70d0611e56fd 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -49,6 +49,7 @@
 #define	ATMEL_US_USCLKS		GENMASK(5, 4)	/* Clock Selection */
 #define		ATMEL_US_USCLKS_MCK		(0 <<  4)
 #define		ATMEL_US_USCLKS_MCK_DIV8	(1 <<  4)
+#define		ATMEL_US_USCLKS_GCLK		(2 <<  4)
 #define		ATMEL_US_USCLKS_SCK		(3 <<  4)
 #define	ATMEL_US_CHRL		GENMASK(7, 6)	/* Character Length */
 #define		ATMEL_US_CHRL_5			(0 <<  6)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 08/13] tty: serial: atmel: Define GCLK as USART baudrate source clock

[Ilpo Järvinen]

On Tue, 6 Sep 2022, Sergiu Moga wrote:

> Define the bit that represents the choice of having GCLK as a baudrate
> source clock inside the USCLKS bitmask of the Mode Register of
> USART IP's.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> v1 -> v2:
> - Nothing, this patch was not here before
> 
> 
> 
>  drivers/tty/serial/atmel_serial.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 0d8a0f9cc5c3..70d0611e56fd 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -49,6 +49,7 @@
>  #define	ATMEL_US_USCLKS		GENMASK(5, 4)	/* Clock Selection */
>  #define		ATMEL_US_USCLKS_MCK		(0 <<  4)
>  #define		ATMEL_US_USCLKS_MCK_DIV8	(1 <<  4)
> +#define		ATMEL_US_USCLKS_GCLK		(2 <<  4)

This would be FIELD_PREP(ATMEL_US_USCLKS, 2) from linux/bitfield.h.

They should all be converted to use FIELD_PREP(), IMHO (in a separate 
patch).

>  #define		ATMEL_US_USCLKS_SCK		(3 <<  4)
>  #define	ATMEL_US_CHRL		GENMASK(7, 6)	/* Character Length */
>  #define		ATMEL_US_CHRL_5			(0 <<  6)
> 

-- 
 i.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 08/13] tty: serial: atmel: Define GCLK as USART baudrate source clock

[Sergiu.Moga]

On 07.09.2022 12:21, Ilpo Järvinen wrote:
> On Tue, 6 Sep 2022, Sergiu Moga wrote:
> 
>> Define the bit that represents the choice of having GCLK as a baudrate
>> source clock inside the USCLKS bitmask of the Mode Register of
>> USART IP's.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
>>
>>
>>
>>   drivers/tty/serial/atmel_serial.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
>> index 0d8a0f9cc5c3..70d0611e56fd 100644
>> --- a/drivers/tty/serial/atmel_serial.h
>> +++ b/drivers/tty/serial/atmel_serial.h
>> @@ -49,6 +49,7 @@
>>   #define      ATMEL_US_USCLKS         GENMASK(5, 4)   /* Clock Selection */
>>   #define              ATMEL_US_USCLKS_MCK             (0 <<  4)
>>   #define              ATMEL_US_USCLKS_MCK_DIV8        (1 <<  4)
>> +#define              ATMEL_US_USCLKS_GCLK            (2 <<  4)
> 
> This would be FIELD_PREP(ATMEL_US_USCLKS, 2) from linux/bitfield.h.
> 
> They should all be converted to use FIELD_PREP(), IMHO (in a separate
> patch).
> 
>>   #define              ATMEL_US_USCLKS_SCK             (3 <<  4)
>>   #define      ATMEL_US_CHRL           GENMASK(7, 6)   /* Character Length */
>>   #define              ATMEL_US_CHRL_5                 (0 <<  6)
>>
> 
> --
>   i.
> 


Yes, I guess that is a good idea. After these additional 
macro-definitions are upstreamed, I will try to send some patches for 
the conversion.

Thanks,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 09/13] tty: serial: atmel: Define BRSRCCK bitmask of UART IP's Mode Register

[Sergiu Moga]

Add definitions for the Baud Rate Source Clock bitmask of the
Mode Register of UART IP's and its bitfields.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---



v1 -> v2:
- Nothing, this patch was not here before



 drivers/tty/serial/atmel_serial.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index 70d0611e56fd..ed64035ba6c3 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -68,6 +68,9 @@
 #define		ATMEL_US_NBSTOP_1		(0 << 12)
 #define		ATMEL_US_NBSTOP_1_5		(1 << 12)
 #define		ATMEL_US_NBSTOP_2		(2 << 12)
+#define	ATMEL_UA_BRSRCCK	GENMASK(13, 12)	/* Clock Selection for UART */
+#define		ATMEL_UA_BRSRCCK_PERIPH_CLK	(0 << 12)
+#define		ATMEL_UA_BRSRCCK_GCLK		(1 << 12)
 #define	ATMEL_US_CHMODE		GENMASK(15, 14)	/* Channel Mode */
 #define		ATMEL_US_CHMODE_NORMAL		(0 << 14)
 #define		ATMEL_US_CHMODE_ECHO		(1 << 14)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/13] tty: serial: atmel: Define BRSRCCK bitmask of UART IP's Mode Register

[Ilpo Järvinen]

On Tue, 6 Sep 2022, Sergiu Moga wrote:

> Add definitions for the Baud Rate Source Clock bitmask of the
> Mode Register of UART IP's and its bitfields.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> 
> v1 -> v2:
> - Nothing, this patch was not here before
> 
> 
> 
>  drivers/tty/serial/atmel_serial.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 70d0611e56fd..ed64035ba6c3 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -68,6 +68,9 @@
>  #define		ATMEL_US_NBSTOP_1		(0 << 12)
>  #define		ATMEL_US_NBSTOP_1_5		(1 << 12)
>  #define		ATMEL_US_NBSTOP_2		(2 << 12)
> +#define	ATMEL_UA_BRSRCCK	GENMASK(13, 12)	/* Clock Selection for UART */
> +#define		ATMEL_UA_BRSRCCK_PERIPH_CLK	(0 << 12)
> +#define		ATMEL_UA_BRSRCCK_GCLK		(1 << 12)

FIELD_PREP(ATMEL_UA_BRSRCCK, ...)

>  #define	ATMEL_US_CHMODE		GENMASK(15, 14)	/* Channel Mode */
>  #define		ATMEL_US_CHMODE_NORMAL		(0 << 14)
>  #define		ATMEL_US_CHMODE_ECHO		(1 << 14)
> 

-- 
 i.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 10/13] tty: serial: atmel: Only divide Clock Divisor if the IP is USART

[Sergiu Moga]

Make sure that the driver only divides the clock divisor if the
IP handled at that point is USART, since UART IP's do not support
implicit peripheral clock division. Instead, in the case of UART,
go with the highest possible clock divisor.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---


v1 -> v2:
- Nothing, this patch was not here before and is mainly meant as both cleanup
and as a way to introduce a new field into struct atmel_uart_port that will be
used by the last patch to diferentiate between USART and UART regarding the
location of the Baudrate Clock Source bitmask.




 drivers/tty/serial/atmel_serial.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7450d3853031..6aa01ca5489c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -150,6 +150,7 @@ struct atmel_uart_port {
 	u32			rts_low;
 	bool			ms_irq_enabled;
 	u32			rtor;	/* address of receiver timeout register if it exists */
+	bool			is_usart;
 	bool			has_frac_baudrate;
 	bool			has_hw_timer;
 	struct timer_list	uart_timer;
@@ -1825,6 +1826,7 @@ static void atmel_get_ip_name(struct uart_port *port)
 	 */
 	atmel_port->has_frac_baudrate = false;
 	atmel_port->has_hw_timer = false;
+	atmel_port->is_usart = false;
 
 	if (name == new_uart) {
 		dev_dbg(port->dev, "Uart with hw timer");
@@ -1834,6 +1836,7 @@ static void atmel_get_ip_name(struct uart_port *port)
 		dev_dbg(port->dev, "Usart\n");
 		atmel_port->has_frac_baudrate = true;
 		atmel_port->has_hw_timer = true;
+		atmel_port->is_usart = true;
 		atmel_port->rtor = ATMEL_US_RTOR;
 		version = atmel_uart_readl(port, ATMEL_US_VERSION);
 		switch (version) {
@@ -1863,6 +1866,7 @@ static void atmel_get_ip_name(struct uart_port *port)
 			dev_dbg(port->dev, "This version is usart\n");
 			atmel_port->has_frac_baudrate = true;
 			atmel_port->has_hw_timer = true;
+			atmel_port->is_usart = true;
 			atmel_port->rtor = ATMEL_US_RTOR;
 			break;
 		case 0x203:
@@ -2282,10 +2286,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		cd = uart_get_divisor(port, baud);
 	}
 
-	if (cd > 65535) {	/* BRGR is 16-bit, so switch to slower clock */
+	/*
+	 * BRGR is 16-bit, so switch to slower clock.
+	 * Otherwise, keep the highest possible value for the clock divisor.
+	 */
+	if (atmel_port->is_usart && cd > 65535) {
 		cd /= 8;
 		mode |= ATMEL_US_USCLKS_MCK_DIV8;
+	} else {
+		cd &= 65535;
 	}
+
 	quot = cd | fp << ATMEL_US_FP_OFFSET;
 
 	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 10/13] tty: serial: atmel: Only divide Clock Divisor if the IP is USART

[Ilpo Järvinen]

On Tue, 6 Sep 2022, Sergiu Moga wrote:

> Make sure that the driver only divides the clock divisor if the
> IP handled at that point is USART, since UART IP's do not support
> implicit peripheral clock division. Instead, in the case of UART,
> go with the highest possible clock divisor.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> v1 -> v2:
> - Nothing, this patch was not here before and is mainly meant as both cleanup
> and as a way to introduce a new field into struct atmel_uart_port that will be
> used by the last patch to diferentiate between USART and UART regarding the
> location of the Baudrate Clock Source bitmask.
> 
> 
> 
> 
>  drivers/tty/serial/atmel_serial.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7450d3853031..6aa01ca5489c 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -150,6 +150,7 @@ struct atmel_uart_port {
>  	u32			rts_low;
>  	bool			ms_irq_enabled;
>  	u32			rtor;	/* address of receiver timeout register if it exists */
> +	bool			is_usart;
>  	bool			has_frac_baudrate;
>  	bool			has_hw_timer;
>  	struct timer_list	uart_timer;
> @@ -1825,6 +1826,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>  	 */
>  	atmel_port->has_frac_baudrate = false;
>  	atmel_port->has_hw_timer = false;
> +	atmel_port->is_usart = false;
>  
>  	if (name == new_uart) {
>  		dev_dbg(port->dev, "Uart with hw timer");
> @@ -1834,6 +1836,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>  		dev_dbg(port->dev, "Usart\n");
>  		atmel_port->has_frac_baudrate = true;
>  		atmel_port->has_hw_timer = true;
> +		atmel_port->is_usart = true;
>  		atmel_port->rtor = ATMEL_US_RTOR;
>  		version = atmel_uart_readl(port, ATMEL_US_VERSION);
>  		switch (version) {
> @@ -1863,6 +1866,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>  			dev_dbg(port->dev, "This version is usart\n");
>  			atmel_port->has_frac_baudrate = true;
>  			atmel_port->has_hw_timer = true;
> +			atmel_port->is_usart = true;
>  			atmel_port->rtor = ATMEL_US_RTOR;
>  			break;
>  		case 0x203:
> @@ -2282,10 +2286,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  		cd = uart_get_divisor(port, baud);
>  	}
>  
> -	if (cd > 65535) {	/* BRGR is 16-bit, so switch to slower clock */
> +	/*
> +	 * BRGR is 16-bit, so switch to slower clock.
> +	 * Otherwise, keep the highest possible value for the clock divisor.
> +	 */
> +	if (atmel_port->is_usart && cd > 65535) {

Should this be cd > ATMEL_US_CD ?

>  		cd /= 8;
>  		mode |= ATMEL_US_USCLKS_MCK_DIV8;
> +	} else {
> +		cd &= 65535;

ATMEL_US_CD?

>  	}
> +
>  	quot = cd | fp << ATMEL_US_FP_OFFSET;
>  
>  	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> 

-- 
 i.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 10/13] tty: serial: atmel: Only divide Clock Divisor if the IP is USART

[Sergiu.Moga]

On 07.09.2022 12:23, Ilpo Järvinen wrote:
> On Tue, 6 Sep 2022, Sergiu Moga wrote:
> 
>> Make sure that the driver only divides the clock divisor if the
>> IP handled at that point is USART, since UART IP's do not support
>> implicit peripheral clock division. Instead, in the case of UART,
>> go with the highest possible clock divisor.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before and is mainly meant as both cleanup
>> and as a way to introduce a new field into struct atmel_uart_port that will be
>> used by the last patch to diferentiate between USART and UART regarding the
>> location of the Baudrate Clock Source bitmask.
>>
>>
>>
>>
>>   drivers/tty/serial/atmel_serial.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 7450d3853031..6aa01ca5489c 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -150,6 +150,7 @@ struct atmel_uart_port {
>>        u32                     rts_low;
>>        bool                    ms_irq_enabled;
>>        u32                     rtor;   /* address of receiver timeout register if it exists */
>> +     bool                    is_usart;
>>        bool                    has_frac_baudrate;
>>        bool                    has_hw_timer;
>>        struct timer_list       uart_timer;
>> @@ -1825,6 +1826,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>>         */
>>        atmel_port->has_frac_baudrate = false;
>>        atmel_port->has_hw_timer = false;
>> +     atmel_port->is_usart = false;
>>
>>        if (name == new_uart) {
>>                dev_dbg(port->dev, "Uart with hw timer");
>> @@ -1834,6 +1836,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>>                dev_dbg(port->dev, "Usart\n");
>>                atmel_port->has_frac_baudrate = true;
>>                atmel_port->has_hw_timer = true;
>> +             atmel_port->is_usart = true;
>>                atmel_port->rtor = ATMEL_US_RTOR;
>>                version = atmel_uart_readl(port, ATMEL_US_VERSION);
>>                switch (version) {
>> @@ -1863,6 +1866,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>>                        dev_dbg(port->dev, "This version is usart\n");
>>                        atmel_port->has_frac_baudrate = true;
>>                        atmel_port->has_hw_timer = true;
>> +                     atmel_port->is_usart = true;
>>                        atmel_port->rtor = ATMEL_US_RTOR;
>>                        break;
>>                case 0x203:
>> @@ -2282,10 +2286,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>                cd = uart_get_divisor(port, baud);
>>        }
>>
>> -     if (cd > 65535) {       /* BRGR is 16-bit, so switch to slower clock */
>> +     /*
>> +      * BRGR is 16-bit, so switch to slower clock.
>> +      * Otherwise, keep the highest possible value for the clock divisor.
>> +      */
>> +     if (atmel_port->is_usart && cd > 65535) {
> 
> Should this be cd > ATMEL_US_CD ?
> 
>>                cd /= 8;
>>                mode |= ATMEL_US_USCLKS_MCK_DIV8;
>> +     } else {
>> +             cd &= 65535;
> 
> ATMEL_US_CD?


Yes, it would seem that would be right. It did not cross my mind to 
check whether there already was something pre-defined or not since the 
code also previously used 65535.

> 
>>        }
>> +
>>        quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>>        if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>>
> 
> --
>   i.
> 


Thanks,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 11/13] clk: at91: sama5d2: Add Generic Clocks for UART/USART

[Sergiu Moga]

Add the generic clocks for UART/USART in the sama5d2 driver to allow them
to be registered in the Common Clock Framework.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---



v1 -> v2:
- Added R-b tag




 drivers/clk/at91/sama5d2.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/at91/sama5d2.c b/drivers/clk/at91/sama5d2.c
index cfd0f5e23b99..84156dc52bff 100644
--- a/drivers/clk/at91/sama5d2.c
+++ b/drivers/clk/at91/sama5d2.c
@@ -120,6 +120,16 @@ static const struct {
 	struct clk_range r;
 	int chg_pid;
 } sama5d2_gck[] = {
+	{ .n = "flx0_gclk",   .id = 19, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "flx1_gclk",   .id = 20, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "flx2_gclk",   .id = 21, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "flx3_gclk",   .id = 22, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "flx4_gclk",   .id = 23, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "uart0_gclk",  .id = 24, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "uart1_gclk",  .id = 25, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "uart2_gclk",  .id = 26, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "uart3_gclk",  .id = 27, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+	{ .n = "uart4_gclk",  .id = 28, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
 	{ .n = "sdmmc0_gclk", .id = 31, .chg_pid = INT_MIN, },
 	{ .n = "sdmmc1_gclk", .id = 32, .chg_pid = INT_MIN, },
 	{ .n = "tcb0_gclk",   .id = 35, .chg_pid = INT_MIN, .r = { .min = 0, .max = 83000000 }, },
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 12/13] tty: serial: atmel: Make the driver aware of the existence of GCLK

[Sergiu Moga]

Previously, the atmel serial driver did not take into account the
possibility of using the more customizable generic clock as its
baudrate generator. Unless there is a Fractional Part available to
increase accuracy, there is a high chance that we may be able to
generate a baudrate closer to the desired one by using the GCLK as the
clock source. Now, depending on the error rate between
the desired baudrate and the actual baudrate, the serial driver will
fallback on the generic clock. The generic clock must be provided
in the DT node of the serial that may need a more flexible clock source.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---



v1 -> v2:
- take into account the different placement of the baudrate clock source
into the IP's Mode Register (USART vs UART)
- don't check for atmel_port->gclk != NULL
- use clk_round_rate instead of clk_set_rate + clk_get_rate
- remove clk_disable_unprepare from the end of the probe method



 drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 6aa01ca5489c..b2b6fd6ea2a5 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/serial.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/sysrq.h>
 #include <linux/tty_flip.h>
@@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
 #endif
 
 #define ATMEL_ISR_PASS_LIMIT	256
+#define ERROR_RATE(desired_value, actual_value) \
+	((int)(100 - ((desired_value) * 100) / (actual_value)))
 
 struct atmel_dma_buffer {
 	unsigned char	*buf;
@@ -110,6 +113,7 @@ struct atmel_uart_char {
 struct atmel_uart_port {
 	struct uart_port	uart;		/* uart */
 	struct clk		*clk;		/* uart clock */
+	struct clk		*gclk;		/* uart generic clock */
 	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
 	u32			backup_imr;	/* IMR saved during suspend */
 	int			break_active;	/* break being received */
@@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
 		 * This is called on uart_close() or a suspend event.
 		 */
 		clk_disable_unprepare(atmel_port->clk);
+		if (__clk_is_enabled(atmel_port->gclk))
+			clk_disable_unprepare(atmel_port->gclk);
 		break;
 	default:
 		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
@@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 {
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 	unsigned long flags;
-	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
+	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
+	unsigned int baud, actual_baud, gclk_rate;
 
 	/* save the current mode register */
 	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
@@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
 		cd &= 65535;
 	}
 
+	/*
+	 * If there is no Fractional Part, there is a high chance that
+	 * we may be able to generate a baudrate closer to the desired one
+	 * if we use the GCLK as the clock source driving the baudrate
+	 * generator.
+	 */
+	if (!atmel_port->has_frac_baudrate) {
+		if (__clk_is_enabled(atmel_port->gclk))
+			clk_disable_unprepare(atmel_port->gclk);
+		gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
+		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
+		if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
+		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
+			clk_set_rate(atmel_port->gclk, 16 * baud);
+			if (!clk_prepare_enable(atmel_port->gclk)) {
+				if (atmel_port->is_usart) {
+					mode &= ~ATMEL_US_USCLKS;
+					mode |= ATMEL_US_USCLKS_GCLK;
+				} else {
+					mode &= ~ATMEL_UA_BRSRCCK;
+					mode |= ATMEL_UA_BRSRCCK_GCLK;
+				}
+
+				/*
+				 * Set the Clock Divisor for GCLK to 1.
+				 * Since we were able to generate the smallest
+				 * multiple of the desired baudrate times 16,
+				 * then we surely can generate a bigger multiple
+				 * with the exact error rate for an equally increased
+				 * CD. Thus no need to take into account
+				 * a higher value for CD.
+				 */
+				cd = 1;
+			}
+		}
+	}
+
 	quot = cd | fp << ATMEL_US_FP_OFFSET;
 
 	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
@@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
 	if (ret)
 		goto err;
 
+	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
+	if (IS_ERR(atmel_port->gclk)) {
+		ret = PTR_ERR(atmel_port->gclk);
+		goto err;
+	}
+
 	ret = atmel_init_port(atmel_port, pdev);
 	if (ret)
 		goto err_clk_disable_unprepare;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 12/13] tty: serial: atmel: Make the driver aware of the existence of GCLK

[Ilpo Järvinen]

On Tue, 6 Sep 2022, Sergiu Moga wrote:

> Previously, the atmel serial driver did not take into account the
> possibility of using the more customizable generic clock as its
> baudrate generator. Unless there is a Fractional Part available to
> increase accuracy, there is a high chance that we may be able to
> generate a baudrate closer to the desired one by using the GCLK as the
> clock source. Now, depending on the error rate between
> the desired baudrate and the actual baudrate, the serial driver will
> fallback on the generic clock. The generic clock must be provided
> in the DT node of the serial that may need a more flexible clock source.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> 
> v1 -> v2:
> - take into account the different placement of the baudrate clock source
> into the IP's Mode Register (USART vs UART)
> - don't check for atmel_port->gclk != NULL
> - use clk_round_rate instead of clk_set_rate + clk_get_rate
> - remove clk_disable_unprepare from the end of the probe method
> 
> 
> 
>  drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 6aa01ca5489c..b2b6fd6ea2a5 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -15,6 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/serial.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/console.h>
>  #include <linux/sysrq.h>
>  #include <linux/tty_flip.h>
> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>  #endif
>  
>  #define ATMEL_ISR_PASS_LIMIT	256
> +#define ERROR_RATE(desired_value, actual_value) \
> +	((int)(100 - ((desired_value) * 100) / (actual_value)))

Make a function instead.

Is percent accurate enough or would you perhaps want something slightly 
more accurate?

Given you've abs() at the caller side, the error rate could be 
underestimated, is underestimating OK?

-- 
 i.

>  struct atmel_dma_buffer {
>  	unsigned char	*buf;
> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>  struct atmel_uart_port {
>  	struct uart_port	uart;		/* uart */
>  	struct clk		*clk;		/* uart clock */
> +	struct clk		*gclk;		/* uart generic clock */
>  	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
>  	u32			backup_imr;	/* IMR saved during suspend */
>  	int			break_active;	/* break being received */
> @@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>  		 * This is called on uart_close() or a suspend event.
>  		 */
>  		clk_disable_unprepare(atmel_port->clk);
> +		if (__clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);
>  		break;
>  	default:
>  		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  {
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	unsigned long flags;
> -	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
> +	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
> +	unsigned int baud, actual_baud, gclk_rate;
>  
>  	/* save the current mode register */
>  	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  		cd &= 65535;
>  	}
>  
> +	/*
> +	 * If there is no Fractional Part, there is a high chance that
> +	 * we may be able to generate a baudrate closer to the desired one
> +	 * if we use the GCLK as the clock source driving the baudrate
> +	 * generator.
> +	 */
> +	if (!atmel_port->has_frac_baudrate) {
> +		if (__clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);
> +		gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
> +		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> +		if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
> +		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
> +			clk_set_rate(atmel_port->gclk, 16 * baud);
> +			if (!clk_prepare_enable(atmel_port->gclk)) {
> +				if (atmel_port->is_usart) {
> +					mode &= ~ATMEL_US_USCLKS;
> +					mode |= ATMEL_US_USCLKS_GCLK;
> +				} else {
> +					mode &= ~ATMEL_UA_BRSRCCK;
> +					mode |= ATMEL_UA_BRSRCCK_GCLK;
> +				}
> +
> +				/*
> +				 * Set the Clock Divisor for GCLK to 1.
> +				 * Since we were able to generate the smallest
> +				 * multiple of the desired baudrate times 16,
> +				 * then we surely can generate a bigger multiple
> +				 * with the exact error rate for an equally increased
> +				 * CD. Thus no need to take into account
> +				 * a higher value for CD.
> +				 */
> +				cd = 1;
> +			}
> +		}
> +	}
> +
>  	quot = cd | fp << ATMEL_US_FP_OFFSET;
>  
>  	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err;
>  
> +	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> +	if (IS_ERR(atmel_port->gclk)) {
> +		ret = PTR_ERR(atmel_port->gclk);
> +		goto err;
> +	}
> +
>  	ret = atmel_init_port(atmel_port, pdev);
>  	if (ret)
>  		goto err_clk_disable_unprepare;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 12/13] tty: serial: atmel: Make the driver aware of the existence of GCLK

[Sergiu.Moga]

On 07.09.2022 12:36, Ilpo Järvinen wrote:
> On Tue, 6 Sep 2022, Sergiu Moga wrote:
> 
>> Previously, the atmel serial driver did not take into account the
>> possibility of using the more customizable generic clock as its
>> baudrate generator. Unless there is a Fractional Part available to
>> increase accuracy, there is a high chance that we may be able to
>> generate a baudrate closer to the desired one by using the GCLK as the
>> clock source. Now, depending on the error rate between
>> the desired baudrate and the actual baudrate, the serial driver will
>> fallback on the generic clock. The generic clock must be provided
>> in the DT node of the serial that may need a more flexible clock source.
>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>>
>> v1 -> v2:
>> - take into account the different placement of the baudrate clock source
>> into the IP's Mode Register (USART vs UART)
>> - don't check for atmel_port->gclk != NULL
>> - use clk_round_rate instead of clk_set_rate + clk_get_rate
>> - remove clk_disable_unprepare from the end of the probe method
>>
>>
>>
>>   drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>>   1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 6aa01ca5489c..b2b6fd6ea2a5 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/init.h>
>>   #include <linux/serial.h>
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/sysrq.h>
>>   #include <linux/tty_flip.h>
>> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>>   #endif
>>
>>   #define ATMEL_ISR_PASS_LIMIT 256
>> +#define ERROR_RATE(desired_value, actual_value) \
>> +     ((int)(100 - ((desired_value) * 100) / (actual_value)))
> 
> Make a function instead.
> 


Alright, I see the point of making it an inline function instead.


> Is percent accurate enough or would you perhaps want something slightly
> more accurate?
> 


It is accurate enough for the all the baudrates I have tested. It 
usually taps into the GCLK whenever high baudrates such as 921600 are 
used. For 115200 for example, the error rate was slightly better in the 
case of the peripheral clock and it acted accordingly, choosing the 
latter as its baudrate source clock. I do not think that a higher 
accuracy than this would be needed though. Say that using percent 
accuracy yields that the error rates are equal, but the gclk would have 
been better in this case by, say, a few 10 ^ -4, but the code logic does 
not see it so it proceeds using the peripheral clock. In that case, the 
error rate of the peripheral clock would still be low enough relative to 
the desired baudrate for the communication to function properly.

The higher the baudrate, the lower the error rate must be in order for 
things to go smoothly. For example, for a baudrate of 57600 I noticed 
that even an error rate as big as 6% is still enough for the 
communication to work properly, while in the case of 921600 anything 
bigger than 2% and things do not go smoothly anymore. So I guess that it 
would be safe to say that, unless you go for baudrates as high as tens 
of millions, things should work well with just percent accuracy. A 
higher accuracy always definetely helps, but I believe it is not needed 
in this case.


> Given you've abs() at the caller side, the error rate could be
> underestimated, is underestimating OK?
> 


Yes, this should be fine. While (both empirically and after looking 
stuff up) I noticed that in the case of negative error rates, their 
absolute value needs to be smaller than the one of positive error rates, 
it must be so by a very small margin that is negligible when estimating 
through percent accuracy.


> --
>   i.
> 
>>   struct atmel_dma_buffer {
>>        unsigned char   *buf;
>> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>>   struct atmel_uart_port {
>>        struct uart_port        uart;           /* uart */
>>        struct clk              *clk;           /* uart clock */
>> +     struct clk              *gclk;          /* uart generic clock */
>>        int                     may_wakeup;     /* cached value of device_may_wakeup for times we need to disable it */
>>        u32                     backup_imr;     /* IMR saved during suspend */
>>        int                     break_active;   /* break being received */
>> @@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>>                 * This is called on uart_close() or a suspend event.
>>                 */
>>                clk_disable_unprepare(atmel_port->clk);
>> +             if (__clk_is_enabled(atmel_port->gclk))
>> +                     clk_disable_unprepare(atmel_port->gclk);
>>                break;
>>        default:
>>                dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>> @@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>   {
>>        struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>>        unsigned long flags;
>> -     unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
>> +     unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
>> +     unsigned int baud, actual_baud, gclk_rate;
>>
>>        /* save the current mode register */
>>        mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
>> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>                cd &= 65535;
>>        }
>>
>> +     /*
>> +      * If there is no Fractional Part, there is a high chance that
>> +      * we may be able to generate a baudrate closer to the desired one
>> +      * if we use the GCLK as the clock source driving the baudrate
>> +      * generator.
>> +      */
>> +     if (!atmel_port->has_frac_baudrate) {
>> +             if (__clk_is_enabled(atmel_port->gclk))
>> +                     clk_disable_unprepare(atmel_port->gclk);
>> +             gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
>> +             actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>> +             if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
>> +                 abs(ERROR_RATE(baud, gclk_rate / 16))) {
>> +                     clk_set_rate(atmel_port->gclk, 16 * baud);
>> +                     if (!clk_prepare_enable(atmel_port->gclk)) {
>> +                             if (atmel_port->is_usart) {
>> +                                     mode &= ~ATMEL_US_USCLKS;
>> +                                     mode |= ATMEL_US_USCLKS_GCLK;
>> +                             } else {
>> +                                     mode &= ~ATMEL_UA_BRSRCCK;
>> +                                     mode |= ATMEL_UA_BRSRCCK_GCLK;
>> +                             }
>> +
>> +                             /*
>> +                              * Set the Clock Divisor for GCLK to 1.
>> +                              * Since we were able to generate the smallest
>> +                              * multiple of the desired baudrate times 16,
>> +                              * then we surely can generate a bigger multiple
>> +                              * with the exact error rate for an equally increased
>> +                              * CD. Thus no need to take into account
>> +                              * a higher value for CD.
>> +                              */
>> +                             cd = 1;
>> +                     }
>> +             }
>> +     }
>> +
>>        quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>>        if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>> @@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>        if (ret)
>>                goto err;
>>
>> +     atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>> +     if (IS_ERR(atmel_port->gclk)) {
>> +             ret = PTR_ERR(atmel_port->gclk);
>> +             goto err;
>> +     }
>> +
>>        ret = atmel_init_port(atmel_port, pdev);
>>        if (ret)
>>                goto err_clk_disable_unprepare;
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 12/13] tty: serial: atmel: Make the driver aware of the existence of GCLK

[Ilpo Järvinen]

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

On Wed, 7 Sep 2022, Sergiu.Moga@microchip.com wrote:

> On 07.09.2022 12:36, Ilpo Järvinen wrote:
> > On Tue, 6 Sep 2022, Sergiu Moga wrote:
> > 
> >> Previously, the atmel serial driver did not take into account the
> >> possibility of using the more customizable generic clock as its
> >> baudrate generator. Unless there is a Fractional Part available to
> >> increase accuracy, there is a high chance that we may be able to
> >> generate a baudrate closer to the desired one by using the GCLK as the
> >> clock source. Now, depending on the error rate between
> >> the desired baudrate and the actual baudrate, the serial driver will
> >> fallback on the generic clock. The generic clock must be provided
> >> in the DT node of the serial that may need a more flexible clock source.
> >>
> >> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> >> ---

> > Is percent accurate enough or would you perhaps want something slightly
> > more accurate?
> > 
> 
> 
> It is accurate enough for the all the baudrates I have tested. It 
> usually taps into the GCLK whenever high baudrates such as 921600 are 
> used. For 115200 for example, the error rate was slightly better in the 
> case of the peripheral clock and it acted accordingly, choosing the 
> latter as its baudrate source clock. I do not think that a higher 
> accuracy than this would be needed though. Say that using percent 
> accuracy yields that the error rates are equal, but the gclk would have 
> been better in this case by, say, a few 10 ^ -4, but the code logic does 
> not see it so it proceeds using the peripheral clock. In that case, the 
> error rate of the peripheral clock would still be low enough relative to 
> the desired baudrate for the communication to function properly.
> 
> The higher the baudrate, the lower the error rate must be in order for 
> things to go smoothly. For example, for a baudrate of 57600 I noticed 
> that even an error rate as big as 6% is still enough for the 
> communication to work properly, while in the case of 921600 anything 
> bigger than 2% and things do not go smoothly anymore. So I guess that it 
> would be safe to say that, unless you go for baudrates as high as tens 
> of millions, things should work well with just percent accuracy. A 
> higher accuracy always definetely helps, but I believe it is not needed 
> in this case.
> 
> 
> > Given you've abs() at the caller side, the error rate could be
> > underestimated, is underestimating OK?
> > 
> 
> 
> Yes, this should be fine. While (both empirically and after looking 
> stuff up) I noticed that in the case of negative error rates, their 
> absolute value needs to be smaller than the one of positive error rates, 
> it must be so by a very small margin that is negligible when estimating 
> through percent accuracy.

OK. Thanks for checking.


-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 12/13] tty: serial: atmel: Make the driver aware of the existence of GCLK

[Claudiu.Beznea]

On 06.09.2022 16:55, Sergiu Moga wrote:
> Previously, the atmel serial driver did not take into account the
> possibility of using the more customizable generic clock as its
> baudrate generator. Unless there is a Fractional Part available to
> increase accuracy, there is a high chance that we may be able to
> generate a baudrate closer to the desired one by using the GCLK as the
> clock source. Now, depending on the error rate between
> the desired baudrate and the actual baudrate, the serial driver will
> fallback on the generic clock. The generic clock must be provided
> in the DT node of the serial that may need a more flexible clock source.
> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> 
> v1 -> v2:
> - take into account the different placement of the baudrate clock source
> into the IP's Mode Register (USART vs UART)
> - don't check for atmel_port->gclk != NULL
> - use clk_round_rate instead of clk_set_rate + clk_get_rate
> - remove clk_disable_unprepare from the end of the probe method
> 
> 
> 
>  drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 6aa01ca5489c..b2b6fd6ea2a5 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -15,6 +15,7 @@
>  #include <linux/init.h>
>  #include <linux/serial.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/console.h>
>  #include <linux/sysrq.h>
>  #include <linux/tty_flip.h>
> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>  #endif
>  
>  #define ATMEL_ISR_PASS_LIMIT	256
> +#define ERROR_RATE(desired_value, actual_value) \
> +	((int)(100 - ((desired_value) * 100) / (actual_value)))
>  
>  struct atmel_dma_buffer {
>  	unsigned char	*buf;
> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>  struct atmel_uart_port {
>  	struct uart_port	uart;		/* uart */
>  	struct clk		*clk;		/* uart clock */
> +	struct clk		*gclk;		/* uart generic clock */
>  	int			may_wakeup;	/* cached value of device_may_wakeup for times we need to disable it */
>  	u32			backup_imr;	/* IMR saved during suspend */
>  	int			break_active;	/* break being received */
> @@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>  		 * This is called on uart_close() or a suspend event.
>  		 */
>  		clk_disable_unprepare(atmel_port->clk);
> +		if (__clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);
>  		break;
>  	default:
>  		dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  {
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  	unsigned long flags;
> -	unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
> +	unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
> +	unsigned int baud, actual_baud, gclk_rate;
>  
>  	/* save the current mode register */
>  	mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>  		cd &= 65535;
>  	}
>  
> +	/*
> +	 * If there is no Fractional Part, there is a high chance that
> +	 * we may be able to generate a baudrate closer to the desired one
> +	 * if we use the GCLK as the clock source driving the baudrate
> +	 * generator.
> +	 */
> +	if (!atmel_port->has_frac_baudrate) {
> +		if (__clk_is_enabled(atmel_port->gclk))
> +			clk_disable_unprepare(atmel_port->gclk);
> +		gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
> +		actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> +		if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
> +		    abs(ERROR_RATE(baud, gclk_rate / 16))) {
> +			clk_set_rate(atmel_port->gclk, 16 * baud);

This is a personal taste: I would do like this:
			ret = clk_prepare_enable();
			if (ret)
				goto clk_prepare_failure;

			// and here the rest of the code...

> +			if (!clk_prepare_enable(atmel_port->gclk)) {
> +				if (atmel_port->is_usart) {
> +					mode &= ~ATMEL_US_USCLKS;
> +					mode |= ATMEL_US_USCLKS_GCLK;
> +				} else {
> +					mode &= ~ATMEL_UA_BRSRCCK;
> +					mode |= ATMEL_UA_BRSRCCK_GCLK;
> +				}
> +
> +				/*
> +				 * Set the Clock Divisor for GCLK to 1.
> +				 * Since we were able to generate the smallest
> +				 * multiple of the desired baudrate times 16,
> +				 * then we surely can generate a bigger multiple
> +				 * with the exact error rate for an equally increased
> +				 * CD. Thus no need to take into account
> +				 * a higher value for CD.
> +				 */
> +				cd = 1;
> +			}
> +		}
> +	}
> +

clk_prepare_failure: // or other label name

>  	quot = cd | fp << ATMEL_US_FP_OFFSET;
>  
>  	if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err;
>  
> +	atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> +	if (IS_ERR(atmel_port->gclk)) {
> +		ret = PTR_ERR(atmel_port->gclk);
> +		goto err;

This should be:
		goto err_clk_disable_unprepare;

to disable the peripheral clock previously enabled.

> +	}
> +
>  	ret = atmel_init_port(atmel_port, pdev);
>  	if (ret)
>  		goto err_clk_disable_unprepare;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 13/13] dt-bindings: serial: atmel,at91-usart: Add gclk as a possible USART clock

[Sergiu Moga]

The Devicetree nodes for FLEXCOM's USART can also have an alternative
clock source for the baudrate generator (other than the peripheral
clock), namely the Generick Clock. Thus make the binding aware of
this potential clock that someone may place in the clock related
properties of the USART node.

Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
---



v1 -> v2:
- Nothing, this patch was not here before



 .../devicetree/bindings/serial/atmel,at91-usart.yaml   | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
index 4d80006963c7..db595b498bad 100644
--- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
+++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
@@ -36,10 +36,16 @@ properties:
     maxItems: 1
 
   clock-names:
-    const: usart
+    minItems: 1
+    items:
+      - const: usart
+      - const: gclk
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: USART Peripheral Clock
+      - description: USART Generic Clock
 
   dmas:
     items:
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 13/13] dt-bindings: serial: atmel,at91-usart: Add gclk as a possible USART clock

[Krzysztof Kozlowski]

On 06/09/2022 15:55, Sergiu Moga wrote:
> The Devicetree nodes for FLEXCOM's USART can also have an alternative
> clock source for the baudrate generator (other than the peripheral
> clock), namely the Generick Clock. Thus make the binding aware of
> this potential clock that someone may place in the clock related
> properties of the USART node.

Last sentence is confusing - what is the potential? Just skip it.

> 
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
> 
> 
> 
> v1 -> v2:
> - Nothing, this patch was not here before

You have confusing order of patches. Bindings mixed with DTS mixed with
drivers. Keep things ordered.
1. DTS changes needed for aligning to schema.
2. all bindings
3. rest

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 13/13] dt-bindings: serial: atmel,at91-usart: Add gclk as a possible USART clock

[Sergiu.Moga]

On 08.09.2022 15:35, Krzysztof Kozlowski wrote:
> On 06/09/2022 15:55, Sergiu Moga wrote:
>> The Devicetree nodes for FLEXCOM's USART can also have an alternative
>> clock source for the baudrate generator (other than the peripheral
>> clock), namely the Generick Clock. Thus make the binding aware of
>> this potential clock that someone may place in the clock related
>> properties of the USART node.
> 
> Last sentence is confusing - what is the potential? Just skip it.
> 


I am sorry, I meant to say "possible". No idea how I ended up writing 
"potential". Guess I will just skip any adjectives entirely.


>>
>> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
>> ---
>>
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
> 
> You have confusing order of patches. Bindings mixed with DTS mixed with
> drivers. Keep things ordered.
> 1. DTS changes needed for aligning to schema.
> 2. all bindings
> 3. rest
> 


Alright, it makes sense, will do so. I thought it would have looked 
better if I were to add the gclk in the schema after adding it in the 
drivers.


Other than that I hope I got the example[1] you have previously given me 
right.


[1] 
https://elixir.bootlin.com/linux/v6.0-rc4/source/Documentation/devicetree/bindings/example-schema.yaml#L91


> Best regards,
> Krzysztof


Thanks,
	Sergiu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel