Add Spreadtrum SoC bindings and serial driver support

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

Originally-by Lanqing Liu <[email protected]>

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

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

Acked-by Arnd Bergmann <[email protected]>

Reviewed-by Peter Hurley <[email protected]>

---
  drivers/tty/serial/Kconfig       |   18 +
  drivers/tty/serial/Makefile      |    1 +
  drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
  include/uapi/linux/serial_core.h |    3 +
  4 files changed, 815 insertions(+)

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

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

+static int sprd_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct uart_port *up;
+	struct clk *clk;
+	int irq;
+	int index;
+	int ret;
+
+	for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
+		if (sprd_port[index] == NULL)
+			break;
+
+	if (index == ARRAY_SIZE(sprd_port))
+		return -EBUSY;
+
+	index = sprd_probe_dt_alias(index, &pdev->dev);
+
+	sprd_port[index] = devm_kzalloc(&pdev->dev,
+		sizeof(*sprd_port[index]), GFP_KERNEL);
+	if (!sprd_port[index])
+		return -ENOMEM;
+
+	up = &sprd_port[index]->port;
+	up->dev = &pdev->dev;
+	up->line = index;
+	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 = UPF_BOOT_AUTOCONF;

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

+	up->mapbase = res->start;
+	up->membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(up->membase))
+		return PTR_ERR(up->membase);
+

Hi,

Quoted 18 lines · Chunyan Zhang

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

Originally-by Lanqing Liu <[email protected]>

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

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

Acked-by Arnd Bergmann <[email protected]>

Reviewed-by Peter Hurley <[email protected]>

---
  drivers/tty/serial/Kconfig       |   18 +
  drivers/tty/serial/Makefile      |    1 +
  drivers/tty/serial/sprd_serial.c |  793 
++++++++++++++++++++++++++++++++++++++
  include/uapi/linux/serial_core.h |    3 +
  4 files changed, 815 insertions(+)

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

(...)

Quoted 36 lines

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

+static int sprd_probe(struct platform_device *pdev)
+{
+    struct resource *res;
+    struct uart_port *up;
+    struct clk *clk;
+    int irq;
+    int index;
+    int ret;
+
+    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
+        if (sprd_port[index] == NULL)
+            break;
+
+    if (index == ARRAY_SIZE(sprd_port))
+        return -EBUSY;
+
+    index = sprd_probe_dt_alias(index, &pdev->dev);
+
+    sprd_port[index] = devm_kzalloc(&pdev->dev,
+        sizeof(*sprd_port[index]), GFP_KERNEL);
+    if (!sprd_port[index])
+        return -ENOMEM;
+
+    up = &sprd_port[index]->port;
+    up->dev = &pdev->dev;
+    up->line = index;
+    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 = UPF_BOOT_AUTOCONF;
+
+    clk = devm_clk_get(&pdev->dev, NULL);
+    if (!IS_ERR(clk))
+        up->uartclk = clk_get_rate(clk);

drivers/tty/serial/sprd_serial.c line 722

Quoted 6 lines

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

+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res) {
+        dev_err(&pdev->dev, "not provide mem resource\n");
+        return -ENODEV;
+    }

This check is not required. It will be done by devm_ioremap_resource()

Quoted 1 line

+ up->mapbase = res->start;

Quoted 5 lines

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

+    up->membase = devm_ioremap_resource(&pdev->dev, res);
+    if (IS_ERR(up->membase))
+        return PTR_ERR(up->membase);
+

Hi,

Quoted 17 lines · Chunyan Zhang

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

Originally-by Lanqing Liu <[email protected]>

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

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

Acked-by Arnd Bergmann <[email protected]>

Reviewed-by Peter Hurley <[email protected]>

---
  drivers/tty/serial/Kconfig       |   18 +
  drivers/tty/serial/Makefile      |    1 +
  drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
  include/uapi/linux/serial_core.h |    3 +
  4 files changed, 815 insertions(+)

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

(...)

Quoted 36 lines

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

+static int sprd_probe(struct platform_device *pdev)
+{
+    struct resource *res;
+    struct uart_port *up;
+    struct clk *clk;
+    int irq;
+    int index;
+    int ret;
+
+    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
+        if (sprd_port[index] == NULL)
+            break;
+
+    if (index == ARRAY_SIZE(sprd_port))
+        return -EBUSY;
+
+    index = sprd_probe_dt_alias(index, &pdev->dev);
+
+    sprd_port[index] = devm_kzalloc(&pdev->dev,
+        sizeof(*sprd_port[index]), GFP_KERNEL);
+    if (!sprd_port[index])
+        return -ENOMEM;
+
+    up = &sprd_port[index]->port;
+    up->dev = &pdev->dev;
+    up->line = index;
+    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 = UPF_BOOT_AUTOCONF;
+
+    clk = devm_clk_get(&pdev->dev, NULL);
+    if (!IS_ERR(clk))
+        up->uartclk = clk_get_rate(clk);

drivers/tty/serial/sprd_serial.c line 722

Quoted 6 lines

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

+
+    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+    if (!res) {
+        dev_err(&pdev->dev, "not provide mem resource\n");
+        return -ENODEV;
+    }

This check is not required. It will be done by devm_ioremap_resource()

drivers/tty/serial/sprd_serial.c line 727

Quoted 6 lines

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

+    up->mapbase = res->start;
+    up->membase = devm_ioremap_resource(&pdev->dev, res);
+    if (IS_ERR(up->membase))
+        return PTR_ERR(up->membase);
+

Hi Varka,

Quoted 57 lines · Varka Bhadram

Hi,

Quoted 17 lines · Chunyan Zhang

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

Originally-by Lanqing Liu <[email protected]>

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

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

Acked-by Arnd Bergmann <[email protected]>

Reviewed-by Peter Hurley <[email protected]>

---
   drivers/tty/serial/Kconfig       |   18 +
   drivers/tty/serial/Makefile      |    1 +
   drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
   include/uapi/linux/serial_core.h |    3 +
   4 files changed, 815 insertions(+)

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

(...)

Quoted 35 lines

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

+static int sprd_probe(struct platform_device *pdev)
+{
+    struct resource *res;
+    struct uart_port *up;
+    struct clk *clk;
+    int irq;
+    int index;
+    int ret;
+
+    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
+        if (sprd_port[index] == NULL)
+            break;
+
+    if (index == ARRAY_SIZE(sprd_port))
+        return -EBUSY;
+
+    index = sprd_probe_dt_alias(index, &pdev->dev);
+
+    sprd_port[index] = devm_kzalloc(&pdev->dev,
+        sizeof(*sprd_port[index]), GFP_KERNEL);
+    if (!sprd_port[index])
+        return -ENOMEM;
+
+    up = &sprd_port[index]->port;
+    up->dev = &pdev->dev;
+    up->line = index;
+    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 = UPF_BOOT_AUTOCONF;
+
+    clk = devm_clk_get(&pdev->dev, NULL);
+    if (!IS_ERR(clk))

Quoted 8 lines

drivers/tty/serial/sprd_serial.c line 722

Quoted 7 lines

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

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

This check is not required. It will be done by devm_ioremap_resource()

I disagree. devm_ioremap_resource() interprets the NULL resource as a bad parameter and returns -EINVAL which is then forwarded as the return value from the probe.

-ENODEV is the correct return value from the probe if the expected resource is not available (either because it doesn't exist or was already claimed by another driver).

Regards,
Peter Hurley

Quoted 6 lines

Quoted 6 lines

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

+    up->mapbase = res->start;
+    up->membase = devm_ioremap_resource(&pdev->dev, res);
+    if (IS_ERR(up->membase))
+        return PTR_ERR(up->membase);
+

Hi Varka,

Quoted 1 line · Varka Bhadram

This check is not required. It will be done by devm_ioremap_resource()

I disagree. devm_ioremap_resource() interprets the NULL resource as a bad parameter and returns -EINVAL which is then forwarded as the return value from the probe.

-ENODEV is the correct return value from the probe if the expected resource is not available (either because it doesn't exist or was already claimed by another driver).

Hi Peter,

Quoted 68 lines · Peter Hurley

Hi Varka,

Quoted 57 lines · Varka Bhadram

Hi,

Quoted 17 lines · Chunyan Zhang

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

Originally-by Lanqing Liu <[email protected]>

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

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

Acked-by Arnd Bergmann <[email protected]>

Reviewed-by Peter Hurley <[email protected]>

---
  drivers/tty/serial/Kconfig       |   18 +
  drivers/tty/serial/Makefile      |    1 +
  drivers/tty/serial/sprd_serial.c |  793 ++++++++++++++++++++++++++++++++++++++
  include/uapi/linux/serial_core.h |    3 +
  4 files changed, 815 insertions(+)

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

(...)

Quoted 35 lines

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

+static int sprd_probe(struct platform_device *pdev)
+{
+    struct resource *res;
+    struct uart_port *up;
+    struct clk *clk;
+    int irq;
+    int index;
+    int ret;
+
+    for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
+        if (sprd_port[index] == NULL)
+            break;
+
+    if (index == ARRAY_SIZE(sprd_port))
+        return -EBUSY;
+
+    index = sprd_probe_dt_alias(index, &pdev->dev);
+
+    sprd_port[index] = devm_kzalloc(&pdev->dev,
+        sizeof(*sprd_port[index]), GFP_KERNEL);
+    if (!sprd_port[index])
+        return -ENOMEM;
+
+    up = &sprd_port[index]->port;
+    up->dev = &pdev->dev;
+    up->line = index;
+    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 = UPF_BOOT_AUTOCONF;
+
+    clk = devm_clk_get(&pdev->dev, NULL);
+    if (!IS_ERR(clk))

Quoted 8 lines

drivers/tty/serial/sprd_serial.c line 722

Quoted 7 lines

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

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

This check is not required. It will be done by devm_ioremap_resource()

Quoted 7 lines

I disagree. devm_ioremap_resource() interprets the NULL resource as a bad parameter and returns -EINVAL which is then forwarded as the return value from the probe.

-ENODEV is the correct return value from the probe if the expected resource is not available (either because it doesn't exist or was already claimed by another driver).

Check on the resource happening with evm_ioremap_resource.

Not necessary to check multiple times.

I did series for all the drivers. see [1]

[1]: https://lkml.org/lkml/2014/11/3/986 <https://lkml.org/lkml/2014/11/3/986>

Quoted 5 lines · Peter Hurley

Hi Varka,

Quoted 1 line · Varka Bhadram

This check is not required. It will be done by devm_ioremap_resource()

Quoted 7 lines

I disagree. devm_ioremap_resource() interprets the NULL resource as a bad parameter and returns -EINVAL which is then forwarded as the return value from the probe.

-ENODEV is the correct return value from the probe if the expected resource is not available (either because it doesn't exist or was already claimed by another driver).

(Adding Thierry as the author of this function.)

I believe devm_ioremap_resource() was explicitly designed to remove such error handling from drivers, and to give drivers a unified error response to such things as missing resources.

See the comments for this function in lib/devres.c.

Quoted 5 lines · Peter Hurley

Hi Varka,

Quoted 1 line · Varka Bhadram

This check is not required. It will be done by devm_ioremap_resource()

Quoted 7 lines

I disagree. devm_ioremap_resource() interprets the NULL resource as a bad parameter and returns -EINVAL which is then forwarded as the return value from the probe.

-ENODEV is the correct return value from the probe if the expected resource is not available (either because it doesn't exist or was already claimed by another driver).

(Adding Thierry as the author of this function.)

I believe devm_ioremap_resource() was explicitly designed to remove such error handling from drivers, and to give drivers a unified error response to such things as missing resources.

See the comments for this function in lib/devres.c.

Quoted 14 lines · Russell King - ARM Linux

Quoted 5 lines · Peter Hurley

Hi Varka,

Quoted 1 line · Varka Bhadram

This check is not required. It will be done by devm_ioremap_resource()

Quoted 7 lines

I disagree. devm_ioremap_resource() interprets the NULL resource as a bad parameter and returns -EINVAL which is then forwarded as the return value from the probe.

-ENODEV is the correct return value from the probe if the expected resource is not available (either because it doesn't exist or was already claimed by another driver).

Quoted 7 lines

(Adding Thierry as the author of this function.)

I believe devm_ioremap_resource() was explicitly designed to remove such error handling from drivers, and to give drivers a unified error response to such things as missing resources.

See the comments for this function in lib/devres.c.

Right. Before the introduction of this function drivers would copy the same boilerplate but would use completely inconsistent return codes. Well, to be correct devm_request_and_ioremap() was introduced first to reduce the boilerplate, but one of the problems was that it returned a NULL pointer on failure, so it was impossible to distinguish between specific error conditions. It also had the negative side-effect of not giving drivers any directions on what to do with the NULL return value other than the example in the kerneldoc. But most people just didn't seem to look at that, so instead of using -EADDRNOTAVAIL they'd again go and do completely inconsistent things everywhere.

When we introduced devm_ioremap_resource(), the idea was to remove any of these inconsistencies once and for all. You call the function and if it fails you simply propagate the error code, so you get consistent behaviour everywhere.

If I remember correctly the error codes for the various conditions were discussed quite extensively, and what we currently have is what we got by concensus.

-ENODEV is certainly not the correct return value if a resource is not available. It translates to "no such device", but the device must clearly be there, otherwise the ->probe() shouldn't have been called.

Or if it really isn't there, then you'd at least need a memory region to probe, otherwise you can't determine whether it's there or not. So from the perspective of a device driver I think a missing, or NULL, resource, is indeed an "invalid argument".

I understand that people might see some ambiguity there, and -EINVAL is certainly not a very accurate description, but such is the nature of error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been under discussion as well if I remember correctly, but they both equally ambiguous. -ENODATA might have been a better fit, but that too has a different meaning in other contexts.

Besides, there's an error message that goes along with the error code return, that should remove any ambiguity for people looking at dmesg.

All of that said, the original assertion that the check is not required is still valid. We can argue at length about the specific error code but if we decide that it needs to change, then we should modify devm_ioremap_resource() rather than requiring all callers to perform the extra check again.

Quoted 3 lines · Thierry Reding

-ENODEV is certainly not the correct return value if a resource is not available. It translates to "no such device", but the device must clearly be there, otherwise the ->probe() shouldn't have been called.

-ENODEV is the appropriate return from the probe(); there is no device.

Quoted 4 lines

Or if it really isn't there, then you'd at least need a memory region to probe, otherwise you can't determine whether it's there or not. So from the perspective of a device driver I think a missing, or NULL, resource, is indeed an "invalid argument".

Trying to argue that a missing host bus window is an "invalid argument" to probe() is just trying to make the shoe fit.

Quoted 10 lines

I understand that people might see some ambiguity there, and -EINVAL is certainly not a very accurate description, but such is the nature of error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been under discussion as well if I remember correctly, but they both equally ambiguous. -ENODATA might have been a better fit, but that too has a different meaning in other contexts.

Besides, there's an error message that goes along with the error code return, that should remove any ambiguity for people looking at dmesg.

Quoted 5 lines

All of that said, the original assertion that the check is not required is still valid. We can argue at length about the specific error code but if we decide that it needs to change, then we should modify devm_ioremap_resource() rather than requiring all callers to perform the extra check again.

None of the devm_ioremap_resource() changes have been submitted for serial drivers.

Quoted 6 lines · Peter Hurley

Quoted 3 lines · Thierry Reding

-ENODEV is certainly not the correct return value if a resource is not available. It translates to "no such device", but the device must clearly be there, otherwise the ->probe() shouldn't have been called.

-ENODEV is the appropriate return from the probe(); there is no device.

No it is not. "no such device" means "the device is not present". If the device is not present, we wouldn't have a struct device associated with it.

The missing resource is an error condition: what it's saying is that the device is there, but something has failed in providing the IO regions necessary to access it. That's really something very different from "there is no device present".

Quoted 7 lines

Quoted 4 lines

Or if it really isn't there, then you'd at least need a memory region to probe, otherwise you can't determine whether it's there or not. So from the perspective of a device driver I think a missing, or NULL, resource, is indeed an "invalid argument".

Trying to argue that a missing host bus window is an "invalid argument" to probe() is just trying to make the shoe fit.

As is arguing that -ENODEV is an appropriate return value for the missing resource.

Moreover, returning -ENODEV is actually *bad* in this case - the kernel's generic probe handling does not report the failure of the driver to bind given this, so a missing resource potentially becomes a silent failure of a driver - leading users to wonder why their devices aren't found.

If we /really/ have a problem with the error code, then why not invent a new error code to cater for this condition - maybe, EBADRES (bad resource).

Quoted 16 lines

Quoted 15 lines

I understand that people might see some ambiguity there, and -EINVAL is certainly not a very accurate description, but such is the nature of error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been under discussion as well if I remember correctly, but they both equally ambiguous. -ENODATA might have been a better fit, but that too has a different meaning in other contexts.

Besides, there's an error message that goes along with the error code return, that should remove any ambiguity for people looking at dmesg.

All of that said, the original assertion that the check is not required is still valid. We can argue at length about the specific error code but if we decide that it needs to change, then we should modify devm_ioremap_resource() rather than requiring all callers to perform the extra check again.

Quoted 2 lines

None of the devm_ioremap_resource() changes have been submitted for serial drivers.

$ grep devm_ioremap_resource drivers/tty/serial/ -r | wc -l
10

Before you say consistency, I think you should look at the stats below. IOW, if you want to change the error code return from probe() for consistency's sake, a tree-wide patch would be the appropriate way.

Quoted 3 lines · Peter Hurley

Before you say consistency, I think you should look at the stats below. IOW, if you want to change the error code return from probe() for consistency's sake, a tree-wide patch would be the appropriate way.

Now look outside the serial driver sub-tree.

There are 1234 instances of platform_get_resource(, IORESOURCE_MEM, ) in the drivers/ sub-tree, with 700 instances of devm_ioremap_resource() being used there. Of the devm_ioremap_resource() instances:

- 555 use platform_get_resource() in the preceding two lines - which is not enough to do anything but rely on the -EINVAL return value.
- 16 mention ENODEV in the preceding three lines.

There are 132 which use platform_get_resource() and return ENODEV within the following three lines (which may intersect with the above 16 number) and 88 which use EINVAL.

So, there are in total 643 instances where a missing resource returns EINVAL, and between 132 and 148 instances which return ENODEV.

Yes, 643 + 148 isn't 1234, but I'm not going to read through all 1234
locations just for the sake of this thread. What's clear though is that
more than 50% of sites using platform_get_resource(, IORESOURCE_MEM, ) return EINVAL for the lack of a resource.