[PATCH 0/2] serial: 8250_dw: Introduce SG2044 uart support.

[PATCH 0/2] serial: 8250_dw: Introduce SG2044 uart support.

[Inochi Amaoto]

SG2044 relys on an internal divisor when calculating bitrate, which
means a wrong clock for the most common bitrates. So a quirk is needed
for this uart device to skip the set rate call and only relys on the
internal UART divisor.

Inochi Amaoto (2):
  dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts
  serial: 8250_dw: Add Sophgo SG2044 quirk

 .../devicetree/bindings/serial/snps-dw-apb-uart.yaml        | 4 ++++
 drivers/tty/serial/8250/8250_dw.c                           | 6 ++++++
 2 files changed, 10 insertions(+)

--
2.47.0

[PATCH 1/2] dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts

[Inochi Amaoto]

Add compatibles string for the Sophgo SG2044 uarts.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 .../devicetree/bindings/serial/snps-dw-apb-uart.yaml          | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
index 4cdb0dcaccf3..6963f89a1848 100644
--- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
@@ -58,6 +58,10 @@ properties:
               - brcm,bcm11351-dw-apb-uart
               - brcm,bcm21664-dw-apb-uart
           - const: snps,dw-apb-uart
+      - items:
+          - enum:
+              - sophgo,sg2044-uart
+          - const: snps,dw-apb-uart
       - items:
           - enum:
               - starfive,jh7100-hsuart
-- 
2.47.0

Re: [PATCH 1/2] dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts

[Krzysztof Kozlowski]

On Thu, Oct 10, 2024 at 07:39:05AM +0800, Inochi Amaoto wrote:
> Add compatibles string for the Sophgo SG2044 uarts.

This we see from the diff, say something about hardware.

> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  .../devicetree/bindings/serial/snps-dw-apb-uart.yaml          | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> index 4cdb0dcaccf3..6963f89a1848 100644
> --- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> @@ -58,6 +58,10 @@ properties:
>                - brcm,bcm11351-dw-apb-uart
>                - brcm,bcm21664-dw-apb-uart
>            - const: snps,dw-apb-uart
> +      - items:
> +          - enum:
> +              - sophgo,sg2044-uart

I would just add it to starfive enum, but this is fine as well.

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

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts

[Inochi Amaoto]

On Thu, Oct 10, 2024 at 08:12:41AM +0200, Krzysztof Kozlowski wrote:
> On Thu, Oct 10, 2024 at 07:39:05AM +0800, Inochi Amaoto wrote:
> > Add compatibles string for the Sophgo SG2044 uarts.
> 
> This we see from the diff, say something about hardware.
> 

The reason for this compatiable (and the hardware) is mainly in the
next patch. Will it be better to submit a new verion with improved
description? If so, I wonder whether I can reserve your ack.

> > 
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > ---
> >  .../devicetree/bindings/serial/snps-dw-apb-uart.yaml          | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> > index 4cdb0dcaccf3..6963f89a1848 100644
> > --- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
> > @@ -58,6 +58,10 @@ properties:
> >                - brcm,bcm11351-dw-apb-uart
> >                - brcm,bcm21664-dw-apb-uart
> >            - const: snps,dw-apb-uart
> > +      - items:
> > +          - enum:
> > +              - sophgo,sg2044-uart
> 
> I would just add it to starfive enum, but this is fine as well.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof
> 

Thanks.

Regard,
Inochi

Re: [PATCH 1/2] dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 04:23:05PM +0800, Inochi Amaoto wrote:
> On Thu, Oct 10, 2024 at 08:12:41AM +0200, Krzysztof Kozlowski wrote:
> > On Thu, Oct 10, 2024 at 07:39:05AM +0800, Inochi Amaoto wrote:
> > > Add compatibles string for the Sophgo SG2044 uarts.
> > 
> > This we see from the diff, say something about hardware.
> 
> The reason for this compatiable (and the hardware) is mainly in the
> next patch. Will it be better to submit a new verion with improved
> description? If so, I wonder whether I can reserve your ack.
> 
> > I would just add it to starfive enum, but this is fine as well.

Even after reading the second patch I don't understand why you shouldn't re-use
the starfive compatible or make a new one that covers this quirk? At least I would
see that as second patch is basically not needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts

[Inochi Amaoto]

On Thu, Oct 10, 2024 at 05:54:48PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 04:23:05PM +0800, Inochi Amaoto wrote:
> > On Thu, Oct 10, 2024 at 08:12:41AM +0200, Krzysztof Kozlowski wrote:
> > > On Thu, Oct 10, 2024 at 07:39:05AM +0800, Inochi Amaoto wrote:
> > > > Add compatibles string for the Sophgo SG2044 uarts.
> > > 
> > > This we see from the diff, say something about hardware.
> > 
> > The reason for this compatiable (and the hardware) is mainly in the
> > next patch. Will it be better to submit a new verion with improved
> > description? If so, I wonder whether I can reserve your ack.
> > 
> > > I would just add it to starfive enum, but this is fine as well.
> 
> Even after reading the second patch I don't understand why you shouldn't re-use
> the starfive compatible or make a new one that covers this quirk? At least I would
> see that as second patch is basically not needed.
> 

I do not think it is good to re-use the starfive compatible, it is weird
that a sophgo SoC has a peripheral on the statfive SoC. Another suggestion
for adding a new one that covers the quirk is a good idea for me, but I am
not sure whether it may cause some misunderstanding like reuse the starfive
compatible. If the second one is possible, it is OK for me to drop the second
patch.

Regard,
Inochi

[PATCH 2/2] serial: 8250_dw: Add Sophgo SG2044 quirk

[Inochi Amaoto]

SG2044 relys on an internal divisor when calculating bitrate, which
means a wrong clock for the most common bitrates. So add a quirk for
this uart device to skip the set rate call and only relys on the
internal UART divisor.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/tty/serial/8250/8250_dw.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index ab9e7f204260..6b7c75670f29 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -750,6 +750,11 @@ static const struct dw8250_platform_data dw8250_renesas_rzn1_data = {
 	.quirks = DW_UART_QUIRK_CPR_VALUE | DW_UART_QUIRK_IS_DMA_FC,
 };
 
+static const struct dw8250_platform_data dw8250_sophgo_sg2044_data = {
+	.usr_reg = DW_UART_USR,
+	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
+};
+
 static const struct dw8250_platform_data dw8250_starfive_jh7100_data = {
 	.usr_reg = DW_UART_USR,
 	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
@@ -760,6 +765,7 @@ static const struct of_device_id dw8250_of_match[] = {
 	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
 	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
 	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
+	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_sophgo_sg2044_data },
 	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
 	{ /* Sentinel */ }
 };
-- 
2.47.0

Re: [PATCH 2/2] serial: 8250_dw: Add Sophgo SG2044 quirk

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 07:39:06AM +0800, Inochi Amaoto wrote:
> SG2044 relys on an internal divisor when calculating bitrate, which
> means a wrong clock for the most common bitrates. So add a quirk for
> this uart device to skip the set rate call and only relys on the
> internal UART divisor.

...

> +static const struct dw8250_platform_data dw8250_sophgo_sg2044_data = {
> +	.usr_reg = DW_UART_USR,
> +	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> +};
> +
>  static const struct dw8250_platform_data dw8250_starfive_jh7100_data = {
>  	.usr_reg = DW_UART_USR,
>  	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,

For the bare minimum this should be deduplicated as to have one record for now.

static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
	.usr_reg = DW_UART_USR,
	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
};

If we need different quirks in the future, they can be split again.
Or, if you certain that new quirks will come, mention this in
the commit message.

...

>  	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
>  	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
>  	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
> +	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_sophgo_sg2044_data },
>  	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },

I think my proposal for having a common compatible for those two is a no-go
as compatible strings are for the (unique) hardware and shouldn't be abstracted
based on some Linux or other OS shortcuts / quirks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] serial: 8250_dw: Add Sophgo SG2044 quirk

[Inochi Amaoto]

On Thu, Oct 10, 2024 at 06:00:34PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 07:39:06AM +0800, Inochi Amaoto wrote:
> > SG2044 relys on an internal divisor when calculating bitrate, which
> > means a wrong clock for the most common bitrates. So add a quirk for
> > this uart device to skip the set rate call and only relys on the
> > internal UART divisor.
> 
> ...
> 
> > +static const struct dw8250_platform_data dw8250_sophgo_sg2044_data = {
> > +	.usr_reg = DW_UART_USR,
> > +	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> > +};
> > +
> >  static const struct dw8250_platform_data dw8250_starfive_jh7100_data = {
> >  	.usr_reg = DW_UART_USR,
> >  	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> 
> For the bare minimum this should be deduplicated as to have one record for now.
> 
> static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
> 	.usr_reg = DW_UART_USR,
> 	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> };
> 
> If we need different quirks in the future, they can be split again.
> Or, if you certain that new quirks will come, mention this in
> the commit message.
> 

Yes, renaming this quirk as a common one is better. I will prefer if
this patch is necessary. Duplication is not a good idea.

> ...
> 
> >  	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
> >  	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
> >  	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
> > +	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_sophgo_sg2044_data },
> >  	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
> 
> I think my proposal for having a common compatible for those two is a no-go
> as compatible strings are for the (unique) hardware and shouldn't be abstracted
> based on some Linux or other OS shortcuts / quirks.
> 

Yes, a common compatible is not a good idea. But it is OK to share a common quirk,
which means they have the same problem.

Regards,
Inochi

Re: [PATCH 0/2] serial: 8250_dw: Introduce SG2044 uart support.

[Chen Wang]

Hi, Inochi,

Can you please post this patchset to linux-riscv next time too?

Thanks,

Chen

On 2024/10/10 7:39, Inochi Amaoto wrote:
> SG2044 relys on an internal divisor when calculating bitrate, which
> means a wrong clock for the most common bitrates. So a quirk is needed
> for this uart device to skip the set rate call and only relys on the
> internal UART divisor.
>
> Inochi Amaoto (2):
>    dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts
>    serial: 8250_dw: Add Sophgo SG2044 quirk
>
>   .../devicetree/bindings/serial/snps-dw-apb-uart.yaml        | 4 ++++
>   drivers/tty/serial/8250/8250_dw.c                           | 6 ++++++
>   2 files changed, 10 insertions(+)
>
> --
> 2.47.0
>