linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/3] serial: sh-sci: Add OF support
  2013-03-04 16:20 [PATCH v4 1/3] serial: sh-sci: Add OF support Bastian Hecht
@ 2013-03-04 16:20 ` Arnd Bergmann
  2013-03-05 12:58   ` Bastian Hecht
  2013-03-04 16:20 ` [PATCH v4 2/3] ARM: mach-shmobile: r8a7740: Add DT names to clock list Bastian Hecht
  2013-03-04 16:20 ` [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT Bastian Hecht
  2 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2013-03-04 16:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 04 March 2013 17:20:52 Bastian Hecht wrote:
> diff --git a/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
> new file mode 100644
> index 0000000..6ad1adf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
> @@ -0,0 +1,53 @@
> +* Renesas SH-Mobile Serial Communication Interface
> +
> +Required properties:
> +- compatible : Should be "renesas,sci-<port type>-uart", where <port type> may be
> +  SCI, SCIF, IRDA, SCIFA or SCIFB.

Why capital letters here? Maybe just list

	"renesas,sci-uart", "renesas,scif-uart", ...

> +- reg : Address and length of the register set for the device
> +- interrupts : Should contain the following IRQs: ERI, RXI, TXI and BRI.

You probably want to require the "interrupt-names" property as well
then.

> +- cell-index : The device id.
> +- renesas,scscr : Should contain a bitfield used by the Serial Control Register.
> +  b7 = SCSCR_TIE
> +  b6 = SCSCR_RIE
> +  b5 = SCSCR_TE
> +  b4 = SCSCR_RE
> +  b3 = SCSCR_REIE
> +  b2 = SCSCR_TOIE
> +  b1 = SCSCR_CKE1
> +  b0 = SCSCR_CKE0
> +- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
> +  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
> +  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
> +  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
> +  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
> +  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)

Maybe replace this with a "clock-frequency" property? This may
be what the registers contain, but it is not very readable.

> +Optional properties:
> +- renesas,autoconf : Set if device is capable of auto configuration
> +- renesas,regtype : Overwrite the register layout. In most cases you can rely
> +  on auto-probing (omit this property or set to 0) but some legacy devices
> +  use a non-default register layout. Possible layouts are
> +  0 = SCIx_PROBE_REGTYPE (default)
> +  1 = SCIx_SCI_REGTYPE
> +  2 = SCIx_IRDA_REGTYPE
> +  3 = SCIx_SCIFA_REGTYPE
> +  4 = SCIx_SCIFB_REGTYPE
> +  5 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
> +  6 = SCIx_SH3_SCIF_REGTYPE
> +  7 = SCIx_SH4_SCIF_REGTYPE
> +  8 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
> +  9 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
> + 10 = SCIx_SH7705_SCIF_REGTYPE

Why do you keep these separate from the "compatible" property?
By definition, the register layout is already determined by
"compatible".

> +#ifdef CONFIG_OF
> +static const struct of_device_id of_sci_match[] = {
> +	{ .compatible = "renesas,sci-SCI-uart",
> +		.data = (void *)PORT_SCI },
> +	{ .compatible = "renesas,sci-SCIF-uart",
> +		.data = (void *)PORT_SCIF },
> +	{ .compatible = "renesas,sci-IRDA-uart",
> +		.data = (void *)PORT_IRDA },
> +	{ .compatible = "renesas,sci-SCIFA-uart",
> +		.data = (void *)PORT_SCIFA },
> +	{ .compatible = "renesas,sci-SCIFB-uart",
> +		.data = (void *)PORT_SCIFB },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_sci_match);
> +
> +static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
> +								int *dev_id)
> +{
> +	struct plat_sci_port *p;
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct of_device_id *match;
> +	struct resource *res;
> +	const __be32 *prop;
> +	int i, irq, val;
> +

You can remove the #ifdef by replacing it with 

	if (!IS_ENABLED(CONFIG_OF) || !np)
		return NULL;

here. This gives better compile time coverage and more readable code.

Otherwise looks very nice!

	Arnd

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

* [PATCH v4 1/3] serial: sh-sci: Add OF support
@ 2013-03-04 16:20 Bastian Hecht
  2013-03-04 16:20 ` Arnd Bergmann
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Bastian Hecht @ 2013-03-04 16:20 UTC (permalink / raw)
  To: linux-arm-kernel

We add the capabilty to probe Renesas SCI devices using Device Tree setup.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
v4:
Added missing OF header

 .../bindings/tty/serial/renesas,sci-serial.txt     |   53 ++++++++
 drivers/tty/serial/sh-sci.c                        |  127 +++++++++++++++++++-
 include/linux/serial_sci.h                         |    4 +
 3 files changed, 180 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt

diff --git a/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
new file mode 100644
index 0000000..6ad1adf
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
@@ -0,0 +1,53 @@
+* Renesas SH-Mobile Serial Communication Interface
+
+Required properties:
+- compatible : Should be "renesas,sci-<port type>-uart", where <port type> may be
+  SCI, SCIF, IRDA, SCIFA or SCIFB.
+- reg : Address and length of the register set for the device
+- interrupts : Should contain the following IRQs: ERI, RXI, TXI and BRI.
+- cell-index : The device id.
+- renesas,scscr : Should contain a bitfield used by the Serial Control Register.
+  b7 = SCSCR_TIE
+  b6 = SCSCR_RIE
+  b5 = SCSCR_TE
+  b4 = SCSCR_RE
+  b3 = SCSCR_REIE
+  b2 = SCSCR_TOIE
+  b1 = SCSCR_CKE1
+  b0 = SCSCR_CKE0
+- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
+  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
+  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
+  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
+  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
+  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)
+
+Optional properties:
+- renesas,autoconf : Set if device is capable of auto configuration
+- renesas,regtype : Overwrite the register layout. In most cases you can rely
+  on auto-probing (omit this property or set to 0) but some legacy devices
+  use a non-default register layout. Possible layouts are
+  0 = SCIx_PROBE_REGTYPE (default)
+  1 = SCIx_SCI_REGTYPE
+  2 = SCIx_IRDA_REGTYPE
+  3 = SCIx_SCIFA_REGTYPE
+  4 = SCIx_SCIFB_REGTYPE
+  5 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
+  6 = SCIx_SH3_SCIF_REGTYPE
+  7 = SCIx_SH4_SCIF_REGTYPE
+  8 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
+  9 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
+ 10 = SCIx_SH7705_SCIF_REGTYPE
+
+
+Example:
+	sci at 0xe6c50000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c50000 0x100>;
+		interrupts = <0x0c20>, <0x0c20>, <0x0c20>, <0x0c20>;
+		cell-index = <1>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 6147756..cc1b69c 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -52,6 +52,7 @@
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
 
 #ifdef CONFIG_SUPERH
 #include <asm/sh_bios.h>
@@ -2353,6 +2354,112 @@ static int sci_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id of_sci_match[] = {
+	{ .compatible = "renesas,sci-SCI-uart",
+		.data = (void *)PORT_SCI },
+	{ .compatible = "renesas,sci-SCIF-uart",
+		.data = (void *)PORT_SCIF },
+	{ .compatible = "renesas,sci-IRDA-uart",
+		.data = (void *)PORT_IRDA },
+	{ .compatible = "renesas,sci-SCIFA-uart",
+		.data = (void *)PORT_SCIFA },
+	{ .compatible = "renesas,sci-SCIFB-uart",
+		.data = (void *)PORT_SCIFB },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_sci_match);
+
+static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
+								int *dev_id)
+{
+	struct plat_sci_port *p;
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct resource *res;
+	const __be32 *prop;
+	int i, irq, val;
+
+	match = of_match_node(of_sci_match, pdev->dev.of_node);
+	if (!match || !match->data) {
+		dev_err(&pdev->dev, "OF match error\n");
+		return NULL;
+	}
+
+	p = devm_kzalloc(&pdev->dev, sizeof(struct plat_sci_port), GFP_KERNEL);
+	if (!p) {
+		dev_err(&pdev->dev, "failed to allocate DT config data\n");
+		return NULL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get I/O memory\n");
+		return NULL;
+	}
+	p->mapbase = res->start;
+
+	for (i = 0; i < SCIx_NR_IRQS; i++) {
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0) {
+			dev_err(&pdev->dev, "failed to get irq data %d\n", i);
+			return NULL;
+		}
+		p->irqs[i] = irq;
+	}
+
+	prop = of_get_property(np, "cell-index", NULL);
+	if (!prop) {
+		dev_err(&pdev->dev, "required DT prop cell-index missing\n");
+		return NULL;
+	}
+	*dev_id = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,scscr", NULL);
+	if (!prop) {
+		dev_err(&pdev->dev, "required DT prop scscr missing\n");
+		return NULL;
+	}
+	p->scscr = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,scbrr-algo-id", NULL);
+	if (!prop) {
+		dev_err(&pdev->dev, "required DT prop scbrr-algo-id missing\n");
+		return NULL;
+	}
+	val = be32_to_cpup(prop);
+	if (val <= SCBRR_ALGO_INVALID || val >= SCBRR_NR_ALGOS) {
+		dev_err(&pdev->dev, "DT prop scbrr-algo-id out of range\n");
+		return NULL;
+	}
+	p->scbrr_algo_id = val;
+
+	p->flags = UPF_IOREMAP;
+	if (of_get_property(np, "renesas,autoconf", NULL))
+		p->flags |= UPF_BOOT_AUTOCONF;
+
+	prop = of_get_property(np, "renesas,regtype", NULL);
+	if (prop) {
+		val = be32_to_cpup(prop);
+		if (val < SCIx_PROBE_REGTYPE || val >= SCIx_NR_REGTYPES) {
+			dev_err(&pdev->dev, "DT prop regtype out of range\n");
+			return NULL;
+		}
+		p->regtype = val;
+	}
+
+	p->type = (unsigned int)match->data;
+
+	return p;
+}
+#else
+static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
+								int *dev_id)
+{
+	return NULL;
+}
+#endif /* CONFIG_OF */
+
 static int sci_probe_single(struct platform_device *dev,
 				      unsigned int index,
 				      struct plat_sci_port *p,
@@ -2385,9 +2492,9 @@ static int sci_probe_single(struct platform_device *dev,
 
 static int sci_probe(struct platform_device *dev)
 {
-	struct plat_sci_port *p = dev->dev.platform_data;
-	struct sci_port *sp = &sci_ports[dev->id];
-	int ret;
+	struct plat_sci_port *p;
+	struct sci_port *sp;
+	int ret, dev_id = dev->id;
 
 	/*
 	 * If we've come here via earlyprintk initialization, head off to
@@ -2397,9 +2504,20 @@ static int sci_probe(struct platform_device *dev)
 	if (is_early_platform_device(dev))
 		return sci_probe_earlyprintk(dev);
 
+	if (dev->dev.of_node)
+		p = sci_parse_dt(dev, &dev_id);
+	else
+		p = dev->dev.platform_data;
+
+	if (!p) {
+		dev_err(&dev->dev, "no setup data supplied\n");
+		return -EINVAL;
+	}
+
+	sp = &sci_ports[dev_id];
 	platform_set_drvdata(dev, sp);
 
-	ret = sci_probe_single(dev, dev->id, p, sp);
+	ret = sci_probe_single(dev, dev_id, p, sp);
 	if (ret)
 		return ret;
 
@@ -2451,6 +2569,7 @@ static struct platform_driver sci_driver = {
 		.name	= "sh-sci",
 		.owner	= THIS_MODULE,
 		.pm	= &sci_dev_pm_ops,
+		.of_match_table = of_match_ptr(of_sci_match),
 	},
 };
 
diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
index eb763ad..857eec4 100644
--- a/include/linux/serial_sci.h
+++ b/include/linux/serial_sci.h
@@ -11,11 +11,15 @@
 #define SCIx_NOT_SUPPORTED	(-1)
 
 enum {
+	SCBRR_ALGO_INVALID,
+
 	SCBRR_ALGO_1,		/* ((clk + 16 * bps) / (16 * bps) - 1) */
 	SCBRR_ALGO_2,		/* ((clk + 16 * bps) / (32 * bps) - 1) */
 	SCBRR_ALGO_3,		/* (((clk * 2) + 16 * bps) / (16 * bps) - 1) */
 	SCBRR_ALGO_4,		/* (((clk * 2) + 16 * bps) / (32 * bps) - 1) */
 	SCBRR_ALGO_5,		/* (((clk * 1000 / 32) / bps) - 1) */
+
+	SCBRR_NR_ALGOS,
 };
 
 #define SCSCR_TIE	(1 << 7)
-- 
1.7.9.5

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

* [PATCH v4 2/3] ARM: mach-shmobile: r8a7740: Add DT names to clock list
  2013-03-04 16:20 [PATCH v4 1/3] serial: sh-sci: Add OF support Bastian Hecht
  2013-03-04 16:20 ` Arnd Bergmann
@ 2013-03-04 16:20 ` Bastian Hecht
  2013-03-04 16:20 ` [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT Bastian Hecht
  2 siblings, 0 replies; 14+ messages in thread
From: Bastian Hecht @ 2013-03-04 16:20 UTC (permalink / raw)
  To: linux-arm-kernel

This adds temporarily the alternative device names to the clock list
that are used when booting via Device Tree setup.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
v4: no changes

 arch/arm/mach-shmobile/clock-r8a7740.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c b/arch/arm/mach-shmobile/clock-r8a7740.c
index 71fc400..0f77823 100644
--- a/arch/arm/mach-shmobile/clock-r8a7740.c
+++ b/arch/arm/mach-shmobile/clock-r8a7740.c
@@ -593,18 +593,27 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_DEV_ID("sh_mobile_ceu.1",	&mstp_clks[MSTP128]),
 
 	CLKDEV_DEV_ID("sh-sci.4",		&mstp_clks[MSTP200]),
+	CLKDEV_DEV_ID("e6c80000.sci",		&mstp_clks[MSTP200]),
 	CLKDEV_DEV_ID("sh-sci.3",		&mstp_clks[MSTP201]),
+	CLKDEV_DEV_ID("e6c70000.sci",		&mstp_clks[MSTP201]),
 	CLKDEV_DEV_ID("sh-sci.2",		&mstp_clks[MSTP202]),
+	CLKDEV_DEV_ID("e6c60000.sci",		&mstp_clks[MSTP202]),
 	CLKDEV_DEV_ID("sh-sci.1",		&mstp_clks[MSTP203]),
+	CLKDEV_DEV_ID("e6c50000.sci",		&mstp_clks[MSTP203]),
 	CLKDEV_DEV_ID("sh-sci.0",		&mstp_clks[MSTP204]),
+	CLKDEV_DEV_ID("e6c40000.sci",		&mstp_clks[MSTP204]),
 	CLKDEV_DEV_ID("sh-sci.8",		&mstp_clks[MSTP206]),
+	CLKDEV_DEV_ID("e6c30000.sci",		&mstp_clks[MSTP206]),
 	CLKDEV_DEV_ID("sh-sci.5",		&mstp_clks[MSTP207]),
+	CLKDEV_DEV_ID("e6cb0000.sci",		&mstp_clks[MSTP207]),
 	CLKDEV_DEV_ID("sh-dma-engine.3",	&mstp_clks[MSTP214]),
 	CLKDEV_DEV_ID("sh-dma-engine.2",	&mstp_clks[MSTP216]),
 	CLKDEV_DEV_ID("sh-dma-engine.1",	&mstp_clks[MSTP217]),
 	CLKDEV_DEV_ID("sh-dma-engine.0",	&mstp_clks[MSTP218]),
 	CLKDEV_DEV_ID("sh-sci.7",		&mstp_clks[MSTP222]),
+	CLKDEV_DEV_ID("e6cd0000.sci",		&mstp_clks[MSTP222]),
 	CLKDEV_DEV_ID("sh-sci.6",		&mstp_clks[MSTP230]),
+	CLKDEV_DEV_ID("e6cc0000.sci",		&mstp_clks[MSTP230]),
 
 	CLKDEV_DEV_ID("sh_cmt.10",		&mstp_clks[MSTP329]),
 	CLKDEV_DEV_ID("sh_fsi2",		&mstp_clks[MSTP328]),
-- 
1.7.9.5

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

* [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT
  2013-03-04 16:20 [PATCH v4 1/3] serial: sh-sci: Add OF support Bastian Hecht
  2013-03-04 16:20 ` Arnd Bergmann
  2013-03-04 16:20 ` [PATCH v4 2/3] ARM: mach-shmobile: r8a7740: Add DT names to clock list Bastian Hecht
@ 2013-03-04 16:20 ` Bastian Hecht
  2013-03-04 16:22   ` Arnd Bergmann
  2 siblings, 1 reply; 14+ messages in thread
From: Bastian Hecht @ 2013-03-04 16:20 UTC (permalink / raw)
  To: linux-arm-kernel

We can now use the Device Tree for bringing up our serial devices on
the SoC r8a7740.

We remove the power domain association. We will move the info into the
DT setup as soon as we have support for it. For now this is fine as we
use the power domain governor "pm_domain_always_on_gov" for A3SP.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
v4:
We don't move the DT into the r8a7740-armadillo800eva-reference.dts, but
r8a7740.dtsi as the serial device setups are not dependent on any board
confguration.

Renamed sci at 0xe6c40000 to sci at e6c40000

 arch/arm/boot/dts/r8a7740.dtsi         |   99 ++++++++++++++++++
 arch/arm/mach-shmobile/setup-r8a7740.c |  180 --------------------------------
 2 files changed, 99 insertions(+), 180 deletions(-)

diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
index 82faf52..ecbc237 100644
--- a/arch/arm/boot/dts/r8a7740.dtsi
+++ b/arch/arm/boot/dts/r8a7740.dtsi
@@ -767,4 +767,103 @@
 		gpio-controller;
 		#gpio-cells = <2>;
 	};
+
+	sci at e6c40000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c40000 0x100>;
+		interrupts = <0x0c00>, <0x0c00>, <0x0c00>, <0x0c00>;
+		cell-index = <0>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6c50000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c50000 0x100>;
+		interrupts = <0x0c20>, <0x0c20>, <0x0c20>, <0x0c20>;
+		cell-index = <1>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6c60000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c60000 0x100>;
+		interrupts = <0x0c40>, <0x0c40>, <0x0c40>, <0x0c40>;
+		cell-index = <2>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6c70000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c70000 0x100>;
+		interrupts = <0x0c60>, <0x0c60>, <0x0c60>, <0x0c60>;
+		cell-index = <3>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6c80000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c80000 0x100>;
+		interrupts = <0x0d20>, <0x0d20>, <0x0d20>, <0x0d20>;
+		cell-index = <4>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6cb0000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6cb0000 0x100>;
+		interrupts = <0x0d40>, <0x0d40>, <0x0d40>, <0x0d40>;
+		cell-index = <5>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6cc0000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6cc0000 0x100>;
+		interrupts = <0x04c0>, <0x04c0>, <0x04c0>, <0x04c0>;
+		cell-index = <6>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at 0xe6cd0000 {
+		compatible = "renesas,sci-SCIFA-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6cd0000 0x100>;
+		interrupts = <0x04e0>, <0x04e0>, <0x04e0>, <0x04e0>;
+		cell-index = <7>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
+
+	sci at e6c30000 {
+		compatible = "renesas,sci-SCIFB-uart";
+		interrupt-parent = <&intca>;
+		reg = <0xe6c30000 0x100>;
+		interrupts = <0x0d60>, <0x0d60>, <0x0d60>, <0x0d60>;
+		cell-index = <8>;
+		renesas,scscr = <0x30>;
+		renesas,scbrr-algo-id = <4>;
+		renesas,autoconf;
+	};
 };
diff --git a/arch/arm/mach-shmobile/setup-r8a7740.c b/arch/arm/mach-shmobile/setup-r8a7740.c
index f35e7c2..5a563cd 100644
--- a/arch/arm/mach-shmobile/setup-r8a7740.c
+++ b/arch/arm/mach-shmobile/setup-r8a7740.c
@@ -93,168 +93,6 @@ void __init r8a7740_pinmux_init(void)
 	platform_device_register(&r8a7740_pfc_device);
 }
 
-/* SCIFA0 */
-static struct plat_sci_port scif0_platform_data = {
-	.mapbase	= 0xe6c40000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0c00)),
-};
-
-static struct platform_device scif0_device = {
-	.name		= "sh-sci",
-	.id		= 0,
-	.dev		= {
-		.platform_data	= &scif0_platform_data,
-	},
-};
-
-/* SCIFA1 */
-static struct plat_sci_port scif1_platform_data = {
-	.mapbase	= 0xe6c50000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0c20)),
-};
-
-static struct platform_device scif1_device = {
-	.name		= "sh-sci",
-	.id		= 1,
-	.dev		= {
-		.platform_data	= &scif1_platform_data,
-	},
-};
-
-/* SCIFA2 */
-static struct plat_sci_port scif2_platform_data = {
-	.mapbase	= 0xe6c60000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0c40)),
-};
-
-static struct platform_device scif2_device = {
-	.name		= "sh-sci",
-	.id		= 2,
-	.dev		= {
-		.platform_data	= &scif2_platform_data,
-	},
-};
-
-/* SCIFA3 */
-static struct plat_sci_port scif3_platform_data = {
-	.mapbase	= 0xe6c70000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0c60)),
-};
-
-static struct platform_device scif3_device = {
-	.name		= "sh-sci",
-	.id		= 3,
-	.dev		= {
-		.platform_data	= &scif3_platform_data,
-	},
-};
-
-/* SCIFA4 */
-static struct plat_sci_port scif4_platform_data = {
-	.mapbase	= 0xe6c80000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0d20)),
-};
-
-static struct platform_device scif4_device = {
-	.name		= "sh-sci",
-	.id		= 4,
-	.dev		= {
-		.platform_data	= &scif4_platform_data,
-	},
-};
-
-/* SCIFA5 */
-static struct plat_sci_port scif5_platform_data = {
-	.mapbase	= 0xe6cb0000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0d40)),
-};
-
-static struct platform_device scif5_device = {
-	.name		= "sh-sci",
-	.id		= 5,
-	.dev		= {
-		.platform_data	= &scif5_platform_data,
-	},
-};
-
-/* SCIFA6 */
-static struct plat_sci_port scif6_platform_data = {
-	.mapbase	= 0xe6cc0000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x04c0)),
-};
-
-static struct platform_device scif6_device = {
-	.name		= "sh-sci",
-	.id		= 6,
-	.dev		= {
-		.platform_data	= &scif6_platform_data,
-	},
-};
-
-/* SCIFA7 */
-static struct plat_sci_port scif7_platform_data = {
-	.mapbase	= 0xe6cd0000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFA,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x04e0)),
-};
-
-static struct platform_device scif7_device = {
-	.name		= "sh-sci",
-	.id		= 7,
-	.dev		= {
-		.platform_data	= &scif7_platform_data,
-	},
-};
-
-/* SCIFB */
-static struct plat_sci_port scifb_platform_data = {
-	.mapbase	= 0xe6c30000,
-	.flags		= UPF_BOOT_AUTOCONF,
-	.scscr		= SCSCR_RE | SCSCR_TE,
-	.scbrr_algo_id	= SCBRR_ALGO_4,
-	.type		= PORT_SCIFB,
-	.irqs		= SCIx_IRQ_MUXED(evt2irq(0x0d60)),
-};
-
-static struct platform_device scifb_device = {
-	.name		= "sh-sci",
-	.id		= 8,
-	.dev		= {
-		.platform_data	= &scifb_platform_data,
-	},
-};
-
 /* CMT */
 static struct sh_timer_config cmt10_platform_data = {
 	.name = "CMT10",
@@ -379,15 +217,6 @@ static struct platform_device tmu02_device = {
 };
 
 static struct platform_device *r8a7740_devices_dt[] __initdata = {
-	&scif0_device,
-	&scif1_device,
-	&scif2_device,
-	&scif3_device,
-	&scif4_device,
-	&scif5_device,
-	&scif6_device,
-	&scif7_device,
-	&scifb_device,
 	&cmt10_device,
 	&tmu00_device,
 	&tmu01_device,
@@ -812,15 +641,6 @@ void __init r8a7740_add_standard_devices(void)
 
 	/* add devices to PM domain  */
 
-	rmobile_add_device_to_domain("A3SP",	&scif0_device);
-	rmobile_add_device_to_domain("A3SP",	&scif1_device);
-	rmobile_add_device_to_domain("A3SP",	&scif2_device);
-	rmobile_add_device_to_domain("A3SP",	&scif3_device);
-	rmobile_add_device_to_domain("A3SP",	&scif4_device);
-	rmobile_add_device_to_domain("A3SP",	&scif5_device);
-	rmobile_add_device_to_domain("A3SP",	&scif6_device);
-	rmobile_add_device_to_domain("A3SP",	&scif7_device);
-	rmobile_add_device_to_domain("A3SP",	&scifb_device);
 	rmobile_add_device_to_domain("A3SP",	&i2c1_device);
 }
 
-- 
1.7.9.5

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

* [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT
  2013-03-04 16:20 ` [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT Bastian Hecht
@ 2013-03-04 16:22   ` Arnd Bergmann
  2013-03-05 13:00     ` Bastian Hecht
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2013-03-04 16:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 04 March 2013 17:20:54 Bastian Hecht wrote:
> 
> +       sci at e6c40000 {
> +               compatible = "renesas,sci-SCIFA-uart";
> +               interrupt-parent = <&intca>;
> +               reg = <0xe6c40000 0x100>;
> +               interrupts = <0x0c00>, <0x0c00>, <0x0c00>, <0x0c00>;
> +               cell-index = <0>;
> +               renesas,scscr = <0x30>;
> +               renesas,scbrr-algo-id = <4>;
> +               renesas,autoconf;
> +       };

The default name for a uart is "serial", I would recommend sticking to
that by convention. You may also want to add an "aliases" node
somewhere, to define which port should get which logical device number
on a given board.

	Arnd

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

* [PATCH v4 1/3] serial: sh-sci: Add OF support
  2013-03-04 16:20 ` Arnd Bergmann
@ 2013-03-05 12:58   ` Bastian Hecht
  2013-03-05 19:26     ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Bastian Hecht @ 2013-03-05 12:58 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Arnd,

thanks for having a look at it.

2013/3/4 Arnd Bergmann <arnd@arndb.de>:
> On Monday 04 March 2013 17:20:52 Bastian Hecht wrote:
>> diff --git a/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
>> new file mode 100644
>> index 0000000..6ad1adf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
>> @@ -0,0 +1,53 @@
>> +* Renesas SH-Mobile Serial Communication Interface
>> +
>> +Required properties:
>> +- compatible : Should be "renesas,sci-<port type>-uart", where <port type> may be
>> +  SCI, SCIF, IRDA, SCIFA or SCIFB.
>
> Why capital letters here? Maybe just list
>
>         "renesas,sci-uart", "renesas,scif-uart", ...

Yes - changed.

>> +- reg : Address and length of the register set for the device
>> +- interrupts : Should contain the following IRQs: ERI, RXI, TXI and BRI.
>
> You probably want to require the "interrupt-names" property as well
> then.

I added the interrupt-names property but still enforce the ordering in
the binding specs.
Don't know if we want the extra overhead to scan for strings or just
take a certain order for granted.
If we want to change it to parsing, it would be better to switch
completely to platform_get_irq_byname() and change all the current
platform code, else we will produce code overhead.

>> +- cell-index : The device id.
>> +- renesas,scscr : Should contain a bitfield used by the Serial Control Register.
>> +  b7 = SCSCR_TIE
>> +  b6 = SCSCR_RIE
>> +  b5 = SCSCR_TE
>> +  b4 = SCSCR_RE
>> +  b3 = SCSCR_REIE
>> +  b2 = SCSCR_TOIE
>> +  b1 = SCSCR_CKE1
>> +  b0 = SCSCR_CKE0
>> +- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
>> +  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
>> +  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
>> +  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
>> +  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
>> +  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)
>
> Maybe replace this with a "clock-frequency" property? This may
> be what the registers contain, but it is not very readable.

Hmm... do you want a frequency in absolute terms? And then calculate
the best SCBRR value for it?
I'm unsure about this, either you must exactly hit it or accept
tolerances? As something in between I renamed the property to
"renesas,clock-algorithm", but I don't know if this is what you want.

>> +Optional properties:
>> +- renesas,autoconf : Set if device is capable of auto configuration
>> +- renesas,regtype : Overwrite the register layout. In most cases you can rely
>> +  on auto-probing (omit this property or set to 0) but some legacy devices
>> +  use a non-default register layout. Possible layouts are
>> +  0 = SCIx_PROBE_REGTYPE (default)
>> +  1 = SCIx_SCI_REGTYPE
>> +  2 = SCIx_IRDA_REGTYPE
>> +  3 = SCIx_SCIFA_REGTYPE
>> +  4 = SCIx_SCIFB_REGTYPE
>> +  5 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
>> +  6 = SCIx_SH3_SCIF_REGTYPE
>> +  7 = SCIx_SH4_SCIF_REGTYPE
>> +  8 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
>> +  9 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
>> + 10 = SCIx_SH7705_SCIF_REGTYPE
>
> Why do you keep these separate from the "compatible" property?
> By definition, the register layout is already determined by
> "compatible".

Ok, I've melted the register type and port type (defined in
include/uapi/linux/serial_core.h) definition into the compatible
property. Looks good!

>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_sci_match[] = {
>> +     { .compatible = "renesas,sci-SCI-uart",
>> +             .data = (void *)PORT_SCI },
>> +     { .compatible = "renesas,sci-SCIF-uart",
>> +             .data = (void *)PORT_SCIF },
>> +     { .compatible = "renesas,sci-IRDA-uart",
>> +             .data = (void *)PORT_IRDA },
>> +     { .compatible = "renesas,sci-SCIFA-uart",
>> +             .data = (void *)PORT_SCIFA },
>> +     { .compatible = "renesas,sci-SCIFB-uart",
>> +             .data = (void *)PORT_SCIFB },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_sci_match);
>> +
>> +static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>> +                                                             int *dev_id)
>> +{
>> +     struct plat_sci_port *p;
>> +     struct device_node *np = pdev->dev.of_node;
>> +     const struct of_device_id *match;
>> +     struct resource *res;
>> +     const __be32 *prop;
>> +     int i, irq, val;
>> +
>
> You can remove the #ifdef by replacing it with
>
>         if (!IS_ENABLED(CONFIG_OF) || !np)
>                 return NULL;
>
> here. This gives better compile time coverage and more readable code.

Alright, done!

> Otherwise looks very nice!

Cheers!

 Bastian


>         Arnd

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

* [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT
  2013-03-04 16:22   ` Arnd Bergmann
@ 2013-03-05 13:00     ` Bastian Hecht
  2013-03-05 13:42       ` Sergei Shtylyov
  2013-03-05 19:22       ` Arnd Bergmann
  0 siblings, 2 replies; 14+ messages in thread
From: Bastian Hecht @ 2013-03-05 13:00 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Arnd,

2013/3/4 Arnd Bergmann <arnd@arndb.de>:
> On Monday 04 March 2013 17:20:54 Bastian Hecht wrote:
>>
>> +       sci at e6c40000 {
>> +               compatible = "renesas,sci-SCIFA-uart";
>> +               interrupt-parent = <&intca>;
>> +               reg = <0xe6c40000 0x100>;
>> +               interrupts = <0x0c00>, <0x0c00>, <0x0c00>, <0x0c00>;
>> +               cell-index = <0>;
>> +               renesas,scscr = <0x30>;
>> +               renesas,scbrr-algo-id = <4>;
>> +               renesas,autoconf;
>> +       };
>
> The default name for a uart is "serial", I would recommend sticking to
> that by convention. You may also want to add an "aliases" node
> somewhere, to define which port should get which logical device number
> on a given board.

Switched to "...-serial".
About the aliases: Would I need to add that now, or can we postpone that?

Thanks,

 Bastian


>         Arnd

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

* [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT
  2013-03-05 13:00     ` Bastian Hecht
@ 2013-03-05 13:42       ` Sergei Shtylyov
  2013-03-05 13:47         ` Bastian Hecht
  2013-03-05 19:22       ` Arnd Bergmann
  1 sibling, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2013-03-05 13:42 UTC (permalink / raw)
  To: linux-arm-kernel

Hello.

On 05-03-2013 17:00, Bastian Hecht wrote:

>>> +       sci at e6c40000 {
>>> +               compatible = "renesas,sci-SCIFA-uart";
>>> +               interrupt-parent = <&intca>;
>>> +               reg = <0xe6c40000 0x100>;
>>> +               interrupts = <0x0c00>, <0x0c00>, <0x0c00>, <0x0c00>;
>>> +               cell-index = <0>;
>>> +               renesas,scscr = <0x30>;
>>> +               renesas,scbrr-algo-id = <4>;
>>> +               renesas,autoconf;
>>> +       };
>>
>> The default name for a uart is "serial", I would recommend sticking to
>> that by convention. You may also want to add an "aliases" node
>> somewhere, to define which port should get which logical device number
>> on a given board.

> Switched to "...-serial".

    I think he was talking about the node name, not "compatible" property.

WBR, Sergei

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

* [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT
  2013-03-05 13:42       ` Sergei Shtylyov
@ 2013-03-05 13:47         ` Bastian Hecht
  0 siblings, 0 replies; 14+ messages in thread
From: Bastian Hecht @ 2013-03-05 13:47 UTC (permalink / raw)
  To: linux-arm-kernel

2013/3/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
>
> On 05-03-2013 17:00, Bastian Hecht wrote:
>
>>>> +       sci at e6c40000 {
>>>> +               compatible = "renesas,sci-SCIFA-uart";
>>>> +               interrupt-parent = <&intca>;
>>>> +               reg = <0xe6c40000 0x100>;
>>>> +               interrupts = <0x0c00>, <0x0c00>, <0x0c00>, <0x0c00>;
>>>> +               cell-index = <0>;
>>>> +               renesas,scscr = <0x30>;
>>>> +               renesas,scbrr-algo-id = <4>;
>>>> +               renesas,autoconf;
>>>> +       };
>>>
>>>
>>> The default name for a uart is "serial", I would recommend sticking to
>>> that by convention. You may also want to add an "aliases" node
>>> somewhere, to define which port should get which logical device number
>>> on a given board.
>
>
>> Switched to "...-serial".
>
>
>    I think he was talking about the node name, not "compatible" property.

Ah sure! I'll wait for responses to v5 and will change it around in v6.

Thanks,

 Bastian


> WBR, Sergei
>

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

* [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT
  2013-03-05 13:00     ` Bastian Hecht
  2013-03-05 13:42       ` Sergei Shtylyov
@ 2013-03-05 19:22       ` Arnd Bergmann
  1 sibling, 0 replies; 14+ messages in thread
From: Arnd Bergmann @ 2013-03-05 19:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday 05 March 2013, Bastian Hecht wrote:
> 2013/3/4 Arnd Bergmann <arnd@arndb.de>:
> > On Monday 04 March 2013 17:20:54 Bastian Hecht wrote:
> >>
> >> +       sci at e6c40000 {
> >> +               compatible = "renesas,sci-SCIFA-uart";
> >> +               interrupt-parent = <&intca>;
> >> +               reg = <0xe6c40000 0x100>;
> >> +               interrupts = <0x0c00>, <0x0c00>, <0x0c00>, <0x0c00>;
> >> +               cell-index = <0>;
> >> +               renesas,scscr = <0x30>;
> >> +               renesas,scbrr-algo-id = <4>;
> >> +               renesas,autoconf;
> >> +       };
> >
> > The default name for a uart is "serial", I would recommend sticking to
> > that by convention. You may also want to add an "aliases" node
> > somewhere, to define which port should get which logical device number
> > on a given board.
> 
> Switched to "...-serial".

I actually meant the name, not the "compatible" value. For compatible, either
version is fine with me. What I think you should change though is renaming
"sci at e6c40000" to "serial at e6c40000".

> About the aliases: Would I need to add that now, or can we postpone that?
> 

No hurry for that. It's just a feature that comes in very handy, but not
all platforms implement it.

	Arnd

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

* [PATCH v4 1/3] serial: sh-sci: Add OF support
  2013-03-05 12:58   ` Bastian Hecht
@ 2013-03-05 19:26     ` Arnd Bergmann
  2013-03-06  0:50       ` Paul Mundt
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2013-03-05 19:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday 05 March 2013, Bastian Hecht wrote:
> 2013/3/4 Arnd Bergmann <arnd@arndb.de>:
> > On Monday 04 March 2013 17:20:52 Bastian Hecht wrote:
> >> +- reg : Address and length of the register set for the device
> >> +- interrupts : Should contain the following IRQs: ERI, RXI, TXI and BRI.
> >
> > You probably want to require the "interrupt-names" property as well
> > then.
> 
> I added the interrupt-names property but still enforce the ordering in
> the binding specs.

Yes, that is the correct approach.

> Don't know if we want the extra overhead to scan for strings or just
> take a certain order for granted.
> If we want to change it to parsing, it would be better to switch
> completely to platform_get_irq_byname() and change all the current
> platform code, else we will produce code overhead.

Right, there is no need to change the code for that, but I think it
makes sense to do the binding the way you describe, in case someone
later wants to move to platform_get_irq_byname. A lot of people prefer
that for some reason, although I think the traditional numbering is
just as good.

> >> +- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
> >> +  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
> >> +  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
> >> +  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
> >> +  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
> >> +  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)
> >
> > Maybe replace this with a "clock-frequency" property? This may
> > be what the registers contain, but it is not very readable.
> 
> Hmm... do you want a frequency in absolute terms? And then calculate
> the best SCBRR value for it?
> I'm unsure about this, either you must exactly hit it or accept
> tolerances? As something in between I renamed the property to
> "renesas,clock-algorithm", but I don't know if this is what you want.

I meant the absolute frequency in HZ, since that is what we do
for the 8250 uart and other devices, but only if that is sufficient
for your device.

	Arnd

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

* [PATCH v4 1/3] serial: sh-sci: Add OF support
  2013-03-05 19:26     ` Arnd Bergmann
@ 2013-03-06  0:50       ` Paul Mundt
  2013-03-06 10:19         ` Bastian Hecht
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Mundt @ 2013-03-06  0:50 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Mar 05, 2013 at 07:26:25PM +0000, Arnd Bergmann wrote:
> On Tuesday 05 March 2013, Bastian Hecht wrote:
> > >> +- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
> > >> +  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
> > >> +  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
> > >> +  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
> > >> +  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
> > >> +  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)
> > >
> > > Maybe replace this with a "clock-frequency" property? This may
> > > be what the registers contain, but it is not very readable.
> > 
> > Hmm... do you want a frequency in absolute terms? And then calculate
> > the best SCBRR value for it?
> > I'm unsure about this, either you must exactly hit it or accept
> > tolerances? As something in between I renamed the property to
> > "renesas,clock-algorithm", but I don't know if this is what you want.
> 
> I meant the absolute frequency in HZ, since that is what we do
> for the 8250 uart and other devices, but only if that is sufficient
> for your device.
> 
No, we still need to figure out how to generate that baud rate, whether
it needs an internal or external clock for driving the rate, etc. The
frequency in and of itself doesn't provide this information, and various
parts use different algorithms for factoring the baud rate generator,
even varying across otherwise identical ports. It's unfortunately not
possible to infer anything about the SCBRR algorithm from port type or
specified baud rate.

The algorithm IDs here are wholly arbitrary anyways, but are the
variations I came up with from roughly 60-70 different CPUs.

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

* [PATCH v4 1/3] serial: sh-sci: Add OF support
  2013-03-06  0:50       ` Paul Mundt
@ 2013-03-06 10:19         ` Bastian Hecht
  2013-03-06 10:28           ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Bastian Hecht @ 2013-03-06 10:19 UTC (permalink / raw)
  To: linux-arm-kernel

Hi all,

2013/3/6 Paul Mundt <lethal@linux-sh.org>:
> On Tue, Mar 05, 2013 at 07:26:25PM +0000, Arnd Bergmann wrote:
>> On Tuesday 05 March 2013, Bastian Hecht wrote:
>> > >> +- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
>> > >> +  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
>> > >> +  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
>> > >> +  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
>> > >> +  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
>> > >> +  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)
>> > >
>> > > Maybe replace this with a "clock-frequency" property? This may
>> > > be what the registers contain, but it is not very readable.
>> >
>> > Hmm... do you want a frequency in absolute terms? And then calculate
>> > the best SCBRR value for it?
>> > I'm unsure about this, either you must exactly hit it or accept
>> > tolerances? As something in between I renamed the property to
>> > "renesas,clock-algorithm", but I don't know if this is what you want.
>>
>> I meant the absolute frequency in HZ, since that is what we do
>> for the 8250 uart and other devices, but only if that is sufficient
>> for your device.
>>
> No, we still need to figure out how to generate that baud rate, whether
> it needs an internal or external clock for driving the rate, etc. The
> frequency in and of itself doesn't provide this information, and various
> parts use different algorithms for factoring the baud rate generator,
> even varying across otherwise identical ports. It's unfortunately not
> possible to infer anything about the SCBRR algorithm from port type or
> specified baud rate.
>
> The algorithm IDs here are wholly arbitrary anyways, but are the
> variations I came up with from roughly 60-70 different CPUs.

So if we stick with the notion of the algrithm ID everything is settled now.

I will prepare a v6 that I will post as the whole patchset and an
additional incremental patch 1/3 from v3 to v6 for Paul's repo. Then
we can see if the SCBRR thing needs further discussion and if the rest
of the patchset is final.

Thanks to you all,

 Bastian

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

* [PATCH v4 1/3] serial: sh-sci: Add OF support
  2013-03-06 10:19         ` Bastian Hecht
@ 2013-03-06 10:28           ` Arnd Bergmann
  0 siblings, 0 replies; 14+ messages in thread
From: Arnd Bergmann @ 2013-03-06 10:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Wednesday 06 March 2013, Bastian Hecht wrote:
> 2013/3/6 Paul Mundt <lethal@linux-sh.org>:
> > No, we still need to figure out how to generate that baud rate, whether
> > it needs an internal or external clock for driving the rate, etc. The
> > frequency in and of itself doesn't provide this information, and various
> > parts use different algorithms for factoring the baud rate generator,
> > even varying across otherwise identical ports. It's unfortunately not
> > possible to infer anything about the SCBRR algorithm from port type or
> > specified baud rate.
> >
> > The algorithm IDs here are wholly arbitrary anyways, but are the
> > variations I came up with from roughly 60-70 different CPUs.

Ok, I see.

> So if we stick with the notion of the algrithm ID everything is settled now.
> 
> I will prepare a v6 that I will post as the whole patchset and an
> additional incremental patch 1/3 from v3 to v6 for Paul's repo. Then
> we can see if the SCBRR thing needs further discussion and if the rest
> of the patchset is final.

I can't think of anything better either, so let's stick to putting the
algorithm ID into the DT binding.

	Arnd

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

end of thread, other threads:[~2013-03-06 10:28 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-04 16:20 [PATCH v4 1/3] serial: sh-sci: Add OF support Bastian Hecht
2013-03-04 16:20 ` Arnd Bergmann
2013-03-05 12:58   ` Bastian Hecht
2013-03-05 19:26     ` Arnd Bergmann
2013-03-06  0:50       ` Paul Mundt
2013-03-06 10:19         ` Bastian Hecht
2013-03-06 10:28           ` Arnd Bergmann
2013-03-04 16:20 ` [PATCH v4 2/3] ARM: mach-shmobile: r8a7740: Add DT names to clock list Bastian Hecht
2013-03-04 16:20 ` [PATCH v4 3/3] ARM: mach-shmobile: r8a7740: Setup the serial devices using DT Bastian Hecht
2013-03-04 16:22   ` Arnd Bergmann
2013-03-05 13:00     ` Bastian Hecht
2013-03-05 13:42       ` Sergei Shtylyov
2013-03-05 13:47         ` Bastian Hecht
2013-03-05 19:22       ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).