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 and differentiate between the SPI of USART and the
SPI of FLEXCOM.
The first three patches of this patch series depend on this patch series
converting atmel-flexcom bindings to json-schema:
https://lore.kernel.org/all/[email protected]/
Sergiu Moga (5):
dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref
binding
dt-bindings: mfd: atmel,at91-usart: convert to json-schema
dt-bindings: mfd: atmel,sama5d2-flexcom: Add USART child node ref
binding
clk: at91: sama5d2: Add Generic Clocks for UART/USART
tty: serial: atmel: Make the driver aware of the existence of GCLK
.../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
.../bindings/mfd/atmel,sama5d2-flexcom.yaml | 18 +-
.../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
drivers/clk/at91/sama5d2.c | 10 +
drivers/tty/serial/atmel_serial.c | 52 ++++-
drivers/tty/serial/atmel_serial.h | 1 +
6 files changed, 264 insertions(+), 105 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
--
2.25.1
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 <[email protected]>
---
drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
drivers/tty/serial/atmel_serial.h | 1 +
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 30ba9eef7b39..0a0b46ee0955 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 */
@@ -2115,6 +2119,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 (atmel_port->gclk && __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);
@@ -2129,7 +2135,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);
@@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
cd /= 8;
mode |= ATMEL_US_USCLKS_MCK_DIV8;
}
+
+ /*
+ * 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 (!fp && atmel_port->gclk) {
+ if (__clk_is_enabled(atmel_port->gclk))
+ clk_disable_unprepare(atmel_port->gclk);
+ clk_set_rate(atmel_port->gclk, 16 * baud);
+ gclk_rate = clk_get_rate(atmel_port->gclk);
+ actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
+ if (abs(ERROR_RATE(baud, actual_baud)) >
+ abs(ERROR_RATE(baud, gclk_rate / 16))) {
+ mode |= ATMEL_US_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_enable(atmel_port->gclk);
+ }
+ }
+
quot = cd | fp << ATMEL_US_FP_OFFSET;
if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
@@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
if (ret)
goto err;
+ atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
+ if (atmel_port->gclk) {
+ ret = clk_prepare_enable(atmel_port->gclk);
+ if (ret) {
+ atmel_port->gclk = NULL;
+ return ret;
+ }
+ clk_disable_unprepare(atmel_port->gclk);
+ }
+
ret = atmel_init_port(atmel_port, pdev);
if (ret)
goto err_clk_disable_unprepare;
@@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
* is used
*/
clk_disable_unprepare(atmel_port->clk);
+ if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
+ clk_disable_unprepare(atmel_port->gclk);
return 0;
diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index 0d8a0f9cc5c3..fb718972f81a 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -63,6 +63,7 @@
#define ATMEL_US_PAR_MARK (3 << 9)
#define ATMEL_US_PAR_NONE (4 << 9)
#define ATMEL_US_PAR_MULTI_DROP (6 << 9)
+#define ATMEL_US_GCLK BIT(12)
#define ATMEL_US_NBSTOP GENMASK(13, 12) /* Number of Stop Bits */
#define ATMEL_US_NBSTOP_1 (0 << 12)
#define ATMEL_US_NBSTOP_1_5 (1 << 12)
--
2.25.1
Convert at91 USART DT Binding for Atmel/Microchip SoCs to
json-schema format.
Signed-off-by: Sergiu Moga <[email protected]>
---
.../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
.../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
2 files changed, 190 insertions(+), 98 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
new file mode 100644
index 000000000000..cf15d73fa1e8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
@@ -0,0 +1,190 @@
+# 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/mfd/atmel,at91-usart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
+
+maintainers:
+ - Richard Genoud <[email protected]>
+
+properties:
+ compatible:
+ oneOf:
+ - const: atmel,at91rm9200-usart
+ - const: atmel,at91sam9260-usart
+ - const: 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
+ - items:
+ - const: microchip,sam9x60-usart
+ - const: atmel,at91sam9260-usart
+ - items:
+ - const: microchip,sam9x60-dbgu
+ - const: microchip,sam9x60-usart
+ - const: atmel,at91sam9260-dbgu
+ - const: atmel,at91sam9260-usart
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clock-names:
+ contains:
+ const: usart
+
+ clocks:
+ minItems: 1
+ maxItems: 2
+
+ 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 1 for SPI or 0 for USART.
+ enum: [ 0, 1 ]
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clock-names
+ - clocks
+
+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 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "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 GPIO_ACTIVE_HIGH>;
+ };
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>;
- };
--
2.25.1
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 <[email protected]>
---
.../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..e158af47c326 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: ../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
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 <[email protected]>
---
.../bindings/mfd/atmel,sama5d2-flexcom.yaml | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
index e158af47c326..617331a5e66e 100644
--- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
+++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
@@ -72,13 +72,20 @@ properties:
patternProperties:
"^serial@[0-9a-f]+$":
- type: object
+ $ref: 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: ../spi/atmel,at91rm9200-spi.yaml
+ if:
+ properties:
+ clock-names:
+ contains:
+ const: usart
+ then:
+ $ref: atmel,at91-usart.yaml
+ else:
+ $ref: ../spi/atmel,at91rm9200-spi.yaml
description:
Child node describing SPI.
--
2.25.1
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 <[email protected]>
---
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
On 17/08/2022 10: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 <[email protected]>
> ---
> .../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..e158af47c326 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: ../spi/atmel,at91rm9200-spi.yaml
Full schemas path, so:
/schemas/spi/atmel....
Best regards,
Krzysztof
On 17/08/2022 10:55, Sergiu Moga wrote:
> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
> json-schema format.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
> 2 files changed, 190 insertions(+), 98 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> new file mode 100644
> index 000000000000..cf15d73fa1e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
One more thing - I think this should be in serial directory, not mfd,
even though it includes SPI. MFD is just a Linux naming/wrapper device.
Best regards,
Krzysztof
On 17/08/2022 10:55, Sergiu Moga wrote:
> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
> json-schema format.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
> 2 files changed, 190 insertions(+), 98 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> new file mode 100644
> index 000000000000..cf15d73fa1e8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> @@ -0,0 +1,190 @@
> +# 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/mfd/atmel,at91-usart.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
> +
> +maintainers:
> + - Richard Genoud <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
This looks quite different than bindings and you commit msg is saying it
is only a conversion. Mention any changes against original bindings.
> + - const: atmel,at91rm9200-usart
> + - const: atmel,at91sam9260-usart
> + - const: microchip,sam9x60-usart
That's an enum
> + - 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
> + - items:
> + - const: microchip,sam9x60-usart
> + - const: atmel,at91sam9260-usart
This is not correct - contradicts earlier one.
> + - items:
> + - const: microchip,sam9x60-dbgu
> + - const: microchip,sam9x60-usart
> + - const: atmel,at91sam9260-dbgu
> + - const: atmel,at91sam9260-usart
What? You wrote above that microchip,sam9x60-dbgu is compatible only
with microchip,sam9x60-usart. Now you write it is also compatible with
other ones?
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clock-names:
> + contains:
> + const: usart
No, this has to be specific/fixed list.
> +
> + clocks:
> + minItems: 1
> + maxItems: 2
Not really - define the items. One item could be optional, though.
> +
> + 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: |
No need for |
> + Must be either 1 for SPI or 0 for USART.
Mention the header.
> + enum: [ 0, 1 ]
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clock-names
> + - clocks
> +
> +if:
Put it under allOf.
> + 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: |
No need for |
> + 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 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "atmel,at91sam9260-usart";
> + atmel,usart-mode = <AT91_USART_MODE_SPI>;
> + reg = <0xf001c000 0x100>;
compatible, then reg then the reset of properties
> + 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>;
> + };
Best regards,
Krzysztof
On 18.08.2022 11:38, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 17/08/2022 10:55, Sergiu Moga wrote:
>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>> json-schema format.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>> 2 files changed, 190 insertions(+), 98 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> new file mode 100644
>> index 000000000000..cf15d73fa1e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> @@ -0,0 +1,190 @@
>> +# 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/mfd/atmel,at91-usart.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>> +
>> +maintainers:
>> + - Richard Genoud <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + oneOf:
> This looks quite different than bindings and you commit msg is saying it
> is only a conversion. Mention any changes against original bindings.
Noted. Will make the commit message more descriptive to also include the
new compatibles. Perhaps the addition of the GCLK should come under a
different patch as well.
>> + - const: atmel,at91rm9200-usart
>> + - const: atmel,at91sam9260-usart
>> + - const: microchip,sam9x60-usart
> That's an enum
>
>> + - 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
>> + - items:
>> + - const: microchip,sam9x60-usart
>> + - const: atmel,at91sam9260-usart
> This is not correct - contradicts earlier one.
>
Yes, this was for a DT node we have internally, my bad. You are right,
it does not really make sense and it should not be the other way around:
having the DT validate the binding. I will remove this combination in
the next version.
>> + - items:
>> + - const: microchip,sam9x60-dbgu
>> + - const: microchip,sam9x60-usart
>> + - const: atmel,at91sam9260-dbgu
>> + - const: atmel,at91sam9260-usart
> What? You wrote above that microchip,sam9x60-dbgu is compatible only
> with microchip,sam9x60-usart. Now you write it is also compatible with
> other ones?
Yes, this one is intended because the 9x60 IP has new functionalities on
top of 9260, and some nodes do keep all four as fallback.
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + clock-names:
>> + contains:
>> + const: usart
> No, this has to be specific/fixed list.
I wanted to highlight the fact that it must contain either:
clock-names = "usart";
or
clock-names = "usart", "gclk";
What would be the recommended way of doing this then?
>> +
>> + clocks:
>> + minItems: 1
>> + maxItems: 2
> Not really - define the items. One item could be optional, though.
>
>> +
>> + 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: |
> No need for |
>
>> + Must be either 1 for SPI or 0 for USART.
> Mention the header.
>
>> + enum: [ 0, 1 ]
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - clock-names
>> + - clocks
>> +
>> +if:
> Put it under allOf.
>
>> + 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: |
> No need for |
>
>> + 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 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "atmel,at91sam9260-usart";
>> + atmel,usart-mode = <AT91_USART_MODE_SPI>;
>> + reg = <0xf001c000 0x100>;
> compatible, then reg then the reset of properties
>
>> + 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>;
>> + };
> Best regards,
> Krzysztof
Regards,
Sergiu
On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 17/08/2022 10:55, Sergiu Moga wrote:
>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>> json-schema format.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>> 2 files changed, 190 insertions(+), 98 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> new file mode 100644
>> index 000000000000..cf15d73fa1e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> One more thing - I think this should be in serial directory, not mfd,
> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>
> Best regards,
> Krzysztof
I would rather keep it in this directory, since its corresponding driver
is also in the mfd directory.
Regards,
Sergiu
On 19/08/2022 11:37, [email protected] wrote:
>>> + - 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
>>> + - items:
>>> + - const: microchip,sam9x60-usart
>>> + - const: atmel,at91sam9260-usart
>> This is not correct - contradicts earlier one.
>>
>
> Yes, this was for a DT node we have internally, my bad. You are right,
> it does not really make sense and it should not be the other way around:
> having the DT validate the binding. I will remove this combination in
> the next version.
You need to fix any out of tree DTS or upstream it.
>
>
>>> + - items:
>>> + - const: microchip,sam9x60-dbgu
>>> + - const: microchip,sam9x60-usart
>>> + - const: atmel,at91sam9260-dbgu
>>> + - const: atmel,at91sam9260-usart
>> What? You wrote above that microchip,sam9x60-dbgu is compatible only
>> with microchip,sam9x60-usart. Now you write it is also compatible with
>> other ones?
>
>
> Yes, this one is intended because the 9x60 IP has new functionalities on
> top of 9260, and some nodes do keep all four as fallback.
Then all nodes should keep fallbacks.
>
>
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + clock-names:
>>> + contains:
>>> + const: usart
>> No, this has to be specific/fixed list.
>
>
> I wanted to highlight the fact that it must contain either:
> clock-names = "usart";
> or
> clock-names = "usart", "gclk";
>
> What would be the recommended way of doing this then?
We have an example for this.
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L91
Best regards,
Krzysztof
On 19/08/2022 11:47, Krzysztof Kozlowski wrote:
> On 19/08/2022 11:38, [email protected] wrote:
>> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 17/08/2022 10:55, Sergiu Moga wrote:
>>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>>>> json-schema format.
>>>>
>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>> ---
>>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>>>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>> new file mode 100644
>>>> index 000000000000..cf15d73fa1e8
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> One more thing - I think this should be in serial directory, not mfd,
>>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> I would rather keep it in this directory, since its corresponding driver
>> is also in the mfd directory.
>
> Sorry, but that's poor argument. Driver subsystems match Linux
> convention, not necessarily hardware type/naming. Bindings directories
> match hardware. MFD bindings are only for MFD wrapper drivers and this
> is a serial interface. Not a MFD. You even do not add MFD devices in the
> driver but add *always one* device depending on serial feature you want.
> This is not even MFD device but regular platform device with children.
>
> You put it in SoC, though, because all other SoCs store it there...
The last one should be:
You could put it in SoC, though, because all other SoCs store it there...
Best regards,
Krzysztof
On 19/08/2022 11:38, [email protected] wrote:
> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 17/08/2022 10:55, Sergiu Moga wrote:
>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>>> json-schema format.
>>>
>>> Signed-off-by: Sergiu Moga <[email protected]>
>>> ---
>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> new file mode 100644
>>> index 000000000000..cf15d73fa1e8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> One more thing - I think this should be in serial directory, not mfd,
>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>>
>> Best regards,
>> Krzysztof
>
> I would rather keep it in this directory, since its corresponding driver
> is also in the mfd directory.
Sorry, but that's poor argument. Driver subsystems match Linux
convention, not necessarily hardware type/naming. Bindings directories
match hardware. MFD bindings are only for MFD wrapper drivers and this
is a serial interface. Not a MFD. You even do not add MFD devices in the
driver but add *always one* device depending on serial feature you want.
This is not even MFD device but regular platform device with children.
You put it in SoC, though, because all other SoCs store it there...
Best regards,
Krzysztof
On 18.08.2022 11:38, Krzysztof Kozlowski wrote:
> On 17/08/2022 10:55, Sergiu Moga wrote:
>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>> json-schema format.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>> 2 files changed, 190 insertions(+), 98 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> new file mode 100644
>> index 000000000000..cf15d73fa1e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>> @@ -0,0 +1,190 @@
>> +# 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/mfd/atmel,at91-usart.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>> +
>> +maintainers:
>> + - Richard Genoud <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + oneOf:
> This looks quite different than bindings and you commit msg is saying it
> is only a conversion. Mention any changes against original bindings.
>
>> + - const: atmel,at91rm9200-usart
>> + - const: atmel,at91sam9260-usart
>> + - const: microchip,sam9x60-usart
> That's an enum
>
>> + - 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
>> + - items:
>> + - const: microchip,sam9x60-usart
>> + - const: atmel,at91sam9260-usart
> This is not correct - contradicts earlier one.
>
>> + - items:
>> + - const: microchip,sam9x60-dbgu
>> + - const: microchip,sam9x60-usart
>> + - const: atmel,at91sam9260-dbgu
>> + - const: atmel,at91sam9260-usart
> What? You wrote above that microchip,sam9x60-dbgu is compatible only
> with microchip,sam9x60-usart. Now you write it is also compatible with
> other ones?
>
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + clock-names:
>> + contains:
>> + const: usart
> No, this has to be specific/fixed list.
>
>> +
>> + clocks:
>> + minItems: 1
>> + maxItems: 2
> Not really - define the items. One item could be optional, though.
>
>> +
>> + 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: |
> No need for |
>
>> + Must be either 1 for SPI or 0 for USART.
> Mention the header.
>
>> + enum: [ 0, 1 ]
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - clock-names
>> + - clocks
>> +
>> +if:
> Put it under allOf.
I missed this in the first read, but what do you mean by under allOf?
The only allOf's in this file are under the then:'s.
>> + 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: |
> No need for |
>
>> + 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 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "atmel,at91sam9260-usart";
>> + atmel,usart-mode = <AT91_USART_MODE_SPI>;
>> + reg = <0xf001c000 0x100>;
> compatible, then reg then the reset of properties
>
>> + 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>;
>> + };
> Best regards,
> Krzysztof
Regards,
Sergiu
On 19/08/2022 12:25, [email protected] wrote:
> On 18.08.2022 11:38, Krzysztof Kozlowski wrote:
>> On 17/08/2022 10:55, Sergiu Moga wrote:
>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>>> json-schema format.
>>>
>>> Signed-off-by: Sergiu Moga <[email protected]>
>>> ---
>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> new file mode 100644
>>> index 000000000000..cf15d73fa1e8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> @@ -0,0 +1,190 @@
>>> +# 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/mfd/atmel,at91-usart.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
>>> +
>>> +maintainers:
>>> + - Richard Genoud <[email protected]>
>>> +
>>> +properties:
>>> + compatible:
>>> + oneOf:
>> This looks quite different than bindings and you commit msg is saying it
>> is only a conversion. Mention any changes against original bindings.
>>
>>> + - const: atmel,at91rm9200-usart
>>> + - const: atmel,at91sam9260-usart
>>> + - const: microchip,sam9x60-usart
>> That's an enum
>>
>>> + - 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
>>> + - items:
>>> + - const: microchip,sam9x60-usart
>>> + - const: atmel,at91sam9260-usart
>> This is not correct - contradicts earlier one.
>>
>>> + - items:
>>> + - const: microchip,sam9x60-dbgu
>>> + - const: microchip,sam9x60-usart
>>> + - const: atmel,at91sam9260-dbgu
>>> + - const: atmel,at91sam9260-usart
>> What? You wrote above that microchip,sam9x60-dbgu is compatible only
>> with microchip,sam9x60-usart. Now you write it is also compatible with
>> other ones?
>>
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + clock-names:
>>> + contains:
>>> + const: usart
>> No, this has to be specific/fixed list.
>>
>>> +
>>> + clocks:
>>> + minItems: 1
>>> + maxItems: 2
>> Not really - define the items. One item could be optional, though.
>>
>>> +
>>> + 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: |
>> No need for |
>>
>>> + Must be either 1 for SPI or 0 for USART.
>> Mention the header.
>>
>>> + enum: [ 0, 1 ]
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> + - clock-names
>>> + - clocks
>>> +
>>> +if:
>> Put it under allOf.
>
>
> I missed this in the first read, but what do you mean by under allOf?
> The only allOf's in this file are under the then:'s.
>
It means that "if:" is preferred to be under allOf, just like example
schema is showing:
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml
Not some other allOf, which could be many in your bindings. The one
allOf in top-level, just like example schema.
Best regards,
Krzysztof
On 17.08.2022 10:55, Sergiu Moga wrote:
> 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 <[email protected]>
Reviewed-by: Claudiu Beznea <[email protected]>
> ---
> 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 }, },
Le 17/08/2022 à 09:55, Sergiu Moga a écrit :
> 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 <[email protected]>
> ---
> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
> drivers/tty/serial/atmel_serial.h | 1 +
> 2 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 30ba9eef7b39..0a0b46ee0955 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 */
> @@ -2115,6 +2119,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 (atmel_port->gclk && __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);
> @@ -2129,7 +2135,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);
> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> cd /= 8;
> mode |= ATMEL_US_USCLKS_MCK_DIV8;
> }
> +
> + /*
> + * 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 (!fp && atmel_port->gclk) {
> + if (__clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
> + clk_set_rate(atmel_port->gclk, 16 * baud);
> + gclk_rate = clk_get_rate(atmel_port->gclk);
> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> + if (abs(ERROR_RATE(baud, actual_baud)) >
> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
> + mode |= ATMEL_US_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_enable(atmel_port->gclk);
> + }
> + }
> +
> quot = cd | fp << ATMEL_US_FP_OFFSET;
>
> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
> if (ret)
> goto err;
>
> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> + if (atmel_port->gclk) {
> + ret = clk_prepare_enable(atmel_port->gclk);
> + if (ret) {
> + atmel_port->gclk = NULL;
> + return ret;
> + }
> + clk_disable_unprepare(atmel_port->gclk);
> + }
> +
> ret = atmel_init_port(atmel_port, pdev);
> if (ret)
> goto err_clk_disable_unprepare;
> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
> * is used
> */
> clk_disable_unprepare(atmel_port->clk);
> + if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
>
> return 0;
>
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 0d8a0f9cc5c3..fb718972f81a 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -63,6 +63,7 @@
> #define ATMEL_US_PAR_MARK (3 << 9)
> #define ATMEL_US_PAR_NONE (4 << 9)
> #define ATMEL_US_PAR_MULTI_DROP (6 << 9)
> +#define ATMEL_US_GCLK BIT(12)
> #define ATMEL_US_NBSTOP GENMASK(13, 12) /* Number of Stop Bits */
> #define ATMEL_US_NBSTOP_1 (0 << 12)
> #define ATMEL_US_NBSTOP_1_5 (1 << 12)
Correct me if I'm wrong, but GCLK is selected by the bit 12 only in UART_MR, not in USART_MR.
In USART_MR, it seems to be controlled by bits 4-5 (and bit 12 is for stop bits, as we can see above, and in the datasheet).
cf https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA5D2-Series-Datasheet-DS60001476H.pdf
page 1637
Regards,
Richard.
Quoting Sergiu Moga (2022-08-17 00:55:17)
> 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 <[email protected]>
> ---
Should I pick this up to clk-next or did you want to take it via another
tree?
On 30.08.2022 20:29, Richard Genoud wrote:
>
> Le 17/08/2022 à 09:55, Sergiu Moga a écrit :
>> 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 <[email protected]>
>> ---
>> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>> drivers/tty/serial/atmel_serial.h | 1 +
>> 2 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c
>> b/drivers/tty/serial/atmel_serial.c
>> index 30ba9eef7b39..0a0b46ee0955 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 */
>> @@ -2115,6 +2119,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 (atmel_port->gclk && __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);
>> @@ -2129,7 +2135,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);
>> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port
>> *port, struct ktermios *termios,
>> cd /= 8;
>> mode |= ATMEL_US_USCLKS_MCK_DIV8;
>> }
>> +
>> + /*
>> + * 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 (!fp && atmel_port->gclk) {
>> + if (__clk_is_enabled(atmel_port->gclk))
>> + clk_disable_unprepare(atmel_port->gclk);
>> + clk_set_rate(atmel_port->gclk, 16 * baud);
>> + gclk_rate = clk_get_rate(atmel_port->gclk);
>> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>> + if (abs(ERROR_RATE(baud, actual_baud)) >
>> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
>> + mode |= ATMEL_US_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_enable(atmel_port->gclk);
>> + }
>> + }
>> +
>> quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct
>> platform_device *pdev)
>> if (ret)
>> goto err;
>>
>> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>> + if (atmel_port->gclk) {
>> + ret = clk_prepare_enable(atmel_port->gclk);
>> + if (ret) {
>> + atmel_port->gclk = NULL;
>> + return ret;
>> + }
>> + clk_disable_unprepare(atmel_port->gclk);
>> + }
>> +
>> ret = atmel_init_port(atmel_port, pdev);
>> if (ret)
>> goto err_clk_disable_unprepare;
>> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct
>> platform_device *pdev)
>> * is used
>> */
>> clk_disable_unprepare(atmel_port->clk);
>> + if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>> + clk_disable_unprepare(atmel_port->gclk);
>>
>> return 0;
>>
>> diff --git a/drivers/tty/serial/atmel_serial.h
>> b/drivers/tty/serial/atmel_serial.h
>> index 0d8a0f9cc5c3..fb718972f81a 100644
>> --- a/drivers/tty/serial/atmel_serial.h
>> +++ b/drivers/tty/serial/atmel_serial.h
>> @@ -63,6 +63,7 @@
>> #define ATMEL_US_PAR_MARK (3 << 9)
>> #define ATMEL_US_PAR_NONE (4 << 9)
>> #define ATMEL_US_PAR_MULTI_DROP (6 << 9)
>> +#define ATMEL_US_GCLK BIT(12)
>> #define ATMEL_US_NBSTOP GENMASK(13, 12) /* Number of
>> Stop Bits */
>> #define ATMEL_US_NBSTOP_1 (0 << 12)
>> #define ATMEL_US_NBSTOP_1_5 (1 << 12)
>
> Correct me if I'm wrong, but GCLK is selected by the bit 12 only in
> UART_MR, not in USART_MR.
> In USART_MR, it seems to be controlled by bits 4-5 (and bit 12 is for
> stop bits, as we can see above, and in the datasheet).
> cf
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA5D2-Series-Datasheet-DS60001476H.pdf
>
> page 1637
>
> Regards,
> Richard.
Yes, you are correct, this should have been called ATMEL_UA_GCLK
instead. I think I will add both ATMEL_UA_GCLK and ATMEL_US_GCLK bits
and an additional field in struct atmel_uart_port to hold ATMEL_UA_GCLK
for UART or ATMEL_US_GCLK for USART. I guess this field should be set in
atmel_get_ip_name(), the same place where the decision between
ATMEL_UA_RTOR and ATMEL_US_RTOR is taken.
Thanks,
Sergiu
On 17.08.2022 10: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 <[email protected]>
> ---
> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
> drivers/tty/serial/atmel_serial.h | 1 +
> 2 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 30ba9eef7b39..0a0b46ee0955 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 */
> @@ -2115,6 +2119,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 (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
No need to check for atmel_port->gclk != NULL. clk APIs are already doing this.
> + clk_disable_unprepare(atmel_port->gclk);
> break;
> default:
> dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2129,7 +2135,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);
> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> cd /= 8;
> mode |= ATMEL_US_USCLKS_MCK_DIV8;
> }
> +
> + /*
> + * 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 (!fp && atmel_port->gclk) {
> + if (__clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
You disabled it here, set new rate but re-enable it conditionally above, is
this intended? the below condition may fail.
> + clk_set_rate(atmel_port->gclk, 16 * baud);
> + gclk_rate = clk_get_rate(atmel_port->gclk);
You should be able to use clk_round_rate() here:
gclk_rate = clk_round_rate(atmel_port->gclk,
16 * baudrate);
With this you can re-write all this block something like:
gclk_rate = clk_round_rate(atmel_port->gclk,
16 * baudrate);
actual_baud = gclk_rate / (16 * cd);
if (abs(ERROR_RATE(baud, actual_baud)) >
abd(ERROR_RATE(baud, gclk_rate / 16))) {
mode |= ATMEL_US_GCLK;
cd = 1;
if (__clk_is_enabled(atmel_port->gclk))
clk_disable_unprepare(atmel_port->gclk)
clk_set_rate(atmel_port->gclk, gclk_rate);
clk_prepare_enable(atmel_port->gclk);
}
> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> + if (abs(ERROR_RATE(baud, actual_baud)) >
> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
> + mode |= ATMEL_US_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_enable(atmel_port->gclk);
> + }
> + }
> +
> quot = cd | fp << ATMEL_US_FP_OFFSET;
>
> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
> if (ret)
> goto err;
>
> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> + if (atmel_port->gclk) {
> + ret = clk_prepare_enable(atmel_port->gclk);
> + if (ret) {
> + atmel_port->gclk = NULL;
> + return ret;
> + }
> + clk_disable_unprepare(atmel_port->gclk);
Is there a reason you enable then disable the clock here?
> + }
> +
> ret = atmel_init_port(atmel_port, pdev);
> if (ret)
> goto err_clk_disable_unprepare;
> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
> * is used
> */
> clk_disable_unprepare(atmel_port->clk);
> + if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
Is this due to the enable in atmel_set_termios()? Is that called on probe
path? Also, there is no need to check for atmel_port->gclk as clk APIs are
already doing this.
>
> return 0;
>
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 0d8a0f9cc5c3..fb718972f81a 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -63,6 +63,7 @@
> #define ATMEL_US_PAR_MARK (3 << 9)
> #define ATMEL_US_PAR_NONE (4 << 9)
> #define ATMEL_US_PAR_MULTI_DROP (6 << 9)
> +#define ATMEL_US_GCLK BIT(12)
It seems there are spaces here.
> #define ATMEL_US_NBSTOP GENMASK(13, 12) /* Number of Stop Bits */
> #define ATMEL_US_NBSTOP_1 (0 << 12)
> #define ATMEL_US_NBSTOP_1_5 (1 << 12)
On 31.08.2022 12:46, Claudiu Beznea wrote:
> On 17.08.2022 10: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 <[email protected]>
>> ---
>> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>> drivers/tty/serial/atmel_serial.h | 1 +
>> 2 files changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 30ba9eef7b39..0a0b46ee0955 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 */
>> @@ -2115,6 +2119,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 (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>
> No need to check for atmel_port->gclk != NULL. clk APIs are already doing this.
>
>> + clk_disable_unprepare(atmel_port->gclk);
>> break;
>> default:
>> dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>> @@ -2129,7 +2135,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);
>> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> cd /= 8;
>> mode |= ATMEL_US_USCLKS_MCK_DIV8;
>> }
>> +
>> + /*
>> + * 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 (!fp && atmel_port->gclk) {
>> + if (__clk_is_enabled(atmel_port->gclk))
>> + clk_disable_unprepare(atmel_port->gclk);
>
> You disabled it here, set new rate but re-enable it conditionally above, is
> this intended? the below condition may fail.
>
Yes, it is intended. Gclk should remain disabled if the below condition
fails.
>> + clk_set_rate(atmel_port->gclk, 16 * baud);
>> + gclk_rate = clk_get_rate(atmel_port->gclk);
>
> You should be able to use clk_round_rate() here:
> gclk_rate = clk_round_rate(atmel_port->gclk,
> 16 * baudrate);
>
> With this you can re-write all this block something like:
>
> gclk_rate = clk_round_rate(atmel_port->gclk,
> 16 * baudrate);
> actual_baud = gclk_rate / (16 * cd);
> if (abs(ERROR_RATE(baud, actual_baud)) >
> abd(ERROR_RATE(baud, gclk_rate / 16))) {
> mode |= ATMEL_US_GCLK;
> cd = 1;
> if (__clk_is_enabled(atmel_port->gclk))
> clk_disable_unprepare(atmel_port->gclk)
> clk_set_rate(atmel_port->gclk, gclk_rate);
> clk_prepare_enable(atmel_port->gclk);
> }
>
>
Hmm, yes, you are right, it is much better with this clk_round_rate()
since there would be no need to disable gclk before setting the new
rate, I did not know of this function at that point. However, in this
case, the if(abs...) will also need an else to disable the gclk in case
the condition failed and it was somehow enabled before.
>> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>> + if (abs(ERROR_RATE(baud, actual_baud)) >
>> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
>> + mode |= ATMEL_US_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_enable(atmel_port->gclk);
>> + }
>> + }
>> +
>> quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
>> if (ret)
>> goto err;
>>
>> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>> + if (atmel_port->gclk) {
>> + ret = clk_prepare_enable(atmel_port->gclk);
>> + if (ret) {
>> + atmel_port->gclk = NULL;
>> + return ret;
>> + }
>> + clk_disable_unprepare(atmel_port->gclk);
>
> Is there a reason you enable then disable the clock here?
>
I think it's better to make sure in the probe method that enabling the
gclk issues no errors, so that the error does not appear in
set_termios(). Since the user must place the optional gclk in DT if they
want a finer rate when missing the Fractional Part, I think they should
know before even trying to open the port that it is first correctly
setup in the clock tree as well by making the probe method fail from the
very beginning in case it is not.
So, I first enable it and make sure there are no errors and then disable
it because I do not see the point of having an enabled clock that might
not be used.
>> + }
>> +
>> ret = atmel_init_port(atmel_port, pdev);
>> if (ret)
>> goto err_clk_disable_unprepare;
>> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
>> * is used
>> */
>> clk_disable_unprepare(atmel_port->clk);
>> + if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>> + clk_disable_unprepare(atmel_port->gclk);
>
> Is this due to the enable in atmel_set_termios()? Is that called on probe
> path? Also, there is no need to check for atmel_port->gclk as clk APIs are
> already doing this.
>
No, I guess this is not really needed, since it is disabled once it is
claimed from DT if enabling it succeeds. I initially placed this code
sequence wherever the peripheral clock is disabled as well.
>>
>> return 0;
>>
>> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
>> index 0d8a0f9cc5c3..fb718972f81a 100644
>> --- a/drivers/tty/serial/atmel_serial.h
>> +++ b/drivers/tty/serial/atmel_serial.h
>> @@ -63,6 +63,7 @@
>> #define ATMEL_US_PAR_MARK (3 << 9)
>> #define ATMEL_US_PAR_NONE (4 << 9)
>> #define ATMEL_US_PAR_MULTI_DROP (6 << 9)
>> +#define ATMEL_US_GCLK BIT(12)
>
> It seems there are spaces here.
>
>> #define ATMEL_US_NBSTOP GENMASK(13, 12) /* Number of Stop Bits */
>> #define ATMEL_US_NBSTOP_1 (0 << 12)
>> #define ATMEL_US_NBSTOP_1_5 (1 << 12)
>
Thanks,
Sergiu
On 31.08.2022 01:23, Stephen Boyd wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Quoting Sergiu Moga (2022-08-17 00:55:17)
>> 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 <[email protected]>
>> ---
>
> Should I pick this up to clk-next or did you want to take it via another
> tree?
For me it's OK if it goes though clk-next.
Thank you,
Claudiu Beznea
On 31.08.2022 14:32, Sergiu Moga - M68701 wrote:
> On 31.08.2022 12:46, Claudiu Beznea wrote:
>> On 17.08.2022 10: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 <[email protected]>
>>> ---
>>> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>>> drivers/tty/serial/atmel_serial.h | 1 +
>>> 2 files changed, 52 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>>> index 30ba9eef7b39..0a0b46ee0955 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 */
>>> @@ -2115,6 +2119,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 (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>>
>> No need to check for atmel_port->gclk != NULL. clk APIs are already doing this.
>>
>>> + clk_disable_unprepare(atmel_port->gclk);
>>> break;
>>> default:
>>> dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>>> @@ -2129,7 +2135,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);
>>> @@ -2288,6 +2295,37 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>>> cd /= 8;
>>> mode |= ATMEL_US_USCLKS_MCK_DIV8;
>>> }
>>> +
>>> + /*
>>> + * 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 (!fp && atmel_port->gclk) {
>>> + if (__clk_is_enabled(atmel_port->gclk))
>>> + clk_disable_unprepare(atmel_port->gclk);
>>
>> You disabled it here, set new rate but re-enable it conditionally above, is
>> this intended? the below condition may fail.
>>
>
> Yes, it is intended. Gclk should remain disabled if the below condition
> fails.
>
>
>>> + clk_set_rate(atmel_port->gclk, 16 * baud);
>>> + gclk_rate = clk_get_rate(atmel_port->gclk);
>>
>> You should be able to use clk_round_rate() here:
>> gclk_rate = clk_round_rate(atmel_port->gclk,
>> 16 * baudrate);
>>
>> With this you can re-write all this block something like:
>>
>> gclk_rate = clk_round_rate(atmel_port->gclk,
>> 16 * baudrate);
>> actual_baud = gclk_rate / (16 * cd);
>> if (abs(ERROR_RATE(baud, actual_baud)) >
>> abd(ERROR_RATE(baud, gclk_rate / 16))) {
>> mode |= ATMEL_US_GCLK;
>> cd = 1;
>> if (__clk_is_enabled(atmel_port->gclk))
>> clk_disable_unprepare(atmel_port->gclk)
>> clk_set_rate(atmel_port->gclk, gclk_rate);
>> clk_prepare_enable(atmel_port->gclk);
>> }
>>
>>
>
> Hmm, yes, you are right, it is much better with this clk_round_rate()
> since there would be no need to disable gclk before setting the new
> rate, I did not know of this function at that point. However, in this
> case, the if(abs...) will also need an else to disable the gclk in case
> the condition failed and it was somehow enabled before.
Then you can keep the disable at the beginning of the block (as it
previously was) and use clk_round_rate() instead of:
clk_set_rate();
clk_get_rate();
And use clk_set_rate() only if the following is true:
+ if (abs(ERROR_RATE(baud, actual_baud)) >
+ abs(ERROR_RATE(baud, gclk_rate / 16))) {
Having this, in case gclk cannot be used it is just disable w/o also having
the rate changed.
>
>
>>> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>>> + if (abs(ERROR_RATE(baud, actual_baud)) >
>>> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
>>> + mode |= ATMEL_US_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_enable(atmel_port->gclk);
>>> + }
>>> + }
>>> +
>>> quot = cd | fp << ATMEL_US_FP_OFFSET;
>>>
>>> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>>> @@ -2883,6 +2921,16 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>> if (ret)
>>> goto err;
>>>
>>> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>>> + if (atmel_port->gclk) {
>>> + ret = clk_prepare_enable(atmel_port->gclk);
>>> + if (ret) {
>>> + atmel_port->gclk = NULL;
>>> + return ret;
>>> + }
>>> + clk_disable_unprepare(atmel_port->gclk);
>>
>> Is there a reason you enable then disable the clock here?
>>
>
>
> I think it's better to make sure in the probe method that enabling the
> gclk issues no errors, so that the error does not appear in
> set_termios(). Since the user must place the optional gclk in DT if they
> want a finer rate when missing the Fractional Part, I think they should
> know before even trying to open the port that it is first correctly
> setup in the clock tree as well by making the probe method fail from the
> very beginning in case it is not.
There is no guarantee that the disable/enable in set_termios() will not
fail. If you want to avoid failures you can just take into account the
returning code of clk_prepare_enable() in set_termios() and decide to use
generic clock for baud rate depending on this.
>
> So, I first enable it and make sure there are no errors and then disable
> it because I do not see the point of having an enabled clock that might
> not be used.
>
>
>>> + }
>>> +
>>> ret = atmel_init_port(atmel_port, pdev);
>>> if (ret)
>>> goto err_clk_disable_unprepare;
>>> @@ -2929,6 +2977,8 @@ static int atmel_serial_probe(struct platform_device *pdev)
>>> * is used
>>> */
>>> clk_disable_unprepare(atmel_port->clk);
>>> + if (atmel_port->gclk && __clk_is_enabled(atmel_port->gclk))
>>> + clk_disable_unprepare(atmel_port->gclk);
>>
>> Is this due to the enable in atmel_set_termios()? Is that called on probe
>> path? Also, there is no need to check for atmel_port->gclk as clk APIs are
>> already doing this.
>>
>
>
> No, I guess this is not really needed, since it is disabled once it is
> claimed from DT if enabling it succeeds. I initially placed this code
> sequence wherever the peripheral clock is disabled as well.
>
>>>
>>> return 0;
>>>
>>> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
>>> index 0d8a0f9cc5c3..fb718972f81a 100644
>>> --- a/drivers/tty/serial/atmel_serial.h
>>> +++ b/drivers/tty/serial/atmel_serial.h
>>> @@ -63,6 +63,7 @@
>>> #define ATMEL_US_PAR_MARK (3 << 9)
>>> #define ATMEL_US_PAR_NONE (4 << 9)
>>> #define ATMEL_US_PAR_MULTI_DROP (6 << 9)
>>> +#define ATMEL_US_GCLK BIT(12)
>>
>> It seems there are spaces here.
>>
>>> #define ATMEL_US_NBSTOP GENMASK(13, 12) /* Number of Stop Bits */
>>> #define ATMEL_US_NBSTOP_1 (0 << 12)
>>> #define ATMEL_US_NBSTOP_1_5 (1 << 12)
>>
>
> Thanks,
> Sergiu
On 8/19/22 12:05 PM, Krzysztof Kozlowski wrote:
> On 19/08/2022 11:47, Krzysztof Kozlowski wrote:
>> On 19/08/2022 11:38, [email protected] wrote:
>>> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 17/08/2022 10:55, Sergiu Moga wrote:
>>>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>>>>> json-schema format.
>>>>>
>>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>>> ---
>>>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>>>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>>>>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..cf15d73fa1e8
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>> One more thing - I think this should be in serial directory, not mfd,
>>>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> I would rather keep it in this directory, since its corresponding driver
>>> is also in the mfd directory.
>>
>> Sorry, but that's poor argument. Driver subsystems match Linux
>> convention, not necessarily hardware type/naming. Bindings directories
>> match hardware. MFD bindings are only for MFD wrapper drivers and this
>> is a serial interface. Not a MFD. You even do not add MFD devices in the
>> driver but add *always one* device depending on serial feature you want.
>> This is not even MFD device but regular platform device with children.
>>
>> You put it in SoC, though, because all other SoCs store it there...
>
> The last one should be:
>
> You could put it in SoC, though, because all other SoCs store it there...
Hi,
If it this is only a conversion to yaml, why do you want it moved to
another dir ?
Perhaps if you consider SoC or serial as a better place, it should be
done through a different patch.
Also, disputing whether this is really a MFD or not, is not in the scope
of this patch.
Eugen
>
> Best regards,
> Krzysztof
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On 01/09/2022 13:31, [email protected] wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..cf15d73fa1e8
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>>> One more thing - I think this should be in serial directory, not mfd,
>>>>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>
>>>> I would rather keep it in this directory, since its corresponding driver
>>>> is also in the mfd directory.
>>>
>>> Sorry, but that's poor argument. Driver subsystems match Linux
>>> convention, not necessarily hardware type/naming. Bindings directories
>>> match hardware. MFD bindings are only for MFD wrapper drivers and this
>>> is a serial interface. Not a MFD. You even do not add MFD devices in the
>>> driver but add *always one* device depending on serial feature you want.
>>> This is not even MFD device but regular platform device with children.
>>>
>>> You put it in SoC, though, because all other SoCs store it there...
>>
>> The last one should be:
>>
>> You could put it in SoC, though, because all other SoCs store it there...
>
> Hi,
>
> If it this is only a conversion to yaml, why do you want it moved to
> another dir ?
> Perhaps if you consider SoC or serial as a better place, it should be
> done through a different patch.
>
> Also, disputing whether this is really a MFD or not, is not in the scope
> of this patch.
>
Because you are converting - thus renaming - the bindings, so this is
the place to put them in proper place. The conversion to DT Schema comes
pretty often with small fixups, so proper location is one of them.
That's quite common case.
Best regards,
Krzysztof
On Fri, 19 Aug 2022, [email protected] wrote:
> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 17/08/2022 10:55, Sergiu Moga wrote:
> >> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
> >> json-schema format.
> >>
> >> Signed-off-by: Sergiu Moga <[email protected]>
> >> ---
> >> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
> >> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
> >> 2 files changed, 190 insertions(+), 98 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> >> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> >> new file mode 100644
> >> index 000000000000..cf15d73fa1e8
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> > One more thing - I think this should be in serial directory, not mfd,
> > even though it includes SPI. MFD is just a Linux naming/wrapper device.
> >
> > Best regards,
> > Krzysztof
>
> I would rather keep it in this directory, since its corresponding driver
> is also in the mfd directory.
Looks like a UART driver to me.
Which MFD driver does this pertain to?
--
Lee Jones [李琼斯]
On 05.09.2022 17:37, Lee Jones wrote:
>
> On Fri, 19 Aug 2022, [email protected] wrote:
>
>> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 17/08/2022 10:55, Sergiu Moga wrote:
>>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>>>> json-schema format.
>>>>
>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>> ---
>>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>>>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>> new file mode 100644
>>>> index 000000000000..cf15d73fa1e8
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>> One more thing - I think this should be in serial directory, not mfd,
>>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> I would rather keep it in this directory, since its corresponding driver
>> is also in the mfd directory.
>
> Looks like a UART driver to me.
>
> Which MFD driver does this pertain to?
>
> --
> Lee Jones [李琼斯]
Hi,
It's this one: drivers/mfd/at91-usart.c[1]
[1]
https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/mfd/at91-usart.c
Regards,
Sergiu
On 05/09/2022 17:22, [email protected] wrote:
> On 05.09.2022 17:37, Lee Jones wrote:
>>
>> On Fri, 19 Aug 2022, [email protected] wrote:
>>
>>> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 17/08/2022 10:55, Sergiu Moga wrote:
>>>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
>>>>> json-schema format.
>>>>>
>>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>>> ---
>>>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
>>>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
>>>>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..cf15d73fa1e8
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
>>>> One more thing - I think this should be in serial directory, not mfd,
>>>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> I would rather keep it in this directory, since its corresponding driver
>>> is also in the mfd directory.
>>
>> Looks like a UART driver to me.
>>
>> Which MFD driver does this pertain to?
>>
>> --
>> Lee Jones [李琼斯]
>
> Hi,
>
> It's this one: drivers/mfd/at91-usart.c[1]
>
>
> [1]
> https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/mfd/at91-usart.c
Which is not a "real MFD driver" because it probes exactly one child
(depending on the chosen serial protocol). Aren't MFD supposed to have
more then one child?
Best regards,
Krzysztof
On Mon, 05 Sep 2022, Krzysztof Kozlowski wrote:
> On 05/09/2022 17:22, [email protected] wrote:
> > On 05.09.2022 17:37, Lee Jones wrote:
> >>
> >> On Fri, 19 Aug 2022, [email protected] wrote:
> >>
> >>> On 18.08.2022 11:39, Krzysztof Kozlowski wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>
> >>>> On 17/08/2022 10:55, Sergiu Moga wrote:
> >>>>> Convert at91 USART DT Binding for Atmel/Microchip SoCs to
> >>>>> json-schema format.
> >>>>>
> >>>>> Signed-off-by: Sergiu Moga <[email protected]>
> >>>>> ---
> >>>>> .../bindings/mfd/atmel,at91-usart.yaml | 190 ++++++++++++++++++
> >>>>> .../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
> >>>>> 2 files changed, 190 insertions(+), 98 deletions(-)
> >>>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> >>>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..cf15d73fa1e8
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,at91-usart.yaml
> >>>> One more thing - I think this should be in serial directory, not mfd,
> >>>> even though it includes SPI. MFD is just a Linux naming/wrapper device.
> >>>>
> >>>> Best regards,
> >>>> Krzysztof
> >>>
> >>> I would rather keep it in this directory, since its corresponding driver
> >>> is also in the mfd directory.
> >>
> >> Looks like a UART driver to me.
> >>
> >> Which MFD driver does this pertain to?
> >>
> >
> > Hi,
> >
> > It's this one: drivers/mfd/at91-usart.c[1]
> >
> >
> > [1]
> > https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/mfd/at91-usart.c
>
> Which is not a "real MFD driver" because it probes exactly one child
> (depending on the chosen serial protocol). Aren't MFD supposed to have
> more then one child?
It's a single piece of silicon which supports multiple functions.
There is no stipulation detailing simultaneous usage.
Still, happy to receive suggestions on implementing this differently.
--
Lee Jones [李琼斯]