[PATCH V5 0/5] The UART driver compatible with

[PATCH V5 0/5] The UART driver compatible with

[Yu Tu]

Using the common Clock code to describe the UART baud rate
clock makes it easier for the UART driver to be compatible
with the baud rate requirements of the UART IP on different
meson chips. Add Meson S4 SoC compatible.

Yu Tu (5):
  dt-bindings: serial: meson: Drop legacy compatible.
  tty: serial: meson: Move request the register region.
  tty: serial: meson: Using the common clock code describe.
  tty: serial: meson: Make some bit of the REG5 register writable.
  tty: serial: meson: Added S4 SOC compatibility.

V4 -> V5: Change error format.
V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
in the email.
V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
the DTS before it can be deleted
V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
discussed in the email

Link:https://lore.kernel.org/linux-amlogic/20220110085604.18042-1-yu.tu@amlogic.com/

 .../bindings/serial/amlogic,meson-uart.yaml   |  10 +-
 drivers/tty/serial/meson_uart.c               | 244 ++++++++++++------
 2 files changed, 177 insertions(+), 77 deletions(-)

-- 
2.33.1

[PATCH V5 0/5] The UART driver compatible with

[Yu Tu]

Using the common Clock code to describe the UART baud rate
clock makes it easier for the UART driver to be compatible
with the baud rate requirements of the UART IP on different
meson chips. Add Meson S4 SoC compatible.

Yu Tu (5):
  dt-bindings: serial: meson: Drop legacy compatible.
  tty: serial: meson: Move request the register region.
  tty: serial: meson: Using the common clock code describe.
  tty: serial: meson: Make some bit of the REG5 register writable.
  tty: serial: meson: Added S4 SOC compatibility.

V4 -> V5: Change error format.
V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
in the email.
V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
the DTS before it can be deleted
V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
discussed in the email

Link:https://lore.kernel.org/linux-amlogic/20220110085604.18042-1-yu.tu@amlogic.com/

 .../bindings/serial/amlogic,meson-uart.yaml   |  10 +-
 drivers/tty/serial/meson_uart.c               | 244 ++++++++++++------
 2 files changed, 177 insertions(+), 77 deletions(-)

-- 
2.33.1


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

[PATCH V5 0/5] The UART driver compatible with

[Yu Tu]

Using the common Clock code to describe the UART baud rate
clock makes it easier for the UART driver to be compatible
with the baud rate requirements of the UART IP on different
meson chips. Add Meson S4 SoC compatible.

Yu Tu (5):
  dt-bindings: serial: meson: Drop legacy compatible.
  tty: serial: meson: Move request the register region.
  tty: serial: meson: Using the common clock code describe.
  tty: serial: meson: Make some bit of the REG5 register writable.
  tty: serial: meson: Added S4 SOC compatibility.

V4 -> V5: Change error format.
V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
in the email.
V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
the DTS before it can be deleted
V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
discussed in the email

Link:https://lore.kernel.org/linux-amlogic/20220110085604.18042-1-yu.tu@amlogic.com/

 .../bindings/serial/amlogic,meson-uart.yaml   |  10 +-
 drivers/tty/serial/meson_uart.c               | 244 ++++++++++++------
 2 files changed, 177 insertions(+), 77 deletions(-)

-- 
2.33.1


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

[PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Yu Tu]

Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
compatible. Don't differentiate between GXBB, GXL and G12A which
have different revisions of the UART IP. So it's split into
GXBB,GXL and G12A.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
index 72e8868db3e0..ad9f1f4537a0 100644
--- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
@@ -28,7 +28,10 @@ properties:
               - amlogic,meson6-uart
               - amlogic,meson8-uart
               - amlogic,meson8b-uart
-              - amlogic,meson-gx-uart
+              - amlogic,meson-gx-uart # deprecated, use revision specific property below
+              - amlogic,meson-gxbb-uart
+              - amlogic,meson-gxl-uart
+              - amlogic,meson-g12a-uart
               - amlogic,meson-s4-uart
           - const: amlogic,meson-ao-uart
       - description: Everything-Else power domain UART controller
@@ -36,7 +39,10 @@ properties:
           - amlogic,meson6-uart
           - amlogic,meson8-uart
           - amlogic,meson8b-uart
-          - amlogic,meson-gx-uart
+          - amlogic,meson-gx-uart # deprecated, use revision specific property below
+          - amlogic,meson-gxbb-uart
+          - amlogic,meson-gxl-uart
+          - amlogic,meson-g12a-uart
           - amlogic,meson-s4-uart
 
   reg:
-- 
2.33.1

[PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Yu Tu]

Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
compatible. Don't differentiate between GXBB, GXL and G12A which
have different revisions of the UART IP. So it's split into
GXBB,GXL and G12A.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
index 72e8868db3e0..ad9f1f4537a0 100644
--- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
@@ -28,7 +28,10 @@ properties:
               - amlogic,meson6-uart
               - amlogic,meson8-uart
               - amlogic,meson8b-uart
-              - amlogic,meson-gx-uart
+              - amlogic,meson-gx-uart # deprecated, use revision specific property below
+              - amlogic,meson-gxbb-uart
+              - amlogic,meson-gxl-uart
+              - amlogic,meson-g12a-uart
               - amlogic,meson-s4-uart
           - const: amlogic,meson-ao-uart
       - description: Everything-Else power domain UART controller
@@ -36,7 +39,10 @@ properties:
           - amlogic,meson6-uart
           - amlogic,meson8-uart
           - amlogic,meson8b-uart
-          - amlogic,meson-gx-uart
+          - amlogic,meson-gx-uart # deprecated, use revision specific property below
+          - amlogic,meson-gxbb-uart
+          - amlogic,meson-gxl-uart
+          - amlogic,meson-g12a-uart
           - amlogic,meson-s4-uart
 
   reg:
-- 
2.33.1


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

[PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Yu Tu]

Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
compatible. Don't differentiate between GXBB, GXL and G12A which
have different revisions of the UART IP. So it's split into
GXBB,GXL and G12A.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 .../devicetree/bindings/serial/amlogic,meson-uart.yaml | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
index 72e8868db3e0..ad9f1f4537a0 100644
--- a/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/amlogic,meson-uart.yaml
@@ -28,7 +28,10 @@ properties:
               - amlogic,meson6-uart
               - amlogic,meson8-uart
               - amlogic,meson8b-uart
-              - amlogic,meson-gx-uart
+              - amlogic,meson-gx-uart # deprecated, use revision specific property below
+              - amlogic,meson-gxbb-uart
+              - amlogic,meson-gxl-uart
+              - amlogic,meson-g12a-uart
               - amlogic,meson-s4-uart
           - const: amlogic,meson-ao-uart
       - description: Everything-Else power domain UART controller
@@ -36,7 +39,10 @@ properties:
           - amlogic,meson6-uart
           - amlogic,meson8-uart
           - amlogic,meson8b-uart
-          - amlogic,meson-gx-uart
+          - amlogic,meson-gx-uart # deprecated, use revision specific property below
+          - amlogic,meson-gxbb-uart
+          - amlogic,meson-gxl-uart
+          - amlogic,meson-g12a-uart
           - amlogic,meson-s4-uart
 
   reg:
-- 
2.33.1


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

Re: [PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Rob Herring]

On Mon, Jan 10, 2022 at 4:42 AM Yu Tu <yu.tu@amlogic.com> wrote:
>
> Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
> compatible. Don't differentiate between GXBB, GXL and G12A which
> have different revisions of the UART IP. So it's split into
> GXBB,GXL and G12A.

You are breaking compatibility. If your dts is updated to use the new
compatible, then a kernel without the change will not work anymore. It
worked before without understanding the changes you are making. It
should continue to. A compatible value like this would accomplish what
you want:

"amlogic,meson-gxbb-uart", "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"

A new kernel will match on "amlogic,meson-gxbb-uart" or
"amlogic,meson-gx-uart" depending if the dtb is updated or not. An old
kernel will continue to match on "amlogic,meson-gx-uart".

If you are going to go breaking things, the power domain distinction
in the compatible is odd...


Rob

Re: [PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Rob Herring]

On Mon, Jan 10, 2022 at 4:42 AM Yu Tu <yu.tu@amlogic.com> wrote:
>
> Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
> compatible. Don't differentiate between GXBB, GXL and G12A which
> have different revisions of the UART IP. So it's split into
> GXBB,GXL and G12A.

You are breaking compatibility. If your dts is updated to use the new
compatible, then a kernel without the change will not work anymore. It
worked before without understanding the changes you are making. It
should continue to. A compatible value like this would accomplish what
you want:

"amlogic,meson-gxbb-uart", "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"

A new kernel will match on "amlogic,meson-gxbb-uart" or
"amlogic,meson-gx-uart" depending if the dtb is updated or not. An old
kernel will continue to match on "amlogic,meson-gx-uart".

If you are going to go breaking things, the power domain distinction
in the compatible is odd...


Rob

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

Re: [PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Rob Herring]

On Mon, Jan 10, 2022 at 4:42 AM Yu Tu <yu.tu@amlogic.com> wrote:
>
> Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
> compatible. Don't differentiate between GXBB, GXL and G12A which
> have different revisions of the UART IP. So it's split into
> GXBB,GXL and G12A.

You are breaking compatibility. If your dts is updated to use the new
compatible, then a kernel without the change will not work anymore. It
worked before without understanding the changes you are making. It
should continue to. A compatible value like this would accomplish what
you want:

"amlogic,meson-gxbb-uart", "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"

A new kernel will match on "amlogic,meson-gxbb-uart" or
"amlogic,meson-gx-uart" depending if the dtb is updated or not. An old
kernel will continue to match on "amlogic,meson-gx-uart".

If you are going to go breaking things, the power domain distinction
in the compatible is odd...


Rob

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

Re: [PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Yu Tu]

Hi Rob,
	Thank you very much for your reply.

On 2022/1/11 0:18, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
> 
> On Mon, Jan 10, 2022 at 4:42 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>
>> Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
>> compatible. Don't differentiate between GXBB, GXL and G12A which
>> have different revisions of the UART IP. So it's split into
>> GXBB,GXL and G12A.
> 
> You are breaking compatibility. If your dts is updated to use the new
> compatible, then a kernel without the change will not work anymore. It
> worked before without understanding the changes you are making. It
> should continue to. A compatible value like this would accomplish what
> you want:
> 
> "amlogic,meson-gxbb-uart", "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"
> 
> A new kernel will match on "amlogic,meson-gxbb-uart" or
> "amlogic,meson-gx-uart" depending if the dtb is updated or not. An old
> kernel will continue to match on "amlogic,meson-gx-uart".
> 
> If you are going to go breaking things, the power domain distinction
> in the compatible is odd...
> 
I will not blindly delete this "amlogic, meson-gx-uart" compatible as 
you suggest, just make a comment until all DTS changes are finished.
> 
> Rob
> 

Re: [PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Yu Tu]

Hi Rob,
	Thank you very much for your reply.

On 2022/1/11 0:18, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
> 
> On Mon, Jan 10, 2022 at 4:42 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>
>> Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
>> compatible. Don't differentiate between GXBB, GXL and G12A which
>> have different revisions of the UART IP. So it's split into
>> GXBB,GXL and G12A.
> 
> You are breaking compatibility. If your dts is updated to use the new
> compatible, then a kernel without the change will not work anymore. It
> worked before without understanding the changes you are making. It
> should continue to. A compatible value like this would accomplish what
> you want:
> 
> "amlogic,meson-gxbb-uart", "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"
> 
> A new kernel will match on "amlogic,meson-gxbb-uart" or
> "amlogic,meson-gx-uart" depending if the dtb is updated or not. An old
> kernel will continue to match on "amlogic,meson-gx-uart".
> 
> If you are going to go breaking things, the power domain distinction
> in the compatible is odd...
> 
I will not blindly delete this "amlogic, meson-gx-uart" compatible as 
you suggest, just make a comment until all DTS changes are finished.
> 
> Rob
> 

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

Re: [PATCH V5 1/5] dt-bindings: serial: meson: Drop legacy compatible.

[Yu Tu]

Hi Rob,
	Thank you very much for your reply.

On 2022/1/11 0:18, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
> 
> On Mon, Jan 10, 2022 at 4:42 AM Yu Tu <yu.tu@amlogic.com> wrote:
>>
>> Deprecated, don't use anymore because legacy amlogic,meson-gx-uart
>> compatible. Don't differentiate between GXBB, GXL and G12A which
>> have different revisions of the UART IP. So it's split into
>> GXBB,GXL and G12A.
> 
> You are breaking compatibility. If your dts is updated to use the new
> compatible, then a kernel without the change will not work anymore. It
> worked before without understanding the changes you are making. It
> should continue to. A compatible value like this would accomplish what
> you want:
> 
> "amlogic,meson-gxbb-uart", "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"
> 
> A new kernel will match on "amlogic,meson-gxbb-uart" or
> "amlogic,meson-gx-uart" depending if the dtb is updated or not. An old
> kernel will continue to match on "amlogic,meson-gx-uart".
> 
> If you are going to go breaking things, the power domain distinction
> in the compatible is odd...
> 
I will not blindly delete this "amlogic, meson-gx-uart" compatible as 
you suggest, just make a comment until all DTS changes are finished.
> 
> Rob
> 

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

[PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Yu Tu]

This simplifies resetting the UART controller during probe
and will make it easier to integrate the common clock code
which will require the registers at probe time as well.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 45e00d928253..7570958d010c 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	devm_iounmap(port->dev, port->membase);
-	port->membase = NULL;
-	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
+	/* nothing to do */
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
-	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
-				     dev_name(port->dev))) {
-		dev_err(port->dev, "Memory region busy\n");
-		return -EBUSY;
-	}
-
-	port->membase = devm_ioremap(port->dev, port->mapbase,
-					     port->mapsize);
-	if (!port->membase)
-		return -ENOMEM;
-
 	return 0;
 }
 
@@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, port);
 
 	/* reset port before registering (and possibly registering console) */
-	if (meson_uart_request_port(port) >= 0) {
-		meson_uart_reset(port);
-		meson_uart_release_port(port);
-	}
+	meson_uart_reset(port);
 
 	ret = uart_add_one_port(&meson_uart_driver, port);
 	if (ret)
-- 
2.33.1

[PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Yu Tu]

This simplifies resetting the UART controller during probe
and will make it easier to integrate the common clock code
which will require the registers at probe time as well.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 45e00d928253..7570958d010c 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	devm_iounmap(port->dev, port->membase);
-	port->membase = NULL;
-	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
+	/* nothing to do */
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
-	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
-				     dev_name(port->dev))) {
-		dev_err(port->dev, "Memory region busy\n");
-		return -EBUSY;
-	}
-
-	port->membase = devm_ioremap(port->dev, port->mapbase,
-					     port->mapsize);
-	if (!port->membase)
-		return -ENOMEM;
-
 	return 0;
 }
 
@@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, port);
 
 	/* reset port before registering (and possibly registering console) */
-	if (meson_uart_request_port(port) >= 0) {
-		meson_uart_reset(port);
-		meson_uart_release_port(port);
-	}
+	meson_uart_reset(port);
 
 	ret = uart_add_one_port(&meson_uart_driver, port);
 	if (ret)
-- 
2.33.1


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

[PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Yu Tu]

This simplifies resetting the UART controller during probe
and will make it easier to integrate the common clock code
which will require the registers at probe time as well.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 45e00d928253..7570958d010c 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	devm_iounmap(port->dev, port->membase);
-	port->membase = NULL;
-	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
+	/* nothing to do */
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
-	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
-				     dev_name(port->dev))) {
-		dev_err(port->dev, "Memory region busy\n");
-		return -EBUSY;
-	}
-
-	port->membase = devm_ioremap(port->dev, port->mapbase,
-					     port->mapsize);
-	if (!port->membase)
-		return -ENOMEM;
-
 	return 0;
 }
 
@@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
+	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(port->membase))
+		return PTR_ERR(port->membase);
+
 	ret = meson_uart_probe_clocks(pdev, port);
 	if (ret)
 		return ret;
@@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, port);
 
 	/* reset port before registering (and possibly registering console) */
-	if (meson_uart_request_port(port) >= 0) {
-		meson_uart_reset(port);
-		meson_uart_release_port(port);
-	}
+	meson_uart_reset(port);
 
 	ret = uart_add_one_port(&meson_uart_driver, port);
 	if (ret)
-- 
2.33.1


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

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Jiri Slaby]

Hi,

On 10. 01. 22, 11:42, Yu Tu wrote:
> This simplifies resetting the UART controller during probe
> and will make it easier to integrate the common clock code
> which will require the registers at probe time as well.

And you are *also* switching devm_request_mem_region+devm_ioremap into 
devm_ioremap_resource without even noting it here. It should've been 
done in a separate patch.

And no, do not resend a new version in the next few days. Having v5 in 
20 days is a bit too much. Give maintainers and reviewers some more 
space to express themselves.
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>   drivers/tty/serial/meson_uart.c | 24 ++++++------------------
>   1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 45e00d928253..7570958d010c 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	devm_iounmap(port->dev, port->membase);
> -	port->membase = NULL;
> -	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
> +	/* nothing to do */
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> -	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
> -				     dev_name(port->dev))) {
> -		dev_err(port->dev, "Memory region busy\n");
> -		return -EBUSY;
> -	}
> -
> -	port->membase = devm_ioremap(port->dev, port->mapbase,
> -					     port->mapsize);
> -	if (!port->membase)
> -		return -ENOMEM;
> -
>   	return 0;
>   }
>   
> @@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	if (!port)
>   		return -ENOMEM;
>   
> +	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
> +	if (IS_ERR(port->membase))
> +		return PTR_ERR(port->membase);
> +
>   	ret = meson_uart_probe_clocks(pdev, port);
>   	if (ret)
>   		return ret;
> @@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, port);
>   
>   	/* reset port before registering (and possibly registering console) */
> -	if (meson_uart_request_port(port) >= 0) {
> -		meson_uart_reset(port);
> -		meson_uart_release_port(port);
> -	}
> +	meson_uart_reset(port);
>   
>   	ret = uart_add_one_port(&meson_uart_driver, port);
>   	if (ret)

thanks,
-- 
js
suse labs

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Jiri Slaby]

Hi,

On 10. 01. 22, 11:42, Yu Tu wrote:
> This simplifies resetting the UART controller during probe
> and will make it easier to integrate the common clock code
> which will require the registers at probe time as well.

And you are *also* switching devm_request_mem_region+devm_ioremap into 
devm_ioremap_resource without even noting it here. It should've been 
done in a separate patch.

And no, do not resend a new version in the next few days. Having v5 in 
20 days is a bit too much. Give maintainers and reviewers some more 
space to express themselves.
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>   drivers/tty/serial/meson_uart.c | 24 ++++++------------------
>   1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 45e00d928253..7570958d010c 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	devm_iounmap(port->dev, port->membase);
> -	port->membase = NULL;
> -	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
> +	/* nothing to do */
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> -	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
> -				     dev_name(port->dev))) {
> -		dev_err(port->dev, "Memory region busy\n");
> -		return -EBUSY;
> -	}
> -
> -	port->membase = devm_ioremap(port->dev, port->mapbase,
> -					     port->mapsize);
> -	if (!port->membase)
> -		return -ENOMEM;
> -
>   	return 0;
>   }
>   
> @@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	if (!port)
>   		return -ENOMEM;
>   
> +	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
> +	if (IS_ERR(port->membase))
> +		return PTR_ERR(port->membase);
> +
>   	ret = meson_uart_probe_clocks(pdev, port);
>   	if (ret)
>   		return ret;
> @@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, port);
>   
>   	/* reset port before registering (and possibly registering console) */
> -	if (meson_uart_request_port(port) >= 0) {
> -		meson_uart_reset(port);
> -		meson_uart_release_port(port);
> -	}
> +	meson_uart_reset(port);
>   
>   	ret = uart_add_one_port(&meson_uart_driver, port);
>   	if (ret)

thanks,
-- 
js
suse labs

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

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Jiri Slaby]

Hi,

On 10. 01. 22, 11:42, Yu Tu wrote:
> This simplifies resetting the UART controller during probe
> and will make it easier to integrate the common clock code
> which will require the registers at probe time as well.

And you are *also* switching devm_request_mem_region+devm_ioremap into 
devm_ioremap_resource without even noting it here. It should've been 
done in a separate patch.

And no, do not resend a new version in the next few days. Having v5 in 
20 days is a bit too much. Give maintainers and reviewers some more 
space to express themselves.
> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>   drivers/tty/serial/meson_uart.c | 24 ++++++------------------
>   1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 45e00d928253..7570958d010c 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	devm_iounmap(port->dev, port->membase);
> -	port->membase = NULL;
> -	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
> +	/* nothing to do */
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> -	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
> -				     dev_name(port->dev))) {
> -		dev_err(port->dev, "Memory region busy\n");
> -		return -EBUSY;
> -	}
> -
> -	port->membase = devm_ioremap(port->dev, port->mapbase,
> -					     port->mapsize);
> -	if (!port->membase)
> -		return -ENOMEM;
> -
>   	return 0;
>   }
>   
> @@ -733,6 +720,10 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	if (!port)
>   		return -ENOMEM;
>   
> +	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
> +	if (IS_ERR(port->membase))
> +		return PTR_ERR(port->membase);
> +
>   	ret = meson_uart_probe_clocks(pdev, port);
>   	if (ret)
>   		return ret;
> @@ -754,10 +745,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, port);
>   
>   	/* reset port before registering (and possibly registering console) */
> -	if (meson_uart_request_port(port) >= 0) {
> -		meson_uart_reset(port);
> -		meson_uart_release_port(port);
> -	}
> +	meson_uart_reset(port);
>   
>   	ret = uart_add_one_port(&meson_uart_driver, port);
>   	if (ret)

thanks,
-- 
js
suse labs

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

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Yu Tu]

Hi Jiri ,
	Thank you very much for your reply.

On 2022/1/10 19:58, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 10. 01. 22, 11:42, Yu Tu wrote:
>> This simplifies resetting the UART controller during probe
>> and will make it easier to integrate the common clock code
>> which will require the registers at probe time as well.
> 
> And you are *also* switching devm_request_mem_region+devm_ioremap into 
> devm_ioremap_resource without even noting it here. It should've been 
> done in a separate patch.
> 
I can change it as you suggest。
The switching devm_request_mem_region+devm_ioremap into
devm_ioremap_resource in a separate patch.

> And no, do not resend a new version in the next few days. Having v5 in 
> 20 days is a bit too much. Give maintainers and reviewers some more 
> space to express themselves.
I am so sorry if PATCH is sent too frequently, which has disturbed you.
How often should I send it? Or under what circumstances can I send the 
next version?

>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 24 ++++++------------------
>>   1 file changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 45e00d928253..7570958d010c 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct 
>> uart_port *port,
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -    devm_iounmap(port->dev, port->membase);
>> -    port->membase = NULL;
>> -    devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
>> +    /* nothing to do */
>>   }
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> -    if (!devm_request_mem_region(port->dev, port->mapbase, 
>> port->mapsize,
>> -                     dev_name(port->dev))) {
>> -        dev_err(port->dev, "Memory region busy\n");
>> -        return -EBUSY;
>> -    }
>> -
>> -    port->membase = devm_ioremap(port->dev, port->mapbase,
>> -                         port->mapsize);
>> -    if (!port->membase)
>> -        return -ENOMEM;
>> -
>>       return 0;
>>   }
>> @@ -733,6 +720,10 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       if (!port)
>>           return -ENOMEM;
>> +    port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
>> +    if (IS_ERR(port->membase))
>> +        return PTR_ERR(port->membase);
>> +
>>       ret = meson_uart_probe_clocks(pdev, port);
>>       if (ret)
>>           return ret;
>> @@ -754,10 +745,7 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       platform_set_drvdata(pdev, port);
>>       /* reset port before registering (and possibly registering 
>> console) */
>> -    if (meson_uart_request_port(port) >= 0) {
>> -        meson_uart_reset(port);
>> -        meson_uart_release_port(port);
>> -    }
>> +    meson_uart_reset(port);
>>       ret = uart_add_one_port(&meson_uart_driver, port);
>>       if (ret)
> 
> thanks,

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Yu Tu]

Hi Jiri ,
	Thank you very much for your reply.

On 2022/1/10 19:58, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 10. 01. 22, 11:42, Yu Tu wrote:
>> This simplifies resetting the UART controller during probe
>> and will make it easier to integrate the common clock code
>> which will require the registers at probe time as well.
> 
> And you are *also* switching devm_request_mem_region+devm_ioremap into 
> devm_ioremap_resource without even noting it here. It should've been 
> done in a separate patch.
> 
I can change it as you suggest。
The switching devm_request_mem_region+devm_ioremap into
devm_ioremap_resource in a separate patch.

> And no, do not resend a new version in the next few days. Having v5 in 
> 20 days is a bit too much. Give maintainers and reviewers some more 
> space to express themselves.
I am so sorry if PATCH is sent too frequently, which has disturbed you.
How often should I send it? Or under what circumstances can I send the 
next version?

>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 24 ++++++------------------
>>   1 file changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 45e00d928253..7570958d010c 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct 
>> uart_port *port,
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -    devm_iounmap(port->dev, port->membase);
>> -    port->membase = NULL;
>> -    devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
>> +    /* nothing to do */
>>   }
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> -    if (!devm_request_mem_region(port->dev, port->mapbase, 
>> port->mapsize,
>> -                     dev_name(port->dev))) {
>> -        dev_err(port->dev, "Memory region busy\n");
>> -        return -EBUSY;
>> -    }
>> -
>> -    port->membase = devm_ioremap(port->dev, port->mapbase,
>> -                         port->mapsize);
>> -    if (!port->membase)
>> -        return -ENOMEM;
>> -
>>       return 0;
>>   }
>> @@ -733,6 +720,10 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       if (!port)
>>           return -ENOMEM;
>> +    port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
>> +    if (IS_ERR(port->membase))
>> +        return PTR_ERR(port->membase);
>> +
>>       ret = meson_uart_probe_clocks(pdev, port);
>>       if (ret)
>>           return ret;
>> @@ -754,10 +745,7 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       platform_set_drvdata(pdev, port);
>>       /* reset port before registering (and possibly registering 
>> console) */
>> -    if (meson_uart_request_port(port) >= 0) {
>> -        meson_uart_reset(port);
>> -        meson_uart_release_port(port);
>> -    }
>> +    meson_uart_reset(port);
>>       ret = uart_add_one_port(&meson_uart_driver, port);
>>       if (ret)
> 
> thanks,

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

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Yu Tu]

Hi Jiri ,
	Thank you very much for your reply.

On 2022/1/10 19:58, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> On 10. 01. 22, 11:42, Yu Tu wrote:
>> This simplifies resetting the UART controller during probe
>> and will make it easier to integrate the common clock code
>> which will require the registers at probe time as well.
> 
> And you are *also* switching devm_request_mem_region+devm_ioremap into 
> devm_ioremap_resource without even noting it here. It should've been 
> done in a separate patch.
> 
I can change it as you suggest。
The switching devm_request_mem_region+devm_ioremap into
devm_ioremap_resource in a separate patch.

> And no, do not resend a new version in the next few days. Having v5 in 
> 20 days is a bit too much. Give maintainers and reviewers some more 
> space to express themselves.
I am so sorry if PATCH is sent too frequently, which has disturbed you.
How often should I send it? Or under what circumstances can I send the 
next version?

>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 24 ++++++------------------
>>   1 file changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 45e00d928253..7570958d010c 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -395,24 +395,11 @@ static int meson_uart_verify_port(struct 
>> uart_port *port,
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -    devm_iounmap(port->dev, port->membase);
>> -    port->membase = NULL;
>> -    devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
>> +    /* nothing to do */
>>   }
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> -    if (!devm_request_mem_region(port->dev, port->mapbase, 
>> port->mapsize,
>> -                     dev_name(port->dev))) {
>> -        dev_err(port->dev, "Memory region busy\n");
>> -        return -EBUSY;
>> -    }
>> -
>> -    port->membase = devm_ioremap(port->dev, port->mapbase,
>> -                         port->mapsize);
>> -    if (!port->membase)
>> -        return -ENOMEM;
>> -
>>       return 0;
>>   }
>> @@ -733,6 +720,10 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       if (!port)
>>           return -ENOMEM;
>> +    port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
>> +    if (IS_ERR(port->membase))
>> +        return PTR_ERR(port->membase);
>> +
>>       ret = meson_uart_probe_clocks(pdev, port);
>>       if (ret)
>>           return ret;
>> @@ -754,10 +745,7 @@ static int meson_uart_probe(struct 
>> platform_device *pdev)
>>       platform_set_drvdata(pdev, port);
>>       /* reset port before registering (and possibly registering 
>> console) */
>> -    if (meson_uart_request_port(port) >= 0) {
>> -        meson_uart_reset(port);
>> -        meson_uart_release_port(port);
>> -    }
>> +    meson_uart_reset(port);
>>       ret = uart_add_one_port(&meson_uart_driver, port);
>>       if (ret)
> 
> thanks,

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

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Jiri Slaby]

On 11. 01. 22, 3:48, Yu Tu wrote:
>> And no, do not resend a new version in the next few days. Having v5 in 
>> 20 days is a bit too much. Give maintainers and reviewers some more 
>> space to express themselves.
> I am so sorry if PATCH is sent too frequently, which has disturbed you.
> How often should I send it? Or under what circumstances can I send the 
> next version?

~ one week after the last reply to your previous patchset is about fine. 
You could see that people were still responding to v4 while v5 was 
already on the list.

thanks,
-- 
js

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Jiri Slaby]

On 11. 01. 22, 3:48, Yu Tu wrote:
>> And no, do not resend a new version in the next few days. Having v5 in 
>> 20 days is a bit too much. Give maintainers and reviewers some more 
>> space to express themselves.
> I am so sorry if PATCH is sent too frequently, which has disturbed you.
> How often should I send it? Or under what circumstances can I send the 
> next version?

~ one week after the last reply to your previous patchset is about fine. 
You could see that people were still responding to v4 while v5 was 
already on the list.

thanks,
-- 
js

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

Re: [PATCH V5 2/5] tty: serial: meson: Move request the register region.

[Jiri Slaby]

On 11. 01. 22, 3:48, Yu Tu wrote:
>> And no, do not resend a new version in the next few days. Having v5 in 
>> 20 days is a bit too much. Give maintainers and reviewers some more 
>> space to express themselves.
> I am so sorry if PATCH is sent too frequently, which has disturbed you.
> How often should I send it? Or under what circumstances can I send the 
> next version?

~ one week after the last reply to your previous patchset is about fine. 
You could see that people were still responding to v4 while v5 was 
already on the list.

thanks,
-- 
js

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

[PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Yu Tu]

Using the common Clock code to describe the UART baud rate clock
makes it easier for the UART driver to be compatible with the
baud rate requirements of the UART IP on different meson chips.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
 1 file changed, 163 insertions(+), 61 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 7570958d010c..1004fd0b0c9e 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/init.h>
@@ -65,9 +66,7 @@
 #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
 
 /* AML_UART_REG5 bits */
-#define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
-#define AML_UART_BAUD_XTAL		BIT(24)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -76,6 +75,13 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct uart_port	port;
+	struct clk		*pclk;
+	struct clk		*baud_clk;
+	bool			use_xtal_clk;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
 static int meson_uart_startup(struct uart_port *port)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
-	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
+	meson_uart_reset(port);
 
+	val = readl(port->membase + AML_UART_CONTROL);
 	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
@@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port *port)
 
 static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
 {
+	struct meson_uart_data *private_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
-	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
-	} else {
-		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
-	}
+	val = readl(port->membase + AML_UART_REG5);
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
+
+	clk_set_rate(private_data->baud_clk, baud);
 }
 
 static void meson_uart_set_termios(struct uart_port *port,
@@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	/* nothing to do */
+	struct meson_uart_data *private_data = port->private_data;
+
+	clk_disable_unprepare(private_data->baud_clk);
+	clk_disable_unprepare(private_data->pclk);
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
+	struct meson_uart_data *private_data = port->private_data;
+	int ret;
+
+	ret = clk_prepare_enable(private_data->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret) {
+		clk_disable_unprepare(private_data->pclk);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
 	.cons		= MESON_SERIAL_CONSOLE,
 };
 
-static inline struct clk *meson_uart_probe_clock(struct device *dev,
-						 const char *id)
+#define CLK_NAME(name) \
+({\
+	char clk_name[32];\
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
+	clk_name;\
+})
+
+static struct clk_div_table xtal_div_table[] = {
+	{ 0, 3  },
+	{ 1, 1  },
+	{ 2, 2  },
+	{ 3, 2  },
+};
+
+static int meson_uart_probe_clocks(struct uart_port *port)
 {
-	struct clk *clk = NULL;
-	int ret;
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk *clk_baud, *clk_xtal;
+	struct clk_hw *hw;
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
 
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
+	private_data->pclk = devm_clk_get(port->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud)) {
+		dev_err(port->dev, "Failed to get the 'baud' clock\n");
+		return PTR_ERR(clk_baud);
 	}
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
-
-	return clk;
-}
-
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
-{
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
-
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
-
-	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
+	clk_xtal = devm_clk_get(port->dev, "xtal");
 	if (IS_ERR(clk_xtal))
-		return PTR_ERR(clk_xtal);
-
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	if (private_data->use_xtal_clk) {
+		hw = devm_clk_hw_register_divider_table(port->dev,
+							CLK_NAME(xtal_div),
+							__clk_get_name(clk_baud),
+							CLK_SET_RATE_NO_REPARENT,
+							port->membase + AML_UART_REG5,
+							26, 2,
+							CLK_DIVIDER_READ_ONLY,
+							xtal_div_table, NULL);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[1].hw = hw;
+	} else {
+		hw = devm_clk_hw_register_fixed_factor(port->dev,
+						       CLK_NAME(clk81_div4),
+						       __clk_get_name(clk_baud),
+						       CLK_SET_RATE_NO_REPARENT,
+						       1, 4);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[0].hw = hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	hw = __devm_clk_hw_register_mux(port->dev, NULL,
+					CLK_NAME(use_xtal),
+					ARRAY_SIZE(use_xtal_mux_parents),
+					NULL, NULL,
+					use_xtal_mux_parents,
+					CLK_SET_RATE_PARENT,
+					port->membase + AML_UART_REG5,
+					24, 0x1,
+					CLK_MUX_READ_ONLY,
+					NULL, NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	port->uartclk = clk_hw_get_rate(hw);
+
+	hw = devm_clk_hw_register_divider(port->dev,
+					  CLK_NAME(baud_div),
+					  clk_hw_get_name(hw),
+					  CLK_SET_RATE_PARENT,
+					  port->membase + AML_UART_REG5,
+					  0, 23,
+					  CLK_DIVIDER_ROUND_CLOSEST,
+					  NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem;
 	struct uart_port *port;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
@@ -716,18 +783,20 @@ static int meson_uart_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port), GFP_KERNEL);
-	if (!port)
+	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
+				    GFP_KERNEL);
+	if (!private_data)
 		return -ENOMEM;
 
+	if (device_get_match_data(&pdev->dev))
+		private_data->use_xtal_clk = true;
+
+	port = &private_data->port;
+
 	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
-
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
@@ -740,7 +809,11 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->private_data = private_data;
 
+	ret = meson_uart_probe_clocks(port);
+	if (ret)
+		return ret;
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
 
@@ -766,10 +839,39 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{
+		.compatible = "amlogic,meson6-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8b-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxbb-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxl-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-uart",
+		.data = (void *)true,
+	},
+	/*
+	 * deprecated, don't use anymore because it doesn't differentiate
+	 * between GXBB, GXL and G12A which have different revisions
+	 * of the UART IP.
+	 */
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)false,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.33.1

[PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Yu Tu]

Using the common Clock code to describe the UART baud rate clock
makes it easier for the UART driver to be compatible with the
baud rate requirements of the UART IP on different meson chips.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
 1 file changed, 163 insertions(+), 61 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 7570958d010c..1004fd0b0c9e 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/init.h>
@@ -65,9 +66,7 @@
 #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
 
 /* AML_UART_REG5 bits */
-#define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
-#define AML_UART_BAUD_XTAL		BIT(24)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -76,6 +75,13 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct uart_port	port;
+	struct clk		*pclk;
+	struct clk		*baud_clk;
+	bool			use_xtal_clk;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
 static int meson_uart_startup(struct uart_port *port)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
-	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
+	meson_uart_reset(port);
 
+	val = readl(port->membase + AML_UART_CONTROL);
 	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
@@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port *port)
 
 static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
 {
+	struct meson_uart_data *private_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
-	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
-	} else {
-		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
-	}
+	val = readl(port->membase + AML_UART_REG5);
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
+
+	clk_set_rate(private_data->baud_clk, baud);
 }
 
 static void meson_uart_set_termios(struct uart_port *port,
@@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	/* nothing to do */
+	struct meson_uart_data *private_data = port->private_data;
+
+	clk_disable_unprepare(private_data->baud_clk);
+	clk_disable_unprepare(private_data->pclk);
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
+	struct meson_uart_data *private_data = port->private_data;
+	int ret;
+
+	ret = clk_prepare_enable(private_data->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret) {
+		clk_disable_unprepare(private_data->pclk);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
 	.cons		= MESON_SERIAL_CONSOLE,
 };
 
-static inline struct clk *meson_uart_probe_clock(struct device *dev,
-						 const char *id)
+#define CLK_NAME(name) \
+({\
+	char clk_name[32];\
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
+	clk_name;\
+})
+
+static struct clk_div_table xtal_div_table[] = {
+	{ 0, 3  },
+	{ 1, 1  },
+	{ 2, 2  },
+	{ 3, 2  },
+};
+
+static int meson_uart_probe_clocks(struct uart_port *port)
 {
-	struct clk *clk = NULL;
-	int ret;
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk *clk_baud, *clk_xtal;
+	struct clk_hw *hw;
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
 
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
+	private_data->pclk = devm_clk_get(port->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud)) {
+		dev_err(port->dev, "Failed to get the 'baud' clock\n");
+		return PTR_ERR(clk_baud);
 	}
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
-
-	return clk;
-}
-
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
-{
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
-
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
-
-	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
+	clk_xtal = devm_clk_get(port->dev, "xtal");
 	if (IS_ERR(clk_xtal))
-		return PTR_ERR(clk_xtal);
-
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	if (private_data->use_xtal_clk) {
+		hw = devm_clk_hw_register_divider_table(port->dev,
+							CLK_NAME(xtal_div),
+							__clk_get_name(clk_baud),
+							CLK_SET_RATE_NO_REPARENT,
+							port->membase + AML_UART_REG5,
+							26, 2,
+							CLK_DIVIDER_READ_ONLY,
+							xtal_div_table, NULL);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[1].hw = hw;
+	} else {
+		hw = devm_clk_hw_register_fixed_factor(port->dev,
+						       CLK_NAME(clk81_div4),
+						       __clk_get_name(clk_baud),
+						       CLK_SET_RATE_NO_REPARENT,
+						       1, 4);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[0].hw = hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	hw = __devm_clk_hw_register_mux(port->dev, NULL,
+					CLK_NAME(use_xtal),
+					ARRAY_SIZE(use_xtal_mux_parents),
+					NULL, NULL,
+					use_xtal_mux_parents,
+					CLK_SET_RATE_PARENT,
+					port->membase + AML_UART_REG5,
+					24, 0x1,
+					CLK_MUX_READ_ONLY,
+					NULL, NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	port->uartclk = clk_hw_get_rate(hw);
+
+	hw = devm_clk_hw_register_divider(port->dev,
+					  CLK_NAME(baud_div),
+					  clk_hw_get_name(hw),
+					  CLK_SET_RATE_PARENT,
+					  port->membase + AML_UART_REG5,
+					  0, 23,
+					  CLK_DIVIDER_ROUND_CLOSEST,
+					  NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem;
 	struct uart_port *port;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
@@ -716,18 +783,20 @@ static int meson_uart_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port), GFP_KERNEL);
-	if (!port)
+	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
+				    GFP_KERNEL);
+	if (!private_data)
 		return -ENOMEM;
 
+	if (device_get_match_data(&pdev->dev))
+		private_data->use_xtal_clk = true;
+
+	port = &private_data->port;
+
 	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
-
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
@@ -740,7 +809,11 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->private_data = private_data;
 
+	ret = meson_uart_probe_clocks(port);
+	if (ret)
+		return ret;
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
 
@@ -766,10 +839,39 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{
+		.compatible = "amlogic,meson6-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8b-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxbb-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxl-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-uart",
+		.data = (void *)true,
+	},
+	/*
+	 * deprecated, don't use anymore because it doesn't differentiate
+	 * between GXBB, GXL and G12A which have different revisions
+	 * of the UART IP.
+	 */
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)false,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.33.1


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

[PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Yu Tu]

Using the common Clock code to describe the UART baud rate clock
makes it easier for the UART driver to be compatible with the
baud rate requirements of the UART IP on different meson chips.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
 1 file changed, 163 insertions(+), 61 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 7570958d010c..1004fd0b0c9e 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/init.h>
@@ -65,9 +66,7 @@
 #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
 
 /* AML_UART_REG5 bits */
-#define AML_UART_BAUD_MASK		0x7fffff
 #define AML_UART_BAUD_USE		BIT(23)
-#define AML_UART_BAUD_XTAL		BIT(24)
 
 #define AML_UART_PORT_NUM		12
 #define AML_UART_PORT_OFFSET		6
@@ -76,6 +75,13 @@
 #define AML_UART_POLL_USEC		5
 #define AML_UART_TIMEOUT_USEC		10000
 
+struct meson_uart_data {
+	struct uart_port	port;
+	struct clk		*pclk;
+	struct clk		*baud_clk;
+	bool			use_xtal_clk;
+};
+
 static struct uart_driver meson_uart_driver;
 
 static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
 static int meson_uart_startup(struct uart_port *port)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
-	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
-	val &= ~AML_UART_CLEAR_ERR;
-	writel(val, port->membase + AML_UART_CONTROL);
+	meson_uart_reset(port);
 
+	val = readl(port->membase + AML_UART_CONTROL);
 	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
 	writel(val, port->membase + AML_UART_CONTROL);
 
@@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port *port)
 
 static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
 {
+	struct meson_uart_data *private_data = port->private_data;
 	u32 val;
 
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
-	if (port->uartclk == 24000000) {
-		val = ((port->uartclk / 3) / baud) - 1;
-		val |= AML_UART_BAUD_XTAL;
-	} else {
-		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
-	}
+	val = readl(port->membase + AML_UART_REG5);
 	val |= AML_UART_BAUD_USE;
 	writel(val, port->membase + AML_UART_REG5);
+
+	clk_set_rate(private_data->baud_clk, baud);
 }
 
 static void meson_uart_set_termios(struct uart_port *port,
@@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	/* nothing to do */
+	struct meson_uart_data *private_data = port->private_data;
+
+	clk_disable_unprepare(private_data->baud_clk);
+	clk_disable_unprepare(private_data->pclk);
 }
 
 static int meson_uart_request_port(struct uart_port *port)
 {
+	struct meson_uart_data *private_data = port->private_data;
+	int ret;
+
+	ret = clk_prepare_enable(private_data->pclk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(private_data->baud_clk);
+	if (ret) {
+		clk_disable_unprepare(private_data->pclk);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
 	.cons		= MESON_SERIAL_CONSOLE,
 };
 
-static inline struct clk *meson_uart_probe_clock(struct device *dev,
-						 const char *id)
+#define CLK_NAME(name) \
+({\
+	char clk_name[32];\
+	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
+	clk_name;\
+})
+
+static struct clk_div_table xtal_div_table[] = {
+	{ 0, 3  },
+	{ 1, 1  },
+	{ 2, 2  },
+	{ 3, 2  },
+};
+
+static int meson_uart_probe_clocks(struct uart_port *port)
 {
-	struct clk *clk = NULL;
-	int ret;
+	struct meson_uart_data *private_data = port->private_data;
+	struct clk *clk_baud, *clk_xtal;
+	struct clk_hw *hw;
+	struct clk_parent_data use_xtal_mux_parents[2] = {
+		{ .index = -1, },
+		{ .index = -1, },
+	};
 
-	clk = devm_clk_get(dev, id);
-	if (IS_ERR(clk))
-		return clk;
+	private_data->pclk = devm_clk_get(port->dev, "pclk");
+	if (IS_ERR(private_data->pclk))
+		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
+				     "Failed to get the 'pclk' clock\n");
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "couldn't enable clk\n");
-		return ERR_PTR(ret);
+	clk_baud = devm_clk_get(port->dev, "baud");
+	if (IS_ERR(clk_baud)) {
+		dev_err(port->dev, "Failed to get the 'baud' clock\n");
+		return PTR_ERR(clk_baud);
 	}
 
-	devm_add_action_or_reset(dev,
-			(void(*)(void *))clk_disable_unprepare,
-			clk);
-
-	return clk;
-}
-
-static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
-{
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
-
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
-
-	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
+	clk_xtal = devm_clk_get(port->dev, "xtal");
 	if (IS_ERR(clk_xtal))
-		return PTR_ERR(clk_xtal);
-
-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
+		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
+				     "Failed to get the 'xtal' clock\n");
+
+	if (private_data->use_xtal_clk) {
+		hw = devm_clk_hw_register_divider_table(port->dev,
+							CLK_NAME(xtal_div),
+							__clk_get_name(clk_baud),
+							CLK_SET_RATE_NO_REPARENT,
+							port->membase + AML_UART_REG5,
+							26, 2,
+							CLK_DIVIDER_READ_ONLY,
+							xtal_div_table, NULL);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[1].hw = hw;
+	} else {
+		hw = devm_clk_hw_register_fixed_factor(port->dev,
+						       CLK_NAME(clk81_div4),
+						       __clk_get_name(clk_baud),
+						       CLK_SET_RATE_NO_REPARENT,
+						       1, 4);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		use_xtal_mux_parents[0].hw = hw;
+	}
 
-	port->uartclk = clk_get_rate(clk_baud);
+	hw = __devm_clk_hw_register_mux(port->dev, NULL,
+					CLK_NAME(use_xtal),
+					ARRAY_SIZE(use_xtal_mux_parents),
+					NULL, NULL,
+					use_xtal_mux_parents,
+					CLK_SET_RATE_PARENT,
+					port->membase + AML_UART_REG5,
+					24, 0x1,
+					CLK_MUX_READ_ONLY,
+					NULL, NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	port->uartclk = clk_hw_get_rate(hw);
+
+	hw = devm_clk_hw_register_divider(port->dev,
+					  CLK_NAME(baud_div),
+					  clk_hw_get_name(hw),
+					  CLK_SET_RATE_PARENT,
+					  port->membase + AML_UART_REG5,
+					  0, 23,
+					  CLK_DIVIDER_ROUND_CLOSEST,
+					  NULL);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+
+	private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
 
 	return 0;
 }
 
 static int meson_uart_probe(struct platform_device *pdev)
 {
+	struct meson_uart_data *private_data;
 	struct resource *res_mem;
 	struct uart_port *port;
 	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
@@ -716,18 +783,20 @@ static int meson_uart_probe(struct platform_device *pdev)
 		return -EBUSY;
 	}
 
-	port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port), GFP_KERNEL);
-	if (!port)
+	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
+				    GFP_KERNEL);
+	if (!private_data)
 		return -ENOMEM;
 
+	if (device_get_match_data(&pdev->dev))
+		private_data->use_xtal_clk = true;
+
+	port = &private_data->port;
+
 	port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
 	if (IS_ERR(port->membase))
 		return PTR_ERR(port->membase);
 
-	ret = meson_uart_probe_clocks(pdev, port);
-	if (ret)
-		return ret;
-
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
@@ -740,7 +809,11 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->x_char = 0;
 	port->ops = &meson_uart_ops;
 	port->fifosize = fifosize;
+	port->private_data = private_data;
 
+	ret = meson_uart_probe_clocks(port);
+	if (ret)
+		return ret;
 	meson_ports[pdev->id] = port;
 	platform_set_drvdata(pdev, port);
 
@@ -766,10 +839,39 @@ static int meson_uart_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id meson_uart_dt_match[] = {
-	{ .compatible = "amlogic,meson6-uart" },
-	{ .compatible = "amlogic,meson8-uart" },
-	{ .compatible = "amlogic,meson8b-uart" },
-	{ .compatible = "amlogic,meson-gx-uart" },
+	{
+		.compatible = "amlogic,meson6-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson8b-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxbb-uart",
+		.data = (void *)false,
+	},
+	{
+		.compatible = "amlogic,meson-gxl-uart",
+		.data = (void *)true,
+	},
+	{
+		.compatible = "amlogic,meson-g12a-uart",
+		.data = (void *)true,
+	},
+	/*
+	 * deprecated, don't use anymore because it doesn't differentiate
+	 * between GXBB, GXL and G12A which have different revisions
+	 * of the UART IP.
+	 */
+	{
+		.compatible = "amlogic,meson-gx-uart",
+		.data = (void *)false,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
-- 
2.33.1


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

Re: [PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Jiri Slaby]

Hi,

"Subject: Using the common clock code describe."

Please reformulate that ^^^ -- I cannot make any sense of it. And avoid 
adding a period to the subject. PLease see others' commit logs.

On 10. 01. 22, 11:42, Yu Tu wrote:
> Using the common Clock code to describe the UART baud rate clock
> makes it easier for the UART driver to be compatible with the
> baud rate requirements of the UART IP on different meson chips.



> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>   drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>   1 file changed, 163 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 7570958d010c..1004fd0b0c9e 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/console.h>
>   #include <linux/delay.h>
>   #include <linux/init.h>
> @@ -65,9 +66,7 @@
>   #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>   
>   /* AML_UART_REG5 bits */
> -#define AML_UART_BAUD_MASK		0x7fffff
>   #define AML_UART_BAUD_USE		BIT(23)
> -#define AML_UART_BAUD_XTAL		BIT(24)
>   
>   #define AML_UART_PORT_NUM		12
>   #define AML_UART_PORT_OFFSET		6
> @@ -76,6 +75,13 @@
>   #define AML_UART_POLL_USEC		5
>   #define AML_UART_TIMEOUT_USEC		10000
>   
> +struct meson_uart_data {
> +	struct uart_port	port;
> +	struct clk		*pclk;
> +	struct clk		*baud_clk;
> +	bool			use_xtal_clk;
> +};
> +
>   static struct uart_driver meson_uart_driver;
>   
>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
>   static int meson_uart_startup(struct uart_port *port)
>   {
>   	u32 val;
> -	int ret = 0;
> +	int ret;
>   
> -	val = readl(port->membase + AML_UART_CONTROL);
> -	val |= AML_UART_CLEAR_ERR;
> -	writel(val, port->membase + AML_UART_CONTROL);
> -	val &= ~AML_UART_CLEAR_ERR;
> -	writel(val, port->membase + AML_UART_CONTROL);
> +	meson_uart_reset(port);

Why is this OK? We didn't use to reset it.

And why is this not done in a separate patch?


> +	val = readl(port->membase + AML_UART_CONTROL);
>   	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>   	writel(val, port->membase + AML_UART_CONTROL);
>   
> @@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port *port)
>   
>   static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
>   	u32 val;
>   
>   	while (!meson_uart_tx_empty(port))
>   		cpu_relax();
>   
> -	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> -	} else {
> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> -	}
> +	val = readl(port->membase + AML_UART_REG5);
>   	val |= AML_UART_BAUD_USE;
>   	writel(val, port->membase + AML_UART_REG5);
> +
> +	clk_set_rate(private_data->baud_clk, baud);
>   }
>   
>   static void meson_uart_set_termios(struct uart_port *port,
> @@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	/* nothing to do */
> +	struct meson_uart_data *private_data = port->private_data;
> +
> +	clk_disable_unprepare(private_data->baud_clk);
> +	clk_disable_unprepare(private_data->pclk);
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
> +	int ret;
> +
> +	ret = clk_prepare_enable(private_data->pclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(private_data->baud_clk);
> +	if (ret) {
> +		clk_disable_unprepare(private_data->pclk);
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
>   	.cons		= MESON_SERIAL_CONSOLE,
>   };
>   
> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
> -						 const char *id)
> +#define CLK_NAME(name) \
> +({\
> +	char clk_name[32];\
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
> +	clk_name;\
> +})

This is cryptic and duplicates the variable on the stack. Why not expand 
the macro and delete it then?

> +
> +static struct clk_div_table xtal_div_table[] = {
> +	{ 0, 3  },

An extra space here and there:

> +	{ 1, 1  },
> +	{ 2, 2  },
> +	{ 3, 2  },
> +};
> +
> +static int meson_uart_probe_clocks(struct uart_port *port)
>   {
> -	struct clk *clk = NULL;
> -	int ret;
> +	struct meson_uart_data *private_data = port->private_data;
> +	struct clk *clk_baud, *clk_xtal;
> +	struct clk_hw *hw;
> +	struct clk_parent_data use_xtal_mux_parents[2] = {
> +		{ .index = -1, },
> +		{ .index = -1, },
> +	};
>   
> -	clk = devm_clk_get(dev, id);
> -	if (IS_ERR(clk))
> -		return clk;
> +	private_data->pclk = devm_clk_get(port->dev, "pclk");
> +	if (IS_ERR(private_data->pclk))
> +		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
> +				     "Failed to get the 'pclk' clock\n");
>   
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "couldn't enable clk\n");
> -		return ERR_PTR(ret);
> +	clk_baud = devm_clk_get(port->dev, "baud");
> +	if (IS_ERR(clk_baud)) {
> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
> +		return PTR_ERR(clk_baud);
>   	}
>   
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> -
> -	return clk;
> -}
> -
> -static int meson_uart_probe_clocks(struct platform_device *pdev,
> -				   struct uart_port *port)
> -{
> -	struct clk *clk_xtal = NULL;
> -	struct clk *clk_pclk = NULL;
> -	struct clk *clk_baud = NULL;
> -
> -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> -	if (IS_ERR(clk_pclk))
> -		return PTR_ERR(clk_pclk);
> -
> -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>   	if (IS_ERR(clk_xtal))
> -		return PTR_ERR(clk_xtal);
> -
> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> -	if (IS_ERR(clk_baud))
> -		return PTR_ERR(clk_baud);
> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
> +				     "Failed to get the 'xtal' clock\n");
> +
> +	if (private_data->use_xtal_clk) {
> +		hw = devm_clk_hw_register_divider_table(port->dev,
> +							CLK_NAME(xtal_div),
> +							__clk_get_name(clk_baud),
> +							CLK_SET_RATE_NO_REPARENT,
> +							port->membase + AML_UART_REG5,
> +							26, 2,
> +							CLK_DIVIDER_READ_ONLY,
> +							xtal_div_table, NULL);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[1].hw = hw;
> +	} else {
> +		hw = devm_clk_hw_register_fixed_factor(port->dev,
> +						       CLK_NAME(clk81_div4),
> +						       __clk_get_name(clk_baud),
> +						       CLK_SET_RATE_NO_REPARENT,
> +						       1, 4);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[0].hw = hw;
> +	}
>   
> -	port->uartclk = clk_get_rate(clk_baud);
> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
> +					CLK_NAME(use_xtal),
> +					ARRAY_SIZE(use_xtal_mux_parents),
> +					NULL, NULL,
> +					use_xtal_mux_parents,
> +					CLK_SET_RATE_PARENT,
> +					port->membase + AML_UART_REG5,
> +					24, 0x1,
> +					CLK_MUX_READ_ONLY,
> +					NULL, NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	port->uartclk = clk_hw_get_rate(hw);
> +
> +	hw = devm_clk_hw_register_divider(port->dev,
> +					  CLK_NAME(baud_div),
> +					  clk_hw_get_name(hw),
> +					  CLK_SET_RATE_PARENT,
> +					  port->membase + AML_UART_REG5,
> +					  0, 23,
> +					  CLK_DIVIDER_ROUND_CLOSEST,
> +					  NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
>   
>   	return 0;
>   }



-- 
js
suse labs

Re: [PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Jiri Slaby]

Hi,

"Subject: Using the common clock code describe."

Please reformulate that ^^^ -- I cannot make any sense of it. And avoid 
adding a period to the subject. PLease see others' commit logs.

On 10. 01. 22, 11:42, Yu Tu wrote:
> Using the common Clock code to describe the UART baud rate clock
> makes it easier for the UART driver to be compatible with the
> baud rate requirements of the UART IP on different meson chips.



> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>   drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>   1 file changed, 163 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 7570958d010c..1004fd0b0c9e 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/console.h>
>   #include <linux/delay.h>
>   #include <linux/init.h>
> @@ -65,9 +66,7 @@
>   #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>   
>   /* AML_UART_REG5 bits */
> -#define AML_UART_BAUD_MASK		0x7fffff
>   #define AML_UART_BAUD_USE		BIT(23)
> -#define AML_UART_BAUD_XTAL		BIT(24)
>   
>   #define AML_UART_PORT_NUM		12
>   #define AML_UART_PORT_OFFSET		6
> @@ -76,6 +75,13 @@
>   #define AML_UART_POLL_USEC		5
>   #define AML_UART_TIMEOUT_USEC		10000
>   
> +struct meson_uart_data {
> +	struct uart_port	port;
> +	struct clk		*pclk;
> +	struct clk		*baud_clk;
> +	bool			use_xtal_clk;
> +};
> +
>   static struct uart_driver meson_uart_driver;
>   
>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
>   static int meson_uart_startup(struct uart_port *port)
>   {
>   	u32 val;
> -	int ret = 0;
> +	int ret;
>   
> -	val = readl(port->membase + AML_UART_CONTROL);
> -	val |= AML_UART_CLEAR_ERR;
> -	writel(val, port->membase + AML_UART_CONTROL);
> -	val &= ~AML_UART_CLEAR_ERR;
> -	writel(val, port->membase + AML_UART_CONTROL);
> +	meson_uart_reset(port);

Why is this OK? We didn't use to reset it.

And why is this not done in a separate patch?


> +	val = readl(port->membase + AML_UART_CONTROL);
>   	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>   	writel(val, port->membase + AML_UART_CONTROL);
>   
> @@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port *port)
>   
>   static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
>   	u32 val;
>   
>   	while (!meson_uart_tx_empty(port))
>   		cpu_relax();
>   
> -	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> -	} else {
> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> -	}
> +	val = readl(port->membase + AML_UART_REG5);
>   	val |= AML_UART_BAUD_USE;
>   	writel(val, port->membase + AML_UART_REG5);
> +
> +	clk_set_rate(private_data->baud_clk, baud);
>   }
>   
>   static void meson_uart_set_termios(struct uart_port *port,
> @@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	/* nothing to do */
> +	struct meson_uart_data *private_data = port->private_data;
> +
> +	clk_disable_unprepare(private_data->baud_clk);
> +	clk_disable_unprepare(private_data->pclk);
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
> +	int ret;
> +
> +	ret = clk_prepare_enable(private_data->pclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(private_data->baud_clk);
> +	if (ret) {
> +		clk_disable_unprepare(private_data->pclk);
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
>   	.cons		= MESON_SERIAL_CONSOLE,
>   };
>   
> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
> -						 const char *id)
> +#define CLK_NAME(name) \
> +({\
> +	char clk_name[32];\
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
> +	clk_name;\
> +})

This is cryptic and duplicates the variable on the stack. Why not expand 
the macro and delete it then?

> +
> +static struct clk_div_table xtal_div_table[] = {
> +	{ 0, 3  },

An extra space here and there:

> +	{ 1, 1  },
> +	{ 2, 2  },
> +	{ 3, 2  },
> +};
> +
> +static int meson_uart_probe_clocks(struct uart_port *port)
>   {
> -	struct clk *clk = NULL;
> -	int ret;
> +	struct meson_uart_data *private_data = port->private_data;
> +	struct clk *clk_baud, *clk_xtal;
> +	struct clk_hw *hw;
> +	struct clk_parent_data use_xtal_mux_parents[2] = {
> +		{ .index = -1, },
> +		{ .index = -1, },
> +	};
>   
> -	clk = devm_clk_get(dev, id);
> -	if (IS_ERR(clk))
> -		return clk;
> +	private_data->pclk = devm_clk_get(port->dev, "pclk");
> +	if (IS_ERR(private_data->pclk))
> +		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
> +				     "Failed to get the 'pclk' clock\n");
>   
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "couldn't enable clk\n");
> -		return ERR_PTR(ret);
> +	clk_baud = devm_clk_get(port->dev, "baud");
> +	if (IS_ERR(clk_baud)) {
> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
> +		return PTR_ERR(clk_baud);
>   	}
>   
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> -
> -	return clk;
> -}
> -
> -static int meson_uart_probe_clocks(struct platform_device *pdev,
> -				   struct uart_port *port)
> -{
> -	struct clk *clk_xtal = NULL;
> -	struct clk *clk_pclk = NULL;
> -	struct clk *clk_baud = NULL;
> -
> -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> -	if (IS_ERR(clk_pclk))
> -		return PTR_ERR(clk_pclk);
> -
> -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>   	if (IS_ERR(clk_xtal))
> -		return PTR_ERR(clk_xtal);
> -
> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> -	if (IS_ERR(clk_baud))
> -		return PTR_ERR(clk_baud);
> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
> +				     "Failed to get the 'xtal' clock\n");
> +
> +	if (private_data->use_xtal_clk) {
> +		hw = devm_clk_hw_register_divider_table(port->dev,
> +							CLK_NAME(xtal_div),
> +							__clk_get_name(clk_baud),
> +							CLK_SET_RATE_NO_REPARENT,
> +							port->membase + AML_UART_REG5,
> +							26, 2,
> +							CLK_DIVIDER_READ_ONLY,
> +							xtal_div_table, NULL);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[1].hw = hw;
> +	} else {
> +		hw = devm_clk_hw_register_fixed_factor(port->dev,
> +						       CLK_NAME(clk81_div4),
> +						       __clk_get_name(clk_baud),
> +						       CLK_SET_RATE_NO_REPARENT,
> +						       1, 4);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[0].hw = hw;
> +	}
>   
> -	port->uartclk = clk_get_rate(clk_baud);
> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
> +					CLK_NAME(use_xtal),
> +					ARRAY_SIZE(use_xtal_mux_parents),
> +					NULL, NULL,
> +					use_xtal_mux_parents,
> +					CLK_SET_RATE_PARENT,
> +					port->membase + AML_UART_REG5,
> +					24, 0x1,
> +					CLK_MUX_READ_ONLY,
> +					NULL, NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	port->uartclk = clk_hw_get_rate(hw);
> +
> +	hw = devm_clk_hw_register_divider(port->dev,
> +					  CLK_NAME(baud_div),
> +					  clk_hw_get_name(hw),
> +					  CLK_SET_RATE_PARENT,
> +					  port->membase + AML_UART_REG5,
> +					  0, 23,
> +					  CLK_DIVIDER_ROUND_CLOSEST,
> +					  NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
>   
>   	return 0;
>   }



-- 
js
suse labs

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

Re: [PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Jiri Slaby]

Hi,

"Subject: Using the common clock code describe."

Please reformulate that ^^^ -- I cannot make any sense of it. And avoid 
adding a period to the subject. PLease see others' commit logs.

On 10. 01. 22, 11:42, Yu Tu wrote:
> Using the common Clock code to describe the UART baud rate clock
> makes it easier for the UART driver to be compatible with the
> baud rate requirements of the UART IP on different meson chips.



> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> ---
>   drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>   1 file changed, 163 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 7570958d010c..1004fd0b0c9e 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/console.h>
>   #include <linux/delay.h>
>   #include <linux/init.h>
> @@ -65,9 +66,7 @@
>   #define AML_UART_RECV_IRQ(c)		((c) & 0xff)
>   
>   /* AML_UART_REG5 bits */
> -#define AML_UART_BAUD_MASK		0x7fffff
>   #define AML_UART_BAUD_USE		BIT(23)
> -#define AML_UART_BAUD_XTAL		BIT(24)
>   
>   #define AML_UART_PORT_NUM		12
>   #define AML_UART_PORT_OFFSET		6
> @@ -76,6 +75,13 @@
>   #define AML_UART_POLL_USEC		5
>   #define AML_UART_TIMEOUT_USEC		10000
>   
> +struct meson_uart_data {
> +	struct uart_port	port;
> +	struct clk		*pclk;
> +	struct clk		*baud_clk;
> +	bool			use_xtal_clk;
> +};
> +
>   static struct uart_driver meson_uart_driver;
>   
>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port *port)
>   static int meson_uart_startup(struct uart_port *port)
>   {
>   	u32 val;
> -	int ret = 0;
> +	int ret;
>   
> -	val = readl(port->membase + AML_UART_CONTROL);
> -	val |= AML_UART_CLEAR_ERR;
> -	writel(val, port->membase + AML_UART_CONTROL);
> -	val &= ~AML_UART_CLEAR_ERR;
> -	writel(val, port->membase + AML_UART_CONTROL);
> +	meson_uart_reset(port);

Why is this OK? We didn't use to reset it.

And why is this not done in a separate patch?


> +	val = readl(port->membase + AML_UART_CONTROL);
>   	val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>   	writel(val, port->membase + AML_UART_CONTROL);
>   
> @@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port *port)
>   
>   static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
>   	u32 val;
>   
>   	while (!meson_uart_tx_empty(port))
>   		cpu_relax();
>   
> -	if (port->uartclk == 24000000) {
> -		val = ((port->uartclk / 3) / baud) - 1;
> -		val |= AML_UART_BAUD_XTAL;
> -	} else {
> -		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
> -	}
> +	val = readl(port->membase + AML_UART_REG5);
>   	val |= AML_UART_BAUD_USE;
>   	writel(val, port->membase + AML_UART_REG5);
> +
> +	clk_set_rate(private_data->baud_clk, baud);
>   }
>   
>   static void meson_uart_set_termios(struct uart_port *port,
> @@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct uart_port *port,
>   
>   static void meson_uart_release_port(struct uart_port *port)
>   {
> -	/* nothing to do */
> +	struct meson_uart_data *private_data = port->private_data;
> +
> +	clk_disable_unprepare(private_data->baud_clk);
> +	clk_disable_unprepare(private_data->pclk);
>   }
>   
>   static int meson_uart_request_port(struct uart_port *port)
>   {
> +	struct meson_uart_data *private_data = port->private_data;
> +	int ret;
> +
> +	ret = clk_prepare_enable(private_data->pclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(private_data->baud_clk);
> +	if (ret) {
> +		clk_disable_unprepare(private_data->pclk);
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
>   	.cons		= MESON_SERIAL_CONSOLE,
>   };
>   
> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
> -						 const char *id)
> +#define CLK_NAME(name) \
> +({\
> +	char clk_name[32];\
> +	snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), #name);\
> +	clk_name;\
> +})

This is cryptic and duplicates the variable on the stack. Why not expand 
the macro and delete it then?

> +
> +static struct clk_div_table xtal_div_table[] = {
> +	{ 0, 3  },

An extra space here and there:

> +	{ 1, 1  },
> +	{ 2, 2  },
> +	{ 3, 2  },
> +};
> +
> +static int meson_uart_probe_clocks(struct uart_port *port)
>   {
> -	struct clk *clk = NULL;
> -	int ret;
> +	struct meson_uart_data *private_data = port->private_data;
> +	struct clk *clk_baud, *clk_xtal;
> +	struct clk_hw *hw;
> +	struct clk_parent_data use_xtal_mux_parents[2] = {
> +		{ .index = -1, },
> +		{ .index = -1, },
> +	};
>   
> -	clk = devm_clk_get(dev, id);
> -	if (IS_ERR(clk))
> -		return clk;
> +	private_data->pclk = devm_clk_get(port->dev, "pclk");
> +	if (IS_ERR(private_data->pclk))
> +		return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
> +				     "Failed to get the 'pclk' clock\n");
>   
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "couldn't enable clk\n");
> -		return ERR_PTR(ret);
> +	clk_baud = devm_clk_get(port->dev, "baud");
> +	if (IS_ERR(clk_baud)) {
> +		dev_err(port->dev, "Failed to get the 'baud' clock\n");
> +		return PTR_ERR(clk_baud);
>   	}
>   
> -	devm_add_action_or_reset(dev,
> -			(void(*)(void *))clk_disable_unprepare,
> -			clk);
> -
> -	return clk;
> -}
> -
> -static int meson_uart_probe_clocks(struct platform_device *pdev,
> -				   struct uart_port *port)
> -{
> -	struct clk *clk_xtal = NULL;
> -	struct clk *clk_pclk = NULL;
> -	struct clk *clk_baud = NULL;
> -
> -	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
> -	if (IS_ERR(clk_pclk))
> -		return PTR_ERR(clk_pclk);
> -
> -	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
> +	clk_xtal = devm_clk_get(port->dev, "xtal");
>   	if (IS_ERR(clk_xtal))
> -		return PTR_ERR(clk_xtal);
> -
> -	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
> -	if (IS_ERR(clk_baud))
> -		return PTR_ERR(clk_baud);
> +		return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
> +				     "Failed to get the 'xtal' clock\n");
> +
> +	if (private_data->use_xtal_clk) {
> +		hw = devm_clk_hw_register_divider_table(port->dev,
> +							CLK_NAME(xtal_div),
> +							__clk_get_name(clk_baud),
> +							CLK_SET_RATE_NO_REPARENT,
> +							port->membase + AML_UART_REG5,
> +							26, 2,
> +							CLK_DIVIDER_READ_ONLY,
> +							xtal_div_table, NULL);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[1].hw = hw;
> +	} else {
> +		hw = devm_clk_hw_register_fixed_factor(port->dev,
> +						       CLK_NAME(clk81_div4),
> +						       __clk_get_name(clk_baud),
> +						       CLK_SET_RATE_NO_REPARENT,
> +						       1, 4);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +
> +		use_xtal_mux_parents[0].hw = hw;
> +	}
>   
> -	port->uartclk = clk_get_rate(clk_baud);
> +	hw = __devm_clk_hw_register_mux(port->dev, NULL,
> +					CLK_NAME(use_xtal),
> +					ARRAY_SIZE(use_xtal_mux_parents),
> +					NULL, NULL,
> +					use_xtal_mux_parents,
> +					CLK_SET_RATE_PARENT,
> +					port->membase + AML_UART_REG5,
> +					24, 0x1,
> +					CLK_MUX_READ_ONLY,
> +					NULL, NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	port->uartclk = clk_hw_get_rate(hw);
> +
> +	hw = devm_clk_hw_register_divider(port->dev,
> +					  CLK_NAME(baud_div),
> +					  clk_hw_get_name(hw),
> +					  CLK_SET_RATE_PARENT,
> +					  port->membase + AML_UART_REG5,
> +					  0, 23,
> +					  CLK_DIVIDER_ROUND_CLOSEST,
> +					  NULL);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
>   
>   	return 0;
>   }



-- 
js
suse labs

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

Re: [PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Yu Tu]

Hi Jiri,
	Thank you very much for your reply.

On 2022/1/10 20:11, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> "Subject: Using the common clock code describe."
> 
> Please reformulate that ^^^ -- I cannot make any sense of it. And avoid 
> adding a period to the subject. PLease see others' commit logs.
> 
I will correct this in the next version.

> On 10. 01. 22, 11:42, Yu Tu wrote:
>> Using the common Clock code to describe the UART baud rate clock
>> makes it easier for the UART driver to be compatible with the
>> baud rate requirements of the UART IP on different meson chips.
> 
> 
> 
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>>   1 file changed, 163 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..1004fd0b0c9e 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -6,6 +6,7 @@
>>    */
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/delay.h>
>>   #include <linux/init.h>
>> @@ -65,9 +66,7 @@
>>   #define AML_UART_RECV_IRQ(c)        ((c) & 0xff)
>>   /* AML_UART_REG5 bits */
>> -#define AML_UART_BAUD_MASK        0x7fffff
>>   #define AML_UART_BAUD_USE        BIT(23)
>> -#define AML_UART_BAUD_XTAL        BIT(24)
>>   #define AML_UART_PORT_NUM        12
>>   #define AML_UART_PORT_OFFSET        6
>> @@ -76,6 +75,13 @@
>>   #define AML_UART_POLL_USEC        5
>>   #define AML_UART_TIMEOUT_USEC        10000
>> +struct meson_uart_data {
>> +    struct uart_port    port;
>> +    struct clk        *pclk;
>> +    struct clk        *baud_clk;
>> +    bool            use_xtal_clk;
>> +};
>> +
>>   static struct uart_driver meson_uart_driver;
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port 
>> *port)
>>   static int meson_uart_startup(struct uart_port *port)
>>   {
>>       u32 val;
>> -    int ret = 0;
>> +    int ret;
>> -    val = readl(port->membase + AML_UART_CONTROL);
>> -    val |= AML_UART_CLEAR_ERR;
>> -    writel(val, port->membase + AML_UART_CONTROL);
>> -    val &= ~AML_UART_CLEAR_ERR;
>> -    writel(val, port->membase + AML_UART_CONTROL);
>> +    meson_uart_reset(port);
> 
> Why is this OK? We didn't use to reset it.
This change simply optimizes the code. Because the meson_uart_reset 
function has implemented the control logic.
> 
> And why is this not done in a separate patch?
I will consider merging with the previous patch as a patch.
> 
> 
>> +    val = readl(port->membase + AML_UART_CONTROL);
>>       val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>>       writel(val, port->membase + AML_UART_CONTROL);
>> @@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port 
>> *port)
>>   static void meson_uart_change_speed(struct uart_port *port, unsigned 
>> long baud)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>>       u32 val;
>>       while (!meson_uart_tx_empty(port))
>>           cpu_relax();
>> -    if (port->uartclk == 24000000) {
>> -        val = ((port->uartclk / 3) / baud) - 1;
>> -        val |= AML_UART_BAUD_XTAL;
>> -    } else {
>> -        val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> -    }
>> +    val = readl(port->membase + AML_UART_REG5);
>>       val |= AML_UART_BAUD_USE;
>>       writel(val, port->membase + AML_UART_REG5);
>> +
>> +    clk_set_rate(private_data->baud_clk, baud);
>>   }
>>   static void meson_uart_set_termios(struct uart_port *port,
>> @@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct 
>> uart_port *port,
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -    /* nothing to do */
>> +    struct meson_uart_data *private_data = port->private_data;
>> +
>> +    clk_disable_unprepare(private_data->baud_clk);
>> +    clk_disable_unprepare(private_data->pclk);
>>   }
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>> +    int ret;
>> +
>> +    ret = clk_prepare_enable(private_data->pclk);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = clk_prepare_enable(private_data->baud_clk);
>> +    if (ret) {
>> +        clk_disable_unprepare(private_data->pclk);
>> +        return ret;
>> +    }
>> +
>>       return 0;
>>   }
>> @@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
>>       .cons        = MESON_SERIAL_CONSOLE,
>>   };
>> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>> -                         const char *id)
>> +#define CLK_NAME(name) \
>> +({\
>> +    char clk_name[32];\
>> +    snprintf(clk_name, sizeof(clk_name), "%s#%s", 
>> dev_name(port->dev), #name);\
>> +    clk_name;\
>> +})
> 
> This is cryptic and duplicates the variable on the stack. Why not expand 
> the macro and delete it then?
> 
I've overdone it. I'll move it to the meson_uart_probe_clocks function 
as you suggested and delete it.
>> +
>> +static struct clk_div_table xtal_div_table[] = {
>> +    { 0, 3  },
> 
> An extra space here and there:
> 
>> +    { 1, 1  },
>> +    { 2, 2  },
>> +    { 3, 2  },
>> +};
>> +
Sorry, stupid mistake. I will correct.

>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>   {
>> -    struct clk *clk = NULL;
>> -    int ret;
>> +    struct meson_uart_data *private_data = port->private_data;
>> +    struct clk *clk_baud, *clk_xtal;
>> +    struct clk_hw *hw;
>> +    struct clk_parent_data use_xtal_mux_parents[2] = {
>> +        { .index = -1, },
>> +        { .index = -1, },
>> +    };
>> -    clk = devm_clk_get(dev, id);
>> -    if (IS_ERR(clk))
>> -        return clk;
>> +    private_data->pclk = devm_clk_get(port->dev, "pclk");
>> +    if (IS_ERR(private_data->pclk))
>> +        return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
>> +                     "Failed to get the 'pclk' clock\n");
>> -    ret = clk_prepare_enable(clk);
>> -    if (ret) {
>> -        dev_err(dev, "couldn't enable clk\n");
>> -        return ERR_PTR(ret);
>> +    clk_baud = devm_clk_get(port->dev, "baud");
>> +    if (IS_ERR(clk_baud)) {
>> +        dev_err(port->dev, "Failed to get the 'baud' clock\n");
>> +        return PTR_ERR(clk_baud);
>>       }
>> -    devm_add_action_or_reset(dev,
>> -            (void(*)(void *))clk_disable_unprepare,
>> -            clk);
>> -
>> -    return clk;
>> -}
>> -
>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>> -                   struct uart_port *port)
>> -{
>> -    struct clk *clk_xtal = NULL;
>> -    struct clk *clk_pclk = NULL;
>> -    struct clk *clk_baud = NULL;
>> -
>> -    clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> -    if (IS_ERR(clk_pclk))
>> -        return PTR_ERR(clk_pclk);
>> -
>> -    clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> +    clk_xtal = devm_clk_get(port->dev, "xtal");
>>       if (IS_ERR(clk_xtal))
>> -        return PTR_ERR(clk_xtal);
>> -
>> -    clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> -    if (IS_ERR(clk_baud))
>> -        return PTR_ERR(clk_baud);
>> +        return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>> +                     "Failed to get the 'xtal' clock\n");
>> +
>> +    if (private_data->use_xtal_clk) {
>> +        hw = devm_clk_hw_register_divider_table(port->dev,
>> +                            CLK_NAME(xtal_div),
>> +                            __clk_get_name(clk_baud),
>> +                            CLK_SET_RATE_NO_REPARENT,
>> +                            port->membase + AML_UART_REG5,
>> +                            26, 2,
>> +                            CLK_DIVIDER_READ_ONLY,
>> +                            xtal_div_table, NULL);
>> +        if (IS_ERR(hw))
>> +            return PTR_ERR(hw);
>> +
>> +        use_xtal_mux_parents[1].hw = hw;
>> +    } else {
>> +        hw = devm_clk_hw_register_fixed_factor(port->dev,
>> +                               CLK_NAME(clk81_div4),
>> +                               __clk_get_name(clk_baud),
>> +                               CLK_SET_RATE_NO_REPARENT,
>> +                               1, 4);
>> +        if (IS_ERR(hw))
>> +            return PTR_ERR(hw);
>> +
>> +        use_xtal_mux_parents[0].hw = hw;
>> +    }
>> -    port->uartclk = clk_get_rate(clk_baud);
>> +    hw = __devm_clk_hw_register_mux(port->dev, NULL,
>> +                    CLK_NAME(use_xtal),
>> +                    ARRAY_SIZE(use_xtal_mux_parents),
>> +                    NULL, NULL,
>> +                    use_xtal_mux_parents,
>> +                    CLK_SET_RATE_PARENT,
>> +                    port->membase + AML_UART_REG5,
>> +                    24, 0x1,
>> +                    CLK_MUX_READ_ONLY,
>> +                    NULL, NULL);
>> +    if (IS_ERR(hw))
>> +        return PTR_ERR(hw);
>> +
>> +    port->uartclk = clk_hw_get_rate(hw);
>> +
>> +    hw = devm_clk_hw_register_divider(port->dev,
>> +                      CLK_NAME(baud_div),
>> +                      clk_hw_get_name(hw),
>> +                      CLK_SET_RATE_PARENT,
>> +                      port->membase + AML_UART_REG5,
>> +                      0, 23,
>> +                      CLK_DIVIDER_ROUND_CLOSEST,
>> +                      NULL);
>> +    if (IS_ERR(hw))
>> +        return PTR_ERR(hw);
>> +
>> +    private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
>>       return 0;
>>   }
> 
> 
> 

Re: [PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Yu Tu]

Hi Jiri,
	Thank you very much for your reply.

On 2022/1/10 20:11, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> "Subject: Using the common clock code describe."
> 
> Please reformulate that ^^^ -- I cannot make any sense of it. And avoid 
> adding a period to the subject. PLease see others' commit logs.
> 
I will correct this in the next version.

> On 10. 01. 22, 11:42, Yu Tu wrote:
>> Using the common Clock code to describe the UART baud rate clock
>> makes it easier for the UART driver to be compatible with the
>> baud rate requirements of the UART IP on different meson chips.
> 
> 
> 
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>>   1 file changed, 163 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..1004fd0b0c9e 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -6,6 +6,7 @@
>>    */
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/delay.h>
>>   #include <linux/init.h>
>> @@ -65,9 +66,7 @@
>>   #define AML_UART_RECV_IRQ(c)        ((c) & 0xff)
>>   /* AML_UART_REG5 bits */
>> -#define AML_UART_BAUD_MASK        0x7fffff
>>   #define AML_UART_BAUD_USE        BIT(23)
>> -#define AML_UART_BAUD_XTAL        BIT(24)
>>   #define AML_UART_PORT_NUM        12
>>   #define AML_UART_PORT_OFFSET        6
>> @@ -76,6 +75,13 @@
>>   #define AML_UART_POLL_USEC        5
>>   #define AML_UART_TIMEOUT_USEC        10000
>> +struct meson_uart_data {
>> +    struct uart_port    port;
>> +    struct clk        *pclk;
>> +    struct clk        *baud_clk;
>> +    bool            use_xtal_clk;
>> +};
>> +
>>   static struct uart_driver meson_uart_driver;
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port 
>> *port)
>>   static int meson_uart_startup(struct uart_port *port)
>>   {
>>       u32 val;
>> -    int ret = 0;
>> +    int ret;
>> -    val = readl(port->membase + AML_UART_CONTROL);
>> -    val |= AML_UART_CLEAR_ERR;
>> -    writel(val, port->membase + AML_UART_CONTROL);
>> -    val &= ~AML_UART_CLEAR_ERR;
>> -    writel(val, port->membase + AML_UART_CONTROL);
>> +    meson_uart_reset(port);
> 
> Why is this OK? We didn't use to reset it.
This change simply optimizes the code. Because the meson_uart_reset 
function has implemented the control logic.
> 
> And why is this not done in a separate patch?
I will consider merging with the previous patch as a patch.
> 
> 
>> +    val = readl(port->membase + AML_UART_CONTROL);
>>       val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>>       writel(val, port->membase + AML_UART_CONTROL);
>> @@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port 
>> *port)
>>   static void meson_uart_change_speed(struct uart_port *port, unsigned 
>> long baud)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>>       u32 val;
>>       while (!meson_uart_tx_empty(port))
>>           cpu_relax();
>> -    if (port->uartclk == 24000000) {
>> -        val = ((port->uartclk / 3) / baud) - 1;
>> -        val |= AML_UART_BAUD_XTAL;
>> -    } else {
>> -        val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> -    }
>> +    val = readl(port->membase + AML_UART_REG5);
>>       val |= AML_UART_BAUD_USE;
>>       writel(val, port->membase + AML_UART_REG5);
>> +
>> +    clk_set_rate(private_data->baud_clk, baud);
>>   }
>>   static void meson_uart_set_termios(struct uart_port *port,
>> @@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct 
>> uart_port *port,
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -    /* nothing to do */
>> +    struct meson_uart_data *private_data = port->private_data;
>> +
>> +    clk_disable_unprepare(private_data->baud_clk);
>> +    clk_disable_unprepare(private_data->pclk);
>>   }
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>> +    int ret;
>> +
>> +    ret = clk_prepare_enable(private_data->pclk);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = clk_prepare_enable(private_data->baud_clk);
>> +    if (ret) {
>> +        clk_disable_unprepare(private_data->pclk);
>> +        return ret;
>> +    }
>> +
>>       return 0;
>>   }
>> @@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
>>       .cons        = MESON_SERIAL_CONSOLE,
>>   };
>> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>> -                         const char *id)
>> +#define CLK_NAME(name) \
>> +({\
>> +    char clk_name[32];\
>> +    snprintf(clk_name, sizeof(clk_name), "%s#%s", 
>> dev_name(port->dev), #name);\
>> +    clk_name;\
>> +})
> 
> This is cryptic and duplicates the variable on the stack. Why not expand 
> the macro and delete it then?
> 
I've overdone it. I'll move it to the meson_uart_probe_clocks function 
as you suggested and delete it.
>> +
>> +static struct clk_div_table xtal_div_table[] = {
>> +    { 0, 3  },
> 
> An extra space here and there:
> 
>> +    { 1, 1  },
>> +    { 2, 2  },
>> +    { 3, 2  },
>> +};
>> +
Sorry, stupid mistake. I will correct.

>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>   {
>> -    struct clk *clk = NULL;
>> -    int ret;
>> +    struct meson_uart_data *private_data = port->private_data;
>> +    struct clk *clk_baud, *clk_xtal;
>> +    struct clk_hw *hw;
>> +    struct clk_parent_data use_xtal_mux_parents[2] = {
>> +        { .index = -1, },
>> +        { .index = -1, },
>> +    };
>> -    clk = devm_clk_get(dev, id);
>> -    if (IS_ERR(clk))
>> -        return clk;
>> +    private_data->pclk = devm_clk_get(port->dev, "pclk");
>> +    if (IS_ERR(private_data->pclk))
>> +        return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
>> +                     "Failed to get the 'pclk' clock\n");
>> -    ret = clk_prepare_enable(clk);
>> -    if (ret) {
>> -        dev_err(dev, "couldn't enable clk\n");
>> -        return ERR_PTR(ret);
>> +    clk_baud = devm_clk_get(port->dev, "baud");
>> +    if (IS_ERR(clk_baud)) {
>> +        dev_err(port->dev, "Failed to get the 'baud' clock\n");
>> +        return PTR_ERR(clk_baud);
>>       }
>> -    devm_add_action_or_reset(dev,
>> -            (void(*)(void *))clk_disable_unprepare,
>> -            clk);
>> -
>> -    return clk;
>> -}
>> -
>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>> -                   struct uart_port *port)
>> -{
>> -    struct clk *clk_xtal = NULL;
>> -    struct clk *clk_pclk = NULL;
>> -    struct clk *clk_baud = NULL;
>> -
>> -    clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> -    if (IS_ERR(clk_pclk))
>> -        return PTR_ERR(clk_pclk);
>> -
>> -    clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> +    clk_xtal = devm_clk_get(port->dev, "xtal");
>>       if (IS_ERR(clk_xtal))
>> -        return PTR_ERR(clk_xtal);
>> -
>> -    clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> -    if (IS_ERR(clk_baud))
>> -        return PTR_ERR(clk_baud);
>> +        return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>> +                     "Failed to get the 'xtal' clock\n");
>> +
>> +    if (private_data->use_xtal_clk) {
>> +        hw = devm_clk_hw_register_divider_table(port->dev,
>> +                            CLK_NAME(xtal_div),
>> +                            __clk_get_name(clk_baud),
>> +                            CLK_SET_RATE_NO_REPARENT,
>> +                            port->membase + AML_UART_REG5,
>> +                            26, 2,
>> +                            CLK_DIVIDER_READ_ONLY,
>> +                            xtal_div_table, NULL);
>> +        if (IS_ERR(hw))
>> +            return PTR_ERR(hw);
>> +
>> +        use_xtal_mux_parents[1].hw = hw;
>> +    } else {
>> +        hw = devm_clk_hw_register_fixed_factor(port->dev,
>> +                               CLK_NAME(clk81_div4),
>> +                               __clk_get_name(clk_baud),
>> +                               CLK_SET_RATE_NO_REPARENT,
>> +                               1, 4);
>> +        if (IS_ERR(hw))
>> +            return PTR_ERR(hw);
>> +
>> +        use_xtal_mux_parents[0].hw = hw;
>> +    }
>> -    port->uartclk = clk_get_rate(clk_baud);
>> +    hw = __devm_clk_hw_register_mux(port->dev, NULL,
>> +                    CLK_NAME(use_xtal),
>> +                    ARRAY_SIZE(use_xtal_mux_parents),
>> +                    NULL, NULL,
>> +                    use_xtal_mux_parents,
>> +                    CLK_SET_RATE_PARENT,
>> +                    port->membase + AML_UART_REG5,
>> +                    24, 0x1,
>> +                    CLK_MUX_READ_ONLY,
>> +                    NULL, NULL);
>> +    if (IS_ERR(hw))
>> +        return PTR_ERR(hw);
>> +
>> +    port->uartclk = clk_hw_get_rate(hw);
>> +
>> +    hw = devm_clk_hw_register_divider(port->dev,
>> +                      CLK_NAME(baud_div),
>> +                      clk_hw_get_name(hw),
>> +                      CLK_SET_RATE_PARENT,
>> +                      port->membase + AML_UART_REG5,
>> +                      0, 23,
>> +                      CLK_DIVIDER_ROUND_CLOSEST,
>> +                      NULL);
>> +    if (IS_ERR(hw))
>> +        return PTR_ERR(hw);
>> +
>> +    private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
>>       return 0;
>>   }
> 
> 
> 

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

Re: [PATCH V5 3/5] tty: serial: meson: Using the common clock code describe.

[Yu Tu]

Hi Jiri,
	Thank you very much for your reply.

On 2022/1/10 20:11, Jiri Slaby wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> "Subject: Using the common clock code describe."
> 
> Please reformulate that ^^^ -- I cannot make any sense of it. And avoid 
> adding a period to the subject. PLease see others' commit logs.
> 
I will correct this in the next version.

> On 10. 01. 22, 11:42, Yu Tu wrote:
>> Using the common Clock code to describe the UART baud rate clock
>> makes it easier for the UART driver to be compatible with the
>> baud rate requirements of the UART IP on different meson chips.
> 
> 
> 
>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>> ---
>>   drivers/tty/serial/meson_uart.c | 224 +++++++++++++++++++++++---------
>>   1 file changed, 163 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c 
>> b/drivers/tty/serial/meson_uart.c
>> index 7570958d010c..1004fd0b0c9e 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -6,6 +6,7 @@
>>    */
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/console.h>
>>   #include <linux/delay.h>
>>   #include <linux/init.h>
>> @@ -65,9 +66,7 @@
>>   #define AML_UART_RECV_IRQ(c)        ((c) & 0xff)
>>   /* AML_UART_REG5 bits */
>> -#define AML_UART_BAUD_MASK        0x7fffff
>>   #define AML_UART_BAUD_USE        BIT(23)
>> -#define AML_UART_BAUD_XTAL        BIT(24)
>>   #define AML_UART_PORT_NUM        12
>>   #define AML_UART_PORT_OFFSET        6
>> @@ -76,6 +75,13 @@
>>   #define AML_UART_POLL_USEC        5
>>   #define AML_UART_TIMEOUT_USEC        10000
>> +struct meson_uart_data {
>> +    struct uart_port    port;
>> +    struct clk        *pclk;
>> +    struct clk        *baud_clk;
>> +    bool            use_xtal_clk;
>> +};
>> +
>>   static struct uart_driver meson_uart_driver;
>>   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>> @@ -268,14 +274,11 @@ static void meson_uart_reset(struct uart_port 
>> *port)
>>   static int meson_uart_startup(struct uart_port *port)
>>   {
>>       u32 val;
>> -    int ret = 0;
>> +    int ret;
>> -    val = readl(port->membase + AML_UART_CONTROL);
>> -    val |= AML_UART_CLEAR_ERR;
>> -    writel(val, port->membase + AML_UART_CONTROL);
>> -    val &= ~AML_UART_CLEAR_ERR;
>> -    writel(val, port->membase + AML_UART_CONTROL);
>> +    meson_uart_reset(port);
> 
> Why is this OK? We didn't use to reset it.
This change simply optimizes the code. Because the meson_uart_reset 
function has implemented the control logic.
> 
> And why is this not done in a separate patch?
I will consider merging with the previous patch as a patch.
> 
> 
>> +    val = readl(port->membase + AML_UART_CONTROL);
>>       val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>>       writel(val, port->membase + AML_UART_CONTROL);
>> @@ -293,19 +296,17 @@ static int meson_uart_startup(struct uart_port 
>> *port)
>>   static void meson_uart_change_speed(struct uart_port *port, unsigned 
>> long baud)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>>       u32 val;
>>       while (!meson_uart_tx_empty(port))
>>           cpu_relax();
>> -    if (port->uartclk == 24000000) {
>> -        val = ((port->uartclk / 3) / baud) - 1;
>> -        val |= AML_UART_BAUD_XTAL;
>> -    } else {
>> -        val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>> -    }
>> +    val = readl(port->membase + AML_UART_REG5);
>>       val |= AML_UART_BAUD_USE;
>>       writel(val, port->membase + AML_UART_REG5);
>> +
>> +    clk_set_rate(private_data->baud_clk, baud);
>>   }
>>   static void meson_uart_set_termios(struct uart_port *port,
>> @@ -395,11 +396,27 @@ static int meson_uart_verify_port(struct 
>> uart_port *port,
>>   static void meson_uart_release_port(struct uart_port *port)
>>   {
>> -    /* nothing to do */
>> +    struct meson_uart_data *private_data = port->private_data;
>> +
>> +    clk_disable_unprepare(private_data->baud_clk);
>> +    clk_disable_unprepare(private_data->pclk);
>>   }
>>   static int meson_uart_request_port(struct uart_port *port)
>>   {
>> +    struct meson_uart_data *private_data = port->private_data;
>> +    int ret;
>> +
>> +    ret = clk_prepare_enable(private_data->pclk);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = clk_prepare_enable(private_data->baud_clk);
>> +    if (ret) {
>> +        clk_disable_unprepare(private_data->pclk);
>> +        return ret;
>> +    }
>> +
>>       return 0;
>>   }
>> @@ -629,55 +646,105 @@ static struct uart_driver meson_uart_driver = {
>>       .cons        = MESON_SERIAL_CONSOLE,
>>   };
>> -static inline struct clk *meson_uart_probe_clock(struct device *dev,
>> -                         const char *id)
>> +#define CLK_NAME(name) \
>> +({\
>> +    char clk_name[32];\
>> +    snprintf(clk_name, sizeof(clk_name), "%s#%s", 
>> dev_name(port->dev), #name);\
>> +    clk_name;\
>> +})
> 
> This is cryptic and duplicates the variable on the stack. Why not expand 
> the macro and delete it then?
> 
I've overdone it. I'll move it to the meson_uart_probe_clocks function 
as you suggested and delete it.
>> +
>> +static struct clk_div_table xtal_div_table[] = {
>> +    { 0, 3  },
> 
> An extra space here and there:
> 
>> +    { 1, 1  },
>> +    { 2, 2  },
>> +    { 3, 2  },
>> +};
>> +
Sorry, stupid mistake. I will correct.

>> +static int meson_uart_probe_clocks(struct uart_port *port)
>>   {
>> -    struct clk *clk = NULL;
>> -    int ret;
>> +    struct meson_uart_data *private_data = port->private_data;
>> +    struct clk *clk_baud, *clk_xtal;
>> +    struct clk_hw *hw;
>> +    struct clk_parent_data use_xtal_mux_parents[2] = {
>> +        { .index = -1, },
>> +        { .index = -1, },
>> +    };
>> -    clk = devm_clk_get(dev, id);
>> -    if (IS_ERR(clk))
>> -        return clk;
>> +    private_data->pclk = devm_clk_get(port->dev, "pclk");
>> +    if (IS_ERR(private_data->pclk))
>> +        return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
>> +                     "Failed to get the 'pclk' clock\n");
>> -    ret = clk_prepare_enable(clk);
>> -    if (ret) {
>> -        dev_err(dev, "couldn't enable clk\n");
>> -        return ERR_PTR(ret);
>> +    clk_baud = devm_clk_get(port->dev, "baud");
>> +    if (IS_ERR(clk_baud)) {
>> +        dev_err(port->dev, "Failed to get the 'baud' clock\n");
>> +        return PTR_ERR(clk_baud);
>>       }
>> -    devm_add_action_or_reset(dev,
>> -            (void(*)(void *))clk_disable_unprepare,
>> -            clk);
>> -
>> -    return clk;
>> -}
>> -
>> -static int meson_uart_probe_clocks(struct platform_device *pdev,
>> -                   struct uart_port *port)
>> -{
>> -    struct clk *clk_xtal = NULL;
>> -    struct clk *clk_pclk = NULL;
>> -    struct clk *clk_baud = NULL;
>> -
>> -    clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>> -    if (IS_ERR(clk_pclk))
>> -        return PTR_ERR(clk_pclk);
>> -
>> -    clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>> +    clk_xtal = devm_clk_get(port->dev, "xtal");
>>       if (IS_ERR(clk_xtal))
>> -        return PTR_ERR(clk_xtal);
>> -
>> -    clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>> -    if (IS_ERR(clk_baud))
>> -        return PTR_ERR(clk_baud);
>> +        return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>> +                     "Failed to get the 'xtal' clock\n");
>> +
>> +    if (private_data->use_xtal_clk) {
>> +        hw = devm_clk_hw_register_divider_table(port->dev,
>> +                            CLK_NAME(xtal_div),
>> +                            __clk_get_name(clk_baud),
>> +                            CLK_SET_RATE_NO_REPARENT,
>> +                            port->membase + AML_UART_REG5,
>> +                            26, 2,
>> +                            CLK_DIVIDER_READ_ONLY,
>> +                            xtal_div_table, NULL);
>> +        if (IS_ERR(hw))
>> +            return PTR_ERR(hw);
>> +
>> +        use_xtal_mux_parents[1].hw = hw;
>> +    } else {
>> +        hw = devm_clk_hw_register_fixed_factor(port->dev,
>> +                               CLK_NAME(clk81_div4),
>> +                               __clk_get_name(clk_baud),
>> +                               CLK_SET_RATE_NO_REPARENT,
>> +                               1, 4);
>> +        if (IS_ERR(hw))
>> +            return PTR_ERR(hw);
>> +
>> +        use_xtal_mux_parents[0].hw = hw;
>> +    }
>> -    port->uartclk = clk_get_rate(clk_baud);
>> +    hw = __devm_clk_hw_register_mux(port->dev, NULL,
>> +                    CLK_NAME(use_xtal),
>> +                    ARRAY_SIZE(use_xtal_mux_parents),
>> +                    NULL, NULL,
>> +                    use_xtal_mux_parents,
>> +                    CLK_SET_RATE_PARENT,
>> +                    port->membase + AML_UART_REG5,
>> +                    24, 0x1,
>> +                    CLK_MUX_READ_ONLY,
>> +                    NULL, NULL);
>> +    if (IS_ERR(hw))
>> +        return PTR_ERR(hw);
>> +
>> +    port->uartclk = clk_hw_get_rate(hw);
>> +
>> +    hw = devm_clk_hw_register_divider(port->dev,
>> +                      CLK_NAME(baud_div),
>> +                      clk_hw_get_name(hw),
>> +                      CLK_SET_RATE_PARENT,
>> +                      port->membase + AML_UART_REG5,
>> +                      0, 23,
>> +                      CLK_DIVIDER_ROUND_CLOSEST,
>> +                      NULL);
>> +    if (IS_ERR(hw))
>> +        return PTR_ERR(hw);
>> +
>> +    private_data->baud_clk = clk_hw_get_clk(hw, "baud_rate");
>>       return 0;
>>   }
> 
> 
> 

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

[PATCH V5 4/5] tty: serial: meson: Make some bit of the REG5 register writable.

[Yu Tu]

The UART_REG5 register defaults to 0. The console port is set in
ROMCODE. But other UART ports default to 0, so make bit24 and
bit[26,27] writable so that the UART can choose a more
appropriate clock.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 1004fd0b0c9e..fd128878e91a 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -693,7 +693,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 							CLK_SET_RATE_NO_REPARENT,
 							port->membase + AML_UART_REG5,
 							26, 2,
-							CLK_DIVIDER_READ_ONLY,
+							CLK_DIVIDER_ROUND_CLOSEST,
 							xtal_div_table, NULL);
 		if (IS_ERR(hw))
 			return PTR_ERR(hw);
@@ -719,7 +719,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 					CLK_SET_RATE_PARENT,
 					port->membase + AML_UART_REG5,
 					24, 0x1,
-					CLK_MUX_READ_ONLY,
+					CLK_MUX_ROUND_CLOSEST,
 					NULL, NULL);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
-- 
2.33.1

[PATCH V5 4/5] tty: serial: meson: Make some bit of the REG5 register writable.

[Yu Tu]

The UART_REG5 register defaults to 0. The console port is set in
ROMCODE. But other UART ports default to 0, so make bit24 and
bit[26,27] writable so that the UART can choose a more
appropriate clock.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 1004fd0b0c9e..fd128878e91a 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -693,7 +693,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 							CLK_SET_RATE_NO_REPARENT,
 							port->membase + AML_UART_REG5,
 							26, 2,
-							CLK_DIVIDER_READ_ONLY,
+							CLK_DIVIDER_ROUND_CLOSEST,
 							xtal_div_table, NULL);
 		if (IS_ERR(hw))
 			return PTR_ERR(hw);
@@ -719,7 +719,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 					CLK_SET_RATE_PARENT,
 					port->membase + AML_UART_REG5,
 					24, 0x1,
-					CLK_MUX_READ_ONLY,
+					CLK_MUX_ROUND_CLOSEST,
 					NULL, NULL);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
-- 
2.33.1


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

[PATCH V5 4/5] tty: serial: meson: Make some bit of the REG5 register writable.

[Yu Tu]

The UART_REG5 register defaults to 0. The console port is set in
ROMCODE. But other UART ports default to 0, so make bit24 and
bit[26,27] writable so that the UART can choose a more
appropriate clock.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 1004fd0b0c9e..fd128878e91a 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -693,7 +693,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 							CLK_SET_RATE_NO_REPARENT,
 							port->membase + AML_UART_REG5,
 							26, 2,
-							CLK_DIVIDER_READ_ONLY,
+							CLK_DIVIDER_ROUND_CLOSEST,
 							xtal_div_table, NULL);
 		if (IS_ERR(hw))
 			return PTR_ERR(hw);
@@ -719,7 +719,7 @@ static int meson_uart_probe_clocks(struct uart_port *port)
 					CLK_SET_RATE_PARENT,
 					port->membase + AML_UART_REG5,
 					24, 0x1,
-					CLK_MUX_READ_ONLY,
+					CLK_MUX_ROUND_CLOSEST,
 					NULL, NULL);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
-- 
2.33.1


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

[PATCH V5 5/5] tty: serial: meson: Added S4 SOC compatibility.

[Yu Tu]

Make UART driver compatible with S4 SOC UART.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index fd128878e91a..d6aa04cc31ba 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -863,6 +863,10 @@ static const struct of_device_id meson_uart_dt_match[] = {
 		.compatible = "amlogic,meson-g12a-uart",
 		.data = (void *)true,
 	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
 	/*
 	 * deprecated, don't use anymore because it doesn't differentiate
 	 * between GXBB, GXL and G12A which have different revisions
-- 
2.33.1


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

[PATCH V5 5/5] tty: serial: meson: Added S4 SOC compatibility.

[Yu Tu]

Make UART driver compatible with S4 SOC UART.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index fd128878e91a..d6aa04cc31ba 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -863,6 +863,10 @@ static const struct of_device_id meson_uart_dt_match[] = {
 		.compatible = "amlogic,meson-g12a-uart",
 		.data = (void *)true,
 	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
 	/*
 	 * deprecated, don't use anymore because it doesn't differentiate
 	 * between GXBB, GXL and G12A which have different revisions
-- 
2.33.1

[PATCH V5 5/5] tty: serial: meson: Added S4 SOC compatibility.

[Yu Tu]

Make UART driver compatible with S4 SOC UART.

Signed-off-by: Yu Tu <yu.tu@amlogic.com>
---
 drivers/tty/serial/meson_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index fd128878e91a..d6aa04cc31ba 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -863,6 +863,10 @@ static const struct of_device_id meson_uart_dt_match[] = {
 		.compatible = "amlogic,meson-g12a-uart",
 		.data = (void *)true,
 	},
+	{
+		.compatible = "amlogic,meson-s4-uart",
+		.data = (void *)true,
+	},
 	/*
 	 * deprecated, don't use anymore because it doesn't differentiate
 	 * between GXBB, GXL and G12A which have different revisions
-- 
2.33.1


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