[PATCH 1/3] serial: sh-sci: Add Device Tree probing

[PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Bastian Hecht]

We add the capabilty to probe SH SCI devices using Device Tree setup. We
split SoC dependent config data from board dependent data and store it
inside the driver. Support for the SoCs sh7372, sh73a0, r8a7740 and
r8a7779 has been added. The required fields for booting via DT are
documented in renesas,sh-sci-serial.txt.

This is an early draft and needs still discussion about SoC/board
related properties.

Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
---
 .../bindings/tty/serial/renesas,sh-sci-serial.txt  |   27 +++++
 drivers/tty/serial/sh-sci.c                        |  120 +++++++++++++++++++-
 2 files changed, 143 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt

diff --git a/Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt b/Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
new file mode 100644
index 0000000..69df44d
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
@@ -0,0 +1,27 @@
+* Renesas SH-Mobile Serial Communication Interface
+
+Required properties:
+- compatible : Should be "renesas,shmobile-sci-hwb-<x>", where <x> denotes a
+  hardware block version number of the following list - choose it according
+  to your SoC in use.
+  1: sh7372, sh73a0, r8a7740
+  2: r8a7779
+- reg : Address and length of the register set for the device
+- interrupts : Should contain the following IRQs: ERI, RXI, TXI and BRI.
+- renesas,serial-type : Specifies the type of the serial port.
+	0: SCI
+	1: SCIF
+	2: IRDA
+	3: SCIFA
+	4: SCIFB
+- cell-index : The device id.
+
+Example:
+        sci@0xe6c50000 {
+                compatible = "renesas,shmobile-sci-hwb-1";
+                interrupt-parent = <&intca>;
+                reg = <0xe6c50000 0x100>;
+                interrupts = <0x0c20>, <0x0c20>, <0x0c20>, <0x0c20>;
+                cell-index = <1>;
+                renesas,serial-type = <3>;
+        };
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 6147756..4f3613d 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2353,6 +2353,106 @@ static int sci_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+/*
+ * We provide setup data for different hardware block versionis. See
+ * Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
+ * for further information.
+ */
+static struct plat_sci_port sci_drv_data_hwb_1 = {
+	.flags		= UPF_BOOT_AUTOCONF,
+	.scscr		= SCSCR_RE | SCSCR_TE,
+	.scbrr_algo_id	= SCBRR_ALGO_4,
+};
+
+static struct plat_sci_port sci_drv_data_hwb_2 = {
+	.flags		= UPF_BOOT_AUTOCONF | UPF_IOREMAP,
+	.scscr		= SCSCR_RE | SCSCR_TE | SCSCR_CKE1,
+	.scbrr_algo_id	= SCBRR_ALGO_2,
+};
+
+static const struct of_device_id of_sci_match[] = {
+	{ .compatible = "renesas,shmobile-sci-hwb-1",
+		.data = &sci_drv_data_hwb_1 },
+	{ .compatible = "renesas,shmobile-sci-hwb-2",
+		.data = &sci_drv_data_hwb_2 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_sci_match);
+
+/* This array corresponds to the DT binding definition of the SCI driver */
+static const char sci_serial_modes[] = {
+	PORT_SCI, PORT_SCIF, PORT_IRDA, PORT_SCIFA, PORT_SCIFB
+};
+
+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;
+
+	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;
+	}
+
+	/*
+	 * Switch to devm_kmalloc if it will be added to the kernel, as we
+	 * will copy match->data that is correctly zeroed for unused fields
+	 */
+	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;
+	}
+
+	memcpy(p, match->data, sizeof(struct plat_sci_port));
+
+	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, "device id missing\n");
+		return NULL;
+	}
+	*dev_id = be32_to_cpup(prop);
+
+	prop = of_get_property(np, "renesas,serial-type", NULL);
+	if (!prop) {
+		dev_err(&pdev->dev, "serial type missing\n");
+		return NULL;
+	}
+	p->type = sci_serial_modes[be32_to_cpup(prop)];
+
+	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 +2485,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 +2497,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 +2562,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),
 	},
 };
 
-- 
1.7.9.5

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Paul Mundt]

On Tue, Feb 19, 2013 at 12:29:44PM -0600, Bastian Hecht wrote:
> +Required properties:
> +- compatible : Should be "renesas,shmobile-sci-hwb-<x>", where <x> denotes a
> +  hardware block version number of the following list - choose it according
> +  to your SoC in use.
> +  1: sh7372, sh73a0, r8a7740
> +  2: r8a7779

This is introducing another layer of arbitrary versioning that pretty
much side-steps all of the existing abstraction that exists for dealing
with block versioning to begin with, so no, this isn't going to fly.
If you want to use version numbering like this, then it should be matched
to the regtype, which handles all possible variations already.

There is also nothing shmobile-specific about any of this, so that should
really be dropped from the string as well.

> index 6147756..4f3613d 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2353,6 +2353,106 @@ static int sci_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +/*
> + * We provide setup data for different hardware block versionis. See
> + * Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
> + * for further information.
> + */
> +static struct plat_sci_port sci_drv_data_hwb_1 = {
> +	.flags		= UPF_BOOT_AUTOCONF,
> +	.scscr		= SCSCR_RE | SCSCR_TE,
> +	.scbrr_algo_id	= SCBRR_ALGO_4,
> +};
> +
> +static struct plat_sci_port sci_drv_data_hwb_2 = {
> +	.flags		= UPF_BOOT_AUTOCONF | UPF_IOREMAP,
> +	.scscr		= SCSCR_RE | SCSCR_TE | SCSCR_CKE1,
> +	.scbrr_algo_id	= SCBRR_ALGO_2,
> +};
> +
While this no doubt works ok for some limited subset of hardware, this
has no place in the driver itself. If you want to use a DT binding, then
this information needs to be represented there somehow, as well.

The rest of it looks ok in any event.

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Bastian Hecht]

Hi Paul,

thanks for commenting on this.

2013/2/20 Paul Mundt <lethal@linux-sh.org>:
> On Tue, Feb 19, 2013 at 12:29:44PM -0600, Bastian Hecht wrote:
>> +Required properties:
>> +- compatible : Should be "renesas,shmobile-sci-hwb-<x>", where <x> denotes a
>> +  hardware block version number of the following list - choose it according
>> +  to your SoC in use.
>> +  1: sh7372, sh73a0, r8a7740
>> +  2: r8a7779
>
> This is introducing another layer of arbitrary versioning that pretty
> much side-steps all of the existing abstraction that exists for dealing
> with block versioning to begin with, so no, this isn't going to fly.

Yes, it kind of felt like introducing the 1000th way of versioning...
Agreed, let's avoid this!

> If you want to use version numbering like this, then it should be matched
> to the regtype, which handles all possible variations already.

What do you mean by "regtype"? Could you give me an example
.compatible line, please?

> There is also nothing shmobile-specific about any of this, so that should
> really be dropped from the string as well.

Alright.

>> index 6147756..4f3613d 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -2353,6 +2353,106 @@ static int sci_remove(struct platform_device *dev)
>>       return 0;
>>  }
>>
>> +#ifdef CONFIG_OF
>> +/*
>> + * We provide setup data for different hardware block versionis. See
>> + * Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
>> + * for further information.
>> + */
>> +static struct plat_sci_port sci_drv_data_hwb_1 = {
>> +     .flags          = UPF_BOOT_AUTOCONF,
>> +     .scscr          = SCSCR_RE | SCSCR_TE,
>> +     .scbrr_algo_id  = SCBRR_ALGO_4,
>> +};
>> +
>> +static struct plat_sci_port sci_drv_data_hwb_2 = {
>> +     .flags          = UPF_BOOT_AUTOCONF | UPF_IOREMAP,
>> +     .scscr          = SCSCR_RE | SCSCR_TE | SCSCR_CKE1,
>> +     .scbrr_algo_id  = SCBRR_ALGO_2,
>> +};
>> +
> While this no doubt works ok for some limited subset of hardware, this
> has no place in the driver itself. If you want to use a DT binding, then
> this information needs to be represented there somehow, as well.

I suppose this boils down to the generic question if we want to have a
lean .dts file or a lean driver.
If I don't get something wrong, Magnus prefers to use the .data field
of the struct of_device_id to hide SoC specific parts from the
external visible bindings, if there is only 1 legal way to setup the
device. I personally prefer the .data way - but I don't want to be a
3rd person joining the discussion. I'd prefer a decision. So please
Magnus, comment on this issue. I have no problem going either way.

> The rest of it looks ok in any event.

Very nice, thanks!

Bastian

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Paul Mundt]

On Thu, Feb 21, 2013 at 09:46:37AM -0600, Bastian Hecht wrote:
> Hi Paul,
> 
> thanks for commenting on this.
> 
> 2013/2/20 Paul Mundt <lethal@linux-sh.org>:
> > On Tue, Feb 19, 2013 at 12:29:44PM -0600, Bastian Hecht wrote:
> >> +Required properties:
> >> +- compatible : Should be "renesas,shmobile-sci-hwb-<x>", where <x> denotes a
> >> +  hardware block version number of the following list - choose it according
> >> +  to your SoC in use.
> >> +  1: sh7372, sh73a0, r8a7740
> >> +  2: r8a7779
> >
> > This is introducing another layer of arbitrary versioning that pretty
> > much side-steps all of the existing abstraction that exists for dealing
> > with block versioning to begin with, so no, this isn't going to fly.
> 
> Yes, it kind of felt like introducing the 1000th way of versioning...
> Agreed, let's avoid this!
> 
> > If you want to use version numbering like this, then it should be matched
> > to the regtype, which handles all possible variations already.
> 
> What do you mean by "regtype"? Could you give me an example
> .compatible line, please?
> 
Looking at it a bit more, the regtype is probably ok just as an added
property, given that most of the time you're going to be using the probed
one anyways.

As far as compatible lines go, I would break them out per-port type, as
many other drivers do, and pass along the PORT_xxx definition under
.data, which should be sufficient to determine the regtype in most cases.
Adding a special regtype property would still be necessary for the case
we don't want the default, however.

So something along the lines of:
	renesas,sci-<port type>

with or without a trailing -uart should work fine.

> >> +#ifdef CONFIG_OF
> >> +/*
> >> + * We provide setup data for different hardware block versionis. See
> >> + * Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
> >> + * for further information.
> >> + */
> >> +static struct plat_sci_port sci_drv_data_hwb_1 = {
> >> +     .flags          = UPF_BOOT_AUTOCONF,
> >> +     .scscr          = SCSCR_RE | SCSCR_TE,
> >> +     .scbrr_algo_id  = SCBRR_ALGO_4,
> >> +};
> >> +
> >> +static struct plat_sci_port sci_drv_data_hwb_2 = {
> >> +     .flags          = UPF_BOOT_AUTOCONF | UPF_IOREMAP,
> >> +     .scscr          = SCSCR_RE | SCSCR_TE | SCSCR_CKE1,
> >> +     .scbrr_algo_id  = SCBRR_ALGO_2,
> >> +};
> >> +
> > While this no doubt works ok for some limited subset of hardware, this
> > has no place in the driver itself. If you want to use a DT binding, then
> > this information needs to be represented there somehow, as well.
> 
> I suppose this boils down to the generic question if we want to have a
> lean .dts file or a lean driver.

I don't think the two are at odds. We already have an abstraction in
place for dealing with this information, and it does need to be
provideded by the platform. Hacking it in to the serial driver is not an
acceptable solution, simply because you happen to be using the device
tree.

If it's really the case that you can use one or two definitions on all of
the boards, then it should be no problem having a tiny .dtsi file with
these definitions that you include in the platform's .dts file.

> If I don't get something wrong, Magnus prefers to use the .data field
> of the struct of_device_id to hide SoC specific parts from the
> external visible bindings, if there is only 1 legal way to setup the
> device. I personally prefer the .data way - but I don't want to be a
> 3rd person joining the discussion. I'd prefer a decision. So please
> Magnus, comment on this issue. I have no problem going either way.
> 
I don't have any strong opinions on how .data gets used, but we will not
be applying anything that adds arbitrary plat_sci_port definitions to the
driver, as that is nothing but lazy and shortsighted.

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Bastian Hecht]

Hi Paul,

2013/2/21 Paul Mundt <lethal@linux-sh.org>:
> On Thu, Feb 21, 2013 at 09:46:37AM -0600, Bastian Hecht wrote:
>> Hi Paul,
>>
>> thanks for commenting on this.
>>
>> 2013/2/20 Paul Mundt <lethal@linux-sh.org>:
>> > On Tue, Feb 19, 2013 at 12:29:44PM -0600, Bastian Hecht wrote:
>> >> +Required properties:
>> >> +- compatible : Should be "renesas,shmobile-sci-hwb-<x>", where <x> denotes a
>> >> +  hardware block version number of the following list - choose it according
>> >> +  to your SoC in use.
>> >> +  1: sh7372, sh73a0, r8a7740
>> >> +  2: r8a7779
>> >
>> > This is introducing another layer of arbitrary versioning that pretty
>> > much side-steps all of the existing abstraction that exists for dealing
>> > with block versioning to begin with, so no, this isn't going to fly.
>>
>> Yes, it kind of felt like introducing the 1000th way of versioning...
>> Agreed, let's avoid this!
>>
>> > If you want to use version numbering like this, then it should be matched
>> > to the regtype, which handles all possible variations already.
>>
>> What do you mean by "regtype"? Could you give me an example
>> .compatible line, please?
>>
> Looking at it a bit more, the regtype is probably ok just as an added
> property, given that most of the time you're going to be using the probed
> one anyways.

Ok.

> As far as compatible lines go, I would break them out per-port type, as
> many other drivers do, and pass along the PORT_xxx definition under
> .data, which should be sufficient to determine the regtype in most cases.
> Adding a special regtype property would still be necessary for the case
> we don't want the default, however.
>
> So something along the lines of:
>         renesas,sci-<port type>
>
> with or without a trailing -uart should work fine.

Ok so I use
- compatible : Should be "renesas,sci-<port type>-uart", where <port
type> may be
  SCI, SCIF, IRDA, SCIFA or SCIFB.

I will forward the type using the .data field as suggested.

Further I introduce an optional regtype property
- regtype : Overwrite the register layout. Some hardware versions use
a non-default register layout. Possible layouts are
  0 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
  1 = SCIx_SH3_SCIF_REGTYPE
  2 = SCIx_SH4_SCIF_REGTYPE
  3 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
  4 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
  5 = SCIx_SH7705_SCIF_REGTYPE

Are the enum names good or should it be like "0 =  SH hardware version
2 SCIF using FIFODATA"?

Further I'll add the fields
- scscr : Should contain a bitfield used by the Serial Control Register.
  b0 = SCSCR_TIE
  b1 = SCSCR_RIE
  b2 = SCSCR_TE
  b3 = SCSCR_RE
  b4 = SCSCR_REIE
  b5 = SCSCR_TOIE
  b6 = SCSCR_CKE1
  b7 = SCSCR_CKE0
- scbrr-algo-id : Algorithm ID for the Bit Rate Register
  1 = ((clk + 16 * bps) / (16 * bps) - 1)
  2 = ((clk + 16 * bps) / (32 * bps) - 1)
  3 = (((clk * 2) + 16 * bps) / (16 * bps) - 1)
  4 = (((clk * 2) + 16 * bps) / (32 * bps) - 1)
  5 = (((clk * 1000 / 32) / bps) - 1)

What I am unsure about is how to deal with the .flags field. There are
28 different possible UPF_* flags. Should I just add
- ioremap : Set this flag if I/O should be remapped.
- autoconf : Set if device is capable of auto configuration.


>> >> +#ifdef CONFIG_OF
>> >> +/*
>> >> + * We provide setup data for different hardware block versionis. See
>> >> + * Documentation/devicetree/bindings/tty/serial/renesas,sh-sci-serial.txt
>> >> + * for further information.
>> >> + */
>> >> +static struct plat_sci_port sci_drv_data_hwb_1 = {
>> >> +     .flags          = UPF_BOOT_AUTOCONF,
>> >> +     .scscr          = SCSCR_RE | SCSCR_TE,
>> >> +     .scbrr_algo_id  = SCBRR_ALGO_4,
>> >> +};
>> >> +
>> >> +static struct plat_sci_port sci_drv_data_hwb_2 = {
>> >> +     .flags          = UPF_BOOT_AUTOCONF | UPF_IOREMAP,
>> >> +     .scscr          = SCSCR_RE | SCSCR_TE | SCSCR_CKE1,
>> >> +     .scbrr_algo_id  = SCBRR_ALGO_2,
>> >> +};
>> >> +
>> > While this no doubt works ok for some limited subset of hardware, this
>> > has no place in the driver itself. If you want to use a DT binding, then
>> > this information needs to be represented there somehow, as well.
>>
>> I suppose this boils down to the generic question if we want to have a
>> lean .dts file or a lean driver.
>
> I don't think the two are at odds. We already have an abstraction in
> place for dealing with this information, and it does need to be
> provideded by the platform. Hacking it in to the serial driver is not an
> acceptable solution, simply because you happen to be using the device
> tree.

Ok so if this is the philosophy to go, I will throw my idea over board
to store setup data in the driver.

> If it's really the case that you can use one or two definitions on all of
> the boards, then it should be no problem having a tiny .dtsi file with
> these definitions that you include in the platform's .dts file.

Ok I get the idea.

>> If I don't get something wrong, Magnus prefers to use the .data field
>> of the struct of_device_id to hide SoC specific parts from the
>> external visible bindings, if there is only 1 legal way to setup the
>> device. I personally prefer the .data way - but I don't want to be a
>> 3rd person joining the discussion. I'd prefer a decision. So please
>> Magnus, comment on this issue. I have no problem going either way.
>>
> I don't have any strong opinions on how .data gets used, but we will not
> be applying anything that adds arbitrary plat_sci_port definitions to the
> driver, as that is nothing but lazy and shortsighted.

Uhmm... if you insist keep the "shortsighted", but I would replace the
laziness with "lack of experience".
I coded it the way I did because I thought it's the cleanest solution
- but I'm willing to learn.

Cheers,

 Bastian

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Paul Mundt]

Hi Bastian,

On Thu, Feb 21, 2013 at 12:11:58PM -0600, Bastian Hecht wrote:
> 2013/2/21 Paul Mundt <lethal@linux-sh.org>:
> > So something along the lines of:
> >         renesas,sci-<port type>
> >
> > with or without a trailing -uart should work fine.
> 
> Ok so I use
> - compatible : Should be "renesas,sci-<port type>-uart", where <port
> type> may be
>   SCI, SCIF, IRDA, SCIFA or SCIFB.
> 
> I will forward the type using the .data field as suggested.
> 
Ok.

> Further I introduce an optional regtype property
> - regtype : Overwrite the register layout. Some hardware versions use
> a non-default register layout. Possible layouts are
>   0 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
>   1 = SCIx_SH3_SCIF_REGTYPE
>   2 = SCIx_SH4_SCIF_REGTYPE
>   3 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
>   4 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
>   5 = SCIx_SH7705_SCIF_REGTYPE
> 
> Are the enum names good or should it be like "0 =  SH hardware version
> 2 SCIF using FIFODATA"?
> 
The enum names should be fine. I admit they aren't the most descriptive,
but they're the best I could come up with on short notice, and any new
SoC implementor should be able to easily figure it out by looking at the
driver.

> Further I'll add the fields
> - scscr : Should contain a bitfield used by the Serial Control Register.
>   b0 = SCSCR_TIE
>   b1 = SCSCR_RIE
>   b2 = SCSCR_TE
>   b3 = SCSCR_RE
>   b4 = SCSCR_REIE
>   b5 = SCSCR_TOIE
>   b6 = SCSCR_CKE1
>   b7 = SCSCR_CKE0
> - scbrr-algo-id : Algorithm ID for the Bit Rate Register
>   1 = ((clk + 16 * bps) / (16 * bps) - 1)
>   2 = ((clk + 16 * bps) / (32 * bps) - 1)
>   3 = (((clk * 2) + 16 * bps) / (16 * bps) - 1)
>   4 = (((clk * 2) + 16 * bps) / (32 * bps) - 1)
>   5 = (((clk * 1000 / 32) / bps) - 1)
> 
Ok.

> What I am unsure about is how to deal with the .flags field. There are
> 28 different possible UPF_* flags. Should I just add
> - ioremap : Set this flag if I/O should be remapped.
> - autoconf : Set if device is capable of auto configuration.
> 
I guess the thing you have to figure out is whether it's sane to support
non-remapped cases for the ARM side or not. On the SH side we can handle
unconditional ioremap, even in the 1:1 case, but traditionally we didn't
use it, so it was always an optional thing.

If it simplifies things for you to say that DT use implies UPF_IOREMAP,
that should be fine. How do other platforms deal with UPF flags?

> > I don't think the two are at odds. We already have an abstraction in
> > place for dealing with this information, and it does need to be
> > provideded by the platform. Hacking it in to the serial driver is not an
> > acceptable solution, simply because you happen to be using the device
> > tree.
> 
> Ok so if this is the philosophy to go, I will throw my idea over board
> to store setup data in the driver.
> 
> > If it's really the case that you can use one or two definitions on all of
> > the boards, then it should be no problem having a tiny .dtsi file with
> > these definitions that you include in the platform's .dts file.
> 
> Ok I get the idea.
> 
> >> If I don't get something wrong, Magnus prefers to use the .data field
> >> of the struct of_device_id to hide SoC specific parts from the
> >> external visible bindings, if there is only 1 legal way to setup the
> >> device. I personally prefer the .data way - but I don't want to be a
> >> 3rd person joining the discussion. I'd prefer a decision. So please
> >> Magnus, comment on this issue. I have no problem going either way.
> >>
> > I don't have any strong opinions on how .data gets used, but we will not
> > be applying anything that adds arbitrary plat_sci_port definitions to the
> > driver, as that is nothing but lazy and shortsighted.
> 
> Uhmm... if you insist keep the "shortsighted", but I would replace the
> laziness with "lack of experience".
> I coded it the way I did because I thought it's the cleanest solution
> - but I'm willing to learn.

I apologize if I seem a bit abrupt over this matter, but it's a road that
we have been down with this driver before, and it's taken years to get
away from that model and in to something more flexible. I don't expect
you to be fully aware of the history, so this wasn't directed at you
specifically so much as just the general approach. Magnus should of
course know better, but that's another matter entirely.

Originally there were only a couple of variants, which meant that the
port definitions were all managed in the driver, this eventually
spiralled out of control with well over 50 variants all with subtle
differences (across roughly double the amount of CPU variations, and half
a dozen diffferent architectures) that pretty much ensured any minor
change would break all over the place, ultimately leaving me to clean it
up (the bulk of this was in 2.4 and later 2.5 after the serial core was
introduced and the driver rewritten). After 10+ years of that, I'm
somewhat less than enthusiastic to begin down the same path again for the
same rationale. Even now, we're still not completely rid of the past
disaster, given that drivers/tty/serial/sh-sci.h still exists, despite
several rewrites.

In any event, I do believe we can come up with a good match for the
driver and the bindings, while keeping complexity to a minimum by virtue
of being able to ignore much of the legacy stuff. It will also be
interesting to see how the pinctrl/pinconf and DMA stuff ties in under
the DT model, but one thing at a time.

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Magnus Damm]

Hi Paul,

On Fri, Feb 22, 2013 at 9:51 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> Hi Bastian,
>
> On Thu, Feb 21, 2013 at 12:11:58PM -0600, Bastian Hecht wrote:
>> 2013/2/21 Paul Mundt <lethal@linux-sh.org>:
>> > So something along the lines of:
>> >         renesas,sci-<port type>
>> >
>> > with or without a trailing -uart should work fine.
>>
>> Ok so I use
>> - compatible : Should be "renesas,sci-<port type>-uart", where <port
>> type> may be
>>   SCI, SCIF, IRDA, SCIFA or SCIFB.
>>
>> I will forward the type using the .data field as suggested.
>>
> Ok.
>
>> Further I introduce an optional regtype property
>> - regtype : Overwrite the register layout. Some hardware versions use
>> a non-default register layout. Possible layouts are
>>   0 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
>>   1 = SCIx_SH3_SCIF_REGTYPE
>>   2 = SCIx_SH4_SCIF_REGTYPE
>>   3 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
>>   4 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
>>   5 = SCIx_SH7705_SCIF_REGTYPE
>>
>> Are the enum names good or should it be like "0 =  SH hardware version
>> 2 SCIF using FIFODATA"?
>>
> The enum names should be fine. I admit they aren't the most descriptive,
> but they're the best I could come up with on short notice, and any new
> SoC implementor should be able to easily figure it out by looking at the
> driver.
>
>> Further I'll add the fields
>> - scscr : Should contain a bitfield used by the Serial Control Register.
>>   b0 = SCSCR_TIE
>>   b1 = SCSCR_RIE
>>   b2 = SCSCR_TE
>>   b3 = SCSCR_RE
>>   b4 = SCSCR_REIE
>>   b5 = SCSCR_TOIE
>>   b6 = SCSCR_CKE1
>>   b7 = SCSCR_CKE0
>> - scbrr-algo-id : Algorithm ID for the Bit Rate Register
>>   1 = ((clk + 16 * bps) / (16 * bps) - 1)
>>   2 = ((clk + 16 * bps) / (32 * bps) - 1)
>>   3 = (((clk * 2) + 16 * bps) / (16 * bps) - 1)
>>   4 = (((clk * 2) + 16 * bps) / (32 * bps) - 1)
>>   5 = (((clk * 1000 / 32) / bps) - 1)
>>
> Ok.
>
>> What I am unsure about is how to deal with the .flags field. There are
>> 28 different possible UPF_* flags. Should I just add
>> - ioremap : Set this flag if I/O should be remapped.
>> - autoconf : Set if device is capable of auto configuration.
>>
> I guess the thing you have to figure out is whether it's sane to support
> non-remapped cases for the ARM side or not. On the SH side we can handle
> unconditional ioremap, even in the 1:1 case, but traditionally we didn't
> use it, so it was always an optional thing.
>
> If it simplifies things for you to say that DT use implies UPF_IOREMAP,
> that should be fine. How do other platforms deal with UPF flags?

Refraining from exposing software implementation details in DT is
probably a good idea. At least from my point of view the lack of or
need for ioremap is something that depends on the availability of
software-defined configuration like fixmaps and such. As you know, not
using ioremap allows us to output data on the serial console early
during boot. With DT that seems quite far away though, so assuming
ioremap will always be used makes sense IMO.

>> > I don't think the two are at odds. We already have an abstraction in
>> > place for dealing with this information, and it does need to be
>> > provideded by the platform. Hacking it in to the serial driver is not an
>> > acceptable solution, simply because you happen to be using the device
>> > tree.
>>
>> Ok so if this is the philosophy to go, I will throw my idea over board
>> to store setup data in the driver.
>>
>> > If it's really the case that you can use one or two definitions on all of
>> > the boards, then it should be no problem having a tiny .dtsi file with
>> > these definitions that you include in the platform's .dts file.
>>
>> Ok I get the idea.
>>
>> >> If I don't get something wrong, Magnus prefers to use the .data field
>> >> of the struct of_device_id to hide SoC specific parts from the
>> >> external visible bindings, if there is only 1 legal way to setup the
>> >> device. I personally prefer the .data way - but I don't want to be a
>> >> 3rd person joining the discussion. I'd prefer a decision. So please
>> >> Magnus, comment on this issue. I have no problem going either way.
>> >>
>> > I don't have any strong opinions on how .data gets used, but we will not
>> > be applying anything that adds arbitrary plat_sci_port definitions to the
>> > driver, as that is nothing but lazy and shortsighted.
>>
>> Uhmm... if you insist keep the "shortsighted", but I would replace the
>> laziness with "lack of experience".
>> I coded it the way I did because I thought it's the cleanest solution
>> - but I'm willing to learn.
>
> I apologize if I seem a bit abrupt over this matter, but it's a road that
> we have been down with this driver before, and it's taken years to get
> away from that model and in to something more flexible. I don't expect
> you to be fully aware of the history, so this wasn't directed at you
> specifically so much as just the general approach. Magnus should of
> course know better, but that's another matter entirely.

What should I know better?

My general approach is to make sure hardware-block specific bits are
kept separate from soc specific bits that also should be separated
from board. This is pretty basic stuff but it seems that I can keep on
repeating it forever. Also I don't want any software implementation
specific configuration to be leaked into the DT bits.

In this particular case I wanted to make sure Bastian didn't start
adding soc-specific stuff in the SCIF driver. So I am not very fond of
any data with sh77.... or r8a.. prefix in the driver and I know that
neither are you.

Regarding the other bits - I would be happy to leave the rest to you
guys. I am fine with most things - including if for instance the
correct way forward is a bunch of flags or more like grouped-together
configurations represented as hardware block version.

Thanks,

/ magnus

Re: [PATCH 1/3] serial: sh-sci: Add Device Tree probing

[Bastian Hecht]

Hi!

2013/2/22 Magnus Damm <magnus.damm@gmail.com>:
> Hi Paul,
>
> On Fri, Feb 22, 2013 at 9:51 PM, Paul Mundt <lethal@linux-sh.org> wrote:
>> Hi Bastian,
>>
>> On Thu, Feb 21, 2013 at 12:11:58PM -0600, Bastian Hecht wrote:
>>> 2013/2/21 Paul Mundt <lethal@linux-sh.org>:
>>> > So something along the lines of:
>>> >         renesas,sci-<port type>
>>> >
>>> > with or without a trailing -uart should work fine.
>>>
>>> Ok so I use
>>> - compatible : Should be "renesas,sci-<port type>-uart", where <port
>>> type> may be
>>>   SCI, SCIF, IRDA, SCIFA or SCIFB.
>>>
>>> I will forward the type using the .data field as suggested.
>>>
>> Ok.
>>
>>> Further I introduce an optional regtype property
>>> - regtype : Overwrite the register layout. Some hardware versions use
>>> a non-default register layout. Possible layouts are
>>>   0 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
>>>   1 = SCIx_SH3_SCIF_REGTYPE
>>>   2 = SCIx_SH4_SCIF_REGTYPE
>>>   3 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
>>>   4 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
>>>   5 = SCIx_SH7705_SCIF_REGTYPE
>>>
>>> Are the enum names good or should it be like "0 =  SH hardware version
>>> 2 SCIF using FIFODATA"?
>>>
>> The enum names should be fine. I admit they aren't the most descriptive,
>> but they're the best I could come up with on short notice, and any new
>> SoC implementor should be able to easily figure it out by looking at the
>> driver.
>>
>>> Further I'll add the fields
>>> - scscr : Should contain a bitfield used by the Serial Control Register.
>>>   b0 = SCSCR_TIE
>>>   b1 = SCSCR_RIE
>>>   b2 = SCSCR_TE
>>>   b3 = SCSCR_RE
>>>   b4 = SCSCR_REIE
>>>   b5 = SCSCR_TOIE
>>>   b6 = SCSCR_CKE1
>>>   b7 = SCSCR_CKE0
>>> - scbrr-algo-id : Algorithm ID for the Bit Rate Register
>>>   1 = ((clk + 16 * bps) / (16 * bps) - 1)
>>>   2 = ((clk + 16 * bps) / (32 * bps) - 1)
>>>   3 = (((clk * 2) + 16 * bps) / (16 * bps) - 1)
>>>   4 = (((clk * 2) + 16 * bps) / (32 * bps) - 1)
>>>   5 = (((clk * 1000 / 32) / bps) - 1)
>>>
>> Ok.
>>
>>> What I am unsure about is how to deal with the .flags field. There are
>>> 28 different possible UPF_* flags. Should I just add
>>> - ioremap : Set this flag if I/O should be remapped.
>>> - autoconf : Set if device is capable of auto configuration.
>>>
>> I guess the thing you have to figure out is whether it's sane to support
>> non-remapped cases for the ARM side or not. On the SH side we can handle
>> unconditional ioremap, even in the 1:1 case, but traditionally we didn't
>> use it, so it was always an optional thing.
>>
>> If it simplifies things for you to say that DT use implies UPF_IOREMAP,
>> that should be fine. How do other platforms deal with UPF flags?
>
> Refraining from exposing software implementation details in DT is
> probably a good idea. At least from my point of view the lack of or
> need for ioremap is something that depends on the availability of
> software-defined configuration like fixmaps and such. As you know, not
> using ioremap allows us to output data on the serial console early
> during boot. With DT that seems quite far away though, so assuming
> ioremap will always be used makes sense IMO.

Ok, I'll go for the ioremap flag for now.

>>> > I don't think the two are at odds. We already have an abstraction in
>>> > place for dealing with this information, and it does need to be
>>> > provideded by the platform. Hacking it in to the serial driver is not an
>>> > acceptable solution, simply because you happen to be using the device
>>> > tree.
>>>
>>> Ok so if this is the philosophy to go, I will throw my idea over board
>>> to store setup data in the driver.
>>>
>>> > If it's really the case that you can use one or two definitions on all of
>>> > the boards, then it should be no problem having a tiny .dtsi file with
>>> > these definitions that you include in the platform's .dts file.
>>>
>>> Ok I get the idea.
>>>
>>> >> If I don't get something wrong, Magnus prefers to use the .data field
>>> >> of the struct of_device_id to hide SoC specific parts from the
>>> >> external visible bindings, if there is only 1 legal way to setup the
>>> >> device. I personally prefer the .data way - but I don't want to be a
>>> >> 3rd person joining the discussion. I'd prefer a decision. So please
>>> >> Magnus, comment on this issue. I have no problem going either way.
>>> >>
>>> > I don't have any strong opinions on how .data gets used, but we will not
>>> > be applying anything that adds arbitrary plat_sci_port definitions to the
>>> > driver, as that is nothing but lazy and shortsighted.
>>>
>>> Uhmm... if you insist keep the "shortsighted", but I would replace the
>>> laziness with "lack of experience".
>>> I coded it the way I did because I thought it's the cleanest solution
>>> - but I'm willing to learn.
>>
>> I apologize if I seem a bit abrupt over this matter, but it's a road that
>> we have been down with this driver before, and it's taken years to get
>> away from that model and in to something more flexible. I don't expect
>> you to be fully aware of the history, so this wasn't directed at you
>> specifically so much as just the general approach. Magnus should of
>> course know better, but that's another matter entirely.

Thanks for the (snipped out) history, Paul. Wow, I see what you mean.
My world evolved more around 3-4 current SoCs, but maintaining a
driver for a hardware era spanning decades, makes me understand why I
need to change my code indeed.

> What should I know better?
>
> My general approach is to make sure hardware-block specific bits are
> kept separate from soc specific bits that also should be separated
> from board. This is pretty basic stuff but it seems that I can keep on
> repeating it forever. Also I don't want any software implementation
> specific configuration to be leaked into the DT bits.
>
> In this particular case I wanted to make sure Bastian didn't start
> adding soc-specific stuff in the SCIF driver. So I am not very fond of
> any data with sh77.... or r8a.. prefix in the driver and I know that
> neither are you.
>
> Regarding the other bits - I would be happy to leave the rest to you
> guys. I am fine with most things - including if for instance the
> correct way forward is a bunch of flags or more like grouped-together
> configurations represented as hardware block version.

So I think everything is clear already. I've prepared a 2nd version
and will post it shortly.

Thanks for the extensive help here,

 Bastian