All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Device Tree probing for bcm63xx-uart
@ 2014-02-19 23:22 Florian Fainelli
  2014-02-19 23:22 ` [PATCH 1/4] tty: serial: bcm63xx_uart: include linux/io.h Florian Fainelli
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Florian Fainelli @ 2014-02-19 23:22 UTC (permalink / raw)
  To: linux-serial; +Cc: devicetree, mbizon, jogo, gregkh, Florian Fainelli

Hi Greg,

This patchset make the bcm63xx_uart architecture agnostic such that it can be
built on something else than MIPS now. Finally it adds support for Device Tree
probing to the driver along with a DT binding documentation.

I would appreciate if this could go via your tree for consistency.

Thanks!

Florian Fainelli (4):
  tty: serial: bcm63xx_uart: include linux/io.h
  tty: serial: bcm63xx_uart: define UART_REG_SIZE constant
  tty: serial: bcm63xx_uart: add support for DT probing
  Documentation: devicetree: add bindings documentation for bcm63xx-uart

 .../devicetree/bindings/serial/bcm63xx-uart.txt    | 24 ++++++++++++++++++++++
 drivers/tty/serial/bcm63xx_uart.c                  | 16 +++++++++++++--
 include/linux/serial_bcm63xx.h                     |  2 ++
 3 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/bcm63xx-uart.txt

-- 
1.8.3.2


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] tty: serial: bcm63xx_uart: include linux/io.h
  2014-02-19 23:22 [PATCH 0/4] Device Tree probing for bcm63xx-uart Florian Fainelli
@ 2014-02-19 23:22 ` Florian Fainelli
  2014-02-19 23:22 ` [PATCH 2/4] tty: serial: bcm63xx_uart: define UART_REG_SIZE constant Florian Fainelli
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2014-02-19 23:22 UTC (permalink / raw)
  To: linux-serial; +Cc: devicetree, mbizon, jogo, gregkh, Florian Fainelli

Include linux/io.h which provides the definition for
__raw_{readl,writel}, this is not necessary on MIPS since there is an
implicit inclusion, but it is on ARM for instance.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/tty/serial/bcm63xx_uart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
index 78e82b0..d71143e 100644
--- a/drivers/tty/serial/bcm63xx_uart.c
+++ b/drivers/tty/serial/bcm63xx_uart.c
@@ -30,6 +30,7 @@
 #include <linux/serial.h>
 #include <linux/serial_core.h>
 #include <linux/serial_bcm63xx.h>
+#include <linux/io.h>
 
 #define BCM63XX_NR_UARTS	2
 
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] tty: serial: bcm63xx_uart: define UART_REG_SIZE constant
  2014-02-19 23:22 [PATCH 0/4] Device Tree probing for bcm63xx-uart Florian Fainelli
  2014-02-19 23:22 ` [PATCH 1/4] tty: serial: bcm63xx_uart: include linux/io.h Florian Fainelli
@ 2014-02-19 23:22 ` Florian Fainelli
  2014-02-19 23:22 ` [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing Florian Fainelli
  2014-02-19 23:22 ` [PATCH 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart Florian Fainelli
  3 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2014-02-19 23:22 UTC (permalink / raw)
  To: linux-serial; +Cc: devicetree, mbizon, jogo, gregkh, Florian Fainelli

The bcm63xx_uart driver uses RSET_UART_SIZE which is a constant defined
for MIPS-based BCM63xx platforms, pull this constant value from the
MIPS-specific header and put it in include/linux/serial_bcm63xx.h to
make the driver platform agnostic.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/tty/serial/bcm63xx_uart.c | 4 ++--
 include/linux/serial_bcm63xx.h    | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
index d71143e..37e7e33 100644
--- a/drivers/tty/serial/bcm63xx_uart.c
+++ b/drivers/tty/serial/bcm63xx_uart.c
@@ -589,7 +589,7 @@ static int bcm_uart_request_port(struct uart_port *port)
 {
 	unsigned int size;
 
-	size = RSET_UART_SIZE;
+	size = UART_REG_SIZE;
 	if (!request_mem_region(port->mapbase, size, "bcm63xx")) {
 		dev_err(port->dev, "Memory region busy\n");
 		return -EBUSY;
@@ -609,7 +609,7 @@ static int bcm_uart_request_port(struct uart_port *port)
  */
 static void bcm_uart_release_port(struct uart_port *port)
 {
-	release_mem_region(port->mapbase, RSET_UART_SIZE);
+	release_mem_region(port->mapbase, UART_REG_SIZE);
 	iounmap(port->membase);
 }
 
diff --git a/include/linux/serial_bcm63xx.h b/include/linux/serial_bcm63xx.h
index 570e964..a80aa1a 100644
--- a/include/linux/serial_bcm63xx.h
+++ b/include/linux/serial_bcm63xx.h
@@ -116,4 +116,6 @@
 					UART_FIFO_PARERR_MASK |		\
 					UART_FIFO_BRKDET_MASK)
 
+#define UART_REG_SIZE			24
+
 #endif /* _LINUX_SERIAL_BCM63XX_H */
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-19 23:22 [PATCH 0/4] Device Tree probing for bcm63xx-uart Florian Fainelli
  2014-02-19 23:22 ` [PATCH 1/4] tty: serial: bcm63xx_uart: include linux/io.h Florian Fainelli
  2014-02-19 23:22 ` [PATCH 2/4] tty: serial: bcm63xx_uart: define UART_REG_SIZE constant Florian Fainelli
@ 2014-02-19 23:22 ` Florian Fainelli
  2014-02-20  0:00   ` Jonas Gorski
  2014-02-19 23:22 ` [PATCH 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart Florian Fainelli
  3 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2014-02-19 23:22 UTC (permalink / raw)
  To: linux-serial; +Cc: devicetree, mbizon, jogo, gregkh, Florian Fainelli

Add a matching table for the the bcm63xx_uart driver on the compatible
string "brcm,bcm63xx-uart" which covers all BCM63xx implementations.
Also make sure that we convert the id based on the uart aliases provided
by the relevant Device Tree.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/tty/serial/bcm63xx_uart.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
index 37e7e33..538e84c 100644
--- a/drivers/tty/serial/bcm63xx_uart.c
+++ b/drivers/tty/serial/bcm63xx_uart.c
@@ -31,6 +31,7 @@
 #include <linux/serial_core.h>
 #include <linux/serial_bcm63xx.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #define BCM63XX_NR_UARTS	2
 
@@ -806,6 +807,9 @@ static int bcm_uart_probe(struct platform_device *pdev)
 	struct clk *clk;
 	int ret;
 
+	if (pdev->dev.of_node)
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "uart");
+
 	if (pdev->id < 0 || pdev->id >= BCM63XX_NR_UARTS)
 		return -EINVAL;
 
@@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id bcm63xx_of_match[] = {
+	{ .compatible = "brcm,bcm63xx-uart" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, bcm63xx_of_match);
+
 /*
  * platform driver stuff
  */
@@ -866,6 +876,7 @@ static struct platform_driver bcm_uart_platform_driver = {
 	.driver	= {
 		.owner = THIS_MODULE,
 		.name  = "bcm63xx_uart",
+		.of_match_table = bcm63xx_of_match,
 	},
 };
 
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart
  2014-02-19 23:22 [PATCH 0/4] Device Tree probing for bcm63xx-uart Florian Fainelli
                   ` (2 preceding siblings ...)
  2014-02-19 23:22 ` [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing Florian Fainelli
@ 2014-02-19 23:22 ` Florian Fainelli
  2014-02-20  9:45   ` Mark Rutland
  3 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2014-02-19 23:22 UTC (permalink / raw)
  To: linux-serial; +Cc: devicetree, mbizon, jogo, gregkh, Florian Fainelli

Add the Device Tree binding documentation for the non-standard BCM63xx
UART hardware block found in the BCM63xx DSL SoCs.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../devicetree/bindings/serial/bcm63xx-uart.txt    | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/bcm63xx-uart.txt

diff --git a/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
new file mode 100644
index 0000000..829441d
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
@@ -0,0 +1,24 @@
+Broadcom BCM63xx UART: Non standard UART used in the Broadcom BCM63xx DSL SoCs
+
+Required properties:
+- compatible: must be "brcm,bcm63xx-uart"
+- reg: offset and length of the register set for the device
+- interrupts: device interrupt
+- clocks: a phandle to the functional clock node
+- clock-names: must be "periph"
+
+Note: each UART port must have an alias correctly numbered in the "aliases"
+node, e.g:
+
+serial0: uart@600 {
+	compatible = "brcm,bcm63xx-uart";
+	reg = <0x600 0x1b>;
+	interrupts = <GIC_SPI 32 0>;
+	clocks = <&periph_clk>;
+	clock-names = "periph";
+};
+
+aliases {
+	uart0 = &serial0;
+	uart1 = &serial1;
+};
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-19 23:22 ` [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing Florian Fainelli
@ 2014-02-20  0:00   ` Jonas Gorski
  2014-02-20  1:29     ` Florian Fainelli
  0 siblings, 1 reply; 12+ messages in thread
From: Jonas Gorski @ 2014-02-20  0:00 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-serial, devicetree, Maxime Bizon, Greg Kroah-Hartman

On Thu, Feb 20, 2014 at 12:22 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> Add a matching table for the the bcm63xx_uart driver on the compatible
> string "brcm,bcm63xx-uart" which covers all BCM63xx implementations.
> Also make sure that we convert the id based on the uart aliases provided
> by the relevant Device Tree.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/tty/serial/bcm63xx_uart.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
> index 37e7e33..538e84c 100644
> --- a/drivers/tty/serial/bcm63xx_uart.c
> +++ b/drivers/tty/serial/bcm63xx_uart.c
> @@ -31,6 +31,7 @@
>  #include <linux/serial_core.h>
>  #include <linux/serial_bcm63xx.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
>
>  #define BCM63XX_NR_UARTS       2
>
> @@ -806,6 +807,9 @@ static int bcm_uart_probe(struct platform_device *pdev)
>         struct clk *clk;
>         int ret;
>
> +       if (pdev->dev.of_node)
> +               pdev->id = of_alias_get_id(pdev->dev.of_node, "uart");
> +
>         if (pdev->id < 0 || pdev->id >= BCM63XX_NR_UARTS)
>                 return -EINVAL;
>
> @@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static const struct of_device_id bcm63xx_of_match[] = {
> +       { .compatible = "brcm,bcm63xx-uart" },

>From my understanding, this should be "brcm,bcm6345-uart", because
this kind of uart appeared first on bcm6345 (well, maybe bcm6335, no
idea which one of these two was first, but the latter was never
supported in mainline anyway).
The same applies to 4/4.

> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, bcm63xx_of_match);
> +
>  /*
>   * platform driver stuff
>   */
> @@ -866,6 +876,7 @@ static struct platform_driver bcm_uart_platform_driver = {
>         .driver = {
>                 .owner = THIS_MODULE,
>                 .name  = "bcm63xx_uart",
> +               .of_match_table = bcm63xx_of_match,

You could guard this one with of_match_ptr (and then bcm63xx_of_match
with CONFIG_OF. Probably not much, but it's the little things ;-)


Regards
Jonas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-20  0:00   ` Jonas Gorski
@ 2014-02-20  1:29     ` Florian Fainelli
  2014-02-20 10:59       ` Jonas Gorski
  0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2014-02-20  1:29 UTC (permalink / raw)
  To: Jonas Gorski; +Cc: linux-serial, devicetree, Maxime Bizon, Greg Kroah-Hartman

2014-02-19 16:00 GMT-08:00 Jonas Gorski <jogo@openwrt.org>:
> On Thu, Feb 20, 2014 at 12:22 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> Add a matching table for the the bcm63xx_uart driver on the compatible
>> string "brcm,bcm63xx-uart" which covers all BCM63xx implementations.
>> Also make sure that we convert the id based on the uart aliases provided
>> by the relevant Device Tree.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/tty/serial/bcm63xx_uart.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/tty/serial/bcm63xx_uart.c b/drivers/tty/serial/bcm63xx_uart.c
>> index 37e7e33..538e84c 100644
>> --- a/drivers/tty/serial/bcm63xx_uart.c
>> +++ b/drivers/tty/serial/bcm63xx_uart.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/serial_core.h>
>>  #include <linux/serial_bcm63xx.h>
>>  #include <linux/io.h>
>> +#include <linux/of.h>
>>
>>  #define BCM63XX_NR_UARTS       2
>>
>> @@ -806,6 +807,9 @@ static int bcm_uart_probe(struct platform_device *pdev)
>>         struct clk *clk;
>>         int ret;
>>
>> +       if (pdev->dev.of_node)
>> +               pdev->id = of_alias_get_id(pdev->dev.of_node, "uart");
>> +
>>         if (pdev->id < 0 || pdev->id >= BCM63XX_NR_UARTS)
>>                 return -EINVAL;
>>
>> @@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
>>         return 0;
>>  }
>>
>> +static const struct of_device_id bcm63xx_of_match[] = {
>> +       { .compatible = "brcm,bcm63xx-uart" },
>
> From my understanding, this should be "brcm,bcm6345-uart", because
> this kind of uart appeared first on bcm6345 (well, maybe bcm6335, no
> idea which one of these two was first, but the latter was never
> supported in mainline anyway).

That's right, in fact, I think it might be desirable to handle both
compatible string, just as a hint that it is compatible with the
entire bcm63xx family. Would that work for you?

> The same applies to 4/4.
>
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, bcm63xx_of_match);
>> +
>>  /*
>>   * platform driver stuff
>>   */
>> @@ -866,6 +876,7 @@ static struct platform_driver bcm_uart_platform_driver = {
>>         .driver = {
>>                 .owner = THIS_MODULE,
>>                 .name  = "bcm63xx_uart",
>> +               .of_match_table = bcm63xx_of_match,
>
> You could guard this one with of_match_ptr (and then bcm63xx_of_match
> with CONFIG_OF. Probably not much, but it's the little things ;-)

Good catch.
-- 
Florian

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart
  2014-02-19 23:22 ` [PATCH 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart Florian Fainelli
@ 2014-02-20  9:45   ` Mark Rutland
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Rutland @ 2014-02-20  9:45 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: linux-serial, devicetree, mbizon, jogo, gregkh

On Wed, Feb 19, 2014 at 11:22:47PM +0000, Florian Fainelli wrote:
> Add the Device Tree binding documentation for the non-standard BCM63xx
> UART hardware block found in the BCM63xx DSL SoCs.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../devicetree/bindings/serial/bcm63xx-uart.txt    | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
> new file mode 100644
> index 0000000..829441d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt
> @@ -0,0 +1,24 @@
> +Broadcom BCM63xx UART: Non standard UART used in the Broadcom BCM63xx DSL SoCs
> +
> +Required properties:
> +- compatible: must be "brcm,bcm63xx-uart"
> +- reg: offset and length of the register set for the device
> +- interrupts: device interrupt
> +- clocks: a phandle to the functional clock node
> +- clock-names: must be "periph"

Minor issues: clocks aren't just phandles, and as the clock is expected
to be named it would be nice to define clocks in terms of clock-names:

- clocks: a list of phandles + clock-specifiers, one for each entry in
  clock-names
- clock-names: should contain "periph" for the functional clock

Thanks,
Mark.

> +
> +Note: each UART port must have an alias correctly numbered in the "aliases"
> +node, e.g:
> +
> +serial0: uart@600 {
> +	compatible = "brcm,bcm63xx-uart";
> +	reg = <0x600 0x1b>;
> +	interrupts = <GIC_SPI 32 0>;
> +	clocks = <&periph_clk>;
> +	clock-names = "periph";
> +};
> +
> +aliases {
> +	uart0 = &serial0;
> +	uart1 = &serial1;
> +};
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-20  1:29     ` Florian Fainelli
@ 2014-02-20 10:59       ` Jonas Gorski
  2014-02-20 12:16         ` Arnd Bergmann
  2014-02-20 12:21         ` Mark Rutland
  0 siblings, 2 replies; 12+ messages in thread
From: Jonas Gorski @ 2014-02-20 10:59 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-serial, devicetree, Maxime Bizon, Greg Kroah-Hartman

On Thu, Feb 20, 2014 at 2:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 2014-02-19 16:00 GMT-08:00 Jonas Gorski <jogo@openwrt.org>:
>> On Thu, Feb 20, 2014 at 12:22 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> @@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
>>>         return 0;
>>>  }
>>>
>>> +static const struct of_device_id bcm63xx_of_match[] = {
>>> +       { .compatible = "brcm,bcm63xx-uart" },
>>
>> From my understanding, this should be "brcm,bcm6345-uart", because
>> this kind of uart appeared first on bcm6345 (well, maybe bcm6335, no
>> idea which one of these two was first, but the latter was never
>> supported in mainline anyway).
>
> That's right, in fact, I think it might be desirable to handle both
> compatible string, just as a hint that it is compatible with the
> entire bcm63xx family. Would that work for you?

I think using a "generic" compatible string is rather frowned upon
(what do you do if there is eventually a bcm63xx chip with an
incompatible uart?), but I'm no device tree expert.


Jonas

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-20 10:59       ` Jonas Gorski
@ 2014-02-20 12:16         ` Arnd Bergmann
  2014-02-20 16:18           ` Florian Fainelli
  2014-02-20 12:21         ` Mark Rutland
  1 sibling, 1 reply; 12+ messages in thread
From: Arnd Bergmann @ 2014-02-20 12:16 UTC (permalink / raw)
  To: Jonas Gorski
  Cc: Florian Fainelli, linux-serial, devicetree, Maxime Bizon,
	Greg Kroah-Hartman

On Thursday 20 February 2014 11:59:04 Jonas Gorski wrote:
> On Thu, Feb 20, 2014 at 2:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > 2014-02-19 16:00 GMT-08:00 Jonas Gorski <jogo@openwrt.org>:
> >> On Thu, Feb 20, 2014 at 12:22 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>> @@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
> >>>         return 0;
> >>>  }
> >>>
> >>> +static const struct of_device_id bcm63xx_of_match[] = {
> >>> +       { .compatible = "brcm,bcm63xx-uart" },
> >>
> >> From my understanding, this should be "brcm,bcm6345-uart", because
> >> this kind of uart appeared first on bcm6345 (well, maybe bcm6335, no
> >> idea which one of these two was first, but the latter was never
> >> supported in mainline anyway).
> >
> > That's right, in fact, I think it might be desirable to handle both
> > compatible string, just as a hint that it is compatible with the
> > entire bcm63xx family. Would that work for you?
> 
> I think using a "generic" compatible string is rather frowned upon
> (what do you do if there is eventually a bcm63xx chip with an
> incompatible uart?), but I'm no device tree expert.

It's ok to have a generic name, it's wildcards like the xx above
that we try to avoid, since that breaks down when you get another
device in the same SoC family that is not compatible. This is different
from the Linux way of naming things.

brcm,bcm6345-uart sounds good, if that is the closest we can get
to a generic name, working under the assumption that it's the oldest
implementation of this UART. Ideally we'd find someone with access
to the design documents of the SoC to tell us what the UART is really
called by whoever designed it (which may not even be Broadcom).

If we think there may be some level of variation between the UARTS
in the various bcm63xx SoCs, it would be good to list both the
specific model of the SoC as well as the generic name in "compatible",
so the driver can later detect those differences without requiring
an updated DT.

A BCM63138 for instance could list this as

	compatible = "brcm,bcm62138-uart", "brcm-bcm6345-uart";

	Arnd

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-20 10:59       ` Jonas Gorski
  2014-02-20 12:16         ` Arnd Bergmann
@ 2014-02-20 12:21         ` Mark Rutland
  1 sibling, 0 replies; 12+ messages in thread
From: Mark Rutland @ 2014-02-20 12:21 UTC (permalink / raw)
  To: Jonas Gorski
  Cc: Florian Fainelli, linux-serial, devicetree, Maxime Bizon,
	Greg Kroah-Hartman

On Thu, Feb 20, 2014 at 10:59:04AM +0000, Jonas Gorski wrote:
> On Thu, Feb 20, 2014 at 2:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > 2014-02-19 16:00 GMT-08:00 Jonas Gorski <jogo@openwrt.org>:
> >> On Thu, Feb 20, 2014 at 12:22 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>> @@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
> >>>         return 0;
> >>>  }
> >>>
> >>> +static const struct of_device_id bcm63xx_of_match[] = {
> >>> +       { .compatible = "brcm,bcm63xx-uart" },
> >>
> >> From my understanding, this should be "brcm,bcm6345-uart", because
> >> this kind of uart appeared first on bcm6345 (well, maybe bcm6335, no
> >> idea which one of these two was first, but the latter was never
> >> supported in mainline anyway).
> >
> > That's right, in fact, I think it might be desirable to handle both
> > compatible string, just as a hint that it is compatible with the
> > entire bcm63xx family. Would that work for you?
> 
> I think using a "generic" compatible string is rather frowned upon
> (what do you do if there is eventually a bcm63xx chip with an
> incompatible uart?), but I'm no device tree expert.

As long as each compatible string entry describes something that has a
compatible programming model, there's no problem. The new UART wouldn't
be described with a string it's not compatible with.

I think "brcm,bcm6345-uart," is a nicer name to use for the moment than
"brcm,bcm63xx-uart" as it clearly describes a specific UART and is less
likely to be problematic in future if an incompatible UART appears.
Additionally, each compatible UART can have a more specific compatible
string entry that allows them to be more uniquely identified in case
they need to be handled specially in future. e.g:

  compatible = "brcm,bcm634x-uart", "brcm,bcm6345-uart";

Then if a bcm634y comes out with a UART that's not compatible with the
bcm6345 UART, it just needs:

  compatible = "brcm,bcm634y-uart";

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing
  2014-02-20 12:16         ` Arnd Bergmann
@ 2014-02-20 16:18           ` Florian Fainelli
  0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2014-02-20 16:18 UTC (permalink / raw)
  To: Arnd Bergmann, Jonas Gorski
  Cc: linux-serial, devicetree, Maxime Bizon, Greg Kroah-Hartman

Hi Arnd,

Le 20/02/2014 04:16, Arnd Bergmann a écrit :
> On Thursday 20 February 2014 11:59:04 Jonas Gorski wrote:
>> On Thu, Feb 20, 2014 at 2:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> 2014-02-19 16:00 GMT-08:00 Jonas Gorski <jogo@openwrt.org>:
>>>> On Thu, Feb 20, 2014 at 12:22 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>> @@ -857,6 +861,12 @@ static int bcm_uart_remove(struct platform_device *pdev)
>>>>>          return 0;
>>>>>   }
>>>>>
>>>>> +static const struct of_device_id bcm63xx_of_match[] = {
>>>>> +       { .compatible = "brcm,bcm63xx-uart" },
>>>>
>>>>  From my understanding, this should be "brcm,bcm6345-uart", because
>>>> this kind of uart appeared first on bcm6345 (well, maybe bcm6335, no
>>>> idea which one of these two was first, but the latter was never
>>>> supported in mainline anyway).
>>>
>>> That's right, in fact, I think it might be desirable to handle both
>>> compatible string, just as a hint that it is compatible with the
>>> entire bcm63xx family. Would that work for you?
>>
>> I think using a "generic" compatible string is rather frowned upon
>> (what do you do if there is eventually a bcm63xx chip with an
>> incompatible uart?), but I'm no device tree expert.
>
> It's ok to have a generic name, it's wildcards like the xx above
> that we try to avoid, since that breaks down when you get another
> device in the same SoC family that is not compatible. This is different
> from the Linux way of naming things.
>
> brcm,bcm6345-uart sounds good, if that is the closest we can get
> to a generic name, working under the assumption that it's the oldest
> implementation of this UART. Ideally we'd find someone with access
> to the design documents of the SoC to tell us what the UART is really
> called by whoever designed it (which may not even be Broadcom).

This is just called an uart, it does not have any specific project name 
as far as I could see, and BCM6345 was indeed the very first SoC using it.

>
> If we think there may be some level of variation between the UARTS
> in the various bcm63xx SoCs, it would be good to list both the
> specific model of the SoC as well as the generic name in "compatible",
> so the driver can later detect those differences without requiring
> an updated DT.

Makes sense. So far this UART has been not modified (as far as a 
programmer may be concerned) since its very first design in a BCM6345.

>
> A BCM63138 for instance could list this as
>
> 	compatible = "brcm,bcm62138-uart", "brcm-bcm6345-uart";
>
> 	Arnd
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-02-20 16:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-19 23:22 [PATCH 0/4] Device Tree probing for bcm63xx-uart Florian Fainelli
2014-02-19 23:22 ` [PATCH 1/4] tty: serial: bcm63xx_uart: include linux/io.h Florian Fainelli
2014-02-19 23:22 ` [PATCH 2/4] tty: serial: bcm63xx_uart: define UART_REG_SIZE constant Florian Fainelli
2014-02-19 23:22 ` [PATCH 3/4] tty: serial: bcm63xx_uart: add support for DT probing Florian Fainelli
2014-02-20  0:00   ` Jonas Gorski
2014-02-20  1:29     ` Florian Fainelli
2014-02-20 10:59       ` Jonas Gorski
2014-02-20 12:16         ` Arnd Bergmann
2014-02-20 16:18           ` Florian Fainelli
2014-02-20 12:21         ` Mark Rutland
2014-02-19 23:22 ` [PATCH 4/4] Documentation: devicetree: add bindings documentation for bcm63xx-uart Florian Fainelli
2014-02-20  9:45   ` Mark Rutland

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.