[PATCH v5 0/5] Add Spreadtrum Sharkl64 Platform support

[PATCH v5 0/5] Add Spreadtrum Sharkl64 Platform support

[Chunyan Zhang]

Spreadtrum is a rapid growing chip vendor providing smart phone total solutions.

Sharkl64 Platform is nominated as a SoC infrastructure that supports 4G/3G/2G
standards based on ARMv8 multiple core architecture. Now we have only one
SoC(SC9836) based on this Platform in developing.

This patchset adds Sharkl64 support in arm64 device tree and adds the serial
driver of SC9836-UART.

This patchset also has patches which address "sprd" prefix and DT compatible
strings for nodes which appear un-documented.

This version code was tesed on SC9836 mobile phone.

Changes from v4:
* Addressed review comments from last version:
	- Remove compitible string "arm,psci-0.2"
	- Add the properties of psci v0.1
	- Remove the "clocks" container node
* Addressed review comments from Shawn Guo, Lea Yan, Jorge Ramirez-Ortiz:
	- Turn the 'clocks' property of uart into a required property
	- Use GIC macro instead of the value
	- Update the CPU mask to match the number of described CPUs
	- Use the more explicit compatible string "arm,gic-400" for GIC
	- sprd_serial.c: Add a cpu_relax() in the busy loop of sprd_putc
	- sprd_serial.c: Add calling uart_suspend_port and uart_resume_port in
	  the function sprd_suspend and sprd_resume respectively
	- sprd_serial.c: Use SIMPLE_DEV_PM_OPS to register sprd_suspend and
	  sprd_resume

Changes from v3:
* Addressed review comments:
	- Added the description of clock property for sc9836-uart
	- Revised the size of GICC to be 8KiB
	- Added another compatible string for psci-0.1
	- Removed the clock-frequency property of timer in DT
	- Removed ARCH_SHARKL64, just left ARCH_SPRD only
	- sprd_serial.c: Removed .owner of platform_driver.driver
	- sprd_serial.c: Removed all usages of unlikely
	- sprd_serial.c: Used the .port member of sprd_port instead of the cast
	- sprd_serial.c: sed dynamic allocation for major/minor device id
	- sprd_serial.c: Added a timeout and a cpu_relax for clearing rx/tx fifo
	- sprd_serial.c: clear CMSPAR in termios->c_cflag
	- sprd_serial.c: Set the resulting baud back into the termios
	- sprd_serial.c: Implemented a shared interrupt handler

Changes from v2:
* Addressed review comments:
	- Added a specific compitible string 'sc9836-uart' for the serial
	- Added a full serial driver
	- Added the property 'clock-frequency' for timer node in dtsi.
	- Replaceed the old macro prefix 'UART_' with 'SPRD_' in the 
	  Spreadtrum serial driver code.
* Revised the name of SoC and board from 'sharkl3' to 'sc9836'
* Used dual-license for DTS files
* Added a menuconfig 'ARCH_SPRD' in arch/arm64/Kconfig

Changes from v1:
* Addressed review comments:
        - Added "sprd" prefix to vendor-prefixes.txt
        - Created serial/sprd-serial.txt and remove the properties for serial-sprd
          from of-serial.txt to it.
        - Renamed of-serial.txt to 8250.txt according to Arnd's review comments
        - Splited and revised .dts for Sharkl64 Platform
        - Changed to PSCI method for cpu power management
        - Revised Kconfig Makefile to match the alphabetical ordering
        - Renamed serial-sprd-earlycon.c to serial-sprd.c

Chunyan Zhang (3):
  Documentation: DT: Renamed of-serial.txt to 8250.txt
  Documentation: DT: Add bindings for Spreadtrum SoC Platform
  tty/serial: Add Spreadtrum sc9836-uart driver support

Zhizhou Zhang (2):
  arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile
  arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and
    defconfig

 Documentation/devicetree/bindings/arm/sprd.txt     |   11 +
 .../bindings/serial/{of-serial.txt => 8250.txt}    |    0
 .../devicetree/bindings/serial/sprd-uart.txt       |    7 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 arch/arm64/Kconfig                                 |    5 +
 arch/arm64/boot/dts/Makefile                       |    1 +
 arch/arm64/boot/dts/sprd/Makefile                  |    5 +
 arch/arm64/boot/dts/sprd/sc9836-openphone.dts      |   49 ++
 arch/arm64/boot/dts/sprd/sc9836.dtsi               |   73 ++
 arch/arm64/boot/dts/sprd/sharkl64.dtsi             |   67 ++
 arch/arm64/configs/defconfig                       |    1 +
 drivers/tty/serial/Kconfig                         |   18 +
 drivers/tty/serial/Makefile                        |    1 +
 drivers/tty/serial/sprd_serial.c                   |  772 ++++++++++++++++++++
 include/uapi/linux/serial_core.h                   |    3 +
 15 files changed, 1014 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/sprd.txt
 rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)
 create mode 100644 Documentation/devicetree/bindings/serial/sprd-uart.txt
 create mode 100644 arch/arm64/boot/dts/sprd/Makefile
 create mode 100644 arch/arm64/boot/dts/sprd/sc9836-openphone.dts
 create mode 100644 arch/arm64/boot/dts/sprd/sc9836.dtsi
 create mode 100644 arch/arm64/boot/dts/sprd/sharkl64.dtsi
 create mode 100644 drivers/tty/serial/sprd_serial.c

-- 
1.7.9.5

[PATCH v5 1/5] Documentation: DT: Renamed of-serial.txt to 8250.txt

[Chunyan Zhang]

The file of-serial.txt was only for 8250 compatible UART implementations,
so renamed it to 8250.txt to avoid confusing other persons.
This is suggested by Arnd, see:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html

Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
 .../bindings/serial/{of-serial.txt => 8250.txt}    |    0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)

diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/8250.txt
similarity index 100%
rename from Documentation/devicetree/bindings/serial/of-serial.txt
rename to Documentation/devicetree/bindings/serial/8250.txt
-- 
1.7.9.5

[PATCH v5 1/5] Documentation: DT: Renamed of-serial.txt to 8250.txt

[Rob Herring]

On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
<chunyan.zhang@spreadtrum.com> wrote:
> The file of-serial.txt was only for 8250 compatible UART implementations,
> so renamed it to 8250.txt to avoid confusing other persons.
> This is suggested by Arnd, see:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Acked-by: Mark Rutland <mark.rutland@arm.com>

This is pretty much un-related to the rest of the series, so I will apply it.

Rob

> ---
>  .../bindings/serial/{of-serial.txt => 8250.txt}    |    0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename Documentation/devicetree/bindings/serial/{of-serial.txt => 8250.txt} (100%)
>
> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/8250.txt
> similarity index 100%
> rename from Documentation/devicetree/bindings/serial/of-serial.txt
> rename to Documentation/devicetree/bindings/serial/8250.txt
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

[Chunyan Zhang]

Adds Spreadtrum's prefix "sprd" to vendor-prefixes file.
Adds the devicetree binding documentations for Spreadtrum's sc9836-uart
and SC9836 SoC based on the Sharkl64 Platform which is a 64-bit SoC
Platform of Spreadtrum.

Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
---
 Documentation/devicetree/bindings/arm/sprd.txt     |   11 +++++++++++
 .../devicetree/bindings/serial/sprd-uart.txt       |    7 +++++++
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 3 files changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/sprd.txt
 create mode 100644 Documentation/devicetree/bindings/serial/sprd-uart.txt

diff --git a/Documentation/devicetree/bindings/arm/sprd.txt b/Documentation/devicetree/bindings/arm/sprd.txt
new file mode 100644
index 0000000..31a629d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/sprd.txt
@@ -0,0 +1,11 @@
+Spreadtrum SoC Platforms Device Tree Bindings
+----------------------------------------------------
+
+Sharkl64 is a Spreadtrum's SoC Platform which is based
+on ARM 64-bit processor.
+
+SC9836 openphone board with SC9836 SoC based on the
+Sharkl64 Platform shall have the following properties.
+
+Required root node properties:
+        - compatible = "sprd,sc9836-openphone", "sprd,sc9836";
diff --git a/Documentation/devicetree/bindings/serial/sprd-uart.txt b/Documentation/devicetree/bindings/serial/sprd-uart.txt
new file mode 100644
index 0000000..2aff0f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/sprd-uart.txt
@@ -0,0 +1,7 @@
+* Spreadtrum serial UART
+
+Required properties:
+- compatible: must be "sprd,sc9836-uart"
+- reg: offset and length of the register set for the device
+- interrupts: exactly one interrupt specifier
+- clocks: phandles to input clocks.
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b1df0ad..0a8384f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -153,6 +153,7 @@ snps	Synopsys, Inc.
 solidrun	SolidRun
 sony	Sony Corporation
 spansion	Spansion Inc.
+sprd	Spreadtrum Communications Inc.
 st	STMicroelectronics
 ste	ST-Ericsson
 stericsson	ST-Ericsson
-- 
1.7.9.5

[PATCH v5 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

[Mark Rutland]

On Fri, Jan 16, 2015 at 10:00:08AM +0000, Chunyan Zhang wrote:
> Adds Spreadtrum's prefix "sprd" to vendor-prefixes file.
> Adds the devicetree binding documentations for Spreadtrum's sc9836-uart
> and SC9836 SoC based on the Sharkl64 Platform which is a 64-bit SoC
> Platform of Spreadtrum.
> 
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> ---
>  Documentation/devicetree/bindings/arm/sprd.txt     |   11 +++++++++++
>  .../devicetree/bindings/serial/sprd-uart.txt       |    7 +++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  3 files changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/sprd.txt
>  create mode 100644 Documentation/devicetree/bindings/serial/sprd-uart.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/sprd.txt b/Documentation/devicetree/bindings/arm/sprd.txt
> new file mode 100644
> index 0000000..31a629d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/sprd.txt
> @@ -0,0 +1,11 @@
> +Spreadtrum SoC Platforms Device Tree Bindings
> +----------------------------------------------------
> +
> +Sharkl64 is a Spreadtrum's SoC Platform which is based
> +on ARM 64-bit processor.
> +
> +SC9836 openphone board with SC9836 SoC based on the
> +Sharkl64 Platform shall have the following properties.
> +
> +Required root node properties:
> +        - compatible = "sprd,sc9836-openphone", "sprd,sc9836";
> diff --git a/Documentation/devicetree/bindings/serial/sprd-uart.txt b/Documentation/devicetree/bindings/serial/sprd-uart.txt
> new file mode 100644
> index 0000000..2aff0f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/sprd-uart.txt
> @@ -0,0 +1,7 @@
> +* Spreadtrum serial UART
> +
> +Required properties:
> +- compatible: must be "sprd,sc9836-uart"
> +- reg: offset and length of the register set for the device
> +- interrupts: exactly one interrupt specifier
> +- clocks: phandles to input clocks.

The order and relevance of each should be specified. If you have
multiple clocks I would strongly recommend you use clock-names to
distinguish them.

Otherwise this looks fine.

Mark.

> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index b1df0ad..0a8384f 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -153,6 +153,7 @@ snps	Synopsys, Inc.
>  solidrun	SolidRun
>  sony	Sony Corporation
>  spansion	Spansion Inc.
> +sprd	Spreadtrum Communications Inc.
>  st	STMicroelectronics
>  ste	ST-Ericsson
>  stericsson	ST-Ericsson
> -- 
> 1.7.9.5
> 
> 

[PATCH v5 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

[Lyra Zhang]

Hi, Mark

>> +
>> +Required properties:
>> +- compatible: must be "sprd,sc9836-uart"
>> +- reg: offset and length of the register set for the device
>> +- interrupts: exactly one interrupt specifier
>> +- clocks: phandles to input clocks.
>
> The order and relevance of each should be specified. If you have
> multiple clocks I would strongly recommend you use clock-names to
> distinguish them.
>

Thank you for the recommendation.
but, since we haven't made the clock driver ready, for this initial
commit, we just let 4 UARTs share a single fixed 26 MHz clock source.
we'll do like you've recommended when we will submit the clock driver
in the future.

Best regards,
Chunyan



> Otherwise this looks fine.
>
> Mark.
>

[PATCH v5 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

[Mark Rutland]

On Fri, Jan 16, 2015 at 12:53:16PM +0000, Lyra Zhang wrote:
> Hi, Mark
> 
> >> +
> >> +Required properties:
> >> +- compatible: must be "sprd,sc9836-uart"
> >> +- reg: offset and length of the register set for the device
> >> +- interrupts: exactly one interrupt specifier
> >> +- clocks: phandles to input clocks.
> >
> > The order and relevance of each should be specified. If you have
> > multiple clocks I would strongly recommend you use clock-names to
> > distinguish them.
> >
> 
> Thank you for the recommendation.
> but, since we haven't made the clock driver ready, for this initial
> commit, we just let 4 UARTs share a single fixed 26 MHz clock source.
> we'll do like you've recommended when we will submit the clock driver
> in the future.

I'm on about the clock input lines on the UART instance, not the
providers they come from.

Is there only a single clock input line on each UART? Perhaps multiple
input lines which are currently fed by the same clock?

Thanks,
Mark.

[PATCH v5 2/5] Documentation: DT: Add bindings for Spreadtrum SoC Platform

[Orson Zhai]

On Fri, Jan 16, 2015 at 10:11 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 16, 2015 at 12:53:16PM +0000, Lyra Zhang wrote:
>> Hi, Mark
>>
>> >> +
>> >> +Required properties:
>> >> +- compatible: must be "sprd,sc9836-uart"
>> >> +- reg: offset and length of the register set for the device
>> >> +- interrupts: exactly one interrupt specifier
>> >> +- clocks: phandles to input clocks.
>> >
>> > The order and relevance of each should be specified. If you have
>> > multiple clocks I would strongly recommend you use clock-names to
>> > distinguish them.
>> >
>>
>> Thank you for the recommendation.
>> but, since we haven't made the clock driver ready, for this initial
>> commit, we just let 4 UARTs share a single fixed 26 MHz clock source.
>> we'll do like you've recommended when we will submit the clock driver
>> in the future.
>
> I'm on about the clock input lines on the UART instance, not the
> providers they come from.
>
> Is there only a single clock input line on each UART? Perhaps multiple
> input lines which are currently fed by the same clock?

________
| 26MHz |-------------------------------------------------
-------------        |                  |
                     |                  |
                _______       ________
               | UART1 |      | UART2 |    .........
               --------------      -------------

the hardware is something like the diagram.

4 Uart modules are all connected to a fixed 26Mhz crystal by power-on default.

There should be a clock-mux between uart and 26Mhz which
could select other clock source such as some pll output.

But as initial commit , we are not ready to describe other inputs by
these muxes.
So  we treat the UART as a simple model with only one fixed-clock input.
And we plan to add the other inputs path back in a not very far future.
Is it appropriate  to do like this?

  Orson


>
> Thanks,
> Mark.

[PATCH v5 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

[Chunyan Zhang]

From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>

Adds the device tree support for Spreadtrum SC9836 SoC which is based on
Sharkl64 platform.

Sharkl64 platform contains the common nodes of Spreadtrum's arm64-based SoCs.

Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
 arch/arm64/boot/dts/Makefile                  |    1 +
 arch/arm64/boot/dts/sprd/Makefile             |    5 ++
 arch/arm64/boot/dts/sprd/sc9836-openphone.dts |   49 +++++++++++++++++
 arch/arm64/boot/dts/sprd/sc9836.dtsi          |   73 +++++++++++++++++++++++++
 arch/arm64/boot/dts/sprd/sharkl64.dtsi        |   67 +++++++++++++++++++++++
 5 files changed, 195 insertions(+)
 create mode 100644 arch/arm64/boot/dts/sprd/Makefile
 create mode 100644 arch/arm64/boot/dts/sprd/sc9836-openphone.dts
 create mode 100644 arch/arm64/boot/dts/sprd/sc9836.dtsi
 create mode 100644 arch/arm64/boot/dts/sprd/sharkl64.dtsi

diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index 3b8d427..806a717 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -2,6 +2,7 @@ dts-dirs += amd
 dts-dirs += apm
 dts-dirs += arm
 dts-dirs += cavium
+dts-dirs += sprd
 
 always		:= $(dtb-y)
 subdir-y	:= $(dts-dirs)
diff --git a/arch/arm64/boot/dts/sprd/Makefile b/arch/arm64/boot/dts/sprd/Makefile
new file mode 100644
index 0000000..b658c5e
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/Makefile
@@ -0,0 +1,5 @@
+dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
+
+always		:= $(dtb-y)
+subdir-y	:= $(dts-dirs)
+clean-files	:= *.dtb
diff --git a/arch/arm64/boot/dts/sprd/sc9836-openphone.dts b/arch/arm64/boot/dts/sprd/sc9836-openphone.dts
new file mode 100644
index 0000000..b8a69b2
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/sc9836-openphone.dts
@@ -0,0 +1,49 @@
+/*
+ * Spreadtrum SC9836 openphone board DTS file
+ *
+ * Copyright (C) 2014, Spreadtrum Communications Inc.
+ *
+ * This file is licensed under a dual GPLv2 or X11 license.
+ */
+
+/dts-v1/;
+
+#include "sc9836.dtsi"
+
+/ {
+	model = "Spreadtrum SC9836 Openphone Board";
+
+	compatible = "sprd,sc9836-openphone", "sprd,sc9836";
+
+	aliases {
+		serial0 = &uart0;
+		serial1 = &uart1;
+		serial2 = &uart2;
+		serial3 = &uart3;
+	};
+
+	memory at 80000000 {
+		device_type = "memory";
+		reg = <0 0x80000000 0 0x20000000>;
+	};
+
+	chosen {
+		stdout-path = &uart0;
+	};
+};
+
+&uart0 {
+	status = "okay";
+};
+
+&uart1 {
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};
+
+&uart3 {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/sprd/sc9836.dtsi b/arch/arm64/boot/dts/sprd/sc9836.dtsi
new file mode 100644
index 0000000..2559d09
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/sc9836.dtsi
@@ -0,0 +1,73 @@
+/*
+ * Spreadtrum SC9836 SoC DTS file
+ *
+ * Copyright (C) 2014, Spreadtrum Communications Inc.
+ *
+ * This file is licensed under a dual GPLv2 or X11 license.
+ */
+
+#include "sharkl64.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+/ {
+	compatible = "sprd,sc9836";
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu at 0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+		};
+
+		cpu at 1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0 0x1>;
+			enable-method = "psci";
+		};
+
+		cpu at 2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0 0x2>;
+			enable-method = "psci";
+		};
+
+		cpu at 3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0 0x3>;
+			enable-method = "psci";
+		};
+	};
+
+	gic: interrupt-controller at 12001000 {
+		compatible = "arm,gic-400";
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		reg = <0 0x12001000 0 0x1000>,
+		      <0 0x12002000 0 0x2000>,
+		      <0 0x12004000 0 0x2000>,
+		      <0 0x12006000 0 0x2000>;
+	};
+
+	psci {
+		compatible	= "arm,psci";
+		method		= "smc";
+		cpu_on		= <0xc4000003>;
+		cpu_off		= <0x84000002>;
+		cpu_suspend	= <0xc4000001>;
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+};
diff --git a/arch/arm64/boot/dts/sprd/sharkl64.dtsi b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
new file mode 100644
index 0000000..b08989d
--- /dev/null
+++ b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
@@ -0,0 +1,67 @@
+/*
+ * Spreadtrum Sharkl64 platform DTS file
+ *
+ * Copyright (C) 2014, Spreadtrum Communications Inc.
+ *
+ * This file is licensed under a dual GPLv2 or X11 license.
+ */
+
+/ {
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	soc {
+		compatible = "simple-bus";
+		reg = <0x0 0x0 0x0 0x80000000>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		ap_apb: apb at 70000000 {
+			compatible = "simple-bus";
+			reg = <0x0 0x70000000 0x0 0x10000000>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+
+			uart0: serial at 70000000 {
+				compatible = "sprd,sc9836-uart";
+				reg = <0 0x70000000 0 0x100>;
+				interrupts = <0 2 0xf04>;
+				clocks = <&clk26mhz>;
+				status = "disabled";
+			};
+
+			uart1: serial at 70100000 {
+				compatible = "sprd,sc9836-uart";
+				reg = <0 0x70100000 0 0x100>;
+				interrupts = <0 3 0xf04>;
+				clocks = <&clk26mhz>;
+				status = "disabled";
+			};
+
+			uart2: serial at 70200000 {
+				compatible = "sprd,sc9836-uart";
+				reg = <0 0x70200000 0 0x100>;
+				interrupts = <0 2 0xf04>;
+				clocks = <&clk26mhz>;
+				status = "disabled";
+			};
+
+			uart3: serial at 70300000 {
+				compatible = "sprd,sc9836-uart";
+				reg = <0 0x70300000 0 0x100>;
+				interrupts = <0 3 0xf04>;
+				clocks = <&clk26mhz>;
+				status = "disabled";
+			};
+		};
+	};
+
+	clk26mhz: clk26mhz {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <26000000>;
+	};
+};
-- 
1.7.9.5

[PATCH v5 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

[Mark Rutland]

On Fri, Jan 16, 2015 at 10:00:09AM +0000, Chunyan Zhang wrote:
> From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
> 
> Adds the device tree support for Spreadtrum SC9836 SoC which is based on
> Sharkl64 platform.
> 
> Sharkl64 platform contains the common nodes of Spreadtrum's arm64-based SoCs.
> 
> Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> ---
>  arch/arm64/boot/dts/Makefile                  |    1 +
>  arch/arm64/boot/dts/sprd/Makefile             |    5 ++
>  arch/arm64/boot/dts/sprd/sc9836-openphone.dts |   49 +++++++++++++++++
>  arch/arm64/boot/dts/sprd/sc9836.dtsi          |   73 +++++++++++++++++++++++++
>  arch/arm64/boot/dts/sprd/sharkl64.dtsi        |   67 +++++++++++++++++++++++
>  5 files changed, 195 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/sprd/Makefile
>  create mode 100644 arch/arm64/boot/dts/sprd/sc9836-openphone.dts
>  create mode 100644 arch/arm64/boot/dts/sprd/sc9836.dtsi
>  create mode 100644 arch/arm64/boot/dts/sprd/sharkl64.dtsi

[...]

> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		cpu at 0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			reg = <0x0 0x0>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu at 1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			reg = <0x0 0x1>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu at 2 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			reg = <0x0 0x2>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu at 3 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			reg = <0x0 0x3>;
> +			enable-method = "psci";
> +		};
> +	};

Just to check, all CPUs may be hotplugged off and on, yes?

Including CPU0?

How is your implementation tested?

You boot CPUs at EL2?

> +
> +	gic: interrupt-controller at 12001000 {
> +		compatible = "arm,gic-400";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		reg = <0 0x12001000 0 0x1000>,
> +		      <0 0x12002000 0 0x2000>,
> +		      <0 0x12004000 0 0x2000>,
> +		      <0 0x12006000 0 0x2000>;
> +	};

You're missing the maintenance interrupt here.

[...]

> diff --git a/arch/arm64/boot/dts/sprd/sharkl64.dtsi b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
> new file mode 100644
> index 0000000..b08989d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
> @@ -0,0 +1,67 @@
> +/*
> + * Spreadtrum Sharkl64 platform DTS file
> + *
> + * Copyright (C) 2014, Spreadtrum Communications Inc.
> + *
> + * This file is licensed under a dual GPLv2 or X11 license.
> + */
> +
> +/ {
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	soc {
> +		compatible = "simple-bus";
> +		reg = <0x0 0x0 0x0 0x80000000>;

What is this reg for? It's not required by simple-bus.

If you want to encode that this covers a particular portion of the
address space, do so with the ranges proeprty.

> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		ap_apb: apb at 70000000 {
> +			compatible = "simple-bus";
> +			reg = <0x0 0x70000000 0x0 0x10000000>;

Likewise here.

Thanks,
Mark.

[PATCH v5 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

[Orson Zhai]

Hi, Mark,

On Fri, Jan 16, 2015 at 6:35 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 16, 2015 at 10:00:09AM +0000, Chunyan Zhang wrote:
>> From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
>>
>> Adds the device tree support for Spreadtrum SC9836 SoC which is based on
>> Sharkl64 platform.
>>
>> Sharkl64 platform contains the common nodes of Spreadtrum's arm64-based SoCs.
>>
>> Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> ---
>>  arch/arm64/boot/dts/Makefile                  |    1 +
>>  arch/arm64/boot/dts/sprd/Makefile             |    5 ++
>>  arch/arm64/boot/dts/sprd/sc9836-openphone.dts |   49 +++++++++++++++++
>>  arch/arm64/boot/dts/sprd/sc9836.dtsi          |   73 +++++++++++++++++++++++++
>>  arch/arm64/boot/dts/sprd/sharkl64.dtsi        |   67 +++++++++++++++++++++++
>>  5 files changed, 195 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/sprd/Makefile
>>  create mode 100644 arch/arm64/boot/dts/sprd/sc9836-openphone.dts
>>  create mode 100644 arch/arm64/boot/dts/sprd/sc9836.dtsi
>>  create mode 100644 arch/arm64/boot/dts/sprd/sharkl64.dtsi
>
> [...]
>
>> +     cpus {
>> +             #address-cells = <2>;
>> +             #size-cells = <0>;
>> +
>> +             cpu at 0 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a53", "arm,armv8";
>> +                     reg = <0x0 0x0>;
>> +                     enable-method = "psci";
>> +             };
>> +
>> +             cpu at 1 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a53", "arm,armv8";
>> +                     reg = <0x0 0x1>;
>> +                     enable-method = "psci";
>> +             };
>> +
>> +             cpu at 2 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a53", "arm,armv8";
>> +                     reg = <0x0 0x2>;
>> +                     enable-method = "psci";
>> +             };
>> +
>> +             cpu at 3 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a53", "arm,armv8";
>> +                     reg = <0x0 0x3>;
>> +                     enable-method = "psci";
>> +             };
>> +     };
>
> Just to check, all CPUs may be hotplugged off and on, yes?
>

Yes, I have tested with them successfully by looking into
`/proc/interrupts` and `top` except CPU0.

> Including CPU0?
>

 It returns "status busy" after I type the command below.


> How is your implementation tested?
>
I build kernel image with 3.19-rc1 + this patchset and run into console.
I use `echo 0 > /sys/devices/system/cpu[0-3]/online`

BTW, I run these on a real phone of sc9836 not fast-model as before.

> You boot CPUs at EL2?
>

Yes, I have confirmed this when working around the BUG() in
arch_counter_get_cntpct() introduced from 3.19.

>> +
>> +     gic: interrupt-controller at 12001000 {
>> +             compatible = "arm,gic-400";
>> +             #interrupt-cells = <3>;
>> +             interrupt-controller;
>> +             reg = <0 0x12001000 0 0x1000>,
>> +                   <0 0x12002000 0 0x2000>,
>> +                   <0 0x12004000 0 0x2000>,
>> +                   <0 0x12006000 0 0x2000>;
>> +     };
>
> You're missing the maintenance interrupt here.
>

Do you mean to declare SGI like this ?

" interrupts = <1 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)> "

> [...]
>
>> diff --git a/arch/arm64/boot/dts/sprd/sharkl64.dtsi b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
>> new file mode 100644
>> index 0000000..b08989d
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
>> @@ -0,0 +1,67 @@
>> +/*
>> + * Spreadtrum Sharkl64 platform DTS file
>> + *
>> + * Copyright (C) 2014, Spreadtrum Communications Inc.
>> + *
>> + * This file is licensed under a dual GPLv2 or X11 license.
>> + */
>> +
>> +/ {
>> +     interrupt-parent = <&gic>;
>> +     #address-cells = <2>;
>> +     #size-cells = <2>;
>> +
>> +     soc {
>> +             compatible = "simple-bus";
>> +             reg = <0x0 0x0 0x0 0x80000000>;
>
> What is this reg for? It's not required by simple-bus.
>

It's added by me.
I want to tell people the register range of what the bus covers, not
for any drivers use.
for this example, It starts from address 0 to 0x80000000.
Because Spreadtrum chip is very large with a lots of registers and matrix buses.


> If you want to encode that this covers a particular portion of the
> address space, do so with the ranges proeprty.

But I look up the ePAPER who says "The ranges property provides a
means of defining a mapping or translation...".
The bus here is flat-memory for all.

>
>> +             #address-cells = <2>;
>> +             #size-cells = <2>;
>> +             ranges;
>> +
>> +             ap_apb: apb at 70000000 {
>> +                     compatible = "simple-bus";
>> +                     reg = <0x0 0x70000000 0x0 0x10000000>;
>
> Likewise here.
>

This initial patch is picked up from a very big dt file.
There are several apb buses in this chip.
So I use apb at starting-address to separate them.
But I remember another rule that the @address needs to equal  first
address in property reg array.
Do I have to delete @7000000 as well if i delete reg line?

    Orson

> Thanks,
> Mark.

[PATCH v5 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

[Mark Rutland]

On Fri, Jan 16, 2015 at 12:49:20PM +0000, Orson Zhai wrote:
> Hi, Mark,
> 
> On Fri, Jan 16, 2015 at 6:35 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Jan 16, 2015 at 10:00:09AM +0000, Chunyan Zhang wrote:
> >> From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
> >>
> >> Adds the device tree support for Spreadtrum SC9836 SoC which is based on
> >> Sharkl64 platform.
> >>
> >> Sharkl64 platform contains the common nodes of Spreadtrum's arm64-based SoCs.
> >>
> >> Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
> >> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> >> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> >> ---
> >>  arch/arm64/boot/dts/Makefile                  |    1 +
> >>  arch/arm64/boot/dts/sprd/Makefile             |    5 ++
> >>  arch/arm64/boot/dts/sprd/sc9836-openphone.dts |   49 +++++++++++++++++
> >>  arch/arm64/boot/dts/sprd/sc9836.dtsi          |   73 +++++++++++++++++++++++++
> >>  arch/arm64/boot/dts/sprd/sharkl64.dtsi        |   67 +++++++++++++++++++++++
> >>  5 files changed, 195 insertions(+)
> >>  create mode 100644 arch/arm64/boot/dts/sprd/Makefile
> >>  create mode 100644 arch/arm64/boot/dts/sprd/sc9836-openphone.dts
> >>  create mode 100644 arch/arm64/boot/dts/sprd/sc9836.dtsi
> >>  create mode 100644 arch/arm64/boot/dts/sprd/sharkl64.dtsi
> >
> > [...]
> >
> >> +     cpus {
> >> +             #address-cells = <2>;
> >> +             #size-cells = <0>;
> >> +
> >> +             cpu at 0 {
> >> +                     device_type = "cpu";
> >> +                     compatible = "arm,cortex-a53", "arm,armv8";
> >> +                     reg = <0x0 0x0>;
> >> +                     enable-method = "psci";
> >> +             };
> >> +
> >> +             cpu at 1 {
> >> +                     device_type = "cpu";
> >> +                     compatible = "arm,cortex-a53", "arm,armv8";
> >> +                     reg = <0x0 0x1>;
> >> +                     enable-method = "psci";
> >> +             };
> >> +
> >> +             cpu at 2 {
> >> +                     device_type = "cpu";
> >> +                     compatible = "arm,cortex-a53", "arm,armv8";
> >> +                     reg = <0x0 0x2>;
> >> +                     enable-method = "psci";
> >> +             };
> >> +
> >> +             cpu at 3 {
> >> +                     device_type = "cpu";
> >> +                     compatible = "arm,cortex-a53", "arm,armv8";
> >> +                     reg = <0x0 0x3>;
> >> +                     enable-method = "psci";
> >> +             };
> >> +     };
> >
> > Just to check, all CPUs may be hotplugged off and on, yes?
> >
> 
> Yes, I have tested with them successfully by looking into
> `/proc/interrupts` and `top` except CPU0.

Ok.

> > Including CPU0?
> >
> 
>  It returns "status busy" after I type the command below.

Is this while other CPUs are active, or after you've hotplugged out all
other CPUs? The latter is expected, but the former is not.

Are you able to hotplug CPU0 out and back in while other CPUs are
active

> > How is your implementation tested?
> >
> I build kernel image with 3.19-rc1 + this patchset and run into console.
> I use `echo 0 > /sys/devices/system/cpu[0-3]/online`
> 
> BTW, I run these on a real phone of sc9836 not fast-model as before.
> 
> > You boot CPUs at EL2?
> >
> 
> Yes, I have confirmed this when working around the BUG() in
> arch_counter_get_cntpct() introduced from 3.19.

Ah, good to hear.

> 
> >> +
> >> +     gic: interrupt-controller at 12001000 {
> >> +             compatible = "arm,gic-400";
> >> +             #interrupt-cells = <3>;
> >> +             interrupt-controller;
> >> +             reg = <0 0x12001000 0 0x1000>,
> >> +                   <0 0x12002000 0 0x2000>,
> >> +                   <0 0x12004000 0 0x2000>,
> >> +                   <0 0x12006000 0 0x2000>;
> >> +     };
> >
> > You're missing the maintenance interrupt here.
> >
> 
> Do you mean to declare SGI like this ?
> 
> " interrupts = <1 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)> "

Something like that, yes. However the maintenance interrupt is wired up
in your system.

> 
> > [...]
> >
> >> diff --git a/arch/arm64/boot/dts/sprd/sharkl64.dtsi b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
> >> new file mode 100644
> >> index 0000000..b08989d
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/sprd/sharkl64.dtsi
> >> @@ -0,0 +1,67 @@
> >> +/*
> >> + * Spreadtrum Sharkl64 platform DTS file
> >> + *
> >> + * Copyright (C) 2014, Spreadtrum Communications Inc.
> >> + *
> >> + * This file is licensed under a dual GPLv2 or X11 license.
> >> + */
> >> +
> >> +/ {
> >> +     interrupt-parent = <&gic>;
> >> +     #address-cells = <2>;
> >> +     #size-cells = <2>;
> >> +
> >> +     soc {
> >> +             compatible = "simple-bus";
> >> +             reg = <0x0 0x0 0x0 0x80000000>;
> >
> > What is this reg for? It's not required by simple-bus.
> >
> 
> It's added by me.
> I want to tell people the register range of what the bus covers, not
> for any drivers use.

My point is that it's not part of the binding, so shouldn't be there.

> for this example, It starts from address 0 to 0x80000000.
> Because Spreadtrum chip is very large with a lots of registers and matrix buses.

Ok.

> 
> > If you want to encode that this covers a particular portion of the
> > address space, do so with the ranges proeprty.
> 
> But I look up the ePAPER who says "The ranges property provides a
> means of defining a mapping or translation...".
> The bus here is flat-memory for all.

In this case you could have:

ranges = <0x0 0x0 0x0 0x0 0x0 0x80000000>;

Which would be a flat mapping for the range you care about.

> 
> >
> >> +             #address-cells = <2>;
> >> +             #size-cells = <2>;
> >> +             ranges;
> >> +
> >> +             ap_apb: apb at 70000000 {
> >> +                     compatible = "simple-bus";
> >> +                     reg = <0x0 0x70000000 0x0 0x10000000>;

For this bus you could instead use addresses relative to the bus inside,
rather than absolute addresses, or you could have:

ranges = <0x0 0x70000000 0x0 0x70000000 0x0 0x10000000>;

> >
> > Likewise here.
> >
> 
> This initial patch is picked up from a very big dt file.
> There are several apb buses in this chip.

At the same level us the bus hierarchy?

> So I use apb at starting-address to separate them.
> But I remember another rule that the @address needs to equal  first
> address in property reg array.
> Do I have to delete @7000000 as well if i delete reg line?

Hmm. I'm not too keen on encoding a reg or unit-address here, because
the control interface of the bus isn't at that address. If we want to
add that later then the reg would be different in those cases. Given
there's no control interface here, there shouldn't be a reg or
unit-address.

Just ensure that the name before the unit-address is unique.

Thanks,
Mark.

[PATCH v5 3/5] arm64: dts: Add support for Spreadtrum SC9836 SoC in dts and Makefile

[Orson Zhai]

On Fri, Jan 16, 2015 at 10:09 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 16, 2015 at 12:49:20PM +0000, Orson Zhai wrote:
>> Hi, Mark,
>>
>> On Fri, Jan 16, 2015 at 6:35 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>[...]
>>
>>  It returns "status busy" after I type the command below.
>
> Is this while other CPUs are active, or after you've hotplugged out all
> other CPUs? The latter is expected, but the former is not.
>
> Are you able to hotplug CPU0 out and back in while other CPUs are
> active

Yes, I have just tested again and confirm that cpu0 could be
turned on/off when one of the other 3 CPUs was active.

>
> [...]
>
> For this bus you could instead use addresses relative to the bus inside,
> rather than absolute addresses,

Do you mean something like  apb at 0 {...} apb at 1 {...} ?

> or you could have:
>
> ranges = <0x0 0x70000000 0x0 0x70000000 0x0 0x10000000>;
>
> [...]
>>
>> This initial patch is picked up from a very big dt file.
>> There are several apb buses in this chip.
>
> At the same level us the bus hierarchy?
>
These buses are led by some bus matrix to different areas.
But I'd like treat them simply at same level.
Is it right to do that?

>> So I use apb at starting-address to separate them.
>> But I remember another rule that the @address needs to equal  first
>> address in property reg array.
>> Do I have to delete @7000000 as well if i delete reg line?
>
> Hmm. I'm not too keen on encoding a reg or unit-address here, because
> the control interface of the bus isn't at that address. If we want to
> add that later then the reg would be different in those cases. Given
> there's no control interface here, there shouldn't be a reg or
> unit-address.
>

Do you mean something like

   ap-ahb { ... };
   aon-ahb { ... };

without using @xxxx ?

   Thanks, Orson

> Just ensure that the name before the unit-address is unique.
>
> Thanks,
> Mark.

[PATCH v5 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

[Chunyan Zhang]

From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>

Adds support for Spreadtrum's SoC Platform in the arm64 Kconfig and
defconfig files.

Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
---
 arch/arm64/Kconfig           |    5 +++++
 arch/arm64/configs/defconfig |    1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1f9a20..885c1f4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -153,6 +153,11 @@ config ARCH_SEATTLE
 	help
 	  This enables support for AMD Seattle SOC Family
 
+config ARCH_SPRD
+	bool "Spreadtrum SoC platform"
+	help
+	  Support for Spreadtrum ARM based SoCs
+
 config ARCH_THUNDER
 	bool "Cavium Inc. Thunder SoC Family"
 	help
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index dd301be..c1677ca 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -33,6 +33,7 @@ CONFIG_MODULE_UNLOAD=y
 # CONFIG_BLK_DEV_BSG is not set
 # CONFIG_IOSCHED_DEADLINE is not set
 CONFIG_ARCH_THUNDER=y
+CONFIG_ARCH_SPRD=y
 CONFIG_ARCH_VEXPRESS=y
 CONFIG_ARCH_XGENE=y
 CONFIG_PCI=y
-- 
1.7.9.5

[PATCH v5 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

[Mark Rutland]

On Fri, Jan 16, 2015 at 10:00:10AM +0000, Chunyan Zhang wrote:
> From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
> 
> Adds support for Spreadtrum's SoC Platform in the arm64 Kconfig and
> defconfig files.
> 
> Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> ---
>  arch/arm64/Kconfig           |    5 +++++
>  arch/arm64/configs/defconfig |    1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b1f9a20..885c1f4 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -153,6 +153,11 @@ config ARCH_SEATTLE
>  	help
>  	  This enables support for AMD Seattle SOC Family
>  
> +config ARCH_SPRD

This should presumably be ARCH_SHARKL64, so you can add other SoC
families later. The other entries in here are already formatted that
way.

I wonder if we should have these of the form ARCH_${VENDOR}_${FAMILY}
(e.g. ARCH_SPRD_SHARKL64) rather than just ARCH_${FAMILY} to save
ourselves from name conflicts in future (and to make it easier to grep
for a particular vendor's config options).

> +	bool "Spreadtrum SoC platform"

bool "Spreadtrun Sharkl64 SoC Family"

> +	help
> +	  Support for Spreadtrum ARM based SoCs


Support for the Spreadtrum Sharkl64 SoC family.

Thanks,
Mark.

> +
>  config ARCH_THUNDER
>  	bool "Cavium Inc. Thunder SoC Family"
>  	help
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index dd301be..c1677ca 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -33,6 +33,7 @@ CONFIG_MODULE_UNLOAD=y
>  # CONFIG_BLK_DEV_BSG is not set
>  # CONFIG_IOSCHED_DEADLINE is not set
>  CONFIG_ARCH_THUNDER=y
> +CONFIG_ARCH_SPRD=y
>  CONFIG_ARCH_VEXPRESS=y
>  CONFIG_ARCH_XGENE=y
>  CONFIG_PCI=y
> -- 
> 1.7.9.5
> 
> 

[PATCH v5 4/5] arm64: Add support for Spreadtrum's Sharkl64 Platform in Kconfig and defconfig

[Lyra Zhang]

Hi, Mark

On Fri, Jan 16, 2015 at 6:48 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 16, 2015 at 10:00:10AM +0000, Chunyan Zhang wrote:
>> From: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
>>
>> Adds support for Spreadtrum's SoC Platform in the arm64 Kconfig and
>> defconfig files.
>>
>> Signed-off-by: Zhizhou Zhang <zhizhou.zhang@spreadtrum.com>
>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> ---
>>  arch/arm64/Kconfig           |    5 +++++
>>  arch/arm64/configs/defconfig |    1 +
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index b1f9a20..885c1f4 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -153,6 +153,11 @@ config ARCH_SEATTLE
>>       help
>>         This enables support for AMD Seattle SOC Family
>>
>> +config ARCH_SPRD
>
> This should presumably be ARCH_SHARKL64, so you can add other SoC
> families later. The other entries in here are already formatted that
> way.
>
> I wonder if we should have these of the form ARCH_${VENDOR}_${FAMILY}
> (e.g. ARCH_SPRD_SHARKL64) rather than just ARCH_${FAMILY} to save
> ourselves from name conflicts in future (and to make it easier to grep
> for a particular vendor's config options).
>

actually we've discussed this question before[1], and I think Arnd's
suggestion is suitable for our case, so I changed this to use
ARCH_SPRD instead of ARCH_SHARKL64.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/306246.html

anyway, thank you very much!

Chunyan

>> +     bool "Spreadtrum SoC platform"
>
> bool "Spreadtrun Sharkl64 SoC Family"
>
>> +     help
>> +       Support for Spreadtrum ARM based SoCs
>
>
> Support for the Spreadtrum Sharkl64 SoC family.
>
> Thanks,
> Mark.
>
>> +

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Chunyan Zhang]

Add a full sc9836-uart driver for SC9836 SoC which is based on the
spreadtrum sharkl64 platform.
This driver also support earlycon.
This patch also replaced the spaces between the macros and their
values with the tabs in serial_core.h

Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
---
 drivers/tty/serial/Kconfig       |   18 +
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sprd_serial.c |  772 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 4 files changed, 794 insertions(+)
 create mode 100644 drivers/tty/serial/sprd_serial.c

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index c79b43c..969d3cd 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135
 	  This driver can also be build as a module. If so, the module will be called
 	  men_z135_uart.ko
 
+config SERIAL_SPRD
+	tristate "Support for SPRD serial"
+	depends on ARCH_SPRD
+	select SERIAL_CORE
+	help
+	  This enables the driver for the Spreadtrum's serial.
+
+config SERIAL_SPRD_CONSOLE
+	bool "SPRD UART console support"
+	depends on SERIAL_SPRD=y
+	select SERIAL_CORE_CONSOLE
+	select SERIAL_EARLYCON
+	help
+	  Support for early debug console using Spreadtrum's serial. This enables
+	  the console before standard serial driver is probed. This is enabled
+	  with "earlycon" on the kernel command line. The console is
+	  enabled when early_param is processed.
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 9a548ac..4801aca 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)	+= arc_uart.o
 obj-$(CONFIG_SERIAL_RP2)	+= rp2.o
 obj-$(CONFIG_SERIAL_FSL_LPUART)	+= fsl_lpuart.o
 obj-$(CONFIG_SERIAL_MEN_Z135)	+= men_z135_uart.o
+obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
new file mode 100644
index 0000000..81839e4
--- /dev/null
+++ b/drivers/tty/serial/sprd_serial.c
@@ -0,0 +1,772 @@
+/*
+ * Copyright (C) 2012 Spreadtrum Communications Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/serial_core.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+/* device name */
+#define UART_NR_MAX		8
+#define SPRD_TTY_NAME		"ttySPX"
+#define SPRD_FIFO_SIZE		128
+#define SPRD_DEF_RATE		26000000
+#define SPRD_TIMEOUT		2048
+
+/* the offset of serial registers and BITs for them */
+/* data registers */
+#define SPRD_TXD		0x0000
+#define SPRD_RXD		0x0004
+
+/* line status register and its BITs  */
+#define SPRD_LSR		0x0008
+#define SPRD_LSR_OE		BIT(4)
+#define SPRD_LSR_FE		BIT(3)
+#define SPRD_LSR_PE		BIT(2)
+#define SPRD_LSR_BI		BIT(7)
+#define SPRD_LSR_TX_OVER	BIT(15)
+
+/* data number in TX and RX fifo */
+#define SPRD_STS1		0x000C
+
+/* interrupt enable register and its BITs */
+#define SPRD_IEN		0x0010
+#define SPRD_IEN_RX_FULL	BIT(0)
+#define SPRD_IEN_TX_EMPTY	BIT(1)
+#define SPRD_IEN_BREAK_DETECT	BIT(7)
+#define SPRD_IEN_TIMEOUT	BIT(13)
+
+/* interrupt clear register */
+#define SPRD_ICLR		0x0014
+
+/* line control register */
+#define SPRD_LCR		0x0018
+#define SPRD_LCR_STOP_1BIT	0x10
+#define SPRD_LCR_STOP_2BIT	0x30
+#define SPRD_LCR_DATA_LEN	(BIT(2) | BIT(3))
+#define SPRD_LCR_DATA_LEN5	0x0
+#define SPRD_LCR_DATA_LEN6	0x4
+#define SPRD_LCR_DATA_LEN7	0x8
+#define SPRD_LCR_DATA_LEN8	0xc
+#define SPRD_LCR_PARITY		(BIT(0) | BIT(1))
+#define SPRD_LCR_PARITY_EN	0x2
+#define SPRD_LCR_EVEN_PAR	0x0
+#define SPRD_LCR_ODD_PAR	0x1
+
+/* control register 1 */
+#define SPRD_CTL1		0x001C
+#define RX_HW_FLOW_CTL_THLD	BIT(6)
+#define RX_HW_FLOW_CTL_EN	BIT(7)
+#define TX_HW_FLOW_CTL_EN	BIT(8)
+
+/* fifo threshold register */
+#define SPRD_CTL2		0x0020
+#define THLD_TX_EMPTY		0x40
+#define THLD_RX_FULL		0x40
+
+/* config baud rate register */
+#define SPRD_CLKD0		0x0024
+#define SPRD_CLKD1		0x0028
+
+/* interrupt mask status register */
+#define SPRD_IMSR		0x002C
+#define SPRD_IMSR_RX_FIFO_FULL	BIT(0)
+#define SPRD_IMSR_TX_FIFO_EMPTY	BIT(1)
+#define SPRD_IMSR_BREAK_DETECT	BIT(7)
+#define SPRD_IMSR_TIMEOUT	BIT(13)
+
+struct reg_backup {
+	uint32_t ien;
+	uint32_t ctrl0;
+	uint32_t ctrl1;
+	uint32_t ctrl2;
+	uint32_t clkd0;
+	uint32_t clkd1;
+	uint32_t dspwait;
+};
+
+struct sprd_uart_port {
+	struct uart_port port;
+	struct reg_backup reg_bak;
+	char name[16];
+};
+
+static struct sprd_uart_port *sprd_port[UART_NR_MAX] = { NULL };
+
+static inline unsigned int serial_in(struct uart_port *port, int offset)
+{
+	return readl_relaxed(port->membase + offset);
+}
+
+static inline void serial_out(struct uart_port *port, int offset, int value)
+{
+	writel_relaxed(value, port->membase + offset);
+}
+
+static unsigned int sprd_tx_empty(struct uart_port *port)
+{
+	if (serial_in(port, SPRD_STS1) & 0xff00)
+		return 0;
+	else
+		return TIOCSER_TEMT;
+}
+
+static unsigned int sprd_get_mctrl(struct uart_port *port)
+{
+	return TIOCM_DSR | TIOCM_CTS;
+}
+
+static void sprd_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+	/* nothing to do */
+}
+
+static void sprd_stop_tx(struct uart_port *port)
+{
+	unsigned int ien, iclr;
+
+	iclr = serial_in(port, SPRD_ICLR);
+	ien = serial_in(port, SPRD_IEN);
+
+	iclr |= SPRD_IEN_TX_EMPTY;
+	ien &= ~SPRD_IEN_TX_EMPTY;
+
+	serial_out(port, SPRD_ICLR, iclr);
+	serial_out(port, SPRD_IEN, ien);
+}
+
+static void sprd_start_tx(struct uart_port *port)
+{
+	unsigned int ien;
+
+	ien = serial_in(port, SPRD_IEN);
+	if (!(ien & SPRD_IEN_TX_EMPTY)) {
+		ien |= SPRD_IEN_TX_EMPTY;
+		serial_out(port, SPRD_IEN, ien);
+	}
+}
+
+static void sprd_stop_rx(struct uart_port *port)
+{
+	unsigned int ien, iclr;
+
+	iclr = serial_in(port, SPRD_ICLR);
+	ien = serial_in(port, SPRD_IEN);
+
+	ien &= ~(SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT);
+	iclr |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT;
+
+	serial_out(port, SPRD_IEN, ien);
+	serial_out(port, SPRD_ICLR, iclr);
+}
+
+/* The Sprd serial does not support this function. */
+static void sprd_break_ctl(struct uart_port *port, int break_state)
+{
+	/* nothing to do */
+}
+
+static inline int handle_lsr_errors(struct uart_port *port,
+				    unsigned int *flag,
+				    unsigned int *lsr)
+{
+	int ret = 0;
+
+	/* statistics */
+	if (*lsr & SPRD_LSR_BI) {
+		*lsr &= ~(SPRD_LSR_FE | SPRD_LSR_PE);
+		port->icount.brk++;
+		ret = uart_handle_break(port);
+		if (ret)
+			return ret;
+	} else if (*lsr & SPRD_LSR_PE)
+		port->icount.parity++;
+	else if (*lsr & SPRD_LSR_FE)
+		port->icount.frame++;
+	if (*lsr & SPRD_LSR_OE)
+		port->icount.overrun++;
+
+	/* mask off conditions which should be ignored */
+	*lsr &= port->read_status_mask;
+	if (*lsr & SPRD_LSR_BI)
+		*flag = TTY_BREAK;
+	else if (*lsr & SPRD_LSR_PE)
+		*flag = TTY_PARITY;
+	else if (*lsr & SPRD_LSR_FE)
+		*flag = TTY_FRAME;
+
+	return ret;
+}
+
+static inline void sprd_rx(int irq, void *dev_id)
+{
+	struct uart_port *port = dev_id;
+	struct tty_port *tty = &port->state->port;
+	unsigned int ch, flag, lsr, max_count = SPRD_TIMEOUT;
+
+	while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
+		lsr = serial_in(port, SPRD_LSR);
+		ch = serial_in(port, SPRD_RXD);
+		flag = TTY_NORMAL;
+		port->icount.rx++;
+
+		if (lsr & (SPRD_LSR_BI | SPRD_LSR_PE
+			| SPRD_LSR_FE | SPRD_LSR_OE))
+			if (handle_lsr_errors(port, &lsr, &flag))
+				continue;
+		if (uart_handle_sysrq_char(port, ch))
+			continue;
+
+		uart_insert_char(port, lsr, SPRD_LSR_OE, ch, flag);
+	}
+
+	tty_flip_buffer_push(tty);
+}
+
+static inline void sprd_tx(int irq, void *dev_id)
+{
+	struct uart_port *port = dev_id;
+	struct circ_buf *xmit = &port->state->xmit;
+	int count;
+
+	if (port->x_char) {
+		serial_out(port, SPRD_TXD, port->x_char);
+		port->icount.tx++;
+		port->x_char = 0;
+		return;
+	}
+
+	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
+		sprd_stop_tx(port);
+		return;
+	}
+
+	count = THLD_TX_EMPTY;
+	do {
+		serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
+		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+		port->icount.tx++;
+		if (uart_circ_empty(xmit))
+			break;
+	} while (--count > 0);
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+
+	if (uart_circ_empty(xmit))
+		sprd_stop_tx(port);
+}
+
+/* this handles the interrupt from one port */
+static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
+{
+	struct uart_port *port = (struct uart_port *)dev_id;
+	unsigned int ims;
+
+	ims = serial_in(port, SPRD_IMSR);
+
+	if (!ims)
+		return IRQ_NONE;
+
+	serial_out(port, SPRD_ICLR, ~0);
+
+	if (ims & (SPRD_IMSR_RX_FIFO_FULL |
+		SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
+		sprd_rx(irq, port);
+
+	if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
+		sprd_tx(irq, port);
+
+	return IRQ_HANDLED;
+}
+
+static int sprd_startup(struct uart_port *port)
+{
+	int ret = 0;
+	unsigned int ien, ctrl1;
+	unsigned int timeout;
+	struct sprd_uart_port *sp;
+
+	serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
+
+	/* clear rx fifo */
+	timeout = SPRD_TIMEOUT;
+	while (timeout-- && serial_in(port, SPRD_STS1) & 0x00ff)
+		serial_in(port, SPRD_RXD);
+
+	/* clear tx fifo */
+	timeout = SPRD_TIMEOUT;
+	while (timeout-- && serial_in(port, SPRD_STS1) & 0xff00)
+		cpu_relax();
+
+	/* clear interrupt */
+	serial_out(port, SPRD_IEN, 0x0);
+	serial_out(port, SPRD_ICLR, ~0);
+
+	/* allocate irq */
+	sp = container_of(port, struct sprd_uart_port, port);
+	snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
+	ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
+				IRQF_SHARED, sp->name, port);
+	if (ret) {
+		dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
+			port->irq, ret);
+		return ret;
+	}
+	ctrl1 = serial_in(port, SPRD_CTL1);
+	ctrl1 |= 0x3e00 | THLD_RX_FULL;
+	serial_out(port, SPRD_CTL1, ctrl1);
+
+	/* enable interrupt */
+	spin_lock(&port->lock);
+	ien = serial_in(port, SPRD_IEN);
+	ien |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT | SPRD_IEN_TIMEOUT;
+	serial_out(port, SPRD_IEN, ien);
+	spin_unlock(&port->lock);
+
+	return 0;
+}
+
+static void sprd_shutdown(struct uart_port *port)
+{
+	serial_out(port, SPRD_IEN, 0x0);
+	serial_out(port, SPRD_ICLR, ~0);
+	devm_free_irq(port->dev, port->irq, port);
+}
+
+static void sprd_set_termios(struct uart_port *port,
+				    struct ktermios *termios,
+				    struct ktermios *old)
+{
+	unsigned int baud, quot;
+	unsigned int lcr, fc;
+
+	/* ask the core to calculate the divisor for us */
+	baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
+
+	quot = (unsigned int)((port->uartclk + baud / 2) / baud);
+
+	/* set data length */
+	lcr = serial_in(port, SPRD_LCR);
+	lcr &= ~SPRD_LCR_DATA_LEN;
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		lcr |= SPRD_LCR_DATA_LEN5;
+		break;
+	case CS6:
+		lcr |= SPRD_LCR_DATA_LEN6;
+		break;
+	case CS7:
+		lcr |= SPRD_LCR_DATA_LEN7;
+		break;
+	case CS8:
+	default:
+		lcr |= SPRD_LCR_DATA_LEN8;
+		break;
+	}
+
+	/* calculate stop bits */
+	lcr &= ~(SPRD_LCR_STOP_1BIT | SPRD_LCR_STOP_2BIT);
+	if (termios->c_cflag & CSTOPB)
+		lcr |= SPRD_LCR_STOP_2BIT;
+	else
+		lcr |= SPRD_LCR_STOP_1BIT;
+
+	/* calculate parity */
+	lcr &= ~SPRD_LCR_PARITY;
+	termios->c_cflag &= ~CMSPAR;	/* no support mark/space */
+	if (termios->c_cflag & PARENB) {
+		lcr |= SPRD_LCR_PARITY_EN;
+		if (termios->c_cflag & PARODD)
+			lcr |= SPRD_LCR_ODD_PAR;
+		else
+			lcr |= SPRD_LCR_EVEN_PAR;
+	}
+
+	/* change the port state. */
+	/* update the per-port timeout */
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	port->read_status_mask = SPRD_LSR_OE;
+	if (termios->c_iflag & INPCK)
+		port->read_status_mask |= SPRD_LSR_FE | SPRD_LSR_PE;
+	if (termios->c_iflag & (BRKINT | PARMRK))
+		port->read_status_mask |= SPRD_LSR_BI;
+
+	/* characters to ignore */
+	port->ignore_status_mask = 0;
+	if (termios->c_iflag & IGNPAR)
+		port->ignore_status_mask |= SPRD_LSR_PE | SPRD_LSR_FE;
+	if (termios->c_iflag & IGNBRK) {
+		port->ignore_status_mask |= SPRD_LSR_BI;
+		/*
+		 * If we're ignoring parity and break indicators,
+		 * ignore overruns too (for real raw support).
+		 */
+		if (termios->c_iflag & IGNPAR)
+			port->ignore_status_mask |= SPRD_LSR_OE;
+	}
+
+	/* flow control */
+	fc = serial_in(port, SPRD_CTL1);
+	fc &= ~(RX_HW_FLOW_CTL_THLD | RX_HW_FLOW_CTL_EN | TX_HW_FLOW_CTL_EN);
+	if (termios->c_cflag & CRTSCTS) {
+		fc |= RX_HW_FLOW_CTL_THLD;
+		fc |= RX_HW_FLOW_CTL_EN;
+		fc |= TX_HW_FLOW_CTL_EN;
+	}
+
+	/* clock divider bit0~bit15 */
+	serial_out(port, SPRD_CLKD0, quot & 0xffff);
+
+	/* clock divider bit16~bit20 */
+	serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
+	serial_out(port, SPRD_LCR, lcr);
+	fc |= 0x3e00 | THLD_RX_FULL;
+	serial_out(port, SPRD_CTL1, fc);
+
+	/* Don't rewrite B0 */
+	if (tty_termios_baud_rate(termios))
+		tty_termios_encode_baud_rate(termios, baud, baud);
+}
+
+static const char *sprd_type(struct uart_port *port)
+{
+	return "SPX";
+}
+
+static void sprd_release_port(struct uart_port *port)
+{
+	/* nothing to do */
+}
+
+static int sprd_request_port(struct uart_port *port)
+{
+	return 0;
+}
+
+static void sprd_config_port(struct uart_port *port, int flags)
+{
+	if (flags & UART_CONFIG_TYPE)
+		port->type = PORT_SPRD;
+}
+
+static int sprd_verify_port(struct uart_port *port,
+				   struct serial_struct *ser)
+{
+	if (ser->type != PORT_SPRD)
+		return -EINVAL;
+	if (port->irq != ser->irq)
+		return -EINVAL;
+	return 0;
+}
+
+static struct uart_ops serial_sprd_ops = {
+	.tx_empty = sprd_tx_empty,
+	.get_mctrl = sprd_get_mctrl,
+	.set_mctrl = sprd_set_mctrl,
+	.stop_tx = sprd_stop_tx,
+	.start_tx = sprd_start_tx,
+	.stop_rx = sprd_stop_rx,
+	.break_ctl = sprd_break_ctl,
+	.startup = sprd_startup,
+	.shutdown = sprd_shutdown,
+	.set_termios = sprd_set_termios,
+	.type = sprd_type,
+	.release_port = sprd_release_port,
+	.request_port = sprd_request_port,
+	.config_port = sprd_config_port,
+	.verify_port = sprd_verify_port,
+};
+
+#ifdef CONFIG_SERIAL_SPRD_CONSOLE
+static inline void wait_for_xmitr(struct uart_port *port)
+{
+	unsigned int status, tmout = 10000;
+
+	/* wait up to 10ms for the character(s) to be sent */
+	do {
+		status = serial_in(port, SPRD_STS1);
+		if (--tmout == 0)
+			break;
+		udelay(1);
+	} while (status & 0xff00);
+}
+
+static void sprd_console_putchar(struct uart_port *port, int ch)
+{
+	wait_for_xmitr(port);
+	serial_out(port, SPRD_TXD, ch);
+}
+
+static void sprd_console_write(struct console *co, const char *s,
+				      unsigned int count)
+{
+	struct uart_port *port = &sprd_port[co->index]->port;
+	int ien;
+	int locked = 1;
+
+	if (oops_in_progress)
+		locked = spin_trylock(&port->lock);
+	else
+		spin_lock(&port->lock);
+	/* save the IEN then disable the interrupts */
+	ien = serial_in(port, SPRD_IEN);
+	serial_out(port, SPRD_IEN, 0x0);
+
+	uart_console_write(port, s, count, sprd_console_putchar);
+
+	/* wait for transmitter to become empty and restore the IEN */
+	wait_for_xmitr(port);
+	serial_out(port, SPRD_IEN, ien);
+	if (locked)
+		spin_unlock(&port->lock);
+}
+
+static int __init sprd_console_setup(struct console *co, char *options)
+{
+	struct uart_port *port;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	if (co->index >= UART_NR_MAX || co->index < 0)
+		co->index = 0;
+
+	port = &sprd_port[co->index]->port;
+	if (port == NULL) {
+		pr_info("serial port %d not yet initialized\n", co->index);
+		return -ENODEV;
+	}
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+	return uart_set_options(port, co, baud, parity, bits, flow);
+}
+
+static struct uart_driver sprd_uart_driver;
+static struct console sprd_console = {
+	.name = SPRD_TTY_NAME,
+	.write = sprd_console_write,
+	.device = uart_console_device,
+	.setup = sprd_console_setup,
+	.flags = CON_PRINTBUFFER,
+	.index = -1,
+	.data = &sprd_uart_driver,
+};
+
+#define SPRD_CONSOLE	(&sprd_console)
+
+/* Support for earlycon */
+static void sprd_putc(struct uart_port *port, int c)
+{
+	unsigned int timeout = SPRD_TIMEOUT;
+
+	while (timeout-- &&
+		   !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
+		cpu_relax();
+
+	writeb(c, port->membase + SPRD_TXD);
+}
+
+static void sprd_early_write(struct console *con, const char *s,
+				    unsigned n)
+{
+	struct earlycon_device *dev = con->data;
+
+	uart_console_write(&dev->port, s, n, sprd_putc);
+}
+
+static int __init sprd_early_console_setup(
+				struct earlycon_device *device,
+				const char *opt)
+{
+	if (!device->port.membase)
+		return -ENODEV;
+
+	device->con->write = sprd_early_write;
+	return 0;
+}
+
+EARLYCON_DECLARE(sprd_serial, sprd_early_console_setup);
+OF_EARLYCON_DECLARE(sprd_serial, "sprd,sc9836-uart",
+		    sprd_early_console_setup);
+
+#else /* !CONFIG_SERIAL_SPRD_CONSOLE */
+#define SPRD_CONSOLE		NULL
+#endif
+
+static struct uart_driver sprd_uart_driver = {
+	.owner = THIS_MODULE,
+	.driver_name = "sprd_serial",
+	.dev_name = SPRD_TTY_NAME,
+	.major = 0,
+	.minor = 0,
+	.nr = UART_NR_MAX,
+	.cons = SPRD_CONSOLE,
+};
+
+static int sprd_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct device_node *np = pdev->dev.of_node;
+	struct uart_port *up;
+	struct clk *clk;
+	int irq;
+
+	if (np)
+		pdev->id = of_alias_get_id(np, "serial");
+
+	if (pdev->id < 0 || pdev->id >= UART_NR_MAX) {
+		dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
+		return -ENXIO;
+	}
+
+	sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
+		sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
+	if (!sprd_port[pdev->id])
+		return -ENOMEM;
+
+	up = &sprd_port[pdev->id]->port;
+	up->dev = &pdev->dev;
+	up->line = pdev->id;
+	up->type = PORT_SPRD;
+	up->iotype = SERIAL_IO_PORT;
+	up->uartclk = SPRD_DEF_RATE;
+	up->fifosize = SPRD_FIFO_SIZE;
+	up->ops = &serial_sprd_ops;
+	up->flags = ASYNC_BOOT_AUTOCONF;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (!IS_ERR(clk))
+		up->uartclk = clk_get_rate(clk);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "not provide mem resource\n");
+		return -ENODEV;
+	}
+	up->mapbase = res->start;
+	up->membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(up->membase))
+		return PTR_ERR(up->membase);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "not provide irq resource\n");
+		return -ENODEV;
+	}
+	up->irq = irq;
+
+	platform_set_drvdata(pdev, up);
+
+	return uart_add_one_port(&sprd_uart_driver, up);
+}
+
+static int sprd_remove(struct platform_device *dev)
+{
+	struct uart_port *up = platform_get_drvdata(dev);
+
+	return uart_remove_one_port(&sprd_uart_driver, up);
+}
+
+static int sprd_suspend(struct device *dev)
+{
+	int id = to_platform_device(dev)->id;
+	struct uart_port *port = &sprd_port[id]->port;
+	struct reg_backup *reg_bak = &sprd_port[id]->reg_bak;
+
+	reg_bak->ien = serial_in(port, SPRD_IEN);
+	reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
+	reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
+	reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
+	reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
+	reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
+
+	uart_suspend_port(&sprd_uart_driver, port);
+
+	return 0;
+}
+
+static int sprd_resume(struct device *dev)
+{
+	int id = to_platform_device(dev)->id;
+	struct uart_port *port = &sprd_port[id]->port;
+	struct reg_backup *reg_bak = &sprd_port[id]->reg_bak;
+
+	serial_out(port, SPRD_LCR, reg_bak->ctrl0);
+	serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
+	serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
+	serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
+	serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
+	serial_out(port, SPRD_IEN, reg_bak->ien);
+
+	uart_resume_port(&sprd_uart_driver, port);
+
+	return 0;
+}
+
+static const struct of_device_id serial_ids[] = {
+	{.compatible = "sprd,sc9836-uart",},
+	{}
+};
+
+static SIMPLE_DEV_PM_OPS(sprd_pm_ops, sprd_suspend, sprd_resume);
+
+static struct platform_driver sprd_platform_driver = {
+	.probe		= sprd_probe,
+	.remove		= sprd_remove,
+	.driver 	= {
+		.name	= "sprd_serial",
+		.of_match_table = of_match_ptr(serial_ids),
+		.pm	= &sprd_pm_ops,
+	},
+};
+
+static int __init sprd_serial_init(void)
+{
+	int ret = 0;
+
+	ret = uart_register_driver(&sprd_uart_driver);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&sprd_platform_driver);
+	if (ret)
+		uart_unregister_driver(&sprd_uart_driver);
+
+	return ret;
+}
+
+static void __exit sprd_serial_exit(void)
+{
+	platform_driver_unregister(&sprd_platform_driver);
+	uart_unregister_driver(&sprd_uart_driver);
+}
+
+module_init(sprd_serial_init);
+module_exit(sprd_serial_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index c172180..7e6eb39 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -248,4 +248,7 @@
 /* MESON */
 #define PORT_MESON	109
 
+/* SPRD SERIAL  */
+#define PORT_SPRD	110
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
-- 
1.7.9.5

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Arnd Bergmann]

On Friday 16 January 2015 18:00:11 Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
> This patch also replaced the spaces between the macros and their
> values with the tabs in serial_core.h
> 
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> 

I see nothing wrong with the patch contents, but a few things regarding
the submission process:

- The 'Signed-off-by' lines are in the wrong order. As the person
  sending it, your S-o-b should be the last one in the list

- You have too many people on 'To'. Please send the patch only to
  the person you expect to apply it, and put the other people that
  may be interested on Cc. A lot of the people who got this mail
  are probably not interested and you can drop them completely.

- Now that everything is reviewed, split the series according to
  subsystem maintainers and send it separately: patches 1,2 and 5
  should go to GregKH as one series, the rest should go to
  'arm at kernel.org'.

- For some reason I did not get patch 4. Can you check if that
  made it to the mailing list?

Feel free to add my 'Acked-by: Arnd Bergmann <arnd@arndb.de>' to
the patches you send to Greg.

	Arnd

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Baruch Siach]

Hi Chunyan Zhang,

On Fri, Jan 16, 2015 at 06:00:11PM +0800, Chunyan Zhang wrote:
> +		if (uart_handle_sysrq_char(port, ch))
> +			continue;

You don't define SUPPORT_SYSRQ, so uart_handle_sysrq_char has no effect. See 
include/linux/serial_core.h.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/16/2015 05:00 AM, Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
> This patch also replaced the spaces between the macros and their
> values with the tabs in serial_core.h

The locking doesn't look correct. Specific notations below.

> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> ---
>  drivers/tty/serial/Kconfig       |   18 +
>  drivers/tty/serial/Makefile      |    1 +
>  drivers/tty/serial/sprd_serial.c |  772 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/serial_core.h |    3 +
>  4 files changed, 794 insertions(+)
>  create mode 100644 drivers/tty/serial/sprd_serial.c
> 
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index c79b43c..969d3cd 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135
>  	  This driver can also be build as a module. If so, the module will be called
>  	  men_z135_uart.ko
>  
> +config SERIAL_SPRD
> +	tristate "Support for SPRD serial"
> +	depends on ARCH_SPRD
> +	select SERIAL_CORE
> +	help
> +	  This enables the driver for the Spreadtrum's serial.
> +
> +config SERIAL_SPRD_CONSOLE
> +	bool "SPRD UART console support"
> +	depends on SERIAL_SPRD=y
> +	select SERIAL_CORE_CONSOLE
> +	select SERIAL_EARLYCON
> +	help
> +	  Support for early debug console using Spreadtrum's serial. This enables
> +	  the console before standard serial driver is probed. This is enabled
> +	  with "earlycon" on the kernel command line. The console is
> +	  enabled when early_param is processed.
> +
>  endmenu
>  
>  config SERIAL_MCTRL_GPIO
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 9a548ac..4801aca 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)	+= arc_uart.o
>  obj-$(CONFIG_SERIAL_RP2)	+= rp2.o
>  obj-$(CONFIG_SERIAL_FSL_LPUART)	+= fsl_lpuart.o
>  obj-$(CONFIG_SERIAL_MEN_Z135)	+= men_z135_uart.o
> +obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
>  
>  # GPIOLIB helpers for modem control lines
>  obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> new file mode 100644
> index 0000000..81839e4
> --- /dev/null
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -0,0 +1,772 @@
> +/*
> + * Copyright (C) 2012 Spreadtrum Communications Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/console.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +/* device name */
> +#define UART_NR_MAX		8
> +#define SPRD_TTY_NAME		"ttySPX"
> +#define SPRD_FIFO_SIZE		128
> +#define SPRD_DEF_RATE		26000000
> +#define SPRD_TIMEOUT		2048
> +
> +/* the offset of serial registers and BITs for them */
> +/* data registers */
> +#define SPRD_TXD		0x0000
> +#define SPRD_RXD		0x0004
> +
> +/* line status register and its BITs  */
> +#define SPRD_LSR		0x0008
> +#define SPRD_LSR_OE		BIT(4)
> +#define SPRD_LSR_FE		BIT(3)
> +#define SPRD_LSR_PE		BIT(2)
> +#define SPRD_LSR_BI		BIT(7)
> +#define SPRD_LSR_TX_OVER	BIT(15)
> +
> +/* data number in TX and RX fifo */
> +#define SPRD_STS1		0x000C
> +
> +/* interrupt enable register and its BITs */
> +#define SPRD_IEN		0x0010
> +#define SPRD_IEN_RX_FULL	BIT(0)
> +#define SPRD_IEN_TX_EMPTY	BIT(1)
> +#define SPRD_IEN_BREAK_DETECT	BIT(7)
> +#define SPRD_IEN_TIMEOUT	BIT(13)
> +
> +/* interrupt clear register */
> +#define SPRD_ICLR		0x0014
> +
> +/* line control register */
> +#define SPRD_LCR		0x0018
> +#define SPRD_LCR_STOP_1BIT	0x10
> +#define SPRD_LCR_STOP_2BIT	0x30
> +#define SPRD_LCR_DATA_LEN	(BIT(2) | BIT(3))
> +#define SPRD_LCR_DATA_LEN5	0x0
> +#define SPRD_LCR_DATA_LEN6	0x4
> +#define SPRD_LCR_DATA_LEN7	0x8
> +#define SPRD_LCR_DATA_LEN8	0xc
> +#define SPRD_LCR_PARITY		(BIT(0) | BIT(1))
> +#define SPRD_LCR_PARITY_EN	0x2
> +#define SPRD_LCR_EVEN_PAR	0x0
> +#define SPRD_LCR_ODD_PAR	0x1
> +
> +/* control register 1 */
> +#define SPRD_CTL1		0x001C
> +#define RX_HW_FLOW_CTL_THLD	BIT(6)
> +#define RX_HW_FLOW_CTL_EN	BIT(7)
> +#define TX_HW_FLOW_CTL_EN	BIT(8)
> +
> +/* fifo threshold register */
> +#define SPRD_CTL2		0x0020
> +#define THLD_TX_EMPTY		0x40
> +#define THLD_RX_FULL		0x40
> +
> +/* config baud rate register */
> +#define SPRD_CLKD0		0x0024
> +#define SPRD_CLKD1		0x0028
> +
> +/* interrupt mask status register */
> +#define SPRD_IMSR		0x002C
> +#define SPRD_IMSR_RX_FIFO_FULL	BIT(0)
> +#define SPRD_IMSR_TX_FIFO_EMPTY	BIT(1)
> +#define SPRD_IMSR_BREAK_DETECT	BIT(7)
> +#define SPRD_IMSR_TIMEOUT	BIT(13)
> +
> +struct reg_backup {
> +	uint32_t ien;
> +	uint32_t ctrl0;
> +	uint32_t ctrl1;
> +	uint32_t ctrl2;
> +	uint32_t clkd0;
> +	uint32_t clkd1;
> +	uint32_t dspwait;
> +};
> +
> +struct sprd_uart_port {
> +	struct uart_port port;
> +	struct reg_backup reg_bak;
> +	char name[16];
> +};
> +
> +static struct sprd_uart_port *sprd_port[UART_NR_MAX] = { NULL };
> +
> +static inline unsigned int serial_in(struct uart_port *port, int offset)
> +{
> +	return readl_relaxed(port->membase + offset);
> +}
> +
> +static inline void serial_out(struct uart_port *port, int offset, int value)
> +{
> +	writel_relaxed(value, port->membase + offset);
> +}
> +
> +static unsigned int sprd_tx_empty(struct uart_port *port)
> +{
> +	if (serial_in(port, SPRD_STS1) & 0xff00)
> +		return 0;
> +	else
> +		return TIOCSER_TEMT;
> +}
> +
> +static unsigned int sprd_get_mctrl(struct uart_port *port)
> +{
> +	return TIOCM_DSR | TIOCM_CTS;
> +}
> +
> +static void sprd_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +	/* nothing to do */
> +}
> +
> +static void sprd_stop_tx(struct uart_port *port)
> +{
> +	unsigned int ien, iclr;
> +
> +	iclr = serial_in(port, SPRD_ICLR);
> +	ien = serial_in(port, SPRD_IEN);
> +
> +	iclr |= SPRD_IEN_TX_EMPTY;
> +	ien &= ~SPRD_IEN_TX_EMPTY;
> +
> +	serial_out(port, SPRD_ICLR, iclr);
> +	serial_out(port, SPRD_IEN, ien);
> +}
> +
> +static void sprd_start_tx(struct uart_port *port)
> +{
> +	unsigned int ien;
> +
> +	ien = serial_in(port, SPRD_IEN);
> +	if (!(ien & SPRD_IEN_TX_EMPTY)) {
> +		ien |= SPRD_IEN_TX_EMPTY;
> +		serial_out(port, SPRD_IEN, ien);
> +	}
> +}
> +
> +static void sprd_stop_rx(struct uart_port *port)
> +{
> +	unsigned int ien, iclr;
> +
> +	iclr = serial_in(port, SPRD_ICLR);
> +	ien = serial_in(port, SPRD_IEN);
> +
> +	ien &= ~(SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT);
> +	iclr |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT;
> +
> +	serial_out(port, SPRD_IEN, ien);
> +	serial_out(port, SPRD_ICLR, iclr);
> +}
> +
> +/* The Sprd serial does not support this function. */
> +static void sprd_break_ctl(struct uart_port *port, int break_state)
> +{
> +	/* nothing to do */
> +}
> +
> +static inline int handle_lsr_errors(struct uart_port *port,
> +				    unsigned int *flag,
> +				    unsigned int *lsr)
> +{
> +	int ret = 0;
> +
> +	/* statistics */
> +	if (*lsr & SPRD_LSR_BI) {
> +		*lsr &= ~(SPRD_LSR_FE | SPRD_LSR_PE);
> +		port->icount.brk++;
> +		ret = uart_handle_break(port);
> +		if (ret)
> +			return ret;
> +	} else if (*lsr & SPRD_LSR_PE)
> +		port->icount.parity++;
> +	else if (*lsr & SPRD_LSR_FE)
> +		port->icount.frame++;
> +	if (*lsr & SPRD_LSR_OE)
> +		port->icount.overrun++;
> +
> +	/* mask off conditions which should be ignored */
> +	*lsr &= port->read_status_mask;
> +	if (*lsr & SPRD_LSR_BI)
> +		*flag = TTY_BREAK;
> +	else if (*lsr & SPRD_LSR_PE)
> +		*flag = TTY_PARITY;
> +	else if (*lsr & SPRD_LSR_FE)
> +		*flag = TTY_FRAME;
> +
> +	return ret;
> +}
> +
> +static inline void sprd_rx(int irq, void *dev_id)
> +{
> +	struct uart_port *port = dev_id;
> +	struct tty_port *tty = &port->state->port;
> +	unsigned int ch, flag, lsr, max_count = SPRD_TIMEOUT;
> +
> +	while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
> +		lsr = serial_in(port, SPRD_LSR);
> +		ch = serial_in(port, SPRD_RXD);
> +		flag = TTY_NORMAL;
> +		port->icount.rx++;
> +
> +		if (lsr & (SPRD_LSR_BI | SPRD_LSR_PE
> +			| SPRD_LSR_FE | SPRD_LSR_OE))
> +			if (handle_lsr_errors(port, &lsr, &flag))
> +				continue;
> +		if (uart_handle_sysrq_char(port, ch))
> +			continue;
> +
> +		uart_insert_char(port, lsr, SPRD_LSR_OE, ch, flag);
> +	}
> +
> +	tty_flip_buffer_push(tty);
> +}
> +
> +static inline void sprd_tx(int irq, void *dev_id)
> +{
> +	struct uart_port *port = dev_id;
> +	struct circ_buf *xmit = &port->state->xmit;
> +	int count;
> +
> +	if (port->x_char) {
> +		serial_out(port, SPRD_TXD, port->x_char);
> +		port->icount.tx++;
> +		port->x_char = 0;
> +		return;
> +	}
> +
> +	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
> +		sprd_stop_tx(port);
> +		return;
> +	}
> +
> +	count = THLD_TX_EMPTY;
> +	do {
> +		serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
> +		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> +		port->icount.tx++;
> +		if (uart_circ_empty(xmit))
> +			break;
> +	} while (--count > 0);
> +
> +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> +		uart_write_wakeup(port);
> +
> +	if (uart_circ_empty(xmit))
> +		sprd_stop_tx(port);
> +}
> +
> +/* this handles the interrupt from one port */
> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
> +{
> +	struct uart_port *port = (struct uart_port *)dev_id;
> +	unsigned int ims;

Why does your isr not have to take port->lock ?

> +	ims = serial_in(port, SPRD_IMSR);
> +
> +	if (!ims)
> +		return IRQ_NONE;
> +
> +	serial_out(port, SPRD_ICLR, ~0);
> +
> +	if (ims & (SPRD_IMSR_RX_FIFO_FULL |
> +		SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
> +		sprd_rx(irq, port);
> +
> +	if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
> +		sprd_tx(irq, port);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sprd_startup(struct uart_port *port)
> +{
> +	int ret = 0;
> +	unsigned int ien, ctrl1;
> +	unsigned int timeout;
> +	struct sprd_uart_port *sp;
> +
> +	serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
> +
> +	/* clear rx fifo */
> +	timeout = SPRD_TIMEOUT;
> +	while (timeout-- && serial_in(port, SPRD_STS1) & 0x00ff)
> +		serial_in(port, SPRD_RXD);
> +
> +	/* clear tx fifo */
> +	timeout = SPRD_TIMEOUT;
> +	while (timeout-- && serial_in(port, SPRD_STS1) & 0xff00)
> +		cpu_relax();
> +
> +	/* clear interrupt */
> +	serial_out(port, SPRD_IEN, 0x0);
> +	serial_out(port, SPRD_ICLR, ~0);
> +
> +	/* allocate irq */
> +	sp = container_of(port, struct sprd_uart_port, port);
> +	snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
> +	ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
> +				IRQF_SHARED, sp->name, port);
> +	if (ret) {
> +		dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
> +			port->irq, ret);
> +		return ret;
> +	}
> +	ctrl1 = serial_in(port, SPRD_CTL1);
> +	ctrl1 |= 0x3e00 | THLD_RX_FULL;
> +	serial_out(port, SPRD_CTL1, ctrl1);
> +
> +	/* enable interrupt */
> +	spin_lock(&port->lock);
> +	ien = serial_in(port, SPRD_IEN);
> +	ien |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT | SPRD_IEN_TIMEOUT;
> +	serial_out(port, SPRD_IEN, ien);
> +	spin_unlock(&port->lock);
> +
> +	return 0;
> +}
> +
> +static void sprd_shutdown(struct uart_port *port)
> +{
> +	serial_out(port, SPRD_IEN, 0x0);
> +	serial_out(port, SPRD_ICLR, ~0);
> +	devm_free_irq(port->dev, port->irq, port);
> +}
> +
> +static void sprd_set_termios(struct uart_port *port,
> +				    struct ktermios *termios,
> +				    struct ktermios *old)
> +{
> +	unsigned int baud, quot;
> +	unsigned int lcr, fc;
> +
> +	/* ask the core to calculate the divisor for us */
> +	baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
> +
> +	quot = (unsigned int)((port->uartclk + baud / 2) / baud);
> +
> +	/* set data length */
> +	lcr = serial_in(port, SPRD_LCR);
> +	lcr &= ~SPRD_LCR_DATA_LEN;
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS5:
> +		lcr |= SPRD_LCR_DATA_LEN5;
> +		break;
> +	case CS6:
> +		lcr |= SPRD_LCR_DATA_LEN6;
> +		break;
> +	case CS7:
> +		lcr |= SPRD_LCR_DATA_LEN7;
> +		break;
> +	case CS8:
> +	default:
> +		lcr |= SPRD_LCR_DATA_LEN8;
> +		break;
> +	}
> +
> +	/* calculate stop bits */
> +	lcr &= ~(SPRD_LCR_STOP_1BIT | SPRD_LCR_STOP_2BIT);
> +	if (termios->c_cflag & CSTOPB)
> +		lcr |= SPRD_LCR_STOP_2BIT;
> +	else
> +		lcr |= SPRD_LCR_STOP_1BIT;
> +
> +	/* calculate parity */
> +	lcr &= ~SPRD_LCR_PARITY;
> +	termios->c_cflag &= ~CMSPAR;	/* no support mark/space */
> +	if (termios->c_cflag & PARENB) {
> +		lcr |= SPRD_LCR_PARITY_EN;
> +		if (termios->c_cflag & PARODD)
> +			lcr |= SPRD_LCR_ODD_PAR;
> +		else
> +			lcr |= SPRD_LCR_EVEN_PAR;
> +	}
> +
> +	/* change the port state. */
           ^^^^^^^^^^^^^^^^^^^^^^

This means you should be taking the port->lock here... (and disabling
local interrupts if your isr takes the port->lock)


> +	/* update the per-port timeout */
> +	uart_update_timeout(port, termios->c_cflag, baud);
> +
> +	port->read_status_mask = SPRD_LSR_OE;
> +	if (termios->c_iflag & INPCK)
> +		port->read_status_mask |= SPRD_LSR_FE | SPRD_LSR_PE;
> +	if (termios->c_iflag & (BRKINT | PARMRK))
> +		port->read_status_mask |= SPRD_LSR_BI;
> +
> +	/* characters to ignore */
> +	port->ignore_status_mask = 0;
> +	if (termios->c_iflag & IGNPAR)
> +		port->ignore_status_mask |= SPRD_LSR_PE | SPRD_LSR_FE;
> +	if (termios->c_iflag & IGNBRK) {
> +		port->ignore_status_mask |= SPRD_LSR_BI;
> +		/*
> +		 * If we're ignoring parity and break indicators,
> +		 * ignore overruns too (for real raw support).
> +		 */
> +		if (termios->c_iflag & IGNPAR)
> +			port->ignore_status_mask |= SPRD_LSR_OE;
> +	}
> +
> +	/* flow control */
> +	fc = serial_in(port, SPRD_CTL1);
> +	fc &= ~(RX_HW_FLOW_CTL_THLD | RX_HW_FLOW_CTL_EN | TX_HW_FLOW_CTL_EN);
> +	if (termios->c_cflag & CRTSCTS) {
> +		fc |= RX_HW_FLOW_CTL_THLD;
> +		fc |= RX_HW_FLOW_CTL_EN;
> +		fc |= TX_HW_FLOW_CTL_EN;
> +	}
> +
> +	/* clock divider bit0~bit15 */
> +	serial_out(port, SPRD_CLKD0, quot & 0xffff);
> +
> +	/* clock divider bit16~bit20 */
> +	serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
> +	serial_out(port, SPRD_LCR, lcr);
> +	fc |= 0x3e00 | THLD_RX_FULL;
> +	serial_out(port, SPRD_CTL1, fc);

and dropping it here.

> +	/* Don't rewrite B0 */
> +	if (tty_termios_baud_rate(termios))
> +		tty_termios_encode_baud_rate(termios, baud, baud);
> +}
> +
> +static const char *sprd_type(struct uart_port *port)
> +{
> +	return "SPX";
> +}
> +
> +static void sprd_release_port(struct uart_port *port)
> +{
> +	/* nothing to do */
> +}
> +
> +static int sprd_request_port(struct uart_port *port)
> +{
> +	return 0;
> +}
> +
> +static void sprd_config_port(struct uart_port *port, int flags)
> +{
> +	if (flags & UART_CONFIG_TYPE)
> +		port->type = PORT_SPRD;
> +}
> +
> +static int sprd_verify_port(struct uart_port *port,
> +				   struct serial_struct *ser)
> +{
> +	if (ser->type != PORT_SPRD)
> +		return -EINVAL;
> +	if (port->irq != ser->irq)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static struct uart_ops serial_sprd_ops = {
> +	.tx_empty = sprd_tx_empty,
> +	.get_mctrl = sprd_get_mctrl,
> +	.set_mctrl = sprd_set_mctrl,
> +	.stop_tx = sprd_stop_tx,
> +	.start_tx = sprd_start_tx,
> +	.stop_rx = sprd_stop_rx,
> +	.break_ctl = sprd_break_ctl,
> +	.startup = sprd_startup,
> +	.shutdown = sprd_shutdown,
> +	.set_termios = sprd_set_termios,
> +	.type = sprd_type,
> +	.release_port = sprd_release_port,
> +	.request_port = sprd_request_port,
> +	.config_port = sprd_config_port,
> +	.verify_port = sprd_verify_port,
> +};
> +
> +#ifdef CONFIG_SERIAL_SPRD_CONSOLE
> +static inline void wait_for_xmitr(struct uart_port *port)
> +{
> +	unsigned int status, tmout = 10000;
> +
> +	/* wait up to 10ms for the character(s) to be sent */
> +	do {
> +		status = serial_in(port, SPRD_STS1);
> +		if (--tmout == 0)
> +			break;
> +		udelay(1);
> +	} while (status & 0xff00);
> +}
> +
> +static void sprd_console_putchar(struct uart_port *port, int ch)
> +{
> +	wait_for_xmitr(port);
> +	serial_out(port, SPRD_TXD, ch);
> +}
> +
> +static void sprd_console_write(struct console *co, const char *s,
> +				      unsigned int count)
> +{
> +	struct uart_port *port = &sprd_port[co->index]->port;
> +	int ien;
> +	int locked = 1;
> +
> +	if (oops_in_progress)
> +		locked = spin_trylock(&port->lock);
> +	else
> +		spin_lock(&port->lock);

If you do need to take the port->lock in your isr, then you need to
disable local irq here.

> +	/* save the IEN then disable the interrupts */
> +	ien = serial_in(port, SPRD_IEN);
> +	serial_out(port, SPRD_IEN, 0x0);
> +
> +	uart_console_write(port, s, count, sprd_console_putchar);
> +
> +	/* wait for transmitter to become empty and restore the IEN */
> +	wait_for_xmitr(port);
> +	serial_out(port, SPRD_IEN, ien);
> +	if (locked)
> +		spin_unlock(&port->lock);
> +}
> +
> +static int __init sprd_console_setup(struct console *co, char *options)
> +{
> +	struct uart_port *port;
> +	int baud = 115200;
> +	int bits = 8;
> +	int parity = 'n';
> +	int flow = 'n';
> +
> +	if (co->index >= UART_NR_MAX || co->index < 0)
> +		co->index = 0;
> +
> +	port = &sprd_port[co->index]->port;
> +	if (port == NULL) {
> +		pr_info("serial port %d not yet initialized\n", co->index);
> +		return -ENODEV;
> +	}
> +	if (options)
> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> +	return uart_set_options(port, co, baud, parity, bits, flow);
> +}
> +
> +static struct uart_driver sprd_uart_driver;
> +static struct console sprd_console = {
> +	.name = SPRD_TTY_NAME,
> +	.write = sprd_console_write,
> +	.device = uart_console_device,
> +	.setup = sprd_console_setup,
> +	.flags = CON_PRINTBUFFER,
> +	.index = -1,
> +	.data = &sprd_uart_driver,
> +};
> +
> +#define SPRD_CONSOLE	(&sprd_console)
> +
> +/* Support for earlycon */
> +static void sprd_putc(struct uart_port *port, int c)
> +{
> +	unsigned int timeout = SPRD_TIMEOUT;
> +
> +	while (timeout-- &&
> +		   !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
> +		cpu_relax();
> +
> +	writeb(c, port->membase + SPRD_TXD);
> +}
> +
> +static void sprd_early_write(struct console *con, const char *s,
> +				    unsigned n)
> +{
> +	struct earlycon_device *dev = con->data;
> +
> +	uart_console_write(&dev->port, s, n, sprd_putc);
> +}
> +
> +static int __init sprd_early_console_setup(
> +				struct earlycon_device *device,
> +				const char *opt)
> +{
> +	if (!device->port.membase)
> +		return -ENODEV;
> +
> +	device->con->write = sprd_early_write;
> +	return 0;
> +}
> +
> +EARLYCON_DECLARE(sprd_serial, sprd_early_console_setup);
> +OF_EARLYCON_DECLARE(sprd_serial, "sprd,sc9836-uart",
> +		    sprd_early_console_setup);
> +
> +#else /* !CONFIG_SERIAL_SPRD_CONSOLE */
> +#define SPRD_CONSOLE		NULL
> +#endif
> +
> +static struct uart_driver sprd_uart_driver = {
> +	.owner = THIS_MODULE,
> +	.driver_name = "sprd_serial",
> +	.dev_name = SPRD_TTY_NAME,
> +	.major = 0,
> +	.minor = 0,
> +	.nr = UART_NR_MAX,
> +	.cons = SPRD_CONSOLE,
> +};
> +
> +static int sprd_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct uart_port *up;
> +	struct clk *clk;
> +	int irq;
> +
> +	if (np)
> +		pdev->id = of_alias_get_id(np, "serial");
> +
> +	if (pdev->id < 0 || pdev->id >= UART_NR_MAX) {
> +		dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
> +		return -ENXIO;
> +	}
> +
> +	sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
> +		sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
> +	if (!sprd_port[pdev->id])
> +		return -ENOMEM;
> +
> +	up = &sprd_port[pdev->id]->port;
> +	up->dev = &pdev->dev;
> +	up->line = pdev->id;
> +	up->type = PORT_SPRD;
> +	up->iotype = SERIAL_IO_PORT;
> +	up->uartclk = SPRD_DEF_RATE;
> +	up->fifosize = SPRD_FIFO_SIZE;
> +	up->ops = &serial_sprd_ops;
> +	up->flags = ASYNC_BOOT_AUTOCONF;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (!IS_ERR(clk))
> +		up->uartclk = clk_get_rate(clk);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "not provide mem resource\n");
> +		return -ENODEV;
> +	}
> +	up->mapbase = res->start;
> +	up->membase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(up->membase))
> +		return PTR_ERR(up->membase);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "not provide irq resource\n");
> +		return -ENODEV;
> +	}
> +	up->irq = irq;
> +
> +	platform_set_drvdata(pdev, up);
> +
> +	return uart_add_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_remove(struct platform_device *dev)
> +{
> +	struct uart_port *up = platform_get_drvdata(dev);
> +
> +	return uart_remove_one_port(&sprd_uart_driver, up);
> +}
> +
> +static int sprd_suspend(struct device *dev)
> +{
> +	int id = to_platform_device(dev)->id;
> +	struct uart_port *port = &sprd_port[id]->port;
> +	struct reg_backup *reg_bak = &sprd_port[id]->reg_bak;
> +
> +	reg_bak->ien = serial_in(port, SPRD_IEN);
> +	reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
> +	reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
> +	reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
> +	reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
> +	reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);

Why are you saving and restoring these register states
across suspend/resume?

The serial core calls your set_termios() handler upon
resume (either for the console or if a tty is open)
so you should be reprogramming the hardware there
based on the termios settings.

Regards,
Peter Hurley

> +
> +	uart_suspend_port(&sprd_uart_driver, port);
> +
> +	return 0;
> +}
> +
> +static int sprd_resume(struct device *dev)
> +{
> +	int id = to_platform_device(dev)->id;
> +	struct uart_port *port = &sprd_port[id]->port;
> +	struct reg_backup *reg_bak = &sprd_port[id]->reg_bak;
> +
> +	serial_out(port, SPRD_LCR, reg_bak->ctrl0);
> +	serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
> +	serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
> +	serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
> +	serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
> +	serial_out(port, SPRD_IEN, reg_bak->ien);
> +
> +	uart_resume_port(&sprd_uart_driver, port);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id serial_ids[] = {
> +	{.compatible = "sprd,sc9836-uart",},
> +	{}
> +};
> +
> +static SIMPLE_DEV_PM_OPS(sprd_pm_ops, sprd_suspend, sprd_resume);
> +
> +static struct platform_driver sprd_platform_driver = {
> +	.probe		= sprd_probe,
> +	.remove		= sprd_remove,
> +	.driver 	= {
> +		.name	= "sprd_serial",
> +		.of_match_table = of_match_ptr(serial_ids),
> +		.pm	= &sprd_pm_ops,
> +	},
> +};
> +
> +static int __init sprd_serial_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = uart_register_driver(&sprd_uart_driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = platform_driver_register(&sprd_platform_driver);
> +	if (ret)
> +		uart_unregister_driver(&sprd_uart_driver);
> +
> +	return ret;
> +}
> +
> +static void __exit sprd_serial_exit(void)
> +{
> +	platform_driver_unregister(&sprd_platform_driver);
> +	uart_unregister_driver(&sprd_uart_driver);
> +}
> +
> +module_init(sprd_serial_init);
> +module_exit(sprd_serial_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index c172180..7e6eb39 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -248,4 +248,7 @@
>  /* MESON */
>  #define PORT_MESON	109
>  
> +/* SPRD SERIAL  */
> +#define PORT_SPRD	110
> +
>  #endif /* _UAPILINUX_SERIAL_CORE_H */
> 

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Orson Zhai]

Hi, Peter,

Thank you for reviewing our code!
Some discussion below.

On 2015?01?16? 23:20, Peter Hurley wrote:
> On 01/16/2015 05:00 AM, Chunyan Zhang wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>> This patch also replaced the spaces between the macros and their
>> values with the tabs in serial_core.h
> The locking doesn't look correct. Specific notations below.
>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
>> ---
>>   drivers/tty/serial/Kconfig       |   18 +
>>   drivers/tty/serial/Makefile      |    1 +
>>   drivers/tty/serial/sprd_serial.c |  772 ++++++++++++++++++++++++++++++++++++++
>>   include/uapi/linux/serial_core.h |    3 +
>>   4 files changed, 794 insertions(+)
>>   create mode 100644 drivers/tty/serial/sprd_serial.c
>>
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index c79b43c..969d3cd 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135
>>   	  This driver can also be build as a module. If so, the module will be called
>>   	  men_z135_uart.ko
>>   
>> +config SERIAL_SPRD
>> +	tristate "Support for SPRD serial"
>> +	depends on ARCH_SPRD
>> +	select SERIAL_CORE
>> +	help
>> +	  This enables the driver for the Spreadtrum's serial.
>> +
>> +config SERIAL_SPRD_CONSOLE
>> +	bool "SPRD UART console support"
>> +	depends on SERIAL_SPRD=y
>> +	select SERIAL_CORE_CONSOLE
>> +	select SERIAL_EARLYCON
>> +	help
>> +	  Support for early debug console using Spreadtrum's serial. This enables
>> +	  the console before standard serial driver is probed. This is enabled
>> +	  with "earlycon" on the kernel command line. The console is
>> +	  enabled when early_param is processed.
>> +
>>   endmenu
>>   
>>   config SERIAL_MCTRL_GPIO
>> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
>> index 9a548ac..4801aca 100644
>> --- a/drivers/tty/serial/Makefile
>> +++ b/drivers/tty/serial/Makefile
>> @@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)	+= arc_uart.o
>>   obj-$(CONFIG_SERIAL_RP2)	+= rp2.o
>>   obj-$(CONFIG_SERIAL_FSL_LPUART)	+= fsl_lpuart.o
>>   obj-$(CONFIG_SERIAL_MEN_Z135)	+= men_z135_uart.o
>> +obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
>>   
>>   # GPIOLIB helpers for modem control lines
>>   obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
>> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
>> new file mode 100644
>> index 0000000..81839e4
>> --- /dev/null
>> +++ b/drivers/tty/serial/sprd_serial.c
>> @@ -0,0 +1,772 @@
>> +/*
>> + * Copyright (C) 2012 Spreadtrum Communications Inc.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/console.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +
>> +/* device name */
>> +#define UART_NR_MAX		8
>> +#define SPRD_TTY_NAME		"ttySPX"
>> +#define SPRD_FIFO_SIZE		128
>> +#define SPRD_DEF_RATE		26000000
>> +#define SPRD_TIMEOUT		2048
>> +
>> +/* the offset of serial registers and BITs for them */
>> +/* data registers */
>> +#define SPRD_TXD		0x0000
>> +#define SPRD_RXD		0x0004
>> +
>> +/* line status register and its BITs  */
>> +#define SPRD_LSR		0x0008
>> +#define SPRD_LSR_OE		BIT(4)
>> +#define SPRD_LSR_FE		BIT(3)
>> +#define SPRD_LSR_PE		BIT(2)
>> +#define SPRD_LSR_BI		BIT(7)
>> +#define SPRD_LSR_TX_OVER	BIT(15)
>> +
>> +/* data number in TX and RX fifo */
>> +#define SPRD_STS1		0x000C
>> +
>> +/* interrupt enable register and its BITs */
>> +#define SPRD_IEN		0x0010
>> +#define SPRD_IEN_RX_FULL	BIT(0)
>> +#define SPRD_IEN_TX_EMPTY	BIT(1)
>> +#define SPRD_IEN_BREAK_DETECT	BIT(7)
>> +#define SPRD_IEN_TIMEOUT	BIT(13)
>> +
>> +/* interrupt clear register */
>> +#define SPRD_ICLR		0x0014
>> +
>> +/* line control register */
>> +#define SPRD_LCR		0x0018
>> +#define SPRD_LCR_STOP_1BIT	0x10
>> +#define SPRD_LCR_STOP_2BIT	0x30
>> +#define SPRD_LCR_DATA_LEN	(BIT(2) | BIT(3))
>> +#define SPRD_LCR_DATA_LEN5	0x0
>> +#define SPRD_LCR_DATA_LEN6	0x4
>> +#define SPRD_LCR_DATA_LEN7	0x8
>> +#define SPRD_LCR_DATA_LEN8	0xc
>> +#define SPRD_LCR_PARITY		(BIT(0) | BIT(1))
>> +#define SPRD_LCR_PARITY_EN	0x2
>> +#define SPRD_LCR_EVEN_PAR	0x0
>> +#define SPRD_LCR_ODD_PAR	0x1
>> +
>> +/* control register 1 */
>> +#define SPRD_CTL1		0x001C
>> +#define RX_HW_FLOW_CTL_THLD	BIT(6)
>> +#define RX_HW_FLOW_CTL_EN	BIT(7)
>> +#define TX_HW_FLOW_CTL_EN	BIT(8)
>> +
>> +/* fifo threshold register */
>> +#define SPRD_CTL2		0x0020
>> +#define THLD_TX_EMPTY		0x40
>> +#define THLD_RX_FULL		0x40
>> +
>> +/* config baud rate register */
>> +#define SPRD_CLKD0		0x0024
>> +#define SPRD_CLKD1		0x0028
>> +
>> +/* interrupt mask status register */
>> +#define SPRD_IMSR		0x002C
>> +#define SPRD_IMSR_RX_FIFO_FULL	BIT(0)
>> +#define SPRD_IMSR_TX_FIFO_EMPTY	BIT(1)
>> +#define SPRD_IMSR_BREAK_DETECT	BIT(7)
>> +#define SPRD_IMSR_TIMEOUT	BIT(13)
>> +
>> +struct reg_backup {
>> +	uint32_t ien;
>> +	uint32_t ctrl0;
>> +	uint32_t ctrl1;
>> +	uint32_t ctrl2;
>> +	uint32_t clkd0;
>> +	uint32_t clkd1;
>> +	uint32_t dspwait;
>> +};
>> +
>> +struct sprd_uart_port {
>> +	struct uart_port port;
>> +	struct reg_backup reg_bak;
>> +	char name[16];
>> +};
>> +
>> +static struct sprd_uart_port *sprd_port[UART_NR_MAX] = { NULL };
>> +
>> +static inline unsigned int serial_in(struct uart_port *port, int offset)
>> +{
>> +	return readl_relaxed(port->membase + offset);
>> +}
>> +
>> +static inline void serial_out(struct uart_port *port, int offset, int value)
>> +{
>> +	writel_relaxed(value, port->membase + offset);
>> +}
>> +
>> +static unsigned int sprd_tx_empty(struct uart_port *port)
>> +{
>> +	if (serial_in(port, SPRD_STS1) & 0xff00)
>> +		return 0;
>> +	else
>> +		return TIOCSER_TEMT;
>> +}
>> +
>> +static unsigned int sprd_get_mctrl(struct uart_port *port)
>> +{
>> +	return TIOCM_DSR | TIOCM_CTS;
>> +}
>> +
>> +static void sprd_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> +{
>> +	/* nothing to do */
>> +}
>> +
>> +static void sprd_stop_tx(struct uart_port *port)
>> +{
>> +	unsigned int ien, iclr;
>> +
>> +	iclr = serial_in(port, SPRD_ICLR);
>> +	ien = serial_in(port, SPRD_IEN);
>> +
>> +	iclr |= SPRD_IEN_TX_EMPTY;
>> +	ien &= ~SPRD_IEN_TX_EMPTY;
>> +
>> +	serial_out(port, SPRD_ICLR, iclr);
>> +	serial_out(port, SPRD_IEN, ien);
>> +}
>> +
>> +static void sprd_start_tx(struct uart_port *port)
>> +{
>> +	unsigned int ien;
>> +
>> +	ien = serial_in(port, SPRD_IEN);
>> +	if (!(ien & SPRD_IEN_TX_EMPTY)) {
>> +		ien |= SPRD_IEN_TX_EMPTY;
>> +		serial_out(port, SPRD_IEN, ien);
>> +	}
>> +}
>> +
>> +static void sprd_stop_rx(struct uart_port *port)
>> +{
>> +	unsigned int ien, iclr;
>> +
>> +	iclr = serial_in(port, SPRD_ICLR);
>> +	ien = serial_in(port, SPRD_IEN);
>> +
>> +	ien &= ~(SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT);
>> +	iclr |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT;
>> +
>> +	serial_out(port, SPRD_IEN, ien);
>> +	serial_out(port, SPRD_ICLR, iclr);
>> +}
>> +
>> +/* The Sprd serial does not support this function. */
>> +static void sprd_break_ctl(struct uart_port *port, int break_state)
>> +{
>> +	/* nothing to do */
>> +}
>> +
>> +static inline int handle_lsr_errors(struct uart_port *port,
>> +				    unsigned int *flag,
>> +				    unsigned int *lsr)
>> +{
>> +	int ret = 0;
>> +
>> +	/* statistics */
>> +	if (*lsr & SPRD_LSR_BI) {
>> +		*lsr &= ~(SPRD_LSR_FE | SPRD_LSR_PE);
>> +		port->icount.brk++;
>> +		ret = uart_handle_break(port);
>> +		if (ret)
>> +			return ret;
>> +	} else if (*lsr & SPRD_LSR_PE)
>> +		port->icount.parity++;
>> +	else if (*lsr & SPRD_LSR_FE)
>> +		port->icount.frame++;
>> +	if (*lsr & SPRD_LSR_OE)
>> +		port->icount.overrun++;
>> +
>> +	/* mask off conditions which should be ignored */
>> +	*lsr &= port->read_status_mask;
>> +	if (*lsr & SPRD_LSR_BI)
>> +		*flag = TTY_BREAK;
>> +	else if (*lsr & SPRD_LSR_PE)
>> +		*flag = TTY_PARITY;
>> +	else if (*lsr & SPRD_LSR_FE)
>> +		*flag = TTY_FRAME;
>> +
>> +	return ret;
>> +}
>> +
>> +static inline void sprd_rx(int irq, void *dev_id)
>> +{
>> +	struct uart_port *port = dev_id;
>> +	struct tty_port *tty = &port->state->port;
>> +	unsigned int ch, flag, lsr, max_count = SPRD_TIMEOUT;
>> +
>> +	while ((serial_in(port, SPRD_STS1) & 0x00ff) && max_count--) {
>> +		lsr = serial_in(port, SPRD_LSR);
>> +		ch = serial_in(port, SPRD_RXD);
>> +		flag = TTY_NORMAL;
>> +		port->icount.rx++;
>> +
>> +		if (lsr & (SPRD_LSR_BI | SPRD_LSR_PE
>> +			| SPRD_LSR_FE | SPRD_LSR_OE))
>> +			if (handle_lsr_errors(port, &lsr, &flag))
>> +				continue;
>> +		if (uart_handle_sysrq_char(port, ch))
>> +			continue;
>> +
>> +		uart_insert_char(port, lsr, SPRD_LSR_OE, ch, flag);
>> +	}
>> +
>> +	tty_flip_buffer_push(tty);
>> +}
>> +
>> +static inline void sprd_tx(int irq, void *dev_id)
>> +{
>> +	struct uart_port *port = dev_id;
>> +	struct circ_buf *xmit = &port->state->xmit;
>> +	int count;
>> +
>> +	if (port->x_char) {
>> +		serial_out(port, SPRD_TXD, port->x_char);
>> +		port->icount.tx++;
>> +		port->x_char = 0;
>> +		return;
>> +	}
>> +
>> +	if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
>> +		sprd_stop_tx(port);
>> +		return;
>> +	}
>> +
>> +	count = THLD_TX_EMPTY;
>> +	do {
>> +		serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
>> +		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>> +		port->icount.tx++;
>> +		if (uart_circ_empty(xmit))
>> +			break;
>> +	} while (--count > 0);
>> +
>> +	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>> +		uart_write_wakeup(port);
>> +
>> +	if (uart_circ_empty(xmit))
>> +		sprd_stop_tx(port);
>> +}
>> +
>> +/* this handles the interrupt from one port */
>> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
>> +{
>> +	struct uart_port *port = (struct uart_port *)dev_id;
>> +	unsigned int ims;
> Why does your isr not have to take port->lock ?

The original consideration is the registers are accessed by isr only.
Interrupt will not be nested because of gic chip driver protection.
So there is not other thread will race on it.
Does this make sense?

>
>> +	ims = serial_in(port, SPRD_IMSR);
>> +
>> +	if (!ims)
>> +		return IRQ_NONE;
>> +
>> +	serial_out(port, SPRD_ICLR, ~0);
>> +
>> +	if (ims & (SPRD_IMSR_RX_FIFO_FULL |
>> +		SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
>> +		sprd_rx(irq, port);
>> +
>> +	if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
>> +		sprd_tx(irq, port);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int sprd_startup(struct uart_port *port)
>> +{
>> +	int ret = 0;
>> +	unsigned int ien, ctrl1;
>> +	unsigned int timeout;
>> +	struct sprd_uart_port *sp;
>> +
>> +	serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
>> +
>> +	/* clear rx fifo */
>> +	timeout = SPRD_TIMEOUT;
>> +	while (timeout-- && serial_in(port, SPRD_STS1) & 0x00ff)
>> +		serial_in(port, SPRD_RXD);
>> +
>> +	/* clear tx fifo */
>> +	timeout = SPRD_TIMEOUT;
>> +	while (timeout-- && serial_in(port, SPRD_STS1) & 0xff00)
>> +		cpu_relax();
>> +
>> +	/* clear interrupt */
>> +	serial_out(port, SPRD_IEN, 0x0);
>> +	serial_out(port, SPRD_ICLR, ~0);
>> +
>> +	/* allocate irq */
>> +	sp = container_of(port, struct sprd_uart_port, port);
>> +	snprintf(sp->name, sizeof(sp->name), "sprd_serial%d", port->line);
>> +	ret = devm_request_irq(port->dev, port->irq, sprd_handle_irq,
>> +				IRQF_SHARED, sp->name, port);
>> +	if (ret) {
>> +		dev_err(port->dev, "fail to request serial irq %d, ret=%d\n",
>> +			port->irq, ret);
>> +		return ret;
>> +	}
>> +	ctrl1 = serial_in(port, SPRD_CTL1);
>> +	ctrl1 |= 0x3e00 | THLD_RX_FULL;
>> +	serial_out(port, SPRD_CTL1, ctrl1);
>> +
>> +	/* enable interrupt */
>> +	spin_lock(&port->lock);
>> +	ien = serial_in(port, SPRD_IEN);
>> +	ien |= SPRD_IEN_RX_FULL | SPRD_IEN_BREAK_DETECT | SPRD_IEN_TIMEOUT;
>> +	serial_out(port, SPRD_IEN, ien);
>> +	spin_unlock(&port->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static void sprd_shutdown(struct uart_port *port)
>> +{
>> +	serial_out(port, SPRD_IEN, 0x0);
>> +	serial_out(port, SPRD_ICLR, ~0);
>> +	devm_free_irq(port->dev, port->irq, port);
>> +}
>> +
>> +static void sprd_set_termios(struct uart_port *port,
>> +				    struct ktermios *termios,
>> +				    struct ktermios *old)
>> +{
>> +	unsigned int baud, quot;
>> +	unsigned int lcr, fc;
>> +
>> +	/* ask the core to calculate the divisor for us */
>> +	baud = uart_get_baud_rate(port, termios, old, 1200, 3000000);
>> +
>> +	quot = (unsigned int)((port->uartclk + baud / 2) / baud);
>> +
>> +	/* set data length */
>> +	lcr = serial_in(port, SPRD_LCR);
>> +	lcr &= ~SPRD_LCR_DATA_LEN;
>> +	switch (termios->c_cflag & CSIZE) {
>> +	case CS5:
>> +		lcr |= SPRD_LCR_DATA_LEN5;
>> +		break;
>> +	case CS6:
>> +		lcr |= SPRD_LCR_DATA_LEN6;
>> +		break;
>> +	case CS7:
>> +		lcr |= SPRD_LCR_DATA_LEN7;
>> +		break;
>> +	case CS8:
>> +	default:
>> +		lcr |= SPRD_LCR_DATA_LEN8;
>> +		break;
>> +	}
>> +
>> +	/* calculate stop bits */
>> +	lcr &= ~(SPRD_LCR_STOP_1BIT | SPRD_LCR_STOP_2BIT);
>> +	if (termios->c_cflag & CSTOPB)
>> +		lcr |= SPRD_LCR_STOP_2BIT;
>> +	else
>> +		lcr |= SPRD_LCR_STOP_1BIT;
>> +
>> +	/* calculate parity */
>> +	lcr &= ~SPRD_LCR_PARITY;
>> +	termios->c_cflag &= ~CMSPAR;	/* no support mark/space */
>> +	if (termios->c_cflag & PARENB) {
>> +		lcr |= SPRD_LCR_PARITY_EN;
>> +		if (termios->c_cflag & PARODD)
>> +			lcr |= SPRD_LCR_ODD_PAR;
>> +		else
>> +			lcr |= SPRD_LCR_EVEN_PAR;
>> +	}
>> +
>> +	/* change the port state. */
>             ^^^^^^^^^^^^^^^^^^^^^^
>
> This means you should be taking the port->lock here... (and disabling
> local interrupts if your isr takes the port->lock)

Yes, I think you are right.


>
>> +	/* update the per-port timeout */
>> +	uart_update_timeout(port, termios->c_cflag, baud);
>> +
>> +	port->read_status_mask = SPRD_LSR_OE;
>> +	if (termios->c_iflag & INPCK)
>> +		port->read_status_mask |= SPRD_LSR_FE | SPRD_LSR_PE;
>> +	if (termios->c_iflag & (BRKINT | PARMRK))
>> +		port->read_status_mask |= SPRD_LSR_BI;
>> +
>> +	/* characters to ignore */
>> +	port->ignore_status_mask = 0;
>> +	if (termios->c_iflag & IGNPAR)
>> +		port->ignore_status_mask |= SPRD_LSR_PE | SPRD_LSR_FE;
>> +	if (termios->c_iflag & IGNBRK) {
>> +		port->ignore_status_mask |= SPRD_LSR_BI;
>> +		/*
>> +		 * If we're ignoring parity and break indicators,
>> +		 * ignore overruns too (for real raw support).
>> +		 */
>> +		if (termios->c_iflag & IGNPAR)
>> +			port->ignore_status_mask |= SPRD_LSR_OE;
>> +	}
>> +
>> +	/* flow control */
>> +	fc = serial_in(port, SPRD_CTL1);
>> +	fc &= ~(RX_HW_FLOW_CTL_THLD | RX_HW_FLOW_CTL_EN | TX_HW_FLOW_CTL_EN);
>> +	if (termios->c_cflag & CRTSCTS) {
>> +		fc |= RX_HW_FLOW_CTL_THLD;
>> +		fc |= RX_HW_FLOW_CTL_EN;
>> +		fc |= TX_HW_FLOW_CTL_EN;
>> +	}
>> +
>> +	/* clock divider bit0~bit15 */
>> +	serial_out(port, SPRD_CLKD0, quot & 0xffff);
>> +
>> +	/* clock divider bit16~bit20 */
>> +	serial_out(port, SPRD_CLKD1, (quot & 0x1f0000) >> 16);
>> +	serial_out(port, SPRD_LCR, lcr);
>> +	fc |= 0x3e00 | THLD_RX_FULL;
>> +	serial_out(port, SPRD_CTL1, fc);
> and dropping it here.
>
>> +	/* Don't rewrite B0 */
>> +	if (tty_termios_baud_rate(termios))
>> +		tty_termios_encode_baud_rate(termios, baud, baud);
>> +}
>> +
>> +static const char *sprd_type(struct uart_port *port)
>> +{
>> +	return "SPX";
>> +}
>> +
>> +static void sprd_release_port(struct uart_port *port)
>> +{
>> +	/* nothing to do */
>> +}
>> +
>> +static int sprd_request_port(struct uart_port *port)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void sprd_config_port(struct uart_port *port, int flags)
>> +{
>> +	if (flags & UART_CONFIG_TYPE)
>> +		port->type = PORT_SPRD;
>> +}
>> +
>> +static int sprd_verify_port(struct uart_port *port,
>> +				   struct serial_struct *ser)
>> +{
>> +	if (ser->type != PORT_SPRD)
>> +		return -EINVAL;
>> +	if (port->irq != ser->irq)
>> +		return -EINVAL;
>> +	return 0;
>> +}
>> +
>> +static struct uart_ops serial_sprd_ops = {
>> +	.tx_empty = sprd_tx_empty,
>> +	.get_mctrl = sprd_get_mctrl,
>> +	.set_mctrl = sprd_set_mctrl,
>> +	.stop_tx = sprd_stop_tx,
>> +	.start_tx = sprd_start_tx,
>> +	.stop_rx = sprd_stop_rx,
>> +	.break_ctl = sprd_break_ctl,
>> +	.startup = sprd_startup,
>> +	.shutdown = sprd_shutdown,
>> +	.set_termios = sprd_set_termios,
>> +	.type = sprd_type,
>> +	.release_port = sprd_release_port,
>> +	.request_port = sprd_request_port,
>> +	.config_port = sprd_config_port,
>> +	.verify_port = sprd_verify_port,
>> +};
>> +
>> +#ifdef CONFIG_SERIAL_SPRD_CONSOLE
>> +static inline void wait_for_xmitr(struct uart_port *port)
>> +{
>> +	unsigned int status, tmout = 10000;
>> +
>> +	/* wait up to 10ms for the character(s) to be sent */
>> +	do {
>> +		status = serial_in(port, SPRD_STS1);
>> +		if (--tmout == 0)
>> +			break;
>> +		udelay(1);
>> +	} while (status & 0xff00);
>> +}
>> +
>> +static void sprd_console_putchar(struct uart_port *port, int ch)
>> +{
>> +	wait_for_xmitr(port);
>> +	serial_out(port, SPRD_TXD, ch);
>> +}
>> +
>> +static void sprd_console_write(struct console *co, const char *s,
>> +				      unsigned int count)
>> +{
>> +	struct uart_port *port = &sprd_port[co->index]->port;
>> +	int ien;
>> +	int locked = 1;
>> +
>> +	if (oops_in_progress)
>> +		locked = spin_trylock(&port->lock);
>> +	else
>> +		spin_lock(&port->lock);
> If you do need to take the port->lock in your isr, then you need to
> disable local irq here.

You mean to use spin_lock_irqsave()?

We do disable irq below....

>
>> +	/* save the IEN then disable the interrupts */
>> +	ien = serial_in(port, SPRD_IEN);
>> +	serial_out(port, SPRD_IEN, 0x0);

Here, we disable port IEN register.

>> +
>> +	uart_console_write(port, s, count, sprd_console_putchar);
>> +
>> +	/* wait for transmitter to become empty and restore the IEN */
>> +	wait_for_xmitr(port);
>> +	serial_out(port, SPRD_IEN, ien);
>> +	if (locked)
>> +		spin_unlock(&port->lock);
>> +}
>> +
>> +static int __init sprd_console_setup(struct console *co, char *options)
>> +{
>> +	struct uart_port *port;
>> +	int baud = 115200;
>> +	int bits = 8;
>> +	int parity = 'n';
>> +	int flow = 'n';
>> +
>> +	if (co->index >= UART_NR_MAX || co->index < 0)
>> +		co->index = 0;
>> +
>> +	port = &sprd_port[co->index]->port;
>> +	if (port == NULL) {
>> +		pr_info("serial port %d not yet initialized\n", co->index);
>> +		return -ENODEV;
>> +	}
>> +	if (options)
>> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
>> +
>> +	return uart_set_options(port, co, baud, parity, bits, flow);
>> +}
>> +
>> +static struct uart_driver sprd_uart_driver;
>> +static struct console sprd_console = {
>> +	.name = SPRD_TTY_NAME,
>> +	.write = sprd_console_write,
>> +	.device = uart_console_device,
>> +	.setup = sprd_console_setup,
>> +	.flags = CON_PRINTBUFFER,
>> +	.index = -1,
>> +	.data = &sprd_uart_driver,
>> +};
>> +
>> +#define SPRD_CONSOLE	(&sprd_console)
>> +
>> +/* Support for earlycon */
>> +static void sprd_putc(struct uart_port *port, int c)
>> +{
>> +	unsigned int timeout = SPRD_TIMEOUT;
>> +
>> +	while (timeout-- &&
>> +		   !(readl(port->membase + SPRD_LSR) & SPRD_LSR_TX_OVER))
>> +		cpu_relax();
>> +
>> +	writeb(c, port->membase + SPRD_TXD);
>> +}
>> +
>> +static void sprd_early_write(struct console *con, const char *s,
>> +				    unsigned n)
>> +{
>> +	struct earlycon_device *dev = con->data;
>> +
>> +	uart_console_write(&dev->port, s, n, sprd_putc);
>> +}
>> +
>> +static int __init sprd_early_console_setup(
>> +				struct earlycon_device *device,
>> +				const char *opt)
>> +{
>> +	if (!device->port.membase)
>> +		return -ENODEV;
>> +
>> +	device->con->write = sprd_early_write;
>> +	return 0;
>> +}
>> +
>> +EARLYCON_DECLARE(sprd_serial, sprd_early_console_setup);
>> +OF_EARLYCON_DECLARE(sprd_serial, "sprd,sc9836-uart",
>> +		    sprd_early_console_setup);
>> +
>> +#else /* !CONFIG_SERIAL_SPRD_CONSOLE */
>> +#define SPRD_CONSOLE		NULL
>> +#endif
>> +
>> +static struct uart_driver sprd_uart_driver = {
>> +	.owner = THIS_MODULE,
>> +	.driver_name = "sprd_serial",
>> +	.dev_name = SPRD_TTY_NAME,
>> +	.major = 0,
>> +	.minor = 0,
>> +	.nr = UART_NR_MAX,
>> +	.cons = SPRD_CONSOLE,
>> +};
>> +
>> +static int sprd_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct uart_port *up;
>> +	struct clk *clk;
>> +	int irq;
>> +
>> +	if (np)
>> +		pdev->id = of_alias_get_id(np, "serial");
>> +
>> +	if (pdev->id < 0 || pdev->id >= UART_NR_MAX) {
>> +		dev_err(&pdev->dev, "does not support id %d\n", pdev->id);
>> +		return -ENXIO;
>> +	}
>> +
>> +	sprd_port[pdev->id] = devm_kzalloc(&pdev->dev,
>> +		sizeof(*sprd_port[pdev->id]), GFP_KERNEL);
>> +	if (!sprd_port[pdev->id])
>> +		return -ENOMEM;
>> +
>> +	up = &sprd_port[pdev->id]->port;
>> +	up->dev = &pdev->dev;
>> +	up->line = pdev->id;
>> +	up->type = PORT_SPRD;
>> +	up->iotype = SERIAL_IO_PORT;
>> +	up->uartclk = SPRD_DEF_RATE;
>> +	up->fifosize = SPRD_FIFO_SIZE;
>> +	up->ops = &serial_sprd_ops;
>> +	up->flags = ASYNC_BOOT_AUTOCONF;
>> +
>> +	clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (!IS_ERR(clk))
>> +		up->uartclk = clk_get_rate(clk);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "not provide mem resource\n");
>> +		return -ENODEV;
>> +	}
>> +	up->mapbase = res->start;
>> +	up->membase = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(up->membase))
>> +		return PTR_ERR(up->membase);
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev, "not provide irq resource\n");
>> +		return -ENODEV;
>> +	}
>> +	up->irq = irq;
>> +
>> +	platform_set_drvdata(pdev, up);
>> +
>> +	return uart_add_one_port(&sprd_uart_driver, up);
>> +}
>> +
>> +static int sprd_remove(struct platform_device *dev)
>> +{
>> +	struct uart_port *up = platform_get_drvdata(dev);
>> +
>> +	return uart_remove_one_port(&sprd_uart_driver, up);
>> +}
>> +
>> +static int sprd_suspend(struct device *dev)
>> +{
>> +	int id = to_platform_device(dev)->id;
>> +	struct uart_port *port = &sprd_port[id]->port;
>> +	struct reg_backup *reg_bak = &sprd_port[id]->reg_bak;
>> +
>> +	reg_bak->ien = serial_in(port, SPRD_IEN);
>> +	reg_bak->ctrl0 = serial_in(port, SPRD_LCR);
>> +	reg_bak->ctrl1 = serial_in(port, SPRD_CTL1);
>> +	reg_bak->ctrl2 = serial_in(port, SPRD_CTL2);
>> +	reg_bak->clkd0 = serial_in(port, SPRD_CLKD0);
>> +	reg_bak->clkd1 = serial_in(port, SPRD_CLKD1);
> Why are you saving and restoring these register states
> across suspend/resume?
>
> The serial core calls your set_termios() handler upon
> resume (either for the console or if a tty is open)
> so you should be reprogramming the hardware there
> based on the termios settings.

We'll try to address it in V6.

Thanks,
Orson


>
> Regards,
> Peter Hurley
>> +
>> +	uart_suspend_port(&sprd_uart_driver, port);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sprd_resume(struct device *dev)
>> +{
>> +	int id = to_platform_device(dev)->id;
>> +	struct uart_port *port = &sprd_port[id]->port;
>> +	struct reg_backup *reg_bak = &sprd_port[id]->reg_bak;
>> +
>> +	serial_out(port, SPRD_LCR, reg_bak->ctrl0);
>> +	serial_out(port, SPRD_CTL1, reg_bak->ctrl1);
>> +	serial_out(port, SPRD_CTL2, reg_bak->ctrl2);
>> +	serial_out(port, SPRD_CLKD0, reg_bak->clkd0);
>> +	serial_out(port, SPRD_CLKD1, reg_bak->clkd1);
>> +	serial_out(port, SPRD_IEN, reg_bak->ien);
>> +
>> +	uart_resume_port(&sprd_uart_driver, port);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id serial_ids[] = {
>> +	{.compatible = "sprd,sc9836-uart",},
>> +	{}
>> +};
>> +
>> +static SIMPLE_DEV_PM_OPS(sprd_pm_ops, sprd_suspend, sprd_resume);
>> +
>> +static struct platform_driver sprd_platform_driver = {
>> +	.probe		= sprd_probe,
>> +	.remove		= sprd_remove,
>> +	.driver 	= {
>> +		.name	= "sprd_serial",
>> +		.of_match_table = of_match_ptr(serial_ids),
>> +		.pm	= &sprd_pm_ops,
>> +	},
>> +};
>> +
>> +static int __init sprd_serial_init(void)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = uart_register_driver(&sprd_uart_driver);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = platform_driver_register(&sprd_platform_driver);
>> +	if (ret)
>> +		uart_unregister_driver(&sprd_uart_driver);
>> +
>> +	return ret;
>> +}
>> +
>> +static void __exit sprd_serial_exit(void)
>> +{
>> +	platform_driver_unregister(&sprd_platform_driver);
>> +	uart_unregister_driver(&sprd_uart_driver);
>> +}
>> +
>> +module_init(sprd_serial_init);
>> +module_exit(sprd_serial_exit);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");
>> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
>> index c172180..7e6eb39 100644
>> --- a/include/uapi/linux/serial_core.h
>> +++ b/include/uapi/linux/serial_core.h
>> @@ -248,4 +248,7 @@
>>   /* MESON */
>>   #define PORT_MESON	109
>>   
>> +/* SPRD SERIAL  */
>> +#define PORT_SPRD	110
>> +
>>   #endif /* _UAPILINUX_SERIAL_CORE_H */
>>

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Peter Hurley]

On 01/20/2015 07:11 AM, Orson Zhai wrote:
> Hi, Peter,
> 
> Thank you for reviewing our code!
> Some discussion below.
> 
> On 2015?01?16? 23:20, Peter Hurley wrote:
>> On 01/16/2015 05:00 AM, Chunyan Zhang wrote:
>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>> spreadtrum sharkl64 platform.
>>> This driver also support earlycon.
>>> This patch also replaced the spaces between the macros and their
>>> values with the tabs in serial_core.h
>> The locking doesn't look correct. Specific notations below.

>>> +static inline void sprd_tx(int irq, void *dev_id)
>>> +{
>>> +    struct uart_port *port = dev_id;
>>> +    struct circ_buf *xmit = &port->state->xmit;
>>> +    int count;
>>> +
>>> +    if (port->x_char) {
>>> +        serial_out(port, SPRD_TXD, port->x_char);
>>> +        port->icount.tx++;
>>> +        port->x_char = 0;
>>> +        return;
>>> +    }
>>> +
>>> +    if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
>>> +        sprd_stop_tx(port);
>>> +        return;
>>> +    }
>>> +
>>> +    count = THLD_TX_EMPTY;
>>> +    do {
>>> +        serial_out(port, SPRD_TXD, xmit->buf[xmit->tail]);
>>> +        xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>>> +        port->icount.tx++;
>>> +        if (uart_circ_empty(xmit))
>>> +            break;
>>> +    } while (--count > 0);
>>> +
>>> +    if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>>> +        uart_write_wakeup(port);
>>> +
>>> +    if (uart_circ_empty(xmit))
>>> +        sprd_stop_tx(port);
>>> +}
>>> +
>>> +/* this handles the interrupt from one port */
>>> +static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
>>> +{
>>> +    struct uart_port *port = (struct uart_port *)dev_id;
>>> +    unsigned int ims;
>> Why does your isr not have to take port->lock ?
> 
> The original consideration is the registers are accessed by isr only.
> Interrupt will not be nested because of gic chip driver protection.
> So there is not other thread will race on it.
> Does this make sense?

The xmit->buf[] and its indexes could be accessed concurrently.
For example,

CPU 0                                      | CPU 1
                                           |
sprd_handle_irq                            | uart_flush_buffer
  sprd_tx                                  |   spin_lock_irqsave
    ...                                    |
    count = 64                             |
    do {                                   |     xmit->tail = 0
      serial_out(xmit->buf[xmit->tail])    |

      whoops - what byte did this just output?

I'm sure there's many more possible races, perhaps with worse
outcomes than just 1 bad byte output.


>>
>>> +    ims = serial_in(port, SPRD_IMSR);
>>> +
>>> +    if (!ims)
>>> +        return IRQ_NONE;
>>> +
>>> +    serial_out(port, SPRD_ICLR, ~0);
>>> +
>>> +    if (ims & (SPRD_IMSR_RX_FIFO_FULL |
>>> +        SPRD_IMSR_BREAK_DETECT | SPRD_IMSR_TIMEOUT))
>>> +        sprd_rx(irq, port);
>>> +
>>> +    if (ims & SPRD_IMSR_TX_FIFO_EMPTY)
>>> +        sprd_tx(irq, port);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}

[...]

>>> +static void sprd_console_putchar(struct uart_port *port, int ch)
>>> +{
>>> +    wait_for_xmitr(port);
>>> +    serial_out(port, SPRD_TXD, ch);
>>> +}
>>> +
>>> +static void sprd_console_write(struct console *co, const char *s,
>>> +                      unsigned int count)
>>> +{
>>> +    struct uart_port *port = &sprd_port[co->index]->port;
>>> +    int ien;
>>> +    int locked = 1;
>>> +
>>> +    if (oops_in_progress)
>>> +        locked = spin_trylock(&port->lock);
>>> +    else
>>> +        spin_lock(&port->lock);
>> If you do need to take the port->lock in your isr, then you need to
>> disable local irq here.
> 
> You mean to use spin_lock_irqsave()?
> 
> We do disable irq below....

But not before an irq could happen with the spin_lock already taken.

printk
  ...
  sprd_console_write
    spin_lock
       <IRQ>
       sprd_handle_irq
         spin_lock

    ** DEADLOCK **

[
  Note: some drivers assume that console->write() will always be
  called with local interrupts disabled. This is a bad idea and I
  have warned those driver authors when this has come up before.
]

Also, since you handle sysrq in your isr the above needs to check
for non-zero port->sysrq and _not_ attempt the spinlock because
the isr will already have it; for example,

<IRQ>
sprd_handle_irq
  spin_lock
    sprd_rx
      ...
      uart_handle_syrq_char
        handle_sysrq
          __handle_sysrq
            printk
              ...
              sprd_console_write
                spin_lock

    ** DEADLOCK **

Regards,
Peter Hurley

>>
>>> +    /* save the IEN then disable the interrupts */
>>> +    ien = serial_in(port, SPRD_IEN);
>>> +    serial_out(port, SPRD_IEN, 0x0);
> 
> Here, we disable port IEN register.
> 
>>> +
>>> +    uart_console_write(port, s, count, sprd_console_putchar);
>>> +
>>> +    /* wait for transmitter to become empty and restore the IEN */
>>> +    wait_for_xmitr(port);
>>> +    serial_out(port, SPRD_IEN, ien);
>>> +    if (locked)
>>> +        spin_unlock(&port->lock);
>>> +}

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Rob Herring]

On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
<chunyan.zhang@spreadtrum.com> wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
> This patch also replaced the spaces between the macros and their
> values with the tabs in serial_core.h
>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> ---
>  drivers/tty/serial/Kconfig       |   18 +
>  drivers/tty/serial/Makefile      |    1 +
>  drivers/tty/serial/sprd_serial.c |  772 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/serial_core.h |    3 +
>  4 files changed, 794 insertions(+)
>  create mode 100644 drivers/tty/serial/sprd_serial.c
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index c79b43c..969d3cd 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135
>           This driver can also be build as a module. If so, the module will be called
>           men_z135_uart.ko
>
> +config SERIAL_SPRD
> +       tristate "Support for SPRD serial"

Can the menu text spell out Spreadtrum. What SPRD means is not obvious.

> +       depends on ARCH_SPRD
> +       select SERIAL_CORE
> +       help
> +         This enables the driver for the Spreadtrum's serial.
> +
> +config SERIAL_SPRD_CONSOLE
> +       bool "SPRD UART console support"

Same here.

> +       depends on SERIAL_SPRD=y
> +       select SERIAL_CORE_CONSOLE
> +       select SERIAL_EARLYCON
> +       help
> +         Support for early debug console using Spreadtrum's serial. This enables
> +         the console before standard serial driver is probed. This is enabled
> +         with "earlycon" on the kernel command line. The console is
> +         enabled when early_param is processed.
> +
>  endmenu
>
>  config SERIAL_MCTRL_GPIO
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 9a548ac..4801aca 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)      += arc_uart.o
>  obj-$(CONFIG_SERIAL_RP2)       += rp2.o
>  obj-$(CONFIG_SERIAL_FSL_LPUART)        += fsl_lpuart.o
>  obj-$(CONFIG_SERIAL_MEN_Z135)  += men_z135_uart.o
> +obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
>
>  # GPIOLIB helpers for modem control lines
>  obj-$(CONFIG_SERIAL_MCTRL_GPIO)        += serial_mctrl_gpio.o
> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> new file mode 100644
> index 0000000..81839e4
> --- /dev/null
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -0,0 +1,772 @@
> +/*
> + * Copyright (C) 2012 Spreadtrum Communications Inc.

This is unchanged in 3 years?

> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/console.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +/* device name */
> +#define UART_NR_MAX            8
> +#define SPRD_TTY_NAME          "ttySPX"

We really want to get away from per SOC serial names and use ttyS for
serial port /dev names. There's issues switching existing drivers
because this creates an ABI, so we can only have new drivers follow
this.

[...]

> +static struct platform_driver sprd_platform_driver = {
> +       .probe          = sprd_probe,
> +       .remove         = sprd_remove,
> +       .driver         = {
> +               .name   = "sprd_serial",
> +               .of_match_table = of_match_ptr(serial_ids),
> +               .pm     = &sprd_pm_ops,
> +       },
> +};
> +
> +static int __init sprd_serial_init(void)
> +{
> +       int ret = 0;
> +
> +       ret = uart_register_driver(&sprd_uart_driver);

This can be done in probe now. Then you can use module_platform_driver().

Rob

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Lyra Zhang]

On Sat, Jan 17, 2015 at 12:41 AM, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
> <chunyan.zhang@spreadtrum.com> wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>> This patch also replaced the spaces between the macros and their
>> values with the tabs in serial_core.h
>>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
>> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
>> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
>> ---
>>  drivers/tty/serial/Kconfig       |   18 +
>>  drivers/tty/serial/Makefile      |    1 +
>>  drivers/tty/serial/sprd_serial.c |  772 ++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/serial_core.h |    3 +
>>  4 files changed, 794 insertions(+)
>>  create mode 100644 drivers/tty/serial/sprd_serial.c
>>
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index c79b43c..969d3cd 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -1577,6 +1577,24 @@ config SERIAL_MEN_Z135
>>           This driver can also be build as a module. If so, the module will be called
>>           men_z135_uart.ko
>>
>> +config SERIAL_SPRD
>> +       tristate "Support for SPRD serial"
>
> Can the menu text spell out Spreadtrum. What SPRD means is not obvious.
>

Ok, I'll address it.

>> +       depends on ARCH_SPRD
>> +       select SERIAL_CORE
>> +       help
>> +         This enables the driver for the Spreadtrum's serial.
>> +
>> +config SERIAL_SPRD_CONSOLE
>> +       bool "SPRD UART console support"
>
> Same here.
>
>> +       depends on SERIAL_SPRD=y
>> +       select SERIAL_CORE_CONSOLE
>> +       select SERIAL_EARLYCON
>> +       help
>> +         Support for early debug console using Spreadtrum's serial. This enables
>> +         the console before standard serial driver is probed. This is enabled
>> +         with "earlycon" on the kernel command line. The console is
>> +         enabled when early_param is processed.
>> +
>>  endmenu
>>
>>  config SERIAL_MCTRL_GPIO
>> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
>> index 9a548ac..4801aca 100644
>> --- a/drivers/tty/serial/Makefile
>> +++ b/drivers/tty/serial/Makefile
>> @@ -93,6 +93,7 @@ obj-$(CONFIG_SERIAL_ARC)      += arc_uart.o
>>  obj-$(CONFIG_SERIAL_RP2)       += rp2.o
>>  obj-$(CONFIG_SERIAL_FSL_LPUART)        += fsl_lpuart.o
>>  obj-$(CONFIG_SERIAL_MEN_Z135)  += men_z135_uart.o
>> +obj-$(CONFIG_SERIAL_SPRD) += sprd_serial.o
>>
>>  # GPIOLIB helpers for modem control lines
>>  obj-$(CONFIG_SERIAL_MCTRL_GPIO)        += serial_mctrl_gpio.o
>> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
>> new file mode 100644
>> index 0000000..81839e4
>> --- /dev/null
>> +++ b/drivers/tty/serial/sprd_serial.c
>> @@ -0,0 +1,772 @@
>> +/*
>> + * Copyright (C) 2012 Spreadtrum Communications Inc.
>
> This is unchanged in 3 years?
>

ok, I'll change it to 2012-2015.

>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/console.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +
>> +/* device name */
>> +#define UART_NR_MAX            8
>> +#define SPRD_TTY_NAME          "ttySPX"
>
> We really want to get away from per SOC serial names and use ttyS for
> serial port /dev names. There's issues switching existing drivers
> because this creates an ABI, so we can only have new drivers follow
> this.
>

ok. i see. I'll convert it to ttyS in the next version.

> [...]
>
>> +static struct platform_driver sprd_platform_driver = {
>> +       .probe          = sprd_probe,
>> +       .remove         = sprd_remove,
>> +       .driver         = {
>> +               .name   = "sprd_serial",
>> +               .of_match_table = of_match_ptr(serial_ids),
>> +               .pm     = &sprd_pm_ops,
>> +       },
>> +};
>> +
>> +static int __init sprd_serial_init(void)
>> +{
>> +       int ret = 0;
>> +
>> +       ret = uart_register_driver(&sprd_uart_driver);
>
> This can be done in probe now. Then you can use module_platform_driver().
>

Question:
1. there are 4 uart ports configured in dt for sprd_serial, so probe
will be called 4 times, but uart_register_driver only needs to be
called one time, so can we use uart_driver.state to check if
uart_register_driver has already been called ?

2. if I use module_platform_driver() instead of
module_init(sprd_serial_init)  and  module_exit(sprd_serial_exit) , I
must move uart_unregister_driver() which is now processed in
sprd_serial_exit() to sprd_remove(), there is a similar problem with
probe(), sprd_remove() will also be called 4 times, and actually it
should be called only one time. How can we deal with this case?

3. for the second question, we can check the platform_device->id, if
it is equal to the index of last port (e.g. 4 for this case), then
uart_unregister_driver() can be called. Does it work correctly? since
for this case, we must keep the order of releasing ports.


Thanks,
Chunyan


> Rob

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Rob Herring]

On Mon, Jan 19, 2015 at 3:55 AM, Lyra Zhang <zhang.lyra@gmail.com> wrote:
> On Sat, Jan 17, 2015 at 12:41 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
>> <chunyan.zhang@spreadtrum.com> wrote:
>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>> spreadtrum sharkl64 platform.
>>> This driver also support earlycon.
>>> This patch also replaced the spaces between the macros and their
>>> values with the tabs in serial_core.h

[...]

>>> +static int __init sprd_serial_init(void)
>>> +{
>>> +       int ret = 0;
>>> +
>>> +       ret = uart_register_driver(&sprd_uart_driver);
>>
>> This can be done in probe now. Then you can use module_platform_driver().
>>
>
> Question:
> 1. there are 4 uart ports configured in dt for sprd_serial, so probe
> will be called 4 times, but uart_register_driver only needs to be
> called one time, so can we use uart_driver.state to check if
> uart_register_driver has already been called ?

Yes.

> 2. if I use module_platform_driver() instead of
> module_init(sprd_serial_init)  and  module_exit(sprd_serial_exit) , I
> must move uart_unregister_driver() which is now processed in
> sprd_serial_exit() to sprd_remove(), there is a similar problem with
> probe(), sprd_remove() will also be called 4 times, and actually it
> should be called only one time. How can we deal with this case?

Look at pl01x or Samsung UART drivers which have done this conversion.

> 3. for the second question, we can check the platform_device->id, if
> it is equal to the index of last port (e.g. 4 for this case), then
> uart_unregister_driver() can be called. Does it work correctly? since
> for this case, we must keep the order of releasing ports.

The id will not be the line index in the DT case. I don't think you
can guarantee the order either.

It would be better to make uart_{un}register_driver deal with being
called multiple times so drivers don't have to deal with getting this
correct. I'm not sure if that is feasible though.

Rob

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Lyra Zhang]

Hi, Rob

I still have a question to be conform, specific describes below:

On Mon, Jan 19, 2015 at 10:11 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Mon, Jan 19, 2015 at 3:55 AM, Lyra Zhang <zhang.lyra@gmail.com> wrote:
>> On Sat, Jan 17, 2015 at 12:41 AM, Rob Herring <robherring2@gmail.com> wrote:
>>> On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
>>> <chunyan.zhang@spreadtrum.com> wrote:
>>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>>> spreadtrum sharkl64 platform.
>>>> This driver also support earlycon.
>>>> This patch also replaced the spaces between the macros and their
>>>> values with the tabs in serial_core.h
>
> [...]
>
>>>> +static int __init sprd_serial_init(void)
>>>> +{
>>>> +       int ret = 0;
>>>> +
>>>> +       ret = uart_register_driver(&sprd_uart_driver);
>>>
>>> This can be done in probe now. Then you can use module_platform_driver().
>>>
>>
>> Question:
>> 1. there are 4 uart ports configured in dt for sprd_serial, so probe
>> will be called 4 times, but uart_register_driver only needs to be
>> called one time, so can we use uart_driver.state to check if
>> uart_register_driver has already been called ?
>
> Yes.
>
>> 2. if I use module_platform_driver() instead of
>> module_init(sprd_serial_init)  and  module_exit(sprd_serial_exit) , I
>> must move uart_unregister_driver() which is now processed in
>> sprd_serial_exit() to sprd_remove(), there is a similar problem with
>> probe(), sprd_remove() will also be called 4 times, and actually it
>> should be called only one time. How can we deal with this case?
>
> Look at pl01x or Samsung UART drivers which have done this conversion.

Samsung UART does use module_platform_driver, but pl010/pl011 doesn't.
In the Samsung UART driver, uart_unregister_driver is processed in
remove(), like below:

static int s3c24xx_serial_remove(struct platform_device *dev)
{
    struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);

    if (port) {
        s3c24xx_serial_cpufreq_deregister(to_ourport(port));
        uart_remove_one_port(&s3c24xx_uart_drv, port);
    }

    uart_unregister_driver(&s3c24xx_uart_drv);
}

if this serial has more than one ports, uart_unregister_driver() must
be called multiple times when the device need to be removed.
I think there may be a problem because that uart_unregister_driver()
will do kfree(drv->state) every time when it's called.

Thanks,
Chunyan

>
>> 3. for the second question, we can check the platform_device->id, if
>> it is equal to the index of last port (e.g. 4 for this case), then
>> uart_unregister_driver() can be called. Does it work correctly? since
>> for this case, we must keep the order of releasing ports.
>
> The id will not be the line index in the DT case. I don't think you
> can guarantee the order either.
>
> It would be better to make uart_{un}register_driver deal with being
> called multiple times so drivers don't have to deal with getting this
> correct. I'm not sure if that is feasible though.
>
> Rob

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Orson Zhai]

On Tue, Jan 20, 2015 at 3:37 PM, Lyra Zhang <zhang.lyra@gmail.com> wrote:
> Hi, Rob
>
> I still have a question to be conform, specific describes below:
>
> On Mon, Jan 19, 2015 at 10:11 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Mon, Jan 19, 2015 at 3:55 AM, Lyra Zhang <zhang.lyra@gmail.com> wrote:
>>> On Sat, Jan 17, 2015 at 12:41 AM, Rob Herring <robherring2@gmail.com> wrote:
>>>> On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
>>>> <chunyan.zhang@spreadtrum.com> wrote:
>>>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>>>> spreadtrum sharkl64 platform.
>>>>> This driver also support earlycon.
>>>>> This patch also replaced the spaces between the macros and their
>>>>> values with the tabs in serial_core.h
>>
>> [...]
>>
>>>>> +static int __init sprd_serial_init(void)
>>>>> +{
>>>>> +       int ret = 0;
>>>>> +
>>>>> +       ret = uart_register_driver(&sprd_uart_driver);
>>>>
>>>> This can be done in probe now. Then you can use module_platform_driver().
>>>>
>>>
>>> Question:
>>> 1. there are 4 uart ports configured in dt for sprd_serial, so probe
>>> will be called 4 times, but uart_register_driver only needs to be
>>> called one time, so can we use uart_driver.state to check if
>>> uart_register_driver has already been called ?
>>
>> Yes.
>>
>>> 2. if I use module_platform_driver() instead of
>>> module_init(sprd_serial_init)  and  module_exit(sprd_serial_exit) , I
>>> must move uart_unregister_driver() which is now processed in
>>> sprd_serial_exit() to sprd_remove(), there is a similar problem with
>>> probe(), sprd_remove() will also be called 4 times, and actually it
>>> should be called only one time. How can we deal with this case?
>>
>> Look at pl01x or Samsung UART drivers which have done this conversion.
>
> Samsung UART does use module_platform_driver, but pl010/pl011 doesn't.
> In the Samsung UART driver, uart_unregister_driver is processed in
> remove(), like below:
>
> static int s3c24xx_serial_remove(struct platform_device *dev)
> {
>     struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>
>     if (port) {
>         s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>         uart_remove_one_port(&s3c24xx_uart_drv, port);
>     }
>
>     uart_unregister_driver(&s3c24xx_uart_drv);
> }
>
> if this serial has more than one ports, uart_unregister_driver() must
> be called multiple times when the device need to be removed.
> I think there may be a problem because that uart_unregister_driver()
> will do kfree(drv->state) every time when it's called.

I think it is no appropriate to call uart_unregister_driver() at first
port removing.
The drv->state buffer was shared with all uart ports.
If there are some cases that only 1 port is needed to be removed, that
will destroy all others, isn't it?

Regards,
Orson


>
> Thanks,
> Chunyan
>
>>
>>> 3. for the second question, we can check the platform_device->id, if
>>> it is equal to the index of last port (e.g. 4 for this case), then
>>> uart_unregister_driver() can be called. Does it work correctly? since
>>> for this case, we must keep the order of releasing ports.
>>
>> The id will not be the line index in the DT case. I don't think you
>> can guarantee the order either.
>>
>> It would be better to make uart_{un}register_driver deal with being
>> called multiple times so drivers don't have to deal with getting this
>> correct. I'm not sure if that is feasible though.
>>
>> Rob

[PATCH v5 5/5] tty/serial: Add Spreadtrum sc9836-uart driver support

[Rob Herring]

On Tue, Jan 20, 2015 at 1:37 AM, Lyra Zhang <zhang.lyra@gmail.com> wrote:
> Hi, Rob
>
> I still have a question to be conform, specific describes below:
>
> On Mon, Jan 19, 2015 at 10:11 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Mon, Jan 19, 2015 at 3:55 AM, Lyra Zhang <zhang.lyra@gmail.com> wrote:
>>> On Sat, Jan 17, 2015 at 12:41 AM, Rob Herring <robherring2@gmail.com> wrote:
>>>> On Fri, Jan 16, 2015 at 4:00 AM, Chunyan Zhang
>>>> <chunyan.zhang@spreadtrum.com> wrote:
>>>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>>>> spreadtrum sharkl64 platform.
>>>>> This driver also support earlycon.
>>>>> This patch also replaced the spaces between the macros and their
>>>>> values with the tabs in serial_core.h
>>
>> [...]
>>
>>>>> +static int __init sprd_serial_init(void)
>>>>> +{
>>>>> +       int ret = 0;
>>>>> +
>>>>> +       ret = uart_register_driver(&sprd_uart_driver);
>>>>
>>>> This can be done in probe now. Then you can use module_platform_driver().
>>>>
>>>
>>> Question:
>>> 1. there are 4 uart ports configured in dt for sprd_serial, so probe
>>> will be called 4 times, but uart_register_driver only needs to be
>>> called one time, so can we use uart_driver.state to check if
>>> uart_register_driver has already been called ?
>>
>> Yes.
>>
>>> 2. if I use module_platform_driver() instead of
>>> module_init(sprd_serial_init)  and  module_exit(sprd_serial_exit) , I
>>> must move uart_unregister_driver() which is now processed in
>>> sprd_serial_exit() to sprd_remove(), there is a similar problem with
>>> probe(), sprd_remove() will also be called 4 times, and actually it
>>> should be called only one time. How can we deal with this case?
>>
>> Look at pl01x or Samsung UART drivers which have done this conversion.
>
> Samsung UART does use module_platform_driver, but pl010/pl011 doesn't.

That is because pl011 is an amba_device rather than a platform_device
and there is not an equivalent macro for this boilerplate (although I
think one has been posted recently).

> In the Samsung UART driver, uart_unregister_driver is processed in
> remove(), like below:
>
> static int s3c24xx_serial_remove(struct platform_device *dev)
> {
>     struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>
>     if (port) {
>         s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>         uart_remove_one_port(&s3c24xx_uart_drv, port);
>     }
>
>     uart_unregister_driver(&s3c24xx_uart_drv);
> }
>
> if this serial has more than one ports, uart_unregister_driver() must
> be called multiple times when the device need to be removed.
> I think there may be a problem because that uart_unregister_driver()
> will do kfree(drv->state) every time when it's called.

Yes. Looks like a bug still in the Samsung driver. So follow what the
pl01x driver does. It had the same problem, but there is a follow-up
commit to fix it.

Rob