[PATCH v2, 0/3] tty/serial: meson_uart: add support for core clock handling

[PATCH v2, 0/3] tty/serial: meson_uart: add support for core clock handling

[Helmut Klein]

To be able to use the three none AO uarts of the s905 SoCs (uart_A, uart_B
& uart_C), the core clock has to be enabled. (see chapter 22.3 of the
public s905 data sheet) At least the u-boot of my s905 based media player
(netxeon minimx) doesn't do this. so the driver must do it.

This patch set does:
- exposes the clock ids to the dtb
- adds documentation for the dt-bindings of meson_uart
- adds the core clock handling to the driver

The patchset is based on the branch master of the repository in [1]

Changes since v1
- use git to produce the patch set
- added the clock ids for uart_B and uart_C

None of the available s905 dts use uart_A actively. So there is no patch
for any of the existing dts files.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-amlogic.git

Helmut Klein (3):
  tty/serial: meson_uart: expose CLKID_UARTx
  tty/serial: meson_uart: add documentation for the dt-bindings
  tty/serial: meson_uart: add the core clock handling to the driver

 .../bindings/serial/amlogic,meson_uart.txt         | 25 ++++++++++++++++++++++
 drivers/clk/meson/gxbb.h                           |  6 +++---
 drivers/tty/serial/meson_uart.c                    | 10 +++++++++
 include/dt-bindings/clock/gxbb-clkc.h              |  3 +++
 4 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt

--
2.11.0

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Helmut Klein]

Expose the clock ids for the three none AO uarts to the dt-bindings

Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
---
 drivers/clk/meson/gxbb.h              | 6 +++---
 include/dt-bindings/clock/gxbb-clkc.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
index 8ee2022ce5d5..1edfaa5fe307 100644
--- a/drivers/clk/meson/gxbb.h
+++ b/drivers/clk/meson/gxbb.h
@@ -194,7 +194,7 @@
 /* #define CLKID_SAR_ADC */
 #define CLKID_SMART_CARD	  24
 #define CLKID_RNG0		  25
-#define CLKID_UART0		  26
+/* CLKID_UART0 */
 #define CLKID_SDHC		  27
 #define CLKID_STREAM		  28
 #define CLKID_ASYNC_FIFO	  29
@@ -216,7 +216,7 @@
 #define CLKID_ADC		  45
 #define CLKID_BLKMV		  46
 #define CLKID_AIU		  47
-#define CLKID_UART1		  48
+/* CLKID_UART1 */
 #define CLKID_G2D		  49
 /* CLKID_USB0 */
 /* CLKID_USB1 */
@@ -236,7 +236,7 @@
 /* CLKID_USB0_DDR_BRIDGE */
 #define CLKID_MMC_PCLK		  66
 #define CLKID_DVIN		  67
-#define CLKID_UART2		  68
+/* CLKID_UART2 */
 /* #define CLKID_SANA */
 #define CLKID_VPU_INTR		  70
 #define CLKID_SEC_AHB_AHB3_BRIDGE 71
diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-bindings/clock/gxbb-clkc.h
index 692846c7941b..7b329df47752 100644
--- a/include/dt-bindings/clock/gxbb-clkc.h
+++ b/include/dt-bindings/clock/gxbb-clkc.h
@@ -15,13 +15,16 @@
 #define CLKID_SPI		34
 #define CLKID_I2C		22
 #define CLKID_SAR_ADC		23
+#define CLKID_UART0		26
 #define CLKID_ETH		36
+#define CLKID_UART1		48
 #define CLKID_USB0		50
 #define CLKID_USB1		51
 #define CLKID_USB		55
 #define CLKID_HDMI_PCLK		63
 #define CLKID_USB1_DDR_BRIDGE	64
 #define CLKID_USB0_DDR_BRIDGE	65
+#define CLKID_UART2		68
 #define CLKID_SANA		69
 #define CLKID_GCLK_VENCI_INT0	77
 #define CLKID_AO_I2C		93
--
2.11.0

[PATCH v2, 2/3] tty/serial: meson_uart: add documentation for the dt-bindings

[Helmut Klein]

Add the documentation for the device tree binding of meson_uart

Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
---
 .../bindings/serial/amlogic,meson_uart.txt         | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt

diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt b/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
new file mode 100644
index 000000000000..6f94ba25760d
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
@@ -0,0 +1,25 @@
+* Amlogic Meson UART, used in multiple SoCs (e.g. S905, s905X, ...)
+
+Required properties:
+- compatible	: "amlogic,meson-uart"
+- reg		: offset and length of the register set for the device.
+- interrupts	: device interrupt
+- clocks	: the baud rate clock for the UART and optionally the core clock for the none AO uarts
+e.g.
+uart_A: serial at 84c0 {
+	compatible = "amlogic,meson-uart";
+	reg = <0x0 0x84c0 0x0 0x14>;
+	pinctrl-0 = <&uart_a_pins &uart_a_cts_rts_pins>;
+	interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>;
+	clocks = <&xtal>, <&clkc CLKID_UART0>;
+	clock-names = "xtal", "core";
+	status = "ok";
+};
+
+Note: Each port should have an alias correctly numbered in "aliases" node.
+
+e.g.
+aliases {
+	serial0 = &uart_AO;
+	serial1 = &uart_A;
+};
--
2.11.0

[PATCH v2, 3/3] tty/serial: meson_uart: add the core clock handling to the driver

[Helmut Klein]

This patch gets the core clock as provided by the DT and enables it.
The code was taken from Amlogic's serial driver, and was tested on my
board.

Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 60f16795d16b..cb99112288eb 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -600,6 +600,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	struct resource *res_mem, *res_irq;
 	struct uart_port *port;
 	struct clk *clk;
+	struct clk *core_clk;
 	int ret = 0;

 	if (pdev->dev.of_node)
@@ -625,6 +626,15 @@ static int meson_uart_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;

+	core_clk = devm_clk_get(&pdev->dev, "core");
+	if (!IS_ERR(core_clk)) {
+		ret = clk_prepare_enable(core_clk);
+		if (ret) {
+			dev_err(&pdev->dev, "couldn't enable clkc\n");
+			return ret;
+		}
+	}
+
 	clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
--
2.11.0

[PATCH v2, 2/3] tty/serial: meson_uart: add documentation for the dt-bindings

[Mark Rutland]

On Tue, Mar 28, 2017 at 11:25:44AM +0200, Helmut Klein wrote:
> Add the documentation for the device tree binding of meson_uart
> 
> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
> ---
>  .../bindings/serial/amlogic,meson_uart.txt         | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt b/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
> new file mode 100644
> index 000000000000..6f94ba25760d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
> @@ -0,0 +1,25 @@
> +* Amlogic Meson UART, used in multiple SoCs (e.g. S905, s905X, ...)
> +
> +Required properties:
> +- compatible	: "amlogic,meson-uart"
> +- reg		: offset and length of the register set for the device.
> +- interrupts	: device interrupt
> +- clocks	: the baud rate clock for the UART and optionally the core clock for the none AO uarts
> +e.g.
> +uart_A: serial at 84c0 {
> +	compatible = "amlogic,meson-uart";
> +	reg = <0x0 0x84c0 0x0 0x14>;
> +	pinctrl-0 = <&uart_a_pins &uart_a_cts_rts_pins>;
> +	interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>;
> +	clocks = <&xtal>, <&clkc CLKID_UART0>;
> +	clock-names = "xtal", "core";

Please define these explicitly above, e.g.

- clocks: a list of phandle + clock-specifier pairs, one for each entry
          in clock names.
- clock-names: contains:
  * "xtal" for the baud rate clock
  * "core" for the non-AO UARTs (optional)

Thanks,
Mark.

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Jerome Brunet]

On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote:
> Expose the clock ids for the three none AO uarts to the dt-bindings

Are they all used in the device tree ?
We try to only expose what we need in the DT
The recent discussion over CLKID_CPUCLK proved it was a sane thing to do.

> 
> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
> ---
> ?drivers/clk/meson/gxbb.h??????????????| 6 +++---
> ?include/dt-bindings/clock/gxbb-clkc.h | 3 +++
> ?2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
> index 8ee2022ce5d5..1edfaa5fe307 100644
> --- a/drivers/clk/meson/gxbb.h
> +++ b/drivers/clk/meson/gxbb.h
> @@ -194,7 +194,7 @@
> ?/* #define CLKID_SAR_ADC */
> ?#define CLKID_SMART_CARD	??24
> ?#define CLKID_RNG0		??25
> -#define CLKID_UART0		??26
> +/* CLKID_UART0 */
> ?#define CLKID_SDHC		??27
> ?#define CLKID_STREAM		??28
> ?#define CLKID_ASYNC_FIFO	??29
> @@ -216,7 +216,7 @@
> ?#define CLKID_ADC		??45
> ?#define CLKID_BLKMV		??46
> ?#define CLKID_AIU		??47
> -#define CLKID_UART1		??48
> +/* CLKID_UART1 */
> ?#define CLKID_G2D		??49
> ?/* CLKID_USB0 */
> ?/* CLKID_USB1 */
> @@ -236,7 +236,7 @@
> ?/* CLKID_USB0_DDR_BRIDGE */
> ?#define CLKID_MMC_PCLK		??66
> ?#define CLKID_DVIN		??67
> -#define CLKID_UART2		??68
> +/* CLKID_UART2 */
> ?/* #define CLKID_SANA */
> ?#define CLKID_VPU_INTR		??70
> ?#define CLKID_SEC_AHB_AHB3_BRIDGE 71
> diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-
> bindings/clock/gxbb-clkc.h
> index 692846c7941b..7b329df47752 100644
> --- a/include/dt-bindings/clock/gxbb-clkc.h
> +++ b/include/dt-bindings/clock/gxbb-clkc.h
> @@ -15,13 +15,16 @@
> ?#define CLKID_SPI		34
> ?#define CLKID_I2C		22
> ?#define CLKID_SAR_ADC		23
> +#define CLKID_UART0		26
> ?#define CLKID_ETH		36
> +#define CLKID_UART1		48
> ?#define CLKID_USB0		50
> ?#define CLKID_USB1		51
> ?#define CLKID_USB		55
> ?#define CLKID_HDMI_PCLK		63
> ?#define CLKID_USB1_DDR_BRIDGE	64
> ?#define CLKID_USB0_DDR_BRIDGE	65
> +#define CLKID_UART2		68
> ?#define CLKID_SANA		69
> ?#define CLKID_GCLK_VENCI_INT0	77
> ?#define CLKID_AO_I2C		93
> --
> 2.11.0
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Helmut Klein]

i know for sure that the bluetooth chip of my system is connected to 
uart_A. so this clock must be exposed.

i don't know if the other 2 uarts are used on other hardware. so i will 
remove them from my patch.

Helmut

On 28.03.2017 17:51, Jerome Brunet wrote:
> On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote:
>> Expose the clock ids for the three none AO uarts to the dt-bindings
>
> Are they all used in the device tree ?
> We try to only expose what we need in the DT
> The recent discussion over CLKID_CPUCLK proved it was a sane thing to do.
>
>>
>> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
>> ---
>>  drivers/clk/meson/gxbb.h              | 6 +++---
>>  include/dt-bindings/clock/gxbb-clkc.h | 3 +++
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
>> index 8ee2022ce5d5..1edfaa5fe307 100644
>> --- a/drivers/clk/meson/gxbb.h
>> +++ b/drivers/clk/meson/gxbb.h
>> @@ -194,7 +194,7 @@
>>  /* #define CLKID_SAR_ADC */
>>  #define CLKID_SMART_CARD	  24
>>  #define CLKID_RNG0		  25
>> -#define CLKID_UART0		  26
>> +/* CLKID_UART0 */
>>  #define CLKID_SDHC		  27
>>  #define CLKID_STREAM		  28
>>  #define CLKID_ASYNC_FIFO	  29
>> @@ -216,7 +216,7 @@
>>  #define CLKID_ADC		  45
>>  #define CLKID_BLKMV		  46
>>  #define CLKID_AIU		  47
>> -#define CLKID_UART1		  48
>> +/* CLKID_UART1 */
>>  #define CLKID_G2D		  49
>>  /* CLKID_USB0 */
>>  /* CLKID_USB1 */
>> @@ -236,7 +236,7 @@
>>  /* CLKID_USB0_DDR_BRIDGE */
>>  #define CLKID_MMC_PCLK		  66
>>  #define CLKID_DVIN		  67
>> -#define CLKID_UART2		  68
>> +/* CLKID_UART2 */
>>  /* #define CLKID_SANA */
>>  #define CLKID_VPU_INTR		  70
>>  #define CLKID_SEC_AHB_AHB3_BRIDGE 71
>> diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-
>> bindings/clock/gxbb-clkc.h
>> index 692846c7941b..7b329df47752 100644
>> --- a/include/dt-bindings/clock/gxbb-clkc.h
>> +++ b/include/dt-bindings/clock/gxbb-clkc.h
>> @@ -15,13 +15,16 @@
>>  #define CLKID_SPI		34
>>  #define CLKID_I2C		22
>>  #define CLKID_SAR_ADC		23
>> +#define CLKID_UART0		26
>>  #define CLKID_ETH		36
>> +#define CLKID_UART1		48
>>  #define CLKID_USB0		50
>>  #define CLKID_USB1		51
>>  #define CLKID_USB		55
>>  #define CLKID_HDMI_PCLK		63
>>  #define CLKID_USB1_DDR_BRIDGE	64
>>  #define CLKID_USB0_DDR_BRIDGE	65
>> +#define CLKID_UART2		68
>>  #define CLKID_SANA		69
>>  #define CLKID_GCLK_VENCI_INT0	77
>>  #define CLKID_AO_I2C		93
>> --
>> 2.11.0
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
>

[PATCH v2, 2/3] tty/serial: meson_uart: add documentation for the dt-bindings

[Helmut Klein]

Hallo Mark,

i will add your text to the patch.

Helmut

On 28.03.2017 12:05, Mark Rutland wrote:
> On Tue, Mar 28, 2017 at 11:25:44AM +0200, Helmut Klein wrote:
>> Add the documentation for the device tree binding of meson_uart
>>
>> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
>> ---
>>  .../bindings/serial/amlogic,meson_uart.txt         | 25 ++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt b/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
>> new file mode 100644
>> index 000000000000..6f94ba25760d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/amlogic,meson_uart.txt
>> @@ -0,0 +1,25 @@
>> +* Amlogic Meson UART, used in multiple SoCs (e.g. S905, s905X, ...)
>> +
>> +Required properties:
>> +- compatible	: "amlogic,meson-uart"
>> +- reg		: offset and length of the register set for the device.
>> +- interrupts	: device interrupt
>> +- clocks	: the baud rate clock for the UART and optionally the core clock for the none AO uarts
>> +e.g.
>> +uart_A: serial at 84c0 {
>> +	compatible = "amlogic,meson-uart";
>> +	reg = <0x0 0x84c0 0x0 0x14>;
>> +	pinctrl-0 = <&uart_a_pins &uart_a_cts_rts_pins>;
>> +	interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>;
>> +	clocks = <&xtal>, <&clkc CLKID_UART0>;
>> +	clock-names = "xtal", "core";
>
> Please define these explicitly above, e.g.
>
> - clocks: a list of phandle + clock-specifier pairs, one for each entry
>           in clock names.
> - clock-names: contains:
>   * "xtal" for the baud rate clock
>   * "core" for the non-AO UARTs (optional)
>
> Thanks,
> Mark.
>

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Jerome Brunet]

On Tue, 2017-03-28 at 20:18 +0200, Helmut Klein wrote:
> i know for sure that the bluetooth chip of my system is connected to?
> uart_A. so this clock must be exposed.
> 
> i don't know if the other 2 uarts are used on other hardware. so i will?
> remove them from my patch.

What I meant is device trees "upstream". 
I would expect such patch as part of a series to add support for your board in
device-tree, with its bluetooth chipset.

I think it would better to drop this patch from the series.
You can either keep it for your personal work, or send it again when upstreaming
the support for your board and/or add the support for the bluetooth chip.

Cheers
Jerome

> 
> Helmut
> 
> On 28.03.2017 17:51, Jerome Brunet wrote:
> > On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote:
> > > Expose the clock ids for the three none AO uarts to the dt-bindings
> > 
> > Are they all used in the device tree ?
> > We try to only expose what we need in the DT
> > The recent discussion over CLKID_CPUCLK proved it was a sane thing to do.
> > 
> > > 
> > > Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
> > > ---
> > > ?drivers/clk/meson/gxbb.h??????????????| 6 +++---
> > > ?include/dt-bindings/clock/gxbb-clkc.h | 3 +++
> > > ?2 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/clk/meson/gxbb.h b/drivers/clk/meson/gxbb.h
> > > index 8ee2022ce5d5..1edfaa5fe307 100644
> > > --- a/drivers/clk/meson/gxbb.h
> > > +++ b/drivers/clk/meson/gxbb.h
> > > @@ -194,7 +194,7 @@
> > > ?/* #define CLKID_SAR_ADC */
> > > ?#define CLKID_SMART_CARD	??24
> > > ?#define CLKID_RNG0		??25
> > > -#define CLKID_UART0		??26
> > > +/* CLKID_UART0 */
> > > ?#define CLKID_SDHC		??27
> > > ?#define CLKID_STREAM		??28
> > > ?#define CLKID_ASYNC_FIFO	??29
> > > @@ -216,7 +216,7 @@
> > > ?#define CLKID_ADC		??45
> > > ?#define CLKID_BLKMV		??46
> > > ?#define CLKID_AIU		??47
> > > -#define CLKID_UART1		??48
> > > +/* CLKID_UART1 */
> > > ?#define CLKID_G2D		??49
> > > ?/* CLKID_USB0 */
> > > ?/* CLKID_USB1 */
> > > @@ -236,7 +236,7 @@
> > > ?/* CLKID_USB0_DDR_BRIDGE */
> > > ?#define CLKID_MMC_PCLK		??66
> > > ?#define CLKID_DVIN		??67
> > > -#define CLKID_UART2		??68
> > > +/* CLKID_UART2 */
> > > ?/* #define CLKID_SANA */
> > > ?#define CLKID_VPU_INTR		??70
> > > ?#define CLKID_SEC_AHB_AHB3_BRIDGE 71
> > > diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-
> > > bindings/clock/gxbb-clkc.h
> > > index 692846c7941b..7b329df47752 100644
> > > --- a/include/dt-bindings/clock/gxbb-clkc.h
> > > +++ b/include/dt-bindings/clock/gxbb-clkc.h
> > > @@ -15,13 +15,16 @@
> > > ?#define CLKID_SPI		34
> > > ?#define CLKID_I2C		22
> > > ?#define CLKID_SAR_ADC		23
> > > +#define CLKID_UART0		26
> > > ?#define CLKID_ETH		36
> > > +#define CLKID_UART1		48
> > > ?#define CLKID_USB0		50
> > > ?#define CLKID_USB1		51
> > > ?#define CLKID_USB		55
> > > ?#define CLKID_HDMI_PCLK		63
> > > ?#define CLKID_USB1_DDR_BRIDGE	64
> > > ?#define CLKID_USB0_DDR_BRIDGE	65
> > > +#define CLKID_UART2		68
> > > ?#define CLKID_SANA		69
> > > ?#define CLKID_GCLK_VENCI_INT0	77
> > > ?#define CLKID_AO_I2C		93
> > > --
> > > 2.11.0
> > > 
> > > 
> > > _______________________________________________
> > > linux-amlogic mailing list
> > > linux-amlogic at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2, 3/3] tty/serial: meson_uart: add the core clock handling to the driver

[Jerome Brunet]

On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote:
> This patch gets the core clock as provided by the DT and enables it.
> The code was taken from Amlogic's serial driver, and was tested on my
> board.
> 
> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
> ---
> ?drivers/tty/serial/meson_uart.c | 10 ++++++++++
> ?1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 60f16795d16b..cb99112288eb 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -600,6 +600,7 @@ static int meson_uart_probe(struct platform_device *pdev)
> ?	struct resource *res_mem, *res_irq;
> ?	struct uart_port *port;
> ?	struct clk *clk;
> +	struct clk *core_clk;
> ?	int ret = 0;
> 
> ?	if (pdev->dev.of_node)
> @@ -625,6 +626,15 @@ static int meson_uart_probe(struct platform_device *pdev)
> ?	if (!port)
> ?		return -ENOMEM;
> 
> +	core_clk = devm_clk_get(&pdev->dev, "core");
> +	if (!IS_ERR(core_clk)) {
> +		ret = clk_prepare_enable(core_clk);
> +		if (ret) {
> +			dev_err(&pdev->dev, "couldn't enable clkc\n");
> +			return ret;
> +		}
> +	}
> +
> ?	clk = clk_get(&pdev->dev, NULL);
Now that you have 2 clocks, shouldn't this be named as well ?

> ?	if (IS_ERR(clk))
> ?		return PTR_ERR(clk);
> --
> 2.11.0
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Martin Blumenstingl]

Hi Jerome,

On Tue, Mar 28, 2017 at 9:14 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Tue, 2017-03-28 at 20:18 +0200, Helmut Klein wrote:
>> i know for sure that the bluetooth chip of my system is connected to
>> uart_A. so this clock must be exposed.
>>
>> i don't know if the other 2 uarts are used on other hardware. so i will
>> remove them from my patch.
>
> What I meant is device trees "upstream".
> I would expect such patch as part of a series to add support for your board in
> device-tree, with its bluetooth chipset.
are you sure about this? Helmut is configuring the core clock of the
UART controller(s) here. we have the same thing for many other drivers
as well (MMC, SAR ADC, I2C, SPI, ethernet, you name it...) - these
clocks are not part of a device specific .dts but rather the SoC
specific dts (for example meson-gxbb.dtsi - because these clocks are
not board specific).
I guess most boards are not affected because the bootloader simply
enables the UART0 core/gate clock and keeps it enabled when booting
the kernel. additionally our clock gates are marked with
CLK_IGNORE_UNUSED so if the bootloader keeps the gates enabled then
we're not disabling it either.

> I think it would better to drop this patch from the series.
> You can either keep it for your personal work, or send it again when upstreaming
> the support for your board and/or add the support for the bluetooth chip.
if we decide to pass the core/gate clock directly in SoC.dtsi then we
should think about doing it for all three non-AO UARTs (uart_a, uart_b
and uart_c).
we can test uart_b on GPIODV_24 and GPIODV_25 on the Khadas VIM board
for example (pins 22 and 23 on the header, default mode of these pins
is i2c_sck_a and i2c_sda_a, but we can re-configure it).
uart_c unfortunately cannot be tested on the Khadas VIM board since
it's only routed to GPIOX_8 and GPIOX_9 which are hard-wired to the
bluetooth module's PCM pins (BTPCM_DOUT and BTPCM_DIN).

for Helmut this would mean that instead of dropping this patch (or
dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather
have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi)
which passes the core clocks to the corresponding UART controllers
(similar to the CLKID_SD_EMMC_ clocks).


Regards,
Martin


[0] http://lxr.free-electrons.com/source/drivers/clk/meson/clkc.h?v=4.10#L110

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Jerome Brunet]

On Tue, 2017-03-28 at 23:24 +0200, Martin Blumenstingl wrote:
> Hi Jerome,
> 
> On Tue, Mar 28, 2017 at 9:14 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > On Tue, 2017-03-28 at 20:18 +0200, Helmut Klein wrote:
> > > i know for sure that the bluetooth chip of my system is connected to
> > > uart_A. so this clock must be exposed.
> > > 
> > > i don't know if the other 2 uarts are used on other hardware. so i will
> > > remove them from my patch.
> > 
> > What I meant is device trees "upstream".
> > I would expect such patch as part of a series to add support for your board
> > in
> > device-tree, with its bluetooth chipset.
> 
> are you sure about this? 

Actually no ;) I started questioning this reply as soon as I sent it.
the moto so far as been "we expose what's needed" but there is many ways to
interpret that.

DT bindings is an ABI. Simply because we don't use part of that ABI upstream,
doesn't mean we shouldn't provide it. My previous statement was clearly wrong
about this, apologies.

> Helmut is configuring the core clock of the
> UART controller(s) here. we have the same thing for many other drivers
> as well (MMC, SAR ADC, I2C, SPI, ethernet, you name it...) - these
> clocks are not part of a device specific .dts but rather the SoC
> specific dts (for example meson-gxbb.dtsi - because these clocks are
> not board specific).

Agreed
Since Helmut tested it, this clock should be added to the dtsi.

> I guess most boards are not affected because the bootloader simply
> enables the UART0 core/gate clock and keeps it enabled when booting
> the kernel. additionally our clock gates are marked with
> CLK_IGNORE_UNUSED so if the bootloader keeps the gates enabled then
> we're not disabling it either.
> 
> > I think it would better to drop this patch from the series.
> > You can either keep it for your personal work, or send it again when
> > upstreaming
> > the support for your board and/or add the support for the bluetooth chip.
> 
> if we decide to pass the core/gate clock directly in SoC.dtsi then we
> should think about doing it for all three non-AO UARTs (uart_a, uart_b
> and uart_c).
> we can test uart_b on GPIODV_24 and GPIODV_25 on the Khadas VIM board
> for example (pins 22 and 23 on the header, default mode of these pins
> is i2c_sck_a and i2c_sda_a, but we can re-configure it).
> uart_c unfortunately cannot be tested on the Khadas VIM board since
> it's only routed to GPIOX_8 and GPIOX_9 which are hard-wired to the
> bluetooth module's PCM pins (BTPCM_DOUT and BTPCM_DIN).
> 
> for Helmut this would mean that instead of dropping this patch (or
> dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather
> have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi)
> which passes the core clocks to the corresponding UART controllers
> (similar to the CLKID_SD_EMMC_ clocks).

You are right. This could be part of another series.
I guess this patch fine as it is.

Acked-by:?Jerome Brunet <jbrunet@baylibre.com>

> 
> 
> Regards,
> Martin
> 
> 
> [0] http://lxr.free-electrons.com/source/drivers/clk/meson/clkc.h?v=4.10#L110

[PATCH v2, 3/3] tty/serial: meson_uart: add the core clock handling to the driver

[Helmut Klein]

On 28.03.2017 21:16, Jerome Brunet wrote:
> On Tue, 2017-03-28 at 11:25 +0200, Helmut Klein wrote:
>> This patch gets the core clock as provided by the DT and enables it.
>> The code was taken from Amlogic's serial driver, and was tested on my
>> board.
>>
>> Signed-off-by: Helmut Klein <hgkr.klein@gmail.com>
>> ---
>>  drivers/tty/serial/meson_uart.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 60f16795d16b..cb99112288eb 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -600,6 +600,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>>  	struct resource *res_mem, *res_irq;
>>  	struct uart_port *port;
>>  	struct clk *clk;
>> +	struct clk *core_clk;
>>  	int ret = 0;
>>
>>  	if (pdev->dev.of_node)
>> @@ -625,6 +626,15 @@ static int meson_uart_probe(struct platform_device *pdev)
>>  	if (!port)
>>  		return -ENOMEM;
>>
>> +	core_clk = devm_clk_get(&pdev->dev, "core");
>> +	if (!IS_ERR(core_clk)) {
>> +		ret = clk_prepare_enable(core_clk);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "couldn't enable clkc\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>>  	clk = clk_get(&pdev->dev, NULL);
> Now that you have 2 clocks, shouldn't this be named as well ?
>
if i change the call to "clk = clk_get(&pdev->dev, "xtal");", then IMHO 
i must add the
"clock-names" parameter (clock-names = "xtal") to the sections of the 2 
AO-UARTs
(uart_AO & uart_AO_B).

Is this ok?  (or is my assumption wrong?)

I'm going to add a patch meson-gx.dtsi anyway, so 2 lines more to add, 
isn't a big deal.

Helmut

>>  	if (IS_ERR(clk))
>>  		return PTR_ERR(clk);
>> --
>> 2.11.0
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
>

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Kevin Hilman]

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

[...]

> for Helmut this would mean that instead of dropping this patch (or
> dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather
> have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi)
> which passes the core clocks to the corresponding UART controllers
> (similar to the CLKID_SD_EMMC_ clocks).

Yes, this is what I would like to see.

If a new CLKID is exposed, I want to see the users of it at the same
time.

Kevin

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Jerome Brunet]

On Wed, 2017-03-29 at 13:21 -0700, Kevin Hilman wrote:
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
> 
> [...]
> 
> > for Helmut this would mean that instead of dropping this patch (or
> > dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather
> > have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi)
> > which passes the core clocks to the corresponding UART controllers
> > (similar to the CLKID_SD_EMMC_ clocks).
> 
> Yes, this is what I would like to see.
> 
> If a new CLKID is exposed, I want to see the users of it at the same
> time.

Helmut,

If you send another version of these patches, considering the feedback of Kevin
and Martin, could you please change the subject of this patch to:

dt-bindings: clock: gxbb: expose?UART clocks

Thx
Jerome

> 
> Kevin

[PATCH v2,1/3] meson_uart: expose CLKID_UARTx

[Helmut Klein]

On 31.03.2017 17:37, Jerome Brunet wrote:
> On Wed, 2017-03-29 at 13:21 -0700, Kevin Hilman wrote:
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>
>> [...]
>>
>>> for Helmut this would mean that instead of dropping this patch (or
>>> dropping CLKID_UART1 and CLKID_UART2 from this patch) he would rather
>>> have to *add* another patch (for meson-gxbb.dtsi and meson-gxl.dtsi)
>>> which passes the core clocks to the corresponding UART controllers
>>> (similar to the CLKID_SD_EMMC_ clocks).
>>
>> Yes, this is what I would like to see.
>>
>> If a new CLKID is exposed, I want to see the users of it at the same
>> time.
>
> Helmut,
>
> If you send another version of these patches, considering the feedback of Kevin
> and Martin, could you please change the subject of this patch to:
>
> dt-bindings: clock: gxbb: expose UART clocks
>
> Thx
> Jerome
>
i've just sent v3 of my patchset.
(i changed the subjects of most patches in the set)

Helmut

>>
>> Kevin
>