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

[PATCH v2 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.

Changed from v1:
1. patch 1: improve the bindings commit message.
2. patch 2: rename jh7100 quirk and rename the quirk to
            dw8250_skip_set_rate_data.

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                            | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

--
2.47.0

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

[Inochi Amaoto]

The UART of SG2044 is modified version of the standard Synopsys
DesignWare UART. The UART on SG2044 relys on the internal divisor
and can not set right clock rate for the common bitrates.

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 v2 1/2] dt-bindings: serial: snps-dw-apb-uart: Add Sophgo SG2044 uarts

[Conor Dooley]

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

On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> The UART of SG2044 is modified version of the standard Synopsys
> DesignWare UART. The UART on SG2044 relys on the internal divisor
> and can not set right clock rate for the common bitrates.
> 
> 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

Why does each vendor have an items entry of its own? Seems like needless
clutter of the file IMO, except for the renesas bit.


Cheers,
Conor.

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

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

[Inochi Amaoto]

On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > The UART of SG2044 is modified version of the standard Synopsys
> > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > and can not set right clock rate for the common bitrates.
> > 
> > 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
> 
> Why does each vendor have an items entry of its own? Seems like needless
> clutter of the file IMO, except for the renesas bit.
> 
> 
> Cheers,
> Conor.


I just follow others when writing this binding. I think it may need
another patch to fix this problem, right?

Regards,
Inochi

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

[Conor Dooley]

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

On Mon, Oct 21, 2024 at 08:18:58PM +0800, Inochi Amaoto wrote:
> On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> > On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > > The UART of SG2044 is modified version of the standard Synopsys
> > > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > > and can not set right clock rate for the common bitrates.
> > > 
> > > 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
> > 
> > Why does each vendor have an items entry of its own? Seems like needless
> > clutter of the file IMO, except for the renesas bit.
> 
> I just follow others when writing this binding. I think it may need
> another patch to fix this problem, right?

Yeah. But I'd hold off to see if someone gives a rationale for it being
done this way before sending that. I've not deleted this thread, and
will send an ack if someone justifies why the binding is written like
this.

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

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

[Inochi Amaoto]

On Mon, Oct 21, 2024 at 01:21:58PM +0100, Conor Dooley wrote:
> On Mon, Oct 21, 2024 at 08:18:58PM +0800, Inochi Amaoto wrote:
> > On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> > > On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > > > The UART of SG2044 is modified version of the standard Synopsys
> > > > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > > > and can not set right clock rate for the common bitrates.
> > > > 
> > > > 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
> > > 
> > > Why does each vendor have an items entry of its own? Seems like needless
> > > clutter of the file IMO, except for the renesas bit.
> > 
> > I just follow others when writing this binding. I think it may need
> > another patch to fix this problem, right?
> 
> Yeah. But I'd hold off to see if someone gives a rationale for it being
> done this way before sending that. I've not deleted this thread, and
> will send an ack if someone justifies why the binding is written like
> this.

Thanks.

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

[Conor Dooley]

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

On Mon, Oct 21, 2024 at 08:23:30PM +0800, Inochi Amaoto wrote:
> On Mon, Oct 21, 2024 at 01:21:58PM +0100, Conor Dooley wrote:
> > On Mon, Oct 21, 2024 at 08:18:58PM +0800, Inochi Amaoto wrote:
> > > On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> > > > On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > > > > The UART of SG2044 is modified version of the standard Synopsys
> > > > > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > > > > and can not set right clock rate for the common bitrates.
> > > > > 
> > > > > 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
> > > > 
> > > > Why does each vendor have an items entry of its own? Seems like needless
> > > > clutter of the file IMO, except for the renesas bit.
> > > 
> > > I just follow others when writing this binding. I think it may need
> > > another patch to fix this problem, right?
> > 
> > Yeah. But I'd hold off to see if someone gives a rationale for it being
> > done this way before sending that. I've not deleted this thread, and
> > will send an ack if someone justifies why the binding is written like
> > this.

Well, Rob doesn't think they should be separate so please add that
additional patch in your next version.

Thanks,
Conor.

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

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

[Inochi Amaoto]

On Tue, Oct 22, 2024 at 06:25:00PM +0100, Conor Dooley wrote:
> On Mon, Oct 21, 2024 at 08:23:30PM +0800, Inochi Amaoto wrote:
> > On Mon, Oct 21, 2024 at 01:21:58PM +0100, Conor Dooley wrote:
> > > On Mon, Oct 21, 2024 at 08:18:58PM +0800, Inochi Amaoto wrote:
> > > > On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> > > > > On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > > > > > The UART of SG2044 is modified version of the standard Synopsys
> > > > > > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > > > > > and can not set right clock rate for the common bitrates.
> > > > > > 
> > > > > > 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
> > > > > 
> > > > > Why does each vendor have an items entry of its own? Seems like needless
> > > > > clutter of the file IMO, except for the renesas bit.
> > > > 
> > > > I just follow others when writing this binding. I think it may need
> > > > another patch to fix this problem, right?
> > > 
> > > Yeah. But I'd hold off to see if someone gives a rationale for it being
> > > done this way before sending that. I've not deleted this thread, and
> > > will send an ack if someone justifies why the binding is written like
> > > this.
> 
> Well, Rob doesn't think they should be separate so please add that
> additional patch in your next version.
> 
> Thanks,
> Conor.

It is OK for me. I will add a fix patch in the next version. Can
I add you with suggested-by tag in this fix patch?

Regards,
Inochi

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

[Conor Dooley]

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

On Wed, Oct 23, 2024 at 08:32:42AM +0800, Inochi Amaoto wrote:
> On Tue, Oct 22, 2024 at 06:25:00PM +0100, Conor Dooley wrote:
> > On Mon, Oct 21, 2024 at 08:23:30PM +0800, Inochi Amaoto wrote:
> > > On Mon, Oct 21, 2024 at 01:21:58PM +0100, Conor Dooley wrote:
> > > > On Mon, Oct 21, 2024 at 08:18:58PM +0800, Inochi Amaoto wrote:
> > > > > On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> > > > > > On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > > > > > > The UART of SG2044 is modified version of the standard Synopsys
> > > > > > > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > > > > > > and can not set right clock rate for the common bitrates.
> > > > > > > 
> > > > > > > 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
> > > > > > 
> > > > > > Why does each vendor have an items entry of its own? Seems like needless
> > > > > > clutter of the file IMO, except for the renesas bit.
> > > > > 
> > > > > I just follow others when writing this binding. I think it may need
> > > > > another patch to fix this problem, right?
> > > > 
> > > > Yeah. But I'd hold off to see if someone gives a rationale for it being
> > > > done this way before sending that. I've not deleted this thread, and
> > > > will send an ack if someone justifies why the binding is written like
> > > > this.
> > 
> > Well, Rob doesn't think they should be separate so please add that
> > additional patch in your next version.
> > 
> > Thanks,
> > Conor.
> 
> It is OK for me. I will add a fix patch in the next version. Can
> I add you with suggested-by tag in this fix patch?

If you want, but I don't really care for one.

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

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

[Rob Herring]

On Mon, Oct 21, 2024 at 01:21:58PM +0100, Conor Dooley wrote:
> On Mon, Oct 21, 2024 at 08:18:58PM +0800, Inochi Amaoto wrote:
> > On Mon, Oct 21, 2024 at 01:10:52PM +0100, Conor Dooley wrote:
> > > On Mon, Oct 21, 2024 at 03:26:05PM +0800, Inochi Amaoto wrote:
> > > > The UART of SG2044 is modified version of the standard Synopsys
> > > > DesignWare UART. The UART on SG2044 relys on the internal divisor
> > > > and can not set right clock rate for the common bitrates.
> > > > 
> > > > 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
> > > 
> > > Why does each vendor have an items entry of its own? Seems like needless
> > > clutter of the file IMO, except for the renesas bit.
> > 
> > I just follow others when writing this binding. I think it may need
> > another patch to fix this problem, right?
> 
> Yeah. But I'd hold off to see if someone gives a rationale for it being
> done this way before sending that. I've not deleted this thread, and
> will send an ack if someone justifies why the binding is written like
> this.

No reason to be separate.

Rob

[PATCH v2 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index ab9e7f204260..51894c93c8a3 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -750,7 +750,7 @@ 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_starfive_jh7100_data = {
+static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
 	.usr_reg = DW_UART_USR,
 	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
 };
@@ -760,7 +760,8 @@ 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 = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
+	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_skip_set_rate_data },
+	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_skip_set_rate_data },
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dw8250_of_match);
-- 
2.47.0

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

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 03:26:06PM +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.

Assuming DT bindings are okay,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Ilpo Järvinen]

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

On Mon, 21 Oct 2024, 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.
> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

I wonder though does this mean the numbers userspace can read from kernel 
are bogus and if something can be done about that?

-- 
 i.

> ---
>  drivers/tty/serial/8250/8250_dw.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index ab9e7f204260..51894c93c8a3 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -750,7 +750,7 @@ 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_starfive_jh7100_data = {
> +static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
>  	.usr_reg = DW_UART_USR,
>  	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
>  };
> @@ -760,7 +760,8 @@ 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 = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
> +	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_skip_set_rate_data },
> +	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_skip_set_rate_data },
>  	{ /* Sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, dw8250_of_match);
> 

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

[Inochi Amaoto]

On Mon, Oct 21, 2024 at 11:52:38AM +0300, Ilpo Järvinen wrote:
> On Mon, 21 Oct 2024, 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.
> > 
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> I wonder though does this mean the numbers userspace can read from kernel 
> are bogus and if something can be done about that?
> 

I am not sure whether the clock rate can be read by the userspace.
At least it report the right baud speed by using stty.

Regards,
Inochi

> 
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index ab9e7f204260..51894c93c8a3 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -750,7 +750,7 @@ 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_starfive_jh7100_data = {
> > +static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
> >  	.usr_reg = DW_UART_USR,
> >  	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> >  };
> > @@ -760,7 +760,8 @@ 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 = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
> > +	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_skip_set_rate_data },
> > +	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_skip_set_rate_data },
> >  	{ /* Sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, dw8250_of_match);
> > 

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

[Ilpo Järvinen]

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

On Mon, 21 Oct 2024, Inochi Amaoto wrote:

> On Mon, Oct 21, 2024 at 11:52:38AM +0300, Ilpo Järvinen wrote:
> > On Mon, 21 Oct 2024, 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.
> > > 
> > > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > I wonder though does this mean the numbers userspace can read from kernel 
> > are bogus and if something can be done about that?
> > 
> 
> I am not sure whether the clock rate can be read by the userspace.
> At least it report the right baud speed by using stty.

Okay, I meant baud & other settings. Thanks for checking it.

-- 
 i.

> Regards,
> Inochi
> 
> > 
> > > ---
> > >  drivers/tty/serial/8250/8250_dw.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > > index ab9e7f204260..51894c93c8a3 100644
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -750,7 +750,7 @@ 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_starfive_jh7100_data = {
> > > +static const struct dw8250_platform_data dw8250_skip_set_rate_data = {
> > >  	.usr_reg = DW_UART_USR,
> > >  	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> > >  };
> > > @@ -760,7 +760,8 @@ 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 = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
> > > +	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_skip_set_rate_data },
> > > +	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_skip_set_rate_data },
> > >  	{ /* Sentinel */ }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, dw8250_of_match);
> > > 
> 

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

[Andy Shevchenko]

On Mon, Oct 21, 2024 at 01:17:55PM +0300, Ilpo Järvinen wrote:
> On Mon, 21 Oct 2024, Inochi Amaoto wrote:
> > On Mon, Oct 21, 2024 at 11:52:38AM +0300, Ilpo Järvinen wrote:
> > > On Mon, 21 Oct 2024, 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.
> > > > 
> > > > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > > 
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > 
> > > I wonder though does this mean the numbers userspace can read from kernel 
> > > are bogus and if something can be done about that?
> > 
> > I am not sure whether the clock rate can be read by the userspace.
> > At least it report the right baud speed by using stty.
> 
> Okay, I meant baud & other settings. Thanks for checking it.

oBut there is clock rate for user space. I think Ilpo has a point.

Documentation/ABI/testing/sysfs-tty:21:What:            /sys/class/tty/ttyS<x>/uartclk

-- 
With Best Regards,
Andy Shevchenko