Add Spreadtrum Sharkl64 Platform support

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 recommended by Arnd, see:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html

Signed-off-by Chunyan Zhang <[email protected]>

---
 .../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%)

Documentation/devicetree/bindings/serial/8250.txt renamed from Documentation/devicetree/bindings/serial/of-serial.txt +0 -0

similarity index 100%
rename from Documentation/devicetree/bindings/serial/of-serial.txt
rename to Documentation/devicetree/bindings/serial/8250.txt

Signature · 1 line

1.7.9.5

Quoted 6 lines

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 recommended by Arnd, see:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/291455.html

Signed-off-by Chunyan Zhang <[email protected]>

My prior Ack [1] still applies (given you've just changed the way the patch was generated, as Arnd and I suggested).

Mark.

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

Quoted 10 lines

---
 .../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%)

Documentation/devicetree/bindings/serial/8250.txt renamed from Documentation/devicetree/bindings/serial/of-serial.txt +0 -1

similarity index 100%
rename from Documentation/devicetree/bindings/serial/of-serial.txt
rename to Documentation/devicetree/bindings/serial/8250.txt
--

Quoted 3 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Zhizhou Zhang <[email protected]>

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 <[email protected]>

Signed-off-by Chunyan Zhang <[email protected]>

Signed-off-by Orson Zhai <[email protected]>

---
 arch/arm64/boot/dts/Makefile                  |    1 +
 arch/arm64/boot/dts/sprd-sc9836-openphone.dts |   85 ++++++++++++++++++++
 arch/arm64/boot/dts/sprd-sc9836.dtsi          |  103 ++++++++++++++++++++++++
 arch/arm64/boot/dts/sprd-sharkl64.dtsi        |  105 +++++++++++++++++++++++++
 4 files changed, 294 insertions(+)
 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

arch/arm64/boot/dts/Makefile +1 -0

index f8001a6..d0aff8a 100644
@@ -1,4 +1,5 @@
 dtb-$(CONFIG_ARCH_THUNDER) += thunder-88xx.dtb
+dtb-$(CONFIG_ARCH_SHARKL64) += sprd-sc9836-openphone.dtb
 dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb
 dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb

arch/arm64/boot/dts/sprd-sc9836.dtsi line 101

Quoted 8 lines · Chunyan Zhang

arch/arm64/boot/dts/sprd-sc9836.dtsi new file +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

arch/arm64/boot/dts/sprd-sc9836.dtsi new file +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

Quoted 12 lines · Mark Rutland

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

How rude would this be?

drivers/clocksource/arm_arch_timer.c +0 -0

index 2133f9d59d06..aaaf3433ccb9 100644

Quoted 30 lines · Catalin Marinas

Quoted 12 lines · Mark Rutland

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

How rude would this be?

drivers/clocksource/arm_arch_timer.c +0 -0

index 2133f9d59d06..aaaf3433ccb9 100644
@@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 		return;
 

drivers/clocksource/arm_arch_timer.c arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) · line 378

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c +2 -1

 	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	if (IS_ENABLED(CONFIG_ARM64) ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else

Probably too rude, given it doesn't WARN() the user.

We should be extremely loud if we see the clock-frequency property on an arm64 system. Whether or not we should ignore the property is another matter.

Quoted 31 lines · Mark Rutland

Quoted 30 lines · Catalin Marinas

Quoted 12 lines · Mark Rutland

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

How rude would this be?

drivers/clocksource/arm_arch_timer.c +0 -0

index 2133f9d59d06..aaaf3433ccb9 100644
@@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 		return;
 

Quoted 10 lines

drivers/clocksource/arm_arch_timer.c arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) · line 378

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c +2 -1

 	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	if (IS_ENABLED(CONFIG_ARM64) ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else

Probably too rude, given it doesn't WARN() the user.

We override broken hardware ID registers all the time in device-tree without dumping stack. Why is this any different?

Quoted 43 lines · Will Deacon

Quoted 41 lines · Mark Rutland

Quoted 29 lines · Catalin Marinas

Quoted 12 lines · Mark Rutland

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

How rude would this be?

drivers/clocksource/arm_arch_timer.c +0 -0

index 2133f9d59d06..aaaf3433ccb9 100644

drivers/clocksource/arm_arch_timer.c arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) · line 378

Quoted 9 lines

drivers/clocksource/arm_arch_timer.c +2 -1

@@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 		return;
 
 	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	if (IS_ENABLED(CONFIG_ARM64) ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else

Probably too rude, given it doesn't WARN() the user.

Quoted 2 lines

We override broken hardware ID registers all the time in device-tree without dumping stack. Why is this any different?

Exposure to guests via KVM, and the fact that it's possible to write to the register from EL3. This isn't so much broken HW (which cannot be fixed) as broken FW (which can be fixed).

Printing the warning gives people the chance to realise and fix the issue during bringup.

Quoted 1 line · Mark Rutland

Probably too rude, given it doesn't WARN() the user.

We override broken hardware ID registers all the time in device-tree without dumping stack. Why is this any different?

Quoted 3 lines

We should be extremely loud if we see the clock-frequency property on an arm64 system. Whether or not we should ignore the property is another matter.

I don't really see the point in ignoring it. We will see broken hardware
[1] and this is just preventing ourselves from working around it. I'd much rather have arch-timers with a "clock-frequence" property than have some other timer instead because the kernel driver is being stubborn.

Quoted 31 lines · Mark Rutland

Quoted 30 lines · Catalin Marinas

Quoted 12 lines · Mark Rutland

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

How rude would this be?

drivers/clocksource/arm_arch_timer.c +0 -0

index 2133f9d59d06..aaaf3433ccb9 100644
@@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 		return;
 

Quoted 10 lines

drivers/clocksource/arm_arch_timer.c arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) · line 378

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c +2 -1

 	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	if (IS_ENABLED(CONFIG_ARM64) ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else

Probably too rude, given it doesn't WARN() the user.

We override broken hardware ID registers all the time in device-tree without dumping stack. Why is this any different?

Quoted 3 lines

We should be extremely loud if we see the clock-frequency property on an arm64 system. Whether or not we should ignore the property is another matter.

I don't really see the point in ignoring it. We will see broken hardware
[1] and this is just preventing ourselves from working around it. I'd much rather have arch-timers with a "clock-frequence" property than have some other timer instead because the kernel driver is being stubborn.

[1] A previous version of the Juno firmware, for example.

Quoted 43 lines · Will Deacon

Quoted 31 lines · Mark Rutland

Quoted 30 lines · Catalin Marinas

Quoted 12 lines · Mark Rutland

Quoted 11 lines · Catalin Marinas

Quoted 10 lines · Mark Rutland

Quoted 8 lines · Chunyan Zhang

(quoted diff) +8 -0

+
+       timer {
+               compatible = "arm,armv8-timer";
+               interrupts = <1 13 0xff01>,
+                            <1 14 0xff01>,
+                            <1 11 0xff01>,
+                            <1 10 0xff01>;
+               clock-frequency = <26000000>;

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Please remove the clock-frequency property. Your FW should initialise CNTFRQ_EL0 on all CPUs (certainly PSCI 0.2 requires that you do this).

Since this comes up regularly, I think we need a dev_warn() in the arch timer driver when CONFIG_ARM64.

I'll ack such a patch ;)

How rude would this be?

drivers/clocksource/arm_arch_timer.c +0 -0

index 2133f9d59d06..aaaf3433ccb9 100644
@@ -371,7 +371,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 		return;
 

Quoted 10 lines

drivers/clocksource/arm_arch_timer.c arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) · line 378

Quoted 8 lines

drivers/clocksource/arm_arch_timer.c +2 -1

 	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	if (IS_ENABLED(CONFIG_ARM64) ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else

Probably too rude, given it doesn't WARN() the user.

Quoted 2 lines

We override broken hardware ID registers all the time in device-tree without dumping stack. Why is this any different?

I'm not for dumping the stack, it's not relevant (just more noise).

Quoted 8 lines

Quoted 3 lines

We should be extremely loud if we see the clock-frequency property on an arm64 system. Whether or not we should ignore the property is another matter.

I don't really see the point in ignoring it. We will see broken hardware
[1] and this is just preventing ourselves from working around it. I'd much rather have arch-timers with a "clock-frequence" property than have some other timer instead because the kernel driver is being stubborn.

I agree that sooner or later we'll need a workaround (we already did for Juno). My point is that many consider such overriding behaviour to be the default - i.e. don't bother writing any sane value in CNTFRQ in firmware at boot because Linux can cope without. It gets worse when companies develop their firmware long before starting to upstream kernel patches, so too late to fix it.

Quoted 1 line

[1] A previous version of the Juno firmware, for example.

What about CONFIG_BROKEN_FIRMWARE, default off?

[...]

Quoted 2 lines

In the meantime I think we can be more tolerant:

drivers/clocksource/arm_arch_timer.c arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np) · line 381

Quoted 11 lines

drivers/clocksource/arm_arch_timer.c +2 -0

index 2133f9d59d06..87f67a93fcc7 100644
@@ -376,6 +376,8 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else
 			arch_timer_rate = arch_timer_get_cntfrq();
+	} else if (IS_ENABLED(CONFIG_ARM64)) {
+		pr_warn("Architected timer frequency overridden by DT (broken firmware?)\n");
 	}

That looks good to me. On a related note, it looks like my documentation patch [1] from a while back fell by the wayside. Does anyone fancy picking that up to go with this?

Quoted 7 lines

arch/arm64/boot/dts/sprd-sharkl64.dtsi line 104

Quoted 7 lines

arch/arm64/boot/dts/sprd-sharkl64.dtsi new file +7 -0

+       clocks {
+               clk26mhz: clk26mhz {
+                       compatible = "fixed-clock";
+                       #clock-cells = <0>;
+                       clock-frequency = <26000000>;
+               };
+       };

Get rid of the "clocks" container node. There's no need for it.

Thanks,
Mark.

arch/arm64/boot/dts/sprd-sc9836.dtsi line 88

Quoted 10 lines

arch/arm64/boot/dts/sprd-sc9836.dtsi new file +9 -0

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

I believe GICC should be 8KiB here.

arch/arm64/boot/dts/sprd-sc9836.dtsi line 93

Quoted 5 lines

arch/arm64/boot/dts/sprd-sc9836.dtsi new file +5 -0

+
+       psci {
+               compatible = "arm,psci-0.2";
+               method = "smc";
+       };

Do you have a complete PSCI 0.2 implementation (e.g. are all the mandatory functions implemented)?

I take it CPUs enter the kernel at EL2?

How have you tested this?

arch/arm64/Kconfig +7 -0

+menuconfig ARCH_SPRD
+       bool "Spreadtrum SoC platform"
+       depends on ARM64
+       help
+         Support for Spreadtrum ARM based SoCs
+
+if ARCH_SPRD

Quoted 8 lines

arch/arm64/Kconfig +7 -0

+menuconfig ARCH_SPRD
+       bool "Spreadtrum SoC platform"
+       depends on ARM64
+       help
+         Support for Spreadtrum ARM based SoCs
+
+if ARCH_SPRD

arch/arm64/Kconfig config ARCH_THUNDER · line 166

Quoted 10 lines

arch/arm64/Kconfig +10 -0

+
+config ARCH_SHARKL64
+       bool "Sharkl64 SoC Platform"
+       help
+         Sharkl64 is a Spreadtrum's SoC Platform which is based
+         on ARM 64-bit processor core including
+           sc9836
+
+endif #ARCH_SPRD
+

I don't think we need multiple levels here, it should be enough to have either ARCH_SPRD or ARCH_SHARKL64, because all device drivers are going to be optional anyway. Typically a Kconfig symbol covers all SoCs that are related, so if you Spreadtrum are doing both phone and server chips and these are designed independently, you would have two symbols, but if you only expect to see phone chips here that are all derived from the same product line, using ARCH_SPRD to refer to all of them should be enough.

Arnd

For now, we have only one platform(Sharkl64) based on ARM64 been submitted, but we're intending to add support for more our platforms based on ARM64 or ARM32 in the future. There are many common devices on these platforms, such as serial. Our idea would be that if we had a 'menuconfig ARCH_SPRD' in the Kconfig, these common devices only need to depend on ARCH_SPRD in the respective Kconfig, otherwise they may depend on a few Kconfig symbols for every platforms which include these common devices.

So, do you think whether we should define a menuconfig(ARCH_SPRD) in the Kconfig for this case ?

Quoted 8 lines

arch/arm64/Kconfig +7 -0

+menuconfig ARCH_SPRD
+       bool "Spreadtrum SoC platform"
+       depends on ARM64
+       help
+         Support for Spreadtrum ARM based SoCs
+
+if ARCH_SPRD

arch/arm64/Kconfig config ARCH_THUNDER · line 166

Quoted 10 lines

arch/arm64/Kconfig +10 -0

+
+config ARCH_SHARKL64
+       bool "Sharkl64 SoC Platform"
+       help
+         Sharkl64 is a Spreadtrum's SoC Platform which is based
+         on ARM 64-bit processor core including
+           sc9836
+
+endif #ARCH_SPRD
+

I don't think we need multiple levels here, it should be enough to have either ARCH_SPRD or ARCH_SHARKL64, because all device drivers are going to be optional anyway. Typically a Kconfig symbol covers all SoCs that are related, so if you Spreadtrum are doing both phone and server chips and these are designed independently, you would have two symbols, but if you only expect to see phone chips here that are all derived from the same product line, using ARCH_SPRD to refer to all of them should be enough.

Arnd

Add a full sc9836-uart driver for SC9836 SoC which is based on the spreadtrum sharkl64 platform.
This driver also support earlycon.

Signed-off-by Chunyan Zhang <[email protected]>

Signed-off-by Orson Zhai <[email protected]>

Originally-by Lanqing Liu <[email protected]>

drivers/tty/serial/sprd_serial.c new file +19 -0

+
+	ret = platform_driver_register(&sprd_platform_driver);
+	if (unlikely(ret != 0))
+		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");
+MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");

include/uapi/linux/serial_core.h +0 -0

index 16ad852..d9a8c88 100644

Quoted 7 lines

Add a full sc9836-uart driver for SC9836 SoC which is based on the spreadtrum sharkl64 platform.
This driver also support earlycon.

Signed-off-by Chunyan Zhang <[email protected]>

Signed-off-by Orson Zhai <[email protected]>

Originally-by Lanqing Liu <[email protected]>

Some objections:

drivers/tty/serial/sprd_serial.c line 722

Quoted 8 lines

drivers/tty/serial/sprd_serial.c new file +8 -0

+static struct platform_driver sprd_platform_driver = {
+     .probe = sprd_probe,
+     .remove = sprd_remove,
+     .suspend = sprd_suspend,
+     .resume = sprd_resume,
+     .driver = {
+                .name = "sprd_serial",
+                .owner = THIS_MODULE,

platform drivers don't need .owner in them anymore, it's handled by the driver core automatically.

drivers/tty/serial/sprd_serial.c line 733

Quoted 12 lines

drivers/tty/serial/sprd_serial.c new file +11 -0

+                .of_match_table = of_match_ptr(serial_ids),
+                },
+};
+
+static int __init sprd_serial_init(void)
+{
+     int ret = 0;
+
+     ret = uart_register_driver(&sprd_uart_driver);
+     if (unlikely(ret != 0))
+             return ret;

NEVER use unlikely unless you can measure the difference without it. And even then, you better be able to justify it. For something as dumb as this type of check, youare working against the complier and cpu which already knows that 0 is the usual response.

So please remove all usages of likely/unlikely in this patch series.

Quoted 27 lines

drivers/tty/serial/sprd_serial.c new file +19 -0

+
+     ret = platform_driver_register(&sprd_platform_driver);
+     if (unlikely(ret != 0))
+             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");
+MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");

include/uapi/linux/serial_core.h +0 -0

index 16ad852..d9a8c88 100644
@@ -247,4 +247,7 @@
 /* MESON */
 #define PORT_MESON   109
 

include/uapi/linux/serial_core.h line 251

Quoted 2 lines

include/uapi/linux/serial_core.h +2 -0

+/* SPRD SERIAL  */
+#define PORT_SPRD   110

Please use a tab character here.

Add a full sc9836-uart driver for SC9836 SoC which is based on the spreadtrum sharkl64 platform.
This driver also support earlycon.

Signed-off-by Chunyan Zhang <[email protected]>

Signed-off-by Orson Zhai <[email protected]>

Originally-by Lanqing Liu <[email protected]>

---
 Documentation/devices.txt        |    3 +
 drivers/tty/serial/Kconfig       |   23 ++
 drivers/tty/serial/Makefile      |    1 +
 drivers/tty/serial/sprd_serial.c |  752 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |    3 +
 5 files changed, 782 insertions(+)
 create mode 100644 drivers/tty/serial/sprd_serial.c

drivers/tty/serial/Kconfig +14 -0

@@ -1573,6 +1573,29 @@ 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_NR
+        int "Maximum number of sprd serial ports"
+        depends on SERIAL_SPRD
+        default "4"
+
+config SERIAL_SPRD_CONSOLE
+        bool "SPRD UART console support"

drivers/tty/serial/sprd_serial.c new file +0 -0

new file mode 100644
index 0000000..58214c8

drivers/tty/serial/sprd_serial.c new file +22 -0

+{
+	int ret = 0;
+
+	/* stastics */
+	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)

drivers/tty/serial/sprd_serial.c new file +21 -0

+	struct tty_port *tty = &port->state->port;
+	unsigned int ch, flag, lsr, max_count = 2048;
+
+	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 (unlikely(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);
+}

drivers/tty/serial/sprd_serial.c new file +22 -0

+	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)
+{

drivers/tty/serial/sprd_serial.c new file +7 -0

+		return -ENODEV;
+	}
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+	return uart_set_options(port, co, baud, parity, bits, flow);
+}

drivers/tty/serial/sprd_serial.c new file +33 -0

+static int sprd_probe(struct platform_device *pdev)
+{
+	struct resource *mem;
+	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 (unlikely(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 = (struct uart_port *)sprd_port[pdev->id];
+	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);

drivers/tty/serial/sprd_serial.c new file +30 -0

+
+	irq = platform_get_irq(pdev, 0);
+	if (unlikely(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 platform_device *dev, pm_message_t state)
+{
+	int id = dev->id;
+	struct uart_port *port = (struct uart_port *)sprd_port[id];
+	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);

drivers/tty/serial/sprd_serial.c new file +11 -0

+	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);
+
+	return 0;
+}

Quoted 2 lines

---

Quoted 8 lines

drivers/tty/serial/Kconfig +8 -0

+
+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

drivers/tty/serial/Kconfig config SERIAL_MEN_Z135 · line 1599

Quoted 5 lines

drivers/tty/serial/Kconfig +4 -0

+       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

Please consistently use tabs instead of spaces for indentation. The help text should be indented by one tabe + 2 spaces.

drivers/tty/serial/sprd_serial.c line 191

Quoted 2 lines

drivers/tty/serial/sprd_serial.c new file +2 -0

+static inline int handle_lsr_errors(struct uart_port *port,
+     unsigned int *flag, unsigned int *lsr)

This line should be aligned with the opening ( above.

drivers/tty/serial/sprd_serial.c line 223

Quoted 3 lines

drivers/tty/serial/sprd_serial.c new file +3 -0

+static inline void sprd_rx(int irq, void *dev_id)
+{
+     struct uart_port *port = (struct uart_port *)dev_id;

No need to cast a void pointer.

drivers/tty/serial/sprd_serial.c line 514

Quoted 4 lines

drivers/tty/serial/sprd_serial.c new file +4 -0

+static void sprd_console_write(struct console *co, const char *s,
+                                   unsigned int count)
+{
+     struct uart_port *port = (struct uart_port *)sprd_port[co->index];

Better explicitly access the .port member of sprd_port[co->index] here instead of casting.

Quoted 1 line

+ port = (struct uart_port *)sprd_port[co->index];

Same here, use the .port member of struct sprd_port[co->index].

drivers/tty/serial/sprd_serial.c line 548

Quoted 2 lines

drivers/tty/serial/sprd_serial.c new file +2 -0

+     if (port == NULL) {
+             pr_info("srial port %d not yet initialized\n", co->index);

Typo: should be serial instead of srial.

drivers/tty/serial/sprd_serial.c line 657

Quoted 2 lines

drivers/tty/serial/sprd_serial.c new file +2 -0

+     up->mapbase = mem->start;
+     up->membase = ioremap(mem->start, resource_size(mem));

Return value of ioremap() should be checked for NULL.

drivers/tty/serial/sprd_serial.c line 697

Quoted 4 lines

drivers/tty/serial/sprd_serial.c new file +4 -0

+static int sprd_resume(struct platform_device *dev)
+{
+     int id = dev->id;
+     struct uart_port *port = (struct uart_port *)sprd_port[id];

Access the .port member instead of the cast.

drivers/tty/serial/sprd_serial.c new file +5 -0

+static int sprd_startup(struct uart_port *port)
+{
+	int ret = 0;
+	unsigned int ien, ctrl1;
+	struct sprd_uart_port *sp;

drivers/tty/serial/sprd_serial.c new file +5 -0

+static void sprd_set_termios(struct uart_port *port,
+				    struct ktermios *termios,
+				    struct ktermios *old)
+{
+	unsigned int baud, quot;

drivers/tty/serial/sprd_serial.c new file +4 -0

+static int __init sprd_console_setup(struct console *co, char *options)
+{
+	struct uart_port *port;
+	int baud = 115200;

Documentation/devices.txt Your cooperation is appreciated. · line 2821

Quoted 3 lines

Documentation/devices.txt +3 -0

+		 213 = /dev/ttySPX0		SPRD serial port 0
+		    ...
+		 216 = /dev/ttySPX3		SPRD serial port 3

Please use dynamic allocation or give a very very good reason why you can't

drivers/tty/serial/Kconfig config SERIAL_MEN_Z135 · line 1586

Quoted 4 lines

drivers/tty/serial/Kconfig +4 -0

+config SERIAL_SPRD_NR
+        int "Maximum number of sprd serial ports"
+        depends on SERIAL_SPRD
+        default "4"

If you are doing dynamic allocation this should pretty much go away

Quoted 6 lines

drivers/tty/serial/sprd_serial.c new file +5 -0

+static int sprd_startup(struct uart_port *port)
+{
+	int ret = 0;
+	unsigned int ien, ctrl1;
+	struct sprd_uart_port *sp;

drivers/tty/serial/sprd_serial.c line 314

Quoted 10 lines

drivers/tty/serial/sprd_serial.c new file +10 -0

+
+	serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY << 8) | THLD_RX_FULL));
+
+	/* clear rx fifo */
+	while (serial_in(port, SPRD_STS1) & 0x00ff)
+		serial_in(port, SPRD_RXD);
+
+	/* clear tx fifo */
+	while (serial_in(port, SPRD_STS1) & 0xff00)
+		;

Missing a cpu_relax and I would have thought a timeout on both of these.

Quoted 6 lines

drivers/tty/serial/sprd_serial.c new file +4 -0

+static void sprd_set_termios(struct uart_port *port,
+				    struct ktermios *termios,
+				    struct ktermios *old)
+{

drivers/tty/serial/sprd_serial.c line 397

Quoted 10 lines

drivers/tty/serial/sprd_serial.c new file +10 -0

+	unsigned int baud, quot;
+	/* calculate parity */
+	lcr &= ~SPRD_LCR_PARITY;
+	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;
+	}

If you don't support mark/space parity then also clear CMSPAR in termios->c_cflag.

If you do then it ought to be handled above.

drivers/tty/serial/sprd_serial.c line 439

Quoted 6 lines

drivers/tty/serial/sprd_serial.c new file +6 -0

+
+	/* 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);

Also set the resulting baud back into the termios (see the 8250 termios for an example)

Quoted 6 lines

drivers/tty/serial/sprd_serial.c new file +4 -0

+static int __init sprd_console_setup(struct console *co, char *options)
+{
+	struct uart_port *port;
+	int baud = 115200;

drivers/tty/serial/sprd_serial.c line 548

Quoted 10 lines

drivers/tty/serial/sprd_serial.c new file +10 -0

+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	if (unlikely(co->index >= UART_NR_MAX || co->index < 0))
+		co->index = 0;
+
+	port = (struct uart_port *)sprd_port[co->index];
+	if (port == NULL) {
+		pr_info("srial port %d not yet initialized\n", co->index);

Typo

Looks basically sound to me

Alan

do we just leave the .major & .minor as NULL in struct uart_driver for dynamic allocation?

Quoted 5 lines

drivers/tty/serial/Kconfig config SERIAL_MEN_Z135 · line 1586

Quoted 4 lines

drivers/tty/serial/Kconfig +4 -0

+config SERIAL_SPRD_NR
+        int "Maximum number of sprd serial ports"
+        depends on SERIAL_SPRD
+        default "4"

If you are doing dynamic allocation this should pretty much go away

I think you mean getting the number of uarts from dt and dynamically allocate their port structure?

Quoted 2 lines

Also set the resulting baud back into the termios (see the 8250 termios for an example)

you mean something like this part ?

/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios)) tty_termios_encode_baud_rate(termios, baud, baud);

Add a full sc9836-uart driver for SC9836 SoC which is based on the spreadtrum sharkl64 platform.
This driver also support earlycon.

Signed-off-by Chunyan Zhang<[email protected]>

Signed-off-by Orson Zhai<[email protected]>

Originally-by Lanqing Liu<[email protected]>

---
  Documentation/devices.txt        |    3 +
  drivers/tty/serial/Kconfig       |   23 ++
  drivers/tty/serial/Makefile      |    1 +
  drivers/tty/serial/sprd_serial.c |  752 ++++++++++++++++++++++++++++++++++++++
  include/uapi/linux/serial_core.h |    3 +
  5 files changed, 782 insertions(+)

create mode 100644 drivers/tty/serial/sprd_serial.c

Documentation/devices.txt +3 -0

index 87b4c5e..1da0432 100644
@@ -2816,6 +2816,9 @@ Your cooperation is appreciated.
  		 210 = /dev/ttyMAX1		MAX3100 serial port 1
  		 211 = /dev/ttyMAX2		MAX3100 serial port 2
  		 212 = /dev/ttyMAX3		MAX3100 serial port 3
+		 213 = /dev/ttySPX0		SPRD serial port 0
+		    ...
+		 216 = /dev/ttySPX3		SPRD serial port 3
 
  205 char	Low-density serial ports (alternate device)
  		  0 = /dev/culu0		Callout device for ttyLU0

drivers/tty/serial/Kconfig +23 -0

index 649b784..2c2cf60 100644
@@ -1573,6 +1573,29 @@ 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_NR
+        int "Maximum number of sprd serial ports"
+        depends on SERIAL_SPRD
+        default "4"
+
+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

drivers/tty/serial/Makefile +1 -0

index 9a548ac..4801aca 100644
@@ -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

drivers/tty/serial/sprd_serial.c new file +19 -0

new file mode 100644
index 0000000..58214c8
@@ -0,0 +1,752 @@
+/*
+ * 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/module.h>
+#include<linux/tty.h>
+#include<linux/ioport.h>
+#include<linux/console.h>
+#include<linux/platform_device.h>
+#include<linux/tty_flip.h>

drivers/tty/serial/sprd_serial.c new file +258 -0

+
+/* device name */
+#define UART_NR_MAX		CONFIG_SERIAL_SPRD_NR
+#define SPRD_TTY_NAME		"ttySPX"
+#define SPRD_TTY_MAJOR		204
+#define SPRD_TTY_MINOR_START	213
+#define SPRD_FIFO_SIZE		128
+#define SPRD_DEF_RATE		26000000
+
+/* 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;
+
+	/* stastics */
+	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 = (struct uart_port *)dev_id;
+	struct tty_port *tty =&port->state->port;
+	unsigned int ch, flag, lsr, max_count = 2048;
+
+	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 (unlikely(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;
+	u32 ims;
+
+	ims = serial_in(port, SPRD_IMSR);
+

drivers/tty/serial/sprd_serial.c new file +455 -0

+}
+
+static int sprd_startup(struct uart_port *port)
+{
+	int ret = 0;
+	unsigned int ien, ctrl1;
+	struct sprd_uart_port *sp;
+
+	serial_out(port, SPRD_CTL2, ((THLD_TX_EMPTY<<  8) | THLD_RX_FULL));
+
+	/* clear rx fifo */
+	while (serial_in(port, SPRD_STS1)&  0x00ff)
+		serial_in(port, SPRD_RXD);
+
+	/* clear tx fifo */
+	while (serial_in(port, SPRD_STS1)&  0xff00)
+		;
+
+	/* 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\n",
+			port->irq);
+		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;
+	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);
+}
+
+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 (unlikely(ser->type != PORT_SPRD))
+		return -EINVAL;
+	if (unlikely(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 = (struct uart_port *)sprd_port[co->index];
+	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 (unlikely(co->index>= UART_NR_MAX || co->index<  0))
+		co->index = 0;
+
+	port = (struct uart_port *)sprd_port[co->index];
+	if (port == NULL) {
+		pr_info("srial 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)
+{
+	while (!(readl(port->membase + SPRD_LSR)&  SPRD_LSR_TX_OVER))
+		;
+	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 = SPRD_TTY_MAJOR,
+	.minor = SPRD_TTY_MINOR_START,
+	.nr = UART_NR_MAX,
+	.cons = SPRD_CONSOLE,
+};
+
+static int sprd_probe(struct platform_device *pdev)
+{
+	struct resource *mem;
+	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 (unlikely(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 = (struct uart_port *)sprd_port[pdev->id];
+	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);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (unlikely(!mem)) {
+		dev_err(&pdev->dev, "not provide mem resource\n");
+		return -ENODEV;
+	}
+	up->mapbase = mem->start;
+	up->membase = ioremap(mem->start, resource_size(mem));
+
+	irq = platform_get_irq(pdev, 0);
+	if (unlikely(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 platform_device *dev, pm_message_t state)
+{
+	int id = dev->id;
+	struct uart_port *port = (struct uart_port *)sprd_port[id];
+	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);
+
+	return 0;
+}
+
+static int sprd_resume(struct platform_device *dev)
+{
+	int id = dev->id;
+	struct uart_port *port = (struct uart_port *)sprd_port[id];
+	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);
+
+	return 0;
+}
+
+static const struct of_device_id serial_ids[] = {
+	{.compatible = "sprd,sc9836-uart",},
+	{}
+};
+
+static struct platform_driver sprd_platform_driver = {
+	.probe = sprd_probe,
+	.remove = sprd_remove,
+	.suspend = sprd_suspend,
+	.resume = sprd_resume,
+	.driver = {
+		   .name = "sprd_serial",
+		   .owner = THIS_MODULE,
+		   .of_match_table = of_match_ptr(serial_ids),
+		   },
+};
+
+static int __init sprd_serial_init(void)
+{
+	int ret = 0;
+
+	ret = uart_register_driver(&sprd_uart_driver);
+	if (unlikely(ret != 0))
+		return ret;
+
+	ret = platform_driver_register(&sprd_platform_driver);
+	if (unlikely(ret != 0))
+		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");
+MODULE_DESCRIPTION("Spreadtrum SoC serial driver series");

include/uapi/linux/serial_core.h +3 -0

index 16ad852..d9a8c88 100644
@@ -247,4 +247,7 @@
  /* MESON */
  #define PORT_MESON	109
 
+/* SPRD SERIAL  */
+#define PORT_SPRD   110
+
  #endif /* _UAPILINUX_SERIAL_CORE_H */

Quoted 4 lines

Add a full sc9836-uart driver for SC9836 SoC which is based on the

+#include<linux/clk.h>

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

Quoted 7 lines

drivers/tty/serial/sprd_serial.c new file +7 -0

+static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
+{
+       struct uart_port *port = (struct uart_port *)dev_id;
+       u32 ims;
+
+       ims = serial_in(port, SPRD_IMSR);
+

drivers/tty/serial/sprd_serial.c line 297

Quoted 10 lines

drivers/tty/serial/sprd_serial.c new file +10 -0

+       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;

You are always returning IRQ_HANDLED and this is registered as a SHARED irq. Is there a chance this handler is called and the irq event doesn't belong to this device?

Murali

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:

Quoted 31 lines · Chunyan Zhang

Quoted 4 lines

Add a full sc9836-uart driver for SC9836 SoC which is based on the

+#include<linux/clk.h>

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

Quoted 7 lines

drivers/tty/serial/sprd_serial.c new file +7 -0

+static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
+{
+       struct uart_port *port = (struct uart_port *)dev_id;
+       u32 ims;
+
+       ims = serial_in(port, SPRD_IMSR);
+

drivers/tty/serial/sprd_serial.c line 297

Quoted 10 lines

drivers/tty/serial/sprd_serial.c new file +10 -0

+       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;

You are always returning IRQ_HANDLED and this is registered as a SHARED irq. Is there a chance this handler is called and the irq event doesn't belong to this device?

Quoted 1 line

Murali

You are right, this is not a SHARED irq. I'll pass 0 for irqflags when called 'devm_request_irq' in the next version patch.

Quoted 32 lines

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:

Quoted 31 lines · Chunyan Zhang

Quoted 4 lines

Add a full sc9836-uart driver for SC9836 SoC which is based on the

+#include<linux/clk.h>

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

Quoted 7 lines

drivers/tty/serial/sprd_serial.c new file +7 -0

+static irqreturn_t sprd_handle_irq(int irq, void *dev_id)
+{
+       struct uart_port *port = (struct uart_port *)dev_id;
+       u32 ims;
+
+       ims = serial_in(port, SPRD_IMSR);
+

drivers/tty/serial/sprd_serial.c line 297

Quoted 10 lines

drivers/tty/serial/sprd_serial.c new file +10 -0

+       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;

You are always returning IRQ_HANDLED and this is registered as a SHARED irq. Is there a chance this handler is called and the irq event doesn't belong to this device?

Quoted 4 lines

Quoted 1 line

Murali

You are right, this is not a SHARED irq. I'll pass 0 for irqflags when called 'devm_request_irq' in the next version patch.

I think you could also add

        if (!ims)
                return IRQ_NONE;

which would make it work on shared interrupt lines.

Arnd

Yes, I saw this way in other serial drivers. But, if then, there are two questions for me:
1. Why did some serial drivers need an UN_SHARED irq?
2. How can we choose a right way?

Yes, I saw this way in other serial drivers. But, if then, there are two questions for me:
1. Why did some serial drivers need an UN_SHARED irq?

2. How can we choose a right way?

Quoted 34 lines

Add a full sc9836-uart driver for SC9836 SoC which is based on the spreadtrum sharkl64 platform.
This driver also support earlycon.

[...]

+++ b/drivers/tty/serial/sprd_serial.c

drivers/tty/serial/sprd_serial.c new file +23 -0

@@ -0,0 +1,752 @@
+/*
+ * 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/module.h>
+#include<linux/tty.h>
+#include<linux/ioport.h>
+#include<linux/console.h>
+#include<linux/platform_device.h>
+#include<linux/tty_flip.h>
+#include<linux/serial_core.h>
+#include<linux/serial.h>
+#include<linux/delay.h>
+#include<linux/io.h>

Review line 29

Quoted 6 lines

(quoted diff) +6 -0

+#include<asm/irq.h>
+#include<linux/slab.h>
+#include<linux/of.h>
+#include<linux/kernel.h>
+#include<linux/slab.h>
+#include<linux/clk.h>

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:

Quoted 1 line

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

There are a few compile warnings if I moved asm/irq.h to the top of all included files.

Warning details are below:

In file included from drivers/tty/serial/sprd_serial.c:14:0:
./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]

 extern void (*handle_arch_irq)(struct pt_regs *);
                                       ^

./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]
extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

Quoted 5 lines · On Wednesday 03 December 2014 17:17:13 Lyra Zhang wrote:

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:

Quoted 1 line

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

There are a few compile warnings if I moved asm/irq.h to the top of all included files.

The order that Murali meant is

- first all linux/*.h headers, sorted alphabetically
- then all asm/*.h headers, again sorted alphabetically

This is the recommended style in general.

Quoted 5 lines

Warning details are below:

In file included from drivers/tty/serial/sprd_serial.c:14:0:
./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]

Quoted 9 lines

 extern void (*handle_arch_irq)(struct pt_regs *);
                                       ^

./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]
extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

I would consider this a (minor) bug in asm/irq.h. If you don't mind, please submit a patch to add a line with 'struct pt_regs;' to asm/irq.h.

Quoted 30 lines · On Wednesday 03 December 2014 17:17:13 Lyra Zhang wrote:

Quoted 6 lines

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:

Quoted 1 line

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

There are a few compile warnings if I moved asm/irq.h to the top of all included files.

The order that Murali meant is

- first all linux/*.h headers, sorted alphabetically
- then all asm/*.h headers, again sorted alphabetically

This is the recommended style in general.

Quoted 5 lines

Warning details are below:

In file included from drivers/tty/serial/sprd_serial.c:14:0:
./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]

Quoted 9 lines

 extern void (*handle_arch_irq)(struct pt_regs *);
                                       ^

./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]
extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

Quoted 3 lines

I would consider this a (minor) bug in asm/irq.h. If you don't mind, please submit a patch to add a line with 'struct pt_regs;' to asm/irq.h.

A better question is: why is a modern driver using asm/irq.h in the first place.

We used to include that file when platforms defined IRQ numbers statically, but modern platforms don't do that, so it shouldn't be required anymore.

Quoted 6 lines

2014-11-27 2:29 GMT+08:00 Murali Karicheri <[email protected]>:

Quoted 1 line

How about sorting this includes? asm/irq.h go first followed linux/ in alphabatical order?

There are a few compile warnings if I moved asm/irq.h to the top of all included files.

The order that Murali meant is

- first all linux/*.h headers, sorted alphabetically
- then all asm/*.h headers, again sorted alphabetically

This is the recommended style in general.

Quoted 5 lines

Warning details are below:

In file included from drivers/tty/serial/sprd_serial.c:14:0:
./arch/arm64/include/asm/irq.h:6:39: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]

Quoted 9 lines

 extern void (*handle_arch_irq)(struct pt_regs *);
                                       ^

./arch/arm64/include/asm/irq.h:6:39: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
./arch/arm64/include/asm/irq.h:8:54: warning: ‘struct pt_regs’ declared inside parameter list [enabled by default]
extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

I would consider this a (minor) bug in asm/irq.h. If you don't mind, please submit a patch to add a line with 'struct pt_regs;' to asm/irq.h.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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.

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 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 Fast Mode.
We use boot-wrapper-aarch64 as the bootloader.

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 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 Fast Mode.

We use boot-wrapper-aarch64 as the bootloader.

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/devices.txt                          |    3 +
 Documentation/devicetree/bindings/arm/sprd.txt     |   11 +
 .../bindings/serial/{of-serial.txt => 8250.txt}    |    0
 .../devicetree/bindings/serial/sprd-uart.txt       |    6 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 arch/arm64/Kconfig                                 |   17 +
 arch/arm64/boot/dts/Makefile                       |    1 +
 arch/arm64/boot/dts/sprd-sc9836-openphone.dts      |   85 +++
 arch/arm64/boot/dts/sprd-sc9836.dtsi               |  103 +++
 arch/arm64/boot/dts/sprd-sharkl64.dtsi             |  105 +++
 arch/arm64/configs/defconfig                       |    2 +
 drivers/tty/serial/Kconfig                         |   23 +
 drivers/tty/serial/Makefile                        |    1 +
 drivers/tty/serial/sprd_serial.c                   |  752 ++++++++++++++++++++
 include/uapi/linux/serial_core.h                   |    3 +
 15 files changed, 1113 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-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

Signature · 1 line

1.7.9.5