This series of patches introduces the GCLK as a potential clock source for
the baudrate generator of UART on sama5d2 SoCs. Unlike the serial mode of
the USART offered by FLEXCOM, the UART does not provide a fractional part
that can be added to the clock divisor to obtain a more accurate result,
which greatly decreases the flexibility available for producing a higher
variety of baudrates. Now, with the last patch of the series, the driver
will check for a GCLK in the DT. If provided, whenever `atmel_set_termios`
is called, unless there is a fractional part, the driver will compare the
error rate between the desired baudrate and the actual baudrate obtained
through each of the available clock sources and will choose the clock source
with the lowest error rate. While at it, convert the DT binding
for UART/USART to json-schema, update the FLEXCOM binding to reference the
new UART/USART binding (while differentiating between the SPI of USART and the
SPI of FLEXCOM) and do some small DT related fixups.
The DT bindings related patches of this patch series depend on this patch
series converting atmel-flexcom bindings to json-schema:
https://lore.kernel.org/all/[email protected]/
v1 -> v2:
- [PATCH 3] dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref
binding:
- use full schema paths
- [PATCH 5] dt-bindings: serial: atmel,at91-usart: convert to json-schema
- only do what the commit says, split the addition of other compatibles
(PATCH 6) and properties (PATCH 13) in other patches
- remove unnecessary "|"'s
- mention header in `atmel,usart-mode`'s description
- place `if:` under `allOf:`
- respect order of spi0's DT properties: compatible, then reg then the
reset of properties
- two new baudrate clock source related patches:
[PATCH 9] tty: serial: atmel: Add definition for GCLK as baudrate source clock
+
[PATCH 10] tty: serial: atmel: Define BRSRCCK bitmask of UART IP's Mode
Register:
- v1's bitfield definition of GCLK was wrong, so add two more patches:
- one for the definition of GCLK of USART IP's
- one for the definition of BRSRCCK bitmask and its bitfields
for UART IP's
- a new cleanup related patch that introduces a new struct atmel_uart_port field:
[PATCH 11] tty: serial: atmel: Only divide Clock Divisor if the IP is USART:
- this ensures a division by 8 which is unnecessary and unappliable to
UART IP's is only done for USART IP's
- four new patches regarding DT fixes and a SPI binding update that I came
upon:
[PATCH 1] spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties
[PATCH 2] ARM: dts: at91: sama7g5: Swap rx and tx for spi11
[PATCH 4] ARM: dts: at91: sam9x60ek: Add DBGU compatibles to uart1
[PATCH 6] dt-bindings: serial: atmel,at91-usart: Highlight SAM9X60 incremental
- [PATCH 12] tty: serial: atmel: Make the driver aware of the existence of GCLK
- take into account the different placement of the baudrate clock source
into the IP's Mode Register (USART vs UART)
- don't check for atmel_port->gclk != NULL
- use clk_round_rate instead of clk_set_rate + clk_get_rate
- remove clk_disable_unprepare from the end of the probe method
Sergiu Moga (13):
spi: dt-bindings: atmel,at91rm9200-spi: Add DMA related properties
ARM: dts: at91: sama7g5: Swap rx and tx for spi11
dt-bindings: mfd: atmel,sama5d2-flexcom: Add SPI child node ref
binding
ARM: dts: at91: sam9x60ek: Add DBGU compatibles to uart1
dt-bindings: serial: atmel,at91-usart: convert to json-schema
dt-bindings: serial: atmel,at91-usart: Add SAM9260 compatibles to
SAM9x60
dt-bindings: mfd: atmel,sama5d2-flexcom: Add USART child node ref
binding
tty: serial: atmel: Define GCLK as USART baudrate source clock
tty: serial: atmel: Define BRSRCCK bitmask of UART IP's Mode Register
tty: serial: atmel: Only divide Clock Divisor if the IP is USART
clk: at91: sama5d2: Add Generic Clocks for UART/USART
tty: serial: atmel: Make the driver aware of the existence of GCLK
dt-bindings: serial: atmel,at91-usart: Add gclk as a possible USART
clock
.../bindings/mfd/atmel,sama5d2-flexcom.yaml | 19 +-
.../devicetree/bindings/mfd/atmel-usart.txt | 98 ---------
.../bindings/serial/atmel,at91-usart.yaml | 191 ++++++++++++++++++
.../bindings/spi/atmel,at91rm9200-spi.yaml | 10 +
arch/arm/boot/dts/at91-sam9x60ek.dts | 2 +-
arch/arm/boot/dts/sama7g5.dtsi | 6 +-
drivers/clk/at91/sama5d2.c | 10 +
drivers/tty/serial/atmel_serial.c | 65 +++++-
drivers/tty/serial/atmel_serial.h | 4 +
9 files changed, 295 insertions(+), 110 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-usart.txt
create mode 100644 Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
--
2.25.1
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]>
---
v1 -> v2:
- Added R-b tag
drivers/clk/at91/sama5d2.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/clk/at91/sama5d2.c b/drivers/clk/at91/sama5d2.c
index cfd0f5e23b99..84156dc52bff 100644
--- a/drivers/clk/at91/sama5d2.c
+++ b/drivers/clk/at91/sama5d2.c
@@ -120,6 +120,16 @@ static const struct {
struct clk_range r;
int chg_pid;
} sama5d2_gck[] = {
+ { .n = "flx0_gclk", .id = 19, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "flx1_gclk", .id = 20, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "flx2_gclk", .id = 21, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "flx3_gclk", .id = 22, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "flx4_gclk", .id = 23, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "uart0_gclk", .id = 24, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "uart1_gclk", .id = 25, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "uart2_gclk", .id = 26, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "uart3_gclk", .id = 27, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
+ { .n = "uart4_gclk", .id = 28, .chg_pid = INT_MIN, .r = { .min = 0, .max = 27666666 }, },
{ .n = "sdmmc0_gclk", .id = 31, .chg_pid = INT_MIN, },
{ .n = "sdmmc1_gclk", .id = 32, .chg_pid = INT_MIN, },
{ .n = "tcb0_gclk", .id = 35, .chg_pid = INT_MIN, .r = { .min = 0, .max = 83000000 }, },
--
2.25.1
Make sure that the driver only divides the clock divisor if the
IP handled at that point is USART, since UART IP's do not support
implicit peripheral clock division. Instead, in the case of UART,
go with the highest possible clock divisor.
Signed-off-by: Sergiu Moga <[email protected]>
---
v1 -> v2:
- Nothing, this patch was not here before and is mainly meant as both cleanup
and as a way to introduce a new field into struct atmel_uart_port that will be
used by the last patch to diferentiate between USART and UART regarding the
location of the Baudrate Clock Source bitmask.
drivers/tty/serial/atmel_serial.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 7450d3853031..6aa01ca5489c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -150,6 +150,7 @@ struct atmel_uart_port {
u32 rts_low;
bool ms_irq_enabled;
u32 rtor; /* address of receiver timeout register if it exists */
+ bool is_usart;
bool has_frac_baudrate;
bool has_hw_timer;
struct timer_list uart_timer;
@@ -1825,6 +1826,7 @@ static void atmel_get_ip_name(struct uart_port *port)
*/
atmel_port->has_frac_baudrate = false;
atmel_port->has_hw_timer = false;
+ atmel_port->is_usart = false;
if (name == new_uart) {
dev_dbg(port->dev, "Uart with hw timer");
@@ -1834,6 +1836,7 @@ static void atmel_get_ip_name(struct uart_port *port)
dev_dbg(port->dev, "Usart\n");
atmel_port->has_frac_baudrate = true;
atmel_port->has_hw_timer = true;
+ atmel_port->is_usart = true;
atmel_port->rtor = ATMEL_US_RTOR;
version = atmel_uart_readl(port, ATMEL_US_VERSION);
switch (version) {
@@ -1863,6 +1866,7 @@ static void atmel_get_ip_name(struct uart_port *port)
dev_dbg(port->dev, "This version is usart\n");
atmel_port->has_frac_baudrate = true;
atmel_port->has_hw_timer = true;
+ atmel_port->is_usart = true;
atmel_port->rtor = ATMEL_US_RTOR;
break;
case 0x203:
@@ -2282,10 +2286,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
cd = uart_get_divisor(port, baud);
}
- if (cd > 65535) { /* BRGR is 16-bit, so switch to slower clock */
+ /*
+ * BRGR is 16-bit, so switch to slower clock.
+ * Otherwise, keep the highest possible value for the clock divisor.
+ */
+ if (atmel_port->is_usart && cd > 65535) {
cd /= 8;
mode |= ATMEL_US_USCLKS_MCK_DIV8;
+ } else {
+ cd &= 65535;
}
+
quot = cd | fp << ATMEL_US_FP_OFFSET;
if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
--
2.25.1
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]>
---
v1 -> v2:
- use full schema paths
.../devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
index 568da7cb630c..0db0f2728b65 100644
--- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
+++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
@@ -78,10 +78,9 @@ patternProperties:
of USART bindings.
"^spi@[0-9a-f]+$":
- type: object
+ $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
description:
- Child node describing SPI. See ../spi/spi_atmel.txt for details
- of SPI bindings.
+ Child node describing SPI.
"^i2c@[0-9a-f]+$":
$ref: ../i2c/atmel,at91sam-i2c.yaml
--
2.25.1
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]>
---
v1 -> v2:
- Nothing
.../bindings/mfd/atmel,sama5d2-flexcom.yaml | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
index 0db0f2728b65..b5fb509f07ec 100644
--- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
+++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
@@ -72,13 +72,21 @@ properties:
patternProperties:
"^serial@[0-9a-f]+$":
- type: object
+ $ref: /schemas/serial/atmel,at91-usart.yaml
description:
- Child node describing USART. See atmel-usart.txt for details
- of USART bindings.
+ Child node describing USART.
"^spi@[0-9a-f]+$":
- $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
+ allOf:
+ - if:
+ properties:
+ clock-names:
+ contains:
+ const: usart
+ then:
+ $ref: /schemas/serial/atmel,at91-usart.yaml
+ else:
+ $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
description:
Child node describing SPI.
--
2.25.1
Define the bit that represents the choice of having GCLK as a baudrate
source clock inside the USCLKS bitmask of the Mode Register of
USART IP's.
Signed-off-by: Sergiu Moga <[email protected]>
---
v1 -> v2:
- Nothing, this patch was not here before
drivers/tty/serial/atmel_serial.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index 0d8a0f9cc5c3..70d0611e56fd 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -49,6 +49,7 @@
#define ATMEL_US_USCLKS GENMASK(5, 4) /* Clock Selection */
#define ATMEL_US_USCLKS_MCK (0 << 4)
#define ATMEL_US_USCLKS_MCK_DIV8 (1 << 4)
+#define ATMEL_US_USCLKS_GCLK (2 << 4)
#define ATMEL_US_USCLKS_SCK (3 << 4)
#define ATMEL_US_CHRL GENMASK(7, 6) /* Character Length */
#define ATMEL_US_CHRL_5 (0 << 6)
--
2.25.1
Swap the rx and tx of the DMA related DT properties of the spi11 node
in order to maintain consistency across Microchip/Atmel SoC files.
Signed-off-by: Sergiu Moga <[email protected]>
---
v1 -> v2:
- Nothing, this patch was not here before
arch/arm/boot/dts/sama7g5.dtsi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/sama7g5.dtsi b/arch/arm/boot/dts/sama7g5.dtsi
index bb6d71e6dfeb..249f9c640b6c 100644
--- a/arch/arm/boot/dts/sama7g5.dtsi
+++ b/arch/arm/boot/dts/sama7g5.dtsi
@@ -866,9 +866,9 @@ spi11: spi@400 {
#address-cells = <1>;
#size-cells = <0>;
atmel,fifo-size = <32>;
- dmas = <&dma0 AT91_XDMAC_DT_PERID(27)>,
- <&dma0 AT91_XDMAC_DT_PERID(28)>;
- dma-names = "rx", "tx";
+ dmas = <&dma0 AT91_XDMAC_DT_PERID(28)>,
+ <&dma0 AT91_XDMAC_DT_PERID(27)>;
+ dma-names = "tx", "rx";
status = "disabled";
};
};
--
2.25.1
The Devicetree nodes for FLEXCOM's USART can also have an alternative
clock source for the baudrate generator (other than the peripheral
clock), namely the Generick Clock. Thus make the binding aware of
this potential clock that someone may place in the clock related
properties of the USART node.
Signed-off-by: Sergiu Moga <[email protected]>
---
v1 -> v2:
- Nothing, this patch was not here before
.../devicetree/bindings/serial/atmel,at91-usart.yaml | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
index 4d80006963c7..db595b498bad 100644
--- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
+++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
@@ -36,10 +36,16 @@ properties:
maxItems: 1
clock-names:
- const: usart
+ minItems: 1
+ items:
+ - const: usart
+ - const: gclk
clocks:
- maxItems: 1
+ minItems: 1
+ items:
+ - description: USART Peripheral Clock
+ - description: USART Generic Clock
dmas:
items:
--
2.25.1
Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
in order to highlight the incremental characteristics of the SAM9X60
serial IP.
Signed-off-by: Sergiu Moga <[email protected]>
---
v1 -> v2:
- Nothing, this patch was not here before
Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
index b25535b7a4d2..4d80006963c7 100644
--- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
+++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
@@ -26,6 +26,8 @@ properties:
- items:
- const: microchip,sam9x60-dbgu
- const: microchip,sam9x60-usart
+ - const: atmel,at91sam9260-dbgu
+ - const: atmel,at91sam9260-usart
reg:
maxItems: 1
--
2.25.1
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]>
---
v1 -> v2:
- take into account the different placement of the baudrate clock source
into the IP's Mode Register (USART vs UART)
- don't check for atmel_port->gclk != NULL
- use clk_round_rate instead of clk_set_rate + clk_get_rate
- remove clk_disable_unprepare from the end of the probe method
drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 6aa01ca5489c..b2b6fd6ea2a5 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -15,6 +15,7 @@
#include <linux/init.h>
#include <linux/serial.h>
#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/console.h>
#include <linux/sysrq.h>
#include <linux/tty_flip.h>
@@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
#endif
#define ATMEL_ISR_PASS_LIMIT 256
+#define ERROR_RATE(desired_value, actual_value) \
+ ((int)(100 - ((desired_value) * 100) / (actual_value)))
struct atmel_dma_buffer {
unsigned char *buf;
@@ -110,6 +113,7 @@ struct atmel_uart_char {
struct atmel_uart_port {
struct uart_port uart; /* uart */
struct clk *clk; /* uart clock */
+ struct clk *gclk; /* uart generic clock */
int may_wakeup; /* cached value of device_may_wakeup for times we need to disable it */
u32 backup_imr; /* IMR saved during suspend */
int break_active; /* break being received */
@@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
* This is called on uart_close() or a suspend event.
*/
clk_disable_unprepare(atmel_port->clk);
+ if (__clk_is_enabled(atmel_port->gclk))
+ clk_disable_unprepare(atmel_port->gclk);
break;
default:
dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
@@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
{
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
unsigned long flags;
- unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
+ unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
+ unsigned int baud, actual_baud, gclk_rate;
/* save the current mode register */
mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
@@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
cd &= 65535;
}
+ /*
+ * If there is no Fractional Part, there is a high chance that
+ * we may be able to generate a baudrate closer to the desired one
+ * if we use the GCLK as the clock source driving the baudrate
+ * generator.
+ */
+ if (!atmel_port->has_frac_baudrate) {
+ if (__clk_is_enabled(atmel_port->gclk))
+ clk_disable_unprepare(atmel_port->gclk);
+ gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
+ actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
+ if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
+ abs(ERROR_RATE(baud, gclk_rate / 16))) {
+ clk_set_rate(atmel_port->gclk, 16 * baud);
+ if (!clk_prepare_enable(atmel_port->gclk)) {
+ if (atmel_port->is_usart) {
+ mode &= ~ATMEL_US_USCLKS;
+ mode |= ATMEL_US_USCLKS_GCLK;
+ } else {
+ mode &= ~ATMEL_UA_BRSRCCK;
+ mode |= ATMEL_UA_BRSRCCK_GCLK;
+ }
+
+ /*
+ * Set the Clock Divisor for GCLK to 1.
+ * Since we were able to generate the smallest
+ * multiple of the desired baudrate times 16,
+ * then we surely can generate a bigger multiple
+ * with the exact error rate for an equally increased
+ * CD. Thus no need to take into account
+ * a higher value for CD.
+ */
+ cd = 1;
+ }
+ }
+ }
+
quot = cd | fp << ATMEL_US_FP_OFFSET;
if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
@@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
if (ret)
goto err;
+ atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
+ if (IS_ERR(atmel_port->gclk)) {
+ ret = PTR_ERR(atmel_port->gclk);
+ goto err;
+ }
+
ret = atmel_init_port(atmel_port, pdev);
if (ret)
goto err_clk_disable_unprepare;
--
2.25.1
On Tue, 6 Sep 2022, Sergiu Moga wrote:
> Make sure that the driver only divides the clock divisor if the
> IP handled at that point is USART, since UART IP's do not support
> implicit peripheral clock division. Instead, in the case of UART,
> go with the highest possible clock divisor.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
> v1 -> v2:
> - Nothing, this patch was not here before and is mainly meant as both cleanup
> and as a way to introduce a new field into struct atmel_uart_port that will be
> used by the last patch to diferentiate between USART and UART regarding the
> location of the Baudrate Clock Source bitmask.
>
>
>
>
> drivers/tty/serial/atmel_serial.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 7450d3853031..6aa01ca5489c 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -150,6 +150,7 @@ struct atmel_uart_port {
> u32 rts_low;
> bool ms_irq_enabled;
> u32 rtor; /* address of receiver timeout register if it exists */
> + bool is_usart;
> bool has_frac_baudrate;
> bool has_hw_timer;
> struct timer_list uart_timer;
> @@ -1825,6 +1826,7 @@ static void atmel_get_ip_name(struct uart_port *port)
> */
> atmel_port->has_frac_baudrate = false;
> atmel_port->has_hw_timer = false;
> + atmel_port->is_usart = false;
>
> if (name == new_uart) {
> dev_dbg(port->dev, "Uart with hw timer");
> @@ -1834,6 +1836,7 @@ static void atmel_get_ip_name(struct uart_port *port)
> dev_dbg(port->dev, "Usart\n");
> atmel_port->has_frac_baudrate = true;
> atmel_port->has_hw_timer = true;
> + atmel_port->is_usart = true;
> atmel_port->rtor = ATMEL_US_RTOR;
> version = atmel_uart_readl(port, ATMEL_US_VERSION);
> switch (version) {
> @@ -1863,6 +1866,7 @@ static void atmel_get_ip_name(struct uart_port *port)
> dev_dbg(port->dev, "This version is usart\n");
> atmel_port->has_frac_baudrate = true;
> atmel_port->has_hw_timer = true;
> + atmel_port->is_usart = true;
> atmel_port->rtor = ATMEL_US_RTOR;
> break;
> case 0x203:
> @@ -2282,10 +2286,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> cd = uart_get_divisor(port, baud);
> }
>
> - if (cd > 65535) { /* BRGR is 16-bit, so switch to slower clock */
> + /*
> + * BRGR is 16-bit, so switch to slower clock.
> + * Otherwise, keep the highest possible value for the clock divisor.
> + */
> + if (atmel_port->is_usart && cd > 65535) {
Should this be cd > ATMEL_US_CD ?
> cd /= 8;
> mode |= ATMEL_US_USCLKS_MCK_DIV8;
> + } else {
> + cd &= 65535;
ATMEL_US_CD?
> }
> +
> quot = cd | fp << ATMEL_US_FP_OFFSET;
>
> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>
--
i.
On 07.09.2022 12:23, Ilpo Järvinen wrote:
> On Tue, 6 Sep 2022, Sergiu Moga wrote:
>
>> Make sure that the driver only divides the clock divisor if the
>> IP handled at that point is USART, since UART IP's do not support
>> implicit peripheral clock division. Instead, in the case of UART,
>> go with the highest possible clock divisor.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before and is mainly meant as both cleanup
>> and as a way to introduce a new field into struct atmel_uart_port that will be
>> used by the last patch to diferentiate between USART and UART regarding the
>> location of the Baudrate Clock Source bitmask.
>>
>>
>>
>>
>> drivers/tty/serial/atmel_serial.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 7450d3853031..6aa01ca5489c 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -150,6 +150,7 @@ struct atmel_uart_port {
>> u32 rts_low;
>> bool ms_irq_enabled;
>> u32 rtor; /* address of receiver timeout register if it exists */
>> + bool is_usart;
>> bool has_frac_baudrate;
>> bool has_hw_timer;
>> struct timer_list uart_timer;
>> @@ -1825,6 +1826,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>> */
>> atmel_port->has_frac_baudrate = false;
>> atmel_port->has_hw_timer = false;
>> + atmel_port->is_usart = false;
>>
>> if (name == new_uart) {
>> dev_dbg(port->dev, "Uart with hw timer");
>> @@ -1834,6 +1836,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>> dev_dbg(port->dev, "Usart\n");
>> atmel_port->has_frac_baudrate = true;
>> atmel_port->has_hw_timer = true;
>> + atmel_port->is_usart = true;
>> atmel_port->rtor = ATMEL_US_RTOR;
>> version = atmel_uart_readl(port, ATMEL_US_VERSION);
>> switch (version) {
>> @@ -1863,6 +1866,7 @@ static void atmel_get_ip_name(struct uart_port *port)
>> dev_dbg(port->dev, "This version is usart\n");
>> atmel_port->has_frac_baudrate = true;
>> atmel_port->has_hw_timer = true;
>> + atmel_port->is_usart = true;
>> atmel_port->rtor = ATMEL_US_RTOR;
>> break;
>> case 0x203:
>> @@ -2282,10 +2286,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> cd = uart_get_divisor(port, baud);
>> }
>>
>> - if (cd > 65535) { /* BRGR is 16-bit, so switch to slower clock */
>> + /*
>> + * BRGR is 16-bit, so switch to slower clock.
>> + * Otherwise, keep the highest possible value for the clock divisor.
>> + */
>> + if (atmel_port->is_usart && cd > 65535) {
>
> Should this be cd > ATMEL_US_CD ?
>
>> cd /= 8;
>> mode |= ATMEL_US_USCLKS_MCK_DIV8;
>> + } else {
>> + cd &= 65535;
>
> ATMEL_US_CD?
Yes, it would seem that would be right. It did not cross my mind to
check whether there already was something pre-defined or not since the
code also previously used 65535.
>
>> }
>> +
>> quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>>
>
> --
> i.
>
Thanks,
Sergiu
On Tue, 6 Sep 2022, Sergiu Moga wrote:
> Define the bit that represents the choice of having GCLK as a baudrate
> source clock inside the USCLKS bitmask of the Mode Register of
> USART IP's.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
> v1 -> v2:
> - Nothing, this patch was not here before
>
>
>
> drivers/tty/serial/atmel_serial.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
> index 0d8a0f9cc5c3..70d0611e56fd 100644
> --- a/drivers/tty/serial/atmel_serial.h
> +++ b/drivers/tty/serial/atmel_serial.h
> @@ -49,6 +49,7 @@
> #define ATMEL_US_USCLKS GENMASK(5, 4) /* Clock Selection */
> #define ATMEL_US_USCLKS_MCK (0 << 4)
> #define ATMEL_US_USCLKS_MCK_DIV8 (1 << 4)
> +#define ATMEL_US_USCLKS_GCLK (2 << 4)
This would be FIELD_PREP(ATMEL_US_USCLKS, 2) from linux/bitfield.h.
They should all be converted to use FIELD_PREP(), IMHO (in a separate
patch).
> #define ATMEL_US_USCLKS_SCK (3 << 4)
> #define ATMEL_US_CHRL GENMASK(7, 6) /* Character Length */
> #define ATMEL_US_CHRL_5 (0 << 6)
>
--
i.
On Tue, 6 Sep 2022, Sergiu Moga wrote:
> Previously, the atmel serial driver did not take into account the
> possibility of using the more customizable generic clock as its
> baudrate generator. Unless there is a Fractional Part available to
> increase accuracy, there is a high chance that we may be able to
> generate a baudrate closer to the desired one by using the GCLK as the
> clock source. Now, depending on the error rate between
> the desired baudrate and the actual baudrate, the serial driver will
> fallback on the generic clock. The generic clock must be provided
> in the DT node of the serial that may need a more flexible clock source.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
>
> v1 -> v2:
> - take into account the different placement of the baudrate clock source
> into the IP's Mode Register (USART vs UART)
> - don't check for atmel_port->gclk != NULL
> - use clk_round_rate instead of clk_set_rate + clk_get_rate
> - remove clk_disable_unprepare from the end of the probe method
>
>
>
> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 6aa01ca5489c..b2b6fd6ea2a5 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -15,6 +15,7 @@
> #include <linux/init.h>
> #include <linux/serial.h>
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/console.h>
> #include <linux/sysrq.h>
> #include <linux/tty_flip.h>
> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
> #endif
>
> #define ATMEL_ISR_PASS_LIMIT 256
> +#define ERROR_RATE(desired_value, actual_value) \
> + ((int)(100 - ((desired_value) * 100) / (actual_value)))
Make a function instead.
Is percent accurate enough or would you perhaps want something slightly
more accurate?
Given you've abs() at the caller side, the error rate could be
underestimated, is underestimating OK?
--
i.
> struct atmel_dma_buffer {
> unsigned char *buf;
> @@ -110,6 +113,7 @@ struct atmel_uart_char {
> struct atmel_uart_port {
> struct uart_port uart; /* uart */
> struct clk *clk; /* uart clock */
> + struct clk *gclk; /* uart generic clock */
> int may_wakeup; /* cached value of device_may_wakeup for times we need to disable it */
> u32 backup_imr; /* IMR saved during suspend */
> int break_active; /* break being received */
> @@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
> * This is called on uart_close() or a suspend event.
> */
> clk_disable_unprepare(atmel_port->clk);
> + if (__clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
> break;
> default:
> dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> {
> struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> unsigned long flags;
> - unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
> + unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
> + unsigned int baud, actual_baud, gclk_rate;
>
> /* save the current mode register */
> mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> cd &= 65535;
> }
>
> + /*
> + * If there is no Fractional Part, there is a high chance that
> + * we may be able to generate a baudrate closer to the desired one
> + * if we use the GCLK as the clock source driving the baudrate
> + * generator.
> + */
> + if (!atmel_port->has_frac_baudrate) {
> + if (__clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
> + gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> + if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
> + clk_set_rate(atmel_port->gclk, 16 * baud);
> + if (!clk_prepare_enable(atmel_port->gclk)) {
> + if (atmel_port->is_usart) {
> + mode &= ~ATMEL_US_USCLKS;
> + mode |= ATMEL_US_USCLKS_GCLK;
> + } else {
> + mode &= ~ATMEL_UA_BRSRCCK;
> + mode |= ATMEL_UA_BRSRCCK_GCLK;
> + }
> +
> + /*
> + * Set the Clock Divisor for GCLK to 1.
> + * Since we were able to generate the smallest
> + * multiple of the desired baudrate times 16,
> + * then we surely can generate a bigger multiple
> + * with the exact error rate for an equally increased
> + * CD. Thus no need to take into account
> + * a higher value for CD.
> + */
> + cd = 1;
> + }
> + }
> + }
> +
> quot = cd | fp << ATMEL_US_FP_OFFSET;
>
> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
> if (ret)
> goto err;
>
> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> + if (IS_ERR(atmel_port->gclk)) {
> + ret = PTR_ERR(atmel_port->gclk);
> + goto err;
> + }
> +
> ret = atmel_init_port(atmel_port, pdev);
> if (ret)
> goto err_clk_disable_unprepare;
>
On 07.09.2022 12:21, Ilpo Järvinen wrote:
> On Tue, 6 Sep 2022, Sergiu Moga wrote:
>
>> Define the bit that represents the choice of having GCLK as a baudrate
>> source clock inside the USCLKS bitmask of the Mode Register of
>> USART IP's.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
>>
>>
>>
>> drivers/tty/serial/atmel_serial.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
>> index 0d8a0f9cc5c3..70d0611e56fd 100644
>> --- a/drivers/tty/serial/atmel_serial.h
>> +++ b/drivers/tty/serial/atmel_serial.h
>> @@ -49,6 +49,7 @@
>> #define ATMEL_US_USCLKS GENMASK(5, 4) /* Clock Selection */
>> #define ATMEL_US_USCLKS_MCK (0 << 4)
>> #define ATMEL_US_USCLKS_MCK_DIV8 (1 << 4)
>> +#define ATMEL_US_USCLKS_GCLK (2 << 4)
>
> This would be FIELD_PREP(ATMEL_US_USCLKS, 2) from linux/bitfield.h.
>
> They should all be converted to use FIELD_PREP(), IMHO (in a separate
> patch).
>
>> #define ATMEL_US_USCLKS_SCK (3 << 4)
>> #define ATMEL_US_CHRL GENMASK(7, 6) /* Character Length */
>> #define ATMEL_US_CHRL_5 (0 << 6)
>>
>
> --
> i.
>
Yes, I guess that is a good idea. After these additional
macro-definitions are upstreamed, I will try to send some patches for
the conversion.
Thanks,
Sergiu
On Wed, 7 Sep 2022, [email protected] wrote:
> On 07.09.2022 12:36, Ilpo Järvinen wrote:
> > On Tue, 6 Sep 2022, Sergiu Moga wrote:
> >
> >> Previously, the atmel serial driver did not take into account the
> >> possibility of using the more customizable generic clock as its
> >> baudrate generator. Unless there is a Fractional Part available to
> >> increase accuracy, there is a high chance that we may be able to
> >> generate a baudrate closer to the desired one by using the GCLK as the
> >> clock source. Now, depending on the error rate between
> >> the desired baudrate and the actual baudrate, the serial driver will
> >> fallback on the generic clock. The generic clock must be provided
> >> in the DT node of the serial that may need a more flexible clock source.
> >>
> >> Signed-off-by: Sergiu Moga <[email protected]>
> >> ---
> > Is percent accurate enough or would you perhaps want something slightly
> > more accurate?
> >
>
>
> It is accurate enough for the all the baudrates I have tested. It
> usually taps into the GCLK whenever high baudrates such as 921600 are
> used. For 115200 for example, the error rate was slightly better in the
> case of the peripheral clock and it acted accordingly, choosing the
> latter as its baudrate source clock. I do not think that a higher
> accuracy than this would be needed though. Say that using percent
> accuracy yields that the error rates are equal, but the gclk would have
> been better in this case by, say, a few 10 ^ -4, but the code logic does
> not see it so it proceeds using the peripheral clock. In that case, the
> error rate of the peripheral clock would still be low enough relative to
> the desired baudrate for the communication to function properly.
>
> The higher the baudrate, the lower the error rate must be in order for
> things to go smoothly. For example, for a baudrate of 57600 I noticed
> that even an error rate as big as 6% is still enough for the
> communication to work properly, while in the case of 921600 anything
> bigger than 2% and things do not go smoothly anymore. So I guess that it
> would be safe to say that, unless you go for baudrates as high as tens
> of millions, things should work well with just percent accuracy. A
> higher accuracy always definetely helps, but I believe it is not needed
> in this case.
>
>
> > Given you've abs() at the caller side, the error rate could be
> > underestimated, is underestimating OK?
> >
>
>
> Yes, this should be fine. While (both empirically and after looking
> stuff up) I noticed that in the case of negative error rates, their
> absolute value needs to be smaller than the one of positive error rates,
> it must be so by a very small margin that is negligible when estimating
> through percent accuracy.
OK. Thanks for checking.
--
i.
On 07.09.2022 12:36, Ilpo Järvinen wrote:
> On Tue, 6 Sep 2022, Sergiu Moga wrote:
>
>> Previously, the atmel serial driver did not take into account the
>> possibility of using the more customizable generic clock as its
>> baudrate generator. Unless there is a Fractional Part available to
>> increase accuracy, there is a high chance that we may be able to
>> generate a baudrate closer to the desired one by using the GCLK as the
>> clock source. Now, depending on the error rate between
>> the desired baudrate and the actual baudrate, the serial driver will
>> fallback on the generic clock. The generic clock must be provided
>> in the DT node of the serial that may need a more flexible clock source.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>>
>>
>>
>> v1 -> v2:
>> - take into account the different placement of the baudrate clock source
>> into the IP's Mode Register (USART vs UART)
>> - don't check for atmel_port->gclk != NULL
>> - use clk_round_rate instead of clk_set_rate + clk_get_rate
>> - remove clk_disable_unprepare from the end of the probe method
>>
>>
>>
>> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
>> 1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 6aa01ca5489c..b2b6fd6ea2a5 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -15,6 +15,7 @@
>> #include <linux/init.h>
>> #include <linux/serial.h>
>> #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> #include <linux/console.h>
>> #include <linux/sysrq.h>
>> #include <linux/tty_flip.h>
>> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
>> #endif
>>
>> #define ATMEL_ISR_PASS_LIMIT 256
>> +#define ERROR_RATE(desired_value, actual_value) \
>> + ((int)(100 - ((desired_value) * 100) / (actual_value)))
>
> Make a function instead.
>
Alright, I see the point of making it an inline function instead.
> Is percent accurate enough or would you perhaps want something slightly
> more accurate?
>
It is accurate enough for the all the baudrates I have tested. It
usually taps into the GCLK whenever high baudrates such as 921600 are
used. For 115200 for example, the error rate was slightly better in the
case of the peripheral clock and it acted accordingly, choosing the
latter as its baudrate source clock. I do not think that a higher
accuracy than this would be needed though. Say that using percent
accuracy yields that the error rates are equal, but the gclk would have
been better in this case by, say, a few 10 ^ -4, but the code logic does
not see it so it proceeds using the peripheral clock. In that case, the
error rate of the peripheral clock would still be low enough relative to
the desired baudrate for the communication to function properly.
The higher the baudrate, the lower the error rate must be in order for
things to go smoothly. For example, for a baudrate of 57600 I noticed
that even an error rate as big as 6% is still enough for the
communication to work properly, while in the case of 921600 anything
bigger than 2% and things do not go smoothly anymore. So I guess that it
would be safe to say that, unless you go for baudrates as high as tens
of millions, things should work well with just percent accuracy. A
higher accuracy always definetely helps, but I believe it is not needed
in this case.
> Given you've abs() at the caller side, the error rate could be
> underestimated, is underestimating OK?
>
Yes, this should be fine. While (both empirically and after looking
stuff up) I noticed that in the case of negative error rates, their
absolute value needs to be smaller than the one of positive error rates,
it must be so by a very small margin that is negligible when estimating
through percent accuracy.
> --
> i.
>
>> struct atmel_dma_buffer {
>> unsigned char *buf;
>> @@ -110,6 +113,7 @@ struct atmel_uart_char {
>> struct atmel_uart_port {
>> struct uart_port uart; /* uart */
>> struct clk *clk; /* uart clock */
>> + struct clk *gclk; /* uart generic clock */
>> int may_wakeup; /* cached value of device_may_wakeup for times we need to disable it */
>> u32 backup_imr; /* IMR saved during suspend */
>> int break_active; /* break being received */
>> @@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
>> * This is called on uart_close() or a suspend event.
>> */
>> clk_disable_unprepare(atmel_port->clk);
>> + if (__clk_is_enabled(atmel_port->gclk))
>> + clk_disable_unprepare(atmel_port->gclk);
>> break;
>> default:
>> dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
>> @@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> {
>> struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>> unsigned long flags;
>> - unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
>> + unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
>> + unsigned int baud, actual_baud, gclk_rate;
>>
>> /* save the current mode register */
>> mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
>> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
>> cd &= 65535;
>> }
>>
>> + /*
>> + * If there is no Fractional Part, there is a high chance that
>> + * we may be able to generate a baudrate closer to the desired one
>> + * if we use the GCLK as the clock source driving the baudrate
>> + * generator.
>> + */
>> + if (!atmel_port->has_frac_baudrate) {
>> + if (__clk_is_enabled(atmel_port->gclk))
>> + clk_disable_unprepare(atmel_port->gclk);
>> + gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
>> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
>> + if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
>> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
>> + clk_set_rate(atmel_port->gclk, 16 * baud);
>> + if (!clk_prepare_enable(atmel_port->gclk)) {
>> + if (atmel_port->is_usart) {
>> + mode &= ~ATMEL_US_USCLKS;
>> + mode |= ATMEL_US_USCLKS_GCLK;
>> + } else {
>> + mode &= ~ATMEL_UA_BRSRCCK;
>> + mode |= ATMEL_UA_BRSRCCK_GCLK;
>> + }
>> +
>> + /*
>> + * Set the Clock Divisor for GCLK to 1.
>> + * Since we were able to generate the smallest
>> + * multiple of the desired baudrate times 16,
>> + * then we surely can generate a bigger multiple
>> + * with the exact error rate for an equally increased
>> + * CD. Thus no need to take into account
>> + * a higher value for CD.
>> + */
>> + cd = 1;
>> + }
>> + }
>> + }
>> +
>> quot = cd | fp << ATMEL_US_FP_OFFSET;
>>
>> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
>> @@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
>> if (ret)
>> goto err;
>>
>> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
>> + if (IS_ERR(atmel_port->gclk)) {
>> + ret = PTR_ERR(atmel_port->gclk);
>> + goto err;
>> + }
>> +
>> ret = atmel_init_port(atmel_port, pdev);
>> if (ret)
>> goto err_clk_disable_unprepare;
>>
On 06/09/2022 15:55, Sergiu Moga wrote:
> Another functionality of FLEXCOM is that of SPI. In order for
> the proper validation of the SPI children nodes through the binding
> to occur, the proper binding for SPI must be referenced.
>
> Signed-off-by: Sergiu Moga <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 06/09/2022 15:55, Sergiu Moga wrote:
> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
> in order to highlight the incremental characteristics of the SAM9X60
> serial IP.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
> v1 -> v2:
> - Nothing, this patch was not here before
>
>
> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> index b25535b7a4d2..4d80006963c7 100644
> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> @@ -26,6 +26,8 @@ properties:
> - items:
> - const: microchip,sam9x60-dbgu
> - const: microchip,sam9x60-usart
> + - const: atmel,at91sam9260-dbgu
> + - const: atmel,at91sam9260-usart
This is weird. You say in commit msg to "highlight the incremental
characteristics" but you basically change here existing compatibles.
This is not enum, but a list.
Best regards,
Krzysztof
On 06/09/2022 15:55, Sergiu Moga wrote:
> FLEXCOM, among other functionalities, has the ability to offer the USART
> serial communication protocol. To have the FLEXCOM binding properly
> validate its USART children nodes, we must reference the correct binding.
> To differentiate between the SPI of FLEXCOM and the SPI of USART in SPI
> mode, use the clock-names property, since the latter's respective
> property is supposed to contain the "usart" string.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
> v1 -> v2:
> - Nothing
>
>
>
> .../bindings/mfd/atmel,sama5d2-flexcom.yaml | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
> index 0db0f2728b65..b5fb509f07ec 100644
> --- a/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
> +++ b/Documentation/devicetree/bindings/mfd/atmel,sama5d2-flexcom.yaml
> @@ -72,13 +72,21 @@ properties:
>
> patternProperties:
> "^serial@[0-9a-f]+$":
> - type: object
> + $ref: /schemas/serial/atmel,at91-usart.yaml
> description:
> - Child node describing USART. See atmel-usart.txt for details
> - of USART bindings.
> + Child node describing USART.
>
> "^spi@[0-9a-f]+$":
> - $ref: /schemas/spi/atmel,at91rm9200-spi.yaml
> + allOf:
> + - if:
> + properties:
> + clock-names:
> + contains:
> + const: usart
Devices are not different because they have or have not clock. Devices
are different... because they are simply different models, so this
should be different compatible.
Best regards,
Krzysztof
On 06/09/2022 15:55, Sergiu Moga wrote:
> The Devicetree nodes for FLEXCOM's USART can also have an alternative
> clock source for the baudrate generator (other than the peripheral
> clock), namely the Generick Clock. Thus make the binding aware of
> this potential clock that someone may place in the clock related
> properties of the USART node.
Last sentence is confusing - what is the potential? Just skip it.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
>
> v1 -> v2:
> - Nothing, this patch was not here before
You have confusing order of patches. Bindings mixed with DTS mixed with
drivers. Keep things ordered.
1. DTS changes needed for aligning to schema.
2. all bindings
3. rest
Best regards,
Krzysztof
On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
> On 06/09/2022 15:55, Sergiu Moga wrote:
>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>> in order to highlight the incremental characteristics of the SAM9X60
>> serial IP.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
>>
>>
>> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> index b25535b7a4d2..4d80006963c7 100644
>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>> @@ -26,6 +26,8 @@ properties:
>> - items:
>> - const: microchip,sam9x60-dbgu
>> - const: microchip,sam9x60-usart
>> + - const: atmel,at91sam9260-dbgu
>> + - const: atmel,at91sam9260-usart
>
> This is weird. You say in commit msg to "highlight the incremental
> characteristics" but you basically change here existing compatibles.
Does "show that they are incremental IP's" sound better then?
> This is not enum, but a list.
>
What do you mean by this? I know it is a list, I specified so in the
commit message.
>
> Best regards,
> Krzysztof
Regards,
Sergiu
On 08.09.2022 15:35, Krzysztof Kozlowski wrote:
> On 06/09/2022 15:55, Sergiu Moga wrote:
>> The Devicetree nodes for FLEXCOM's USART can also have an alternative
>> clock source for the baudrate generator (other than the peripheral
>> clock), namely the Generick Clock. Thus make the binding aware of
>> this potential clock that someone may place in the clock related
>> properties of the USART node.
>
> Last sentence is confusing - what is the potential? Just skip it.
>
I am sorry, I meant to say "possible". No idea how I ended up writing
"potential". Guess I will just skip any adjectives entirely.
>>
>> Signed-off-by: Sergiu Moga <[email protected]>
>> ---
>>
>>
>>
>> v1 -> v2:
>> - Nothing, this patch was not here before
>
> You have confusing order of patches. Bindings mixed with DTS mixed with
> drivers. Keep things ordered.
> 1. DTS changes needed for aligning to schema.
> 2. all bindings
> 3. rest
>
Alright, it makes sense, will do so. I thought it would have looked
better if I were to add the gclk in the schema after adding it in the
drivers.
Other than that I hope I got the example[1] you have previously given me
right.
[1]
https://elixir.bootlin.com/linux/v6.0-rc4/source/Documentation/devicetree/bindings/example-schema.yaml#L91
> Best regards,
> Krzysztof
Thanks,
Sergiu
On Thu, Sep 08, 2022 at 03:15:44PM +0000, [email protected] wrote:
> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
> > On 06/09/2022 15:55, Sergiu Moga wrote:
> >> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
> >> in order to highlight the incremental characteristics of the SAM9X60
> >> serial IP.
> >>
> >> Signed-off-by: Sergiu Moga <[email protected]>
> >> ---
> >>
> >>
> >> v1 -> v2:
> >> - Nothing, this patch was not here before
> >>
> >>
> >> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> index b25535b7a4d2..4d80006963c7 100644
> >> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
> >> @@ -26,6 +26,8 @@ properties:
> >> - items:
> >> - const: microchip,sam9x60-dbgu
> >> - const: microchip,sam9x60-usart
> >> + - const: atmel,at91sam9260-dbgu
> >> + - const: atmel,at91sam9260-usart
> >
> > This is weird. You say in commit msg to "highlight the incremental
> > characteristics" but you basically change here existing compatibles.
>
>
> Does "show that they are incremental IP's" sound better then?
>
>
> > This is not enum, but a list.
> >
>
>
> What do you mean by this? I know it is a list, I specified so in the
> commit message.
You are saying that compatible must be exactly the 4 strings above in
the order listed. You need another entry with another 'items' list.
Rob
On 09.09.2022 04:36, Rob Herring wrote:
> On Thu, Sep 08, 2022 at 03:15:44PM +0000, [email protected] wrote:
>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>> serial IP.
>>>>
>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>> ---
>>>>
>>>>
>>>> v1 -> v2:
>>>> - Nothing, this patch was not here before
>>>>
>>>>
>>>> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>> index b25535b7a4d2..4d80006963c7 100644
>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>> @@ -26,6 +26,8 @@ properties:
>>>> - items:
>>>> - const: microchip,sam9x60-dbgu
>>>> - const: microchip,sam9x60-usart
>>>> + - const: atmel,at91sam9260-dbgu
>>>> + - const: atmel,at91sam9260-usart
>>>
>>> This is weird. You say in commit msg to "highlight the incremental
>>> characteristics" but you basically change here existing compatibles.
>>
>>
>> Does "show that they are incremental IP's" sound better then?
>>
>>
>>> This is not enum, but a list.
>>>
>>
>>
>> What do you mean by this? I know it is a list, I specified so in the
>> commit message.
>
> You are saying that compatible must be exactly the 4 strings above in
> the order listed. You need another entry with another 'items' list.
>
> Rob
That is what was intended though: a list of the 4 compatibles in that
exact order. The 4th patch of this series also ensures that all 9x60
nodes have that exact list of 4 compatibles.
Regards,
Sergiu
On 06.09.2022 16:55, Sergiu Moga wrote:
> Previously, the atmel serial driver did not take into account the
> possibility of using the more customizable generic clock as its
> baudrate generator. Unless there is a Fractional Part available to
> increase accuracy, there is a high chance that we may be able to
> generate a baudrate closer to the desired one by using the GCLK as the
> clock source. Now, depending on the error rate between
> the desired baudrate and the actual baudrate, the serial driver will
> fallback on the generic clock. The generic clock must be provided
> in the DT node of the serial that may need a more flexible clock source.
>
> Signed-off-by: Sergiu Moga <[email protected]>
> ---
>
>
>
> v1 -> v2:
> - take into account the different placement of the baudrate clock source
> into the IP's Mode Register (USART vs UART)
> - don't check for atmel_port->gclk != NULL
> - use clk_round_rate instead of clk_set_rate + clk_get_rate
> - remove clk_disable_unprepare from the end of the probe method
>
>
>
> drivers/tty/serial/atmel_serial.c | 52 ++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 6aa01ca5489c..b2b6fd6ea2a5 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -15,6 +15,7 @@
> #include <linux/init.h>
> #include <linux/serial.h>
> #include <linux/clk.h>
> +#include <linux/clk-provider.h>
> #include <linux/console.h>
> #include <linux/sysrq.h>
> #include <linux/tty_flip.h>
> @@ -77,6 +78,8 @@ static void atmel_stop_rx(struct uart_port *port);
> #endif
>
> #define ATMEL_ISR_PASS_LIMIT 256
> +#define ERROR_RATE(desired_value, actual_value) \
> + ((int)(100 - ((desired_value) * 100) / (actual_value)))
>
> struct atmel_dma_buffer {
> unsigned char *buf;
> @@ -110,6 +113,7 @@ struct atmel_uart_char {
> struct atmel_uart_port {
> struct uart_port uart; /* uart */
> struct clk *clk; /* uart clock */
> + struct clk *gclk; /* uart generic clock */
> int may_wakeup; /* cached value of device_may_wakeup for times we need to disable it */
> u32 backup_imr; /* IMR saved during suspend */
> int break_active; /* break being received */
> @@ -2117,6 +2121,8 @@ static void atmel_serial_pm(struct uart_port *port, unsigned int state,
> * This is called on uart_close() or a suspend event.
> */
> clk_disable_unprepare(atmel_port->clk);
> + if (__clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
> break;
> default:
> dev_err(port->dev, "atmel_serial: unknown pm %d\n", state);
> @@ -2131,7 +2137,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> {
> struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> unsigned long flags;
> - unsigned int old_mode, mode, imr, quot, baud, div, cd, fp = 0;
> + unsigned int old_mode, mode, imr, quot, div, cd, fp = 0;
> + unsigned int baud, actual_baud, gclk_rate;
>
> /* save the current mode register */
> mode = old_mode = atmel_uart_readl(port, ATMEL_US_MR);
> @@ -2297,6 +2304,43 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
> cd &= 65535;
> }
>
> + /*
> + * If there is no Fractional Part, there is a high chance that
> + * we may be able to generate a baudrate closer to the desired one
> + * if we use the GCLK as the clock source driving the baudrate
> + * generator.
> + */
> + if (!atmel_port->has_frac_baudrate) {
> + if (__clk_is_enabled(atmel_port->gclk))
> + clk_disable_unprepare(atmel_port->gclk);
> + gclk_rate = clk_round_rate(atmel_port->gclk, 16 * baud);
> + actual_baud = clk_get_rate(atmel_port->clk) / (16 * cd);
> + if (gclk_rate && abs(ERROR_RATE(baud, actual_baud)) >
> + abs(ERROR_RATE(baud, gclk_rate / 16))) {
> + clk_set_rate(atmel_port->gclk, 16 * baud);
This is a personal taste: I would do like this:
ret = clk_prepare_enable();
if (ret)
goto clk_prepare_failure;
// and here the rest of the code...
> + if (!clk_prepare_enable(atmel_port->gclk)) {
> + if (atmel_port->is_usart) {
> + mode &= ~ATMEL_US_USCLKS;
> + mode |= ATMEL_US_USCLKS_GCLK;
> + } else {
> + mode &= ~ATMEL_UA_BRSRCCK;
> + mode |= ATMEL_UA_BRSRCCK_GCLK;
> + }
> +
> + /*
> + * Set the Clock Divisor for GCLK to 1.
> + * Since we were able to generate the smallest
> + * multiple of the desired baudrate times 16,
> + * then we surely can generate a bigger multiple
> + * with the exact error rate for an equally increased
> + * CD. Thus no need to take into account
> + * a higher value for CD.
> + */
> + cd = 1;
> + }
> + }
> + }
> +
clk_prepare_failure: // or other label name
> quot = cd | fp << ATMEL_US_FP_OFFSET;
>
> if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
> @@ -2892,6 +2936,12 @@ static int atmel_serial_probe(struct platform_device *pdev)
> if (ret)
> goto err;
>
> + atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
> + if (IS_ERR(atmel_port->gclk)) {
> + ret = PTR_ERR(atmel_port->gclk);
> + goto err;
This should be:
goto err_clk_disable_unprepare;
to disable the peripheral clock previously enabled.
> + }
> +
> ret = atmel_init_port(atmel_port, pdev);
> if (ret)
> goto err_clk_disable_unprepare;
On 12/09/2022 09:45, [email protected] wrote:
> On 09.09.2022 04:36, Rob Herring wrote:
>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, [email protected] wrote:
>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>> serial IP.
>>>>>
>>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>>> ---
>>>>>
>>>>>
>>>>> v1 -> v2:
>>>>> - Nothing, this patch was not here before
>>>>>
>>>>>
>>>>> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>> @@ -26,6 +26,8 @@ properties:
>>>>> - items:
>>>>> - const: microchip,sam9x60-dbgu
>>>>> - const: microchip,sam9x60-usart
>>>>> + - const: atmel,at91sam9260-dbgu
>>>>> + - const: atmel,at91sam9260-usart
>>>>
>>>> This is weird. You say in commit msg to "highlight the incremental
>>>> characteristics" but you basically change here existing compatibles.
>>>
>>>
>>> Does "show that they are incremental IP's" sound better then?
>>>
>>>
>>>> This is not enum, but a list.
>>>>
>>>
>>>
>>> What do you mean by this? I know it is a list, I specified so in the
>>> commit message.
>>
>> You are saying that compatible must be exactly the 4 strings above in
>> the order listed. You need another entry with another 'items' list.
>>
>> Rob
>
>
> That is what was intended though: a list of the 4 compatibles in that
> exact order. The 4th patch of this series also ensures that all 9x60
> nodes have that exact list of 4 compatibles.
The commit msg suggest otherwise - two options, because it is
incremental... But this one is not really incremental - you require this
one, only one, configuration. It's in general fine, but commit msg
should reflect what you are really intend to do here and why you are
doing it.
Best regards,
Krzysztof
On 12.09.2022 13:44, Krzysztof Kozlowski wrote:
> On 12/09/2022 09:45, [email protected] wrote:
>> On 09.09.2022 04:36, Rob Herring wrote:
>>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, [email protected] wrote:
>>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>>> serial IP.
>>>>>>
>>>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>>>> ---
>>>>>>
>>>>>>
>>>>>> v1 -> v2:
>>>>>> - Nothing, this patch was not here before
>>>>>>
>>>>>>
>>>>>> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>> - items:
>>>>>> - const: microchip,sam9x60-dbgu
>>>>>> - const: microchip,sam9x60-usart
>>>>>> + - const: atmel,at91sam9260-dbgu
>>>>>> + - const: atmel,at91sam9260-usart
>>>>>
>>>>> This is weird. You say in commit msg to "highlight the incremental
>>>>> characteristics" but you basically change here existing compatibles.
>>>>
>>>>
>>>> Does "show that they are incremental IP's" sound better then?
>>>>
>>>>
>>>>> This is not enum, but a list.
>>>>>
>>>>
>>>>
>>>> What do you mean by this? I know it is a list, I specified so in the
>>>> commit message.
>>>
>>> You are saying that compatible must be exactly the 4 strings above in
>>> the order listed. You need another entry with another 'items' list.
>>>
>>> Rob
>>
>>
>> That is what was intended though: a list of the 4 compatibles in that
>> exact order. The 4th patch of this series also ensures that all 9x60
>> nodes have that exact list of 4 compatibles.
>
> The commit msg suggest otherwise - two options, because it is
> incremental... But this one is not really incremental - you require this
> one, only one, configuration. It's in general fine, but commit msg
> should reflect what you are really intend to do here and why you are
> doing it.
>
>
> Best regards,
> Krzysztof
My apologies, I still do not understand what is wrong with the commit
message. My intention was to ensure that every 9x60 usart compatible is
followed by the 9260 compatibles because the 9x60 serial IP is an
improvement over the 9260 one. Would you prefer it to be just the first
part of the commit message: `Add the AT91SAM9260 serial compatibles to
the list of SAM9X60 compatibles`? That way it would really only be what
the commit does.
Thank you,
Sergiu
On 12/09/2022 15:09, [email protected] wrote:
> On 12.09.2022 13:44, Krzysztof Kozlowski wrote:
>> On 12/09/2022 09:45, [email protected] wrote:
>>> On 09.09.2022 04:36, Rob Herring wrote:
>>>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, [email protected] wrote:
>>>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>>>> serial IP.
>>>>>>>
>>>>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>>
>>>>>>> v1 -> v2:
>>>>>>> - Nothing, this patch was not here before
>>>>>>>
>>>>>>>
>>>>>>> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>>> - items:
>>>>>>> - const: microchip,sam9x60-dbgu
>>>>>>> - const: microchip,sam9x60-usart
>>>>>>> + - const: atmel,at91sam9260-dbgu
>>>>>>> + - const: atmel,at91sam9260-usart
>>>>>>
>>>>>> This is weird. You say in commit msg to "highlight the incremental
>>>>>> characteristics" but you basically change here existing compatibles.
>>>>>
>>>>>
>>>>> Does "show that they are incremental IP's" sound better then?
>>>>>
>>>>>
>>>>>> This is not enum, but a list.
>>>>>>
>>>>>
>>>>>
>>>>> What do you mean by this? I know it is a list, I specified so in the
>>>>> commit message.
>>>>
>>>> You are saying that compatible must be exactly the 4 strings above in
>>>> the order listed. You need another entry with another 'items' list.
>>>>
>>>> Rob
>>>
>>>
>>> That is what was intended though: a list of the 4 compatibles in that
>>> exact order. The 4th patch of this series also ensures that all 9x60
>>> nodes have that exact list of 4 compatibles.
>>
>> The commit msg suggest otherwise - two options, because it is
>> incremental... But this one is not really incremental - you require this
>> one, only one, configuration. It's in general fine, but commit msg
>> should reflect what you are really intend to do here and why you are
>> doing it.
>>
>>
>> Best regards,
>> Krzysztof
>
>
> My apologies, I still do not understand what is wrong with the commit
> message. My intention was to ensure that every 9x60 usart compatible is
> followed by the 9260 compatibles because the 9x60 serial IP is an
> improvement over the 9260 one. Would you prefer it to be just the first
> part of the commit message: `Add the AT91SAM9260 serial compatibles to
> the list of SAM9X60 compatibles`? That way it would really only be what
> the commit does.
Let me rephrase it:
What your commit is doing is requiring additional fallback compatibles.
Therefore the commit msg should answer - why do you require additional
fallback compatibles?
Incremental characteristics sound to me optional. I can increment
sam9x60 with something or I can skip it. But you are not doing it...
sam9x60 was already there and now you require a fallback.
Best regards,
Krzysztof
On 13/09/2022 11:19, [email protected] wrote:
>>
>> Let me rephrase it:
>>
>> What your commit is doing is requiring additional fallback compatibles.
>> Therefore the commit msg should answer - why do you require additional
>> fallback compatibles?
>>
>
>
> The additional fallback compatibles are required because the driver in
> question only knows about the atmel,at91sam9260-usart compatible.
> Furthermore, it is also a better representation of the fact that the
> serial IP of 9x60 is an improvement over the serial IP of 9260 (it
> contains more hardware features not yet implemented in the driver).
>
>
>> Incremental characteristics sound to me optional. I can increment
>> sam9x60 with something or I can skip it. But you are not doing it...
>> sam9x60 was already there and now you require a fallback.
>>
>> Best regards,
>> Krzysztof
>
> So, what is your opinion on the following commit message:
>
> "Fix sam9x60 compatible list by adding the sam9260 compatibles as
> fallback, since the atmel_serial driver only knows of the latter's
> compatible. The atmel_serial driver only has knowledge of the sam9260
> compatible because it does not have the sam9x60's serial IP specific
> features implemented yet and adding an empty compatible without adding
> support specific to that compatible would be misleading. Thus prefer the
> fallback mechanism in the detriment of adding an empty compatible in the
> driver."
It's fine. Also could work:
"Require sam9260 fallback compatible for sam9x60, because sam9x60 is
fully compatible with sam9260 and Linux driver requires the latter."
If it fixes any observable issue like lack of driver binding to DTS, you
can also mention that.
Best regards,
Krzysztof
On 13.09.2022 12:30, Krzysztof Kozlowski wrote:
> On 13/09/2022 11:19, [email protected] wrote:
>>>
>>> Let me rephrase it:
>>>
>>> What your commit is doing is requiring additional fallback compatibles.
>>> Therefore the commit msg should answer - why do you require additional
>>> fallback compatibles?
>>>
>>
>>
>> The additional fallback compatibles are required because the driver in
>> question only knows about the atmel,at91sam9260-usart compatible.
>> Furthermore, it is also a better representation of the fact that the
>> serial IP of 9x60 is an improvement over the serial IP of 9260 (it
>> contains more hardware features not yet implemented in the driver).
>>
>>
>>> Incremental characteristics sound to me optional. I can increment
>>> sam9x60 with something or I can skip it. But you are not doing it...
>>> sam9x60 was already there and now you require a fallback.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> So, what is your opinion on the following commit message:
>>
>> "Fix sam9x60 compatible list by adding the sam9260 compatibles as
>> fallback, since the atmel_serial driver only knows of the latter's
>> compatible. The atmel_serial driver only has knowledge of the sam9260
>> compatible because it does not have the sam9x60's serial IP specific
>> features implemented yet and adding an empty compatible without adding
>> support specific to that compatible would be misleading. Thus prefer the
>> fallback mechanism in the detriment of adding an empty compatible in the
>> driver."
>
> It's fine. Also could work:
>
> "Require sam9260 fallback compatible for sam9x60, because sam9x60 is
> fully compatible with sam9260 and Linux driver requires the latter."
>
This version looks better indeed. Sums it all up and is only 2 lines :).
Thank you very much for the suggestion it is greatly appeciated.
> If it fixes any observable issue like lack of driver binding to DTS, you
> can also mention that.
>
> Best regards,
> Krzysztof
Thanks,
Sergiu
On 13.09.2022 11:58, Krzysztof Kozlowski wrote:
> On 12/09/2022 15:09, [email protected] wrote:
>> On 12.09.2022 13:44, Krzysztof Kozlowski wrote:
>>> On 12/09/2022 09:45, [email protected] wrote:
>>>> On 09.09.2022 04:36, Rob Herring wrote:
>>>>> On Thu, Sep 08, 2022 at 03:15:44PM +0000, [email protected] wrote:
>>>>>> On 08.09.2022 15:30, Krzysztof Kozlowski wrote:
>>>>>>> On 06/09/2022 15:55, Sergiu Moga wrote:
>>>>>>>> Add the AT91SAM9260 serial compatibles to the list of SAM9X60 compatibles
>>>>>>>> in order to highlight the incremental characteristics of the SAM9X60
>>>>>>>> serial IP.
>>>>>>>>
>>>>>>>> Signed-off-by: Sergiu Moga <[email protected]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>
>>>>>>>> v1 -> v2:
>>>>>>>> - Nothing, this patch was not here before
>>>>>>>>
>>>>>>>>
>>>>>>>> Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml | 2 ++
>>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>>> index b25535b7a4d2..4d80006963c7 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/serial/atmel,at91-usart.yaml
>>>>>>>> @@ -26,6 +26,8 @@ properties:
>>>>>>>> - items:
>>>>>>>> - const: microchip,sam9x60-dbgu
>>>>>>>> - const: microchip,sam9x60-usart
>>>>>>>> + - const: atmel,at91sam9260-dbgu
>>>>>>>> + - const: atmel,at91sam9260-usart
>>>>>>>
>>>>>>> This is weird. You say in commit msg to "highlight the incremental
>>>>>>> characteristics" but you basically change here existing compatibles.
>>>>>>
>>>>>>
>>>>>> Does "show that they are incremental IP's" sound better then?
>>>>>>
>>>>>>
>>>>>>> This is not enum, but a list.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> What do you mean by this? I know it is a list, I specified so in the
>>>>>> commit message.
>>>>>
>>>>> You are saying that compatible must be exactly the 4 strings above in
>>>>> the order listed. You need another entry with another 'items' list.
>>>>>
>>>>> Rob
>>>>
>>>>
>>>> That is what was intended though: a list of the 4 compatibles in that
>>>> exact order. The 4th patch of this series also ensures that all 9x60
>>>> nodes have that exact list of 4 compatibles.
>>>
>>> The commit msg suggest otherwise - two options, because it is
>>> incremental... But this one is not really incremental - you require this
>>> one, only one, configuration. It's in general fine, but commit msg
>>> should reflect what you are really intend to do here and why you are
>>> doing it.
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>
>>
>> My apologies, I still do not understand what is wrong with the commit
>> message. My intention was to ensure that every 9x60 usart compatible is
>> followed by the 9260 compatibles because the 9x60 serial IP is an
>> improvement over the 9260 one. Would you prefer it to be just the first
>> part of the commit message: `Add the AT91SAM9260 serial compatibles to
>> the list of SAM9X60 compatibles`? That way it would really only be what
>> the commit does.
>
> Let me rephrase it:
>
> What your commit is doing is requiring additional fallback compatibles.
> Therefore the commit msg should answer - why do you require additional
> fallback compatibles?
>
The additional fallback compatibles are required because the driver in
question only knows about the atmel,at91sam9260-usart compatible.
Furthermore, it is also a better representation of the fact that the
serial IP of 9x60 is an improvement over the serial IP of 9260 (it
contains more hardware features not yet implemented in the driver).
> Incremental characteristics sound to me optional. I can increment
> sam9x60 with something or I can skip it. But you are not doing it...
> sam9x60 was already there and now you require a fallback.
>
> Best regards,
> Krzysztof
So, what is your opinion on the following commit message:
"Fix sam9x60 compatible list by adding the sam9260 compatibles as
fallback, since the atmel_serial driver only knows of the latter's
compatible. The atmel_serial driver only has knowledge of the sam9260
compatible because it does not have the sam9x60's serial IP specific
features implemented yet and adding an empty compatible without adding
support specific to that compatible would be misleading. Thus prefer the
fallback mechanism in the detriment of adding an empty compatible in the
driver."
Thanks,
Sergiu