| Message ID | 1362569437-11133-1-git-send-email-hechtb+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Bastian, Thanks for the patch. On Wednesday 06 March 2013 12:30:35 Bastian Hecht wrote: > We add the capabilty to probe Renesas SCI devices using Device Tree setup. > > Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com> > Reviewed-by: Paul Mundt <lethal@linux-sh.org> > Acked-by: Arnd Bergmann <arnd@arndb.de> > --- > v6: > putting it all together: I posted v5 as an incremental patch - here > the full version based on Simon Horman's tree topic/intc-of. > > I will post another incremental version as well. > > - rename sci@xxxxxxxx to serial@xxxxxxxx > - stick to scbrr-algorithm > > .../bindings/tty/serial/renesas,sci-serial.txt | 47 +++++++ > drivers/tty/serial/sh-sci.c | 146 > +++++++++++++++++++- include/linux/serial_sci.h | > 4 + > 3 files changed, 193 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..d80039e > --- /dev/null > +++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt > @@ -0,0 +1,47 @@ > +* Renesas SH-Mobile Serial Communication Interface > + > +Required properties: > +- compatible : Should be "renesas,sci-<port type>-uart", where <port type> > is + "sci", "scif", "irda", "scifa", "scifb" > + or for legacy devices > + "sh2_scif_fifodata", "sh3_scif", "sh4_scif", "sh4_scif_no_scsptr", > + "sh4_scif_fifodata", "sh7705_scif". > +- reg : Address and length of the register set for the device > +- interrupts : Should contain the following IRQs in this order: > + ERI: receive-error interrupt > + RXI: receive-FIFO-data-full interrupt > + TXI: transmit-FIFO-data-empty interrupt > + BRI: break reception interrupt > +- interrupt-names: The IRQ names "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 What is that for exactly ? > +- renesas,scbrr-algorithm : Choose an algorithm ID for the baud rate > generator. > + 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) Isn't it a property specific to our implementation of the driver instead of a hardware property ? How is one supposed to select the right algorithm ? > +Optional properties: > +- renesas,autoconf : Set if device is capable of auto configuration. > + > +Example: > + serial@e6c50000 { > + compatible = "renesas,sci-scifa-uart"; > + interrupt-parent = <&intca>; > + reg = <0xe6c50000 0x100>; > + interrupts = <0x0c20>, <0x0c20>, <0x0c20>, <0x0c20>; > + interrupt-names = "eri", "rxi", "txi", "bri"; > + cell-index = <1>; > + renesas,scscr = <0x30>; > + renesas,scbrr-algorithm = <4>; > + renesas,autoconf; > + }; > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index 6147756..03bb740 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,131 @@ static int sci_remove(struct platform_device *dev) > return 0; > } > > +static const struct of_device_id of_sci_match[] = { > + { .compatible = "renesas,sci-sci-uart", > + .data = (void *)SCIx_SCI_REGTYPE }, > + { .compatible = "renesas,sci-scif-uart", > + .data = (void *)SCIx_SH4_SCIF_REGTYPE, }, > + { .compatible = "renesas,sci-irda-uart", > + .data = (void *)SCIx_IRDA_REGTYPE }, > + { .compatible = "renesas,sci-scifa-uart", > + .data = (void *)SCIx_SCIFA_REGTYPE }, > + { .compatible = "renesas,sci-scifb-uart", > + .data = (void *)SCIx_SCIFB_REGTYPE }, > + { .compatible = "renesas,sci-sh2_scif_fifodata-uart", > + .data = (void *)SCIx_SH2_SCIF_FIFODATA_REGTYPE }, > + { .compatible = "renesas,sci-sh3_scif-uart", > + .data = (void *)SCIx_SH3_SCIF_REGTYPE }, > + { .compatible = "renesas,sci-sh4_scif-uart", > + .data = (void *)SCIx_SH4_SCIF_REGTYPE }, > + { .compatible = "renesas,sci-sh4_scif_no_scsptr-uart", > + .data = (void *)SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE }, > + { .compatible = "renesas,sci-sh4_scif_fifodata-uart", > + .data = (void *)SCIx_SH4_SCIF_FIFODATA_REGTYPE }, > + { .compatible = "renesas,sci-sh7705_scif-uart", > + .data = (void *)SCIx_SH7705_SCIF_REGTYPE }, > + {}, > +}; > +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; > + > + if (!IS_ENABLED(CONFIG_OF) || !np) > + return NULL; > + > + 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-algorithm", NULL); > + if (!prop) { > + dev_err(&pdev->dev, "required DT prop scbrr-algorithm missing\n"); > + return NULL; > + } > + val = be32_to_cpup(prop); > + if (val <= SCBRR_ALGO_INVALID || val >= SCBRR_NR_ALGOS) { > + dev_err(&pdev->dev, "DT prop clock-algorithm 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; > + > + p->regtype = (unsigned int)match->data; > + > + switch (p->regtype) { > + case SCIx_SCI_REGTYPE: > + p->type = PORT_SCI; > + break; > + case SCIx_SH4_SCIF_REGTYPE: > + p->type = PORT_SCIF; > + break; > + case SCIx_IRDA_REGTYPE: > + p->type = PORT_IRDA; > + break; > + case SCIx_SCIFA_REGTYPE: > + p->type = PORT_SCIFA; > + break; > + case SCIx_SCIFB_REGTYPE: > + p->type = PORT_SCIFB; > + break; > + default: > + /* legacy register sets default to PORT_SCIF */ > + p->type = PORT_SCIF; > + break; > + } > + > + return p; > +} > + > static int sci_probe_single(struct platform_device *dev, > unsigned int index, > struct plat_sci_port *p, > @@ -2385,9 +2511,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 +2523,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 +2588,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)
On Wednesday 06 March 2013 13:10:55 Laurent Pinchart wrote: > On Wednesday 06 March 2013 12:30:35 Bastian Hecht wrote: > > We add the capabilty to probe Renesas SCI devices using Device Tree setup. > > > > Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com> > > Reviewed-by: Paul Mundt <lethal@linux-sh.org> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > --- > > v6: > > putting it all together: I posted v5 as an incremental patch - here > > the full version based on Simon Horman's tree topic/intc-of. > > > > I will post another incremental version as well. > > > > - rename sci@xxxxxxxx to serial@xxxxxxxx > > - stick to scbrr-algorithm > > > > .../bindings/tty/serial/renesas,sci-serial.txt | 47 +++++++ > > drivers/tty/serial/sh-sci.c | 146 ++++++++++++++- > > include/linux/serial_sci.h | 4 + > > 3 files changed, 193 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..d80039e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt > > @@ -0,0 +1,47 @@ > > +* Renesas SH-Mobile Serial Communication Interface > > + > > +Required properties: > > +- compatible : Should be "renesas,sci-<port type>-uart", where <port > > type> > > is + "sci", "scif", "irda", "scifa", "scifb" > > + or for legacy devices > > + "sh2_scif_fifodata", "sh3_scif", "sh4_scif", "sh4_scif_no_scsptr", > > + "sh4_scif_fifodata", "sh7705_scif". > > +- reg : Address and length of the register set for the device > > +- interrupts : Should contain the following IRQs in this order: > > + ERI: receive-error interrupt > > + RXI: receive-FIFO-data-full interrupt > > + TXI: transmit-FIFO-data-empty interrupt > > + BRI: break reception interrupt > > +- interrupt-names: The IRQ names "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 > > What is that for exactly ? > > > +- renesas,scbrr-algorithm : Choose an algorithm ID for the baud rate > > generator. > > + 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) > > Isn't it a property specific to our implementation of the driver instead of > a hardware property ? How is one supposed to select the right algorithm ? I realize I haven't expressed myself very clearly here. What I mean is that expressing how the baud rate generator works internally using an algorithm ID is pretty specific to the Linux driver. I assume that, for a given serial port on a given SoC, the algorithm that matches the baud rate generator hardware implementation needs to be selected. Can't this information be inferred from the compatible string and port number ?
On Wed, Mar 06, 2013 at 01:25:16PM +0100, Laurent Pinchart wrote: > On Wednesday 06 March 2013 13:10:55 Laurent Pinchart wrote: > > On Wednesday 06 March 2013 12:30:35 Bastian Hecht wrote: > > > +- renesas,scbrr-algorithm : Choose an algorithm ID for the baud rate > > > generator. > > > + 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) > > > > Isn't it a property specific to our implementation of the driver instead of > > a hardware property ? How is one supposed to select the right algorithm ? > > I realize I haven't expressed myself very clearly here. What I mean is that > expressing how the baud rate generator works internally using an algorithm ID > is pretty specific to the Linux driver. > > I assume that, for a given serial port on a given SoC, the algorithm that > matches the baud rate generator hardware implementation needs to be selected. > Can't this information be inferred from the compatible string and port number > ? > Now that we have split things out in to regtype for the compat string we may be able to get away with selecting defaults that match the regtype and drop it from the binding (assuming all future parts remain relatively sane), but it would still complicate things if we were to ever do DT support for legacy CPUs. It's not enough to tie to the port type, as there are plenty of cases where the port type uses a non-standard algo on some CPU subtypes (although things are pretty consistent for the arm side so far). -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paul, 2013/3/6 Paul Mundt <lethal@linux-sh.org>: > On Wed, Mar 06, 2013 at 01:25:16PM +0100, Laurent Pinchart wrote: >> On Wednesday 06 March 2013 13:10:55 Laurent Pinchart wrote: >> > On Wednesday 06 March 2013 12:30:35 Bastian Hecht wrote: >> > > +- renesas,scbrr-algorithm : Choose an algorithm ID for the baud rate >> > > generator. >> > > + 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) >> > >> > Isn't it a property specific to our implementation of the driver instead of >> > a hardware property ? How is one supposed to select the right algorithm ? >> >> I realize I haven't expressed myself very clearly here. What I mean is that >> expressing how the baud rate generator works internally using an algorithm ID >> is pretty specific to the Linux driver. >> >> I assume that, for a given serial port on a given SoC, the algorithm that >> matches the baud rate generator hardware implementation needs to be selected. >> Can't this information be inferred from the compatible string and port number >> ? >> > Now that we have split things out in to regtype for the compat string we > may be able to get away with selecting defaults that match the regtype > and drop it from the binding (assuming all future parts remain relatively > sane), but it would still complicate things if we were to ever do DT > support for legacy CPUs. It's not enough to tie to the port type, as > there are plenty of cases where the port type uses a non-standard algo on > some CPU subtypes (although things are pretty consistent for the arm side > so far). regarding Laurent's question about the SCSCR register, do you think we can assume the same there? If so we could start out omitting these 2 properties and rely on adding optional properties later in case we want to support legacy stuff. I don't see where it complicates things as we probably got the minimal binding set then. Thanks, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 06 March 2013, Paul Mundt wrote: > Now that we have split things out in to regtype for the compat string we > may be able to get away with selecting defaults that match the regtype > and drop it from the binding (assuming all future parts remain relatively > sane), but it would still complicate things if we were to ever do DT > support for legacy CPUs. It's not enough to tie to the port type, as > there are plenty of cases where the port type uses a non-standard algo on > some CPU subtypes (although things are pretty consistent for the arm side > so far). Maybe we could document it as an optional property that we may use for the legacy CPUs if necessary, but put it into the ARM dts files or into the driver implementation for the time being. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, >> +- 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 > > What is that for exactly ? What I can see from 3 different datasheets (sh7372, sh73a0, r8a7740) the first 2 bits differ in meaning. On r8a7740 it depends: In asynchronous mode the first 2 bits CKE0/1 define whether you want to use an external clock to drive the baud generator or if you want to use the internal SUB clock. If you use the SUB clock you can further define if you want to use a subscaled SUB clock in the SCSMR register. In synchronous mode you must rely on the internal clock for the baud generator and can select if the SCK pin is used as clock input or output. On sh73a0 and sh7372 it's used to control the output clock SCK (set it to high impedance or actually output a clock when in sychronous mode) Bit 2 and 3 don't exist in my datasheets. The other bits define under which conditions you receive interrupts (send FIFO empty, receive FIFO full) and which bits you need to start transfers (one bit to start sending, one receiving). The bits 8 to 31 are used to set up DMA transfers and various interrupt events like error status and finish transfers. I haven't included them as they are not used in the driver. Thanks, Bastian -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bastian, On Thursday 07 March 2013 11:26:29 Bastian Hecht wrote: > Hi Laurent, > > >> +- 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 > > > > What is that for exactly ? > > What I can see from 3 different datasheets (sh7372, sh73a0, r8a7740) > the first 2 bits differ in meaning. > > On r8a7740 it depends: In asynchronous mode the first 2 bits CKE0/1 > define whether you want to use an external clock to drive the baud > generator or if you want to use the internal SUB clock. If you use the > SUB clock you can further define if you want to use a subscaled SUB > clock in the SCSMR register. In synchronous mode you must rely on the > internal clock for the baud generator and can select if the SCK pin is > used as clock input or output. What about adding an optional source clock property to the bindings, that would contain the phandle of the source clock ? If the property is absent then the internal clock would be used. How is subscaling used in practice ? > On sh73a0 and sh7372 it's used to control the output clock SCK (set it > to high impedance or actually output a clock when in sychronous mode) Maybe we could add a synchronous mode property to the DT bindings and use it to infer the bit values. > Bit 2 and 3 don't exist in my datasheets. > > The other bits define under which conditions you receive interrupts > (send FIFO empty, receive FIFO full) and which bits you need to start > transfers (one bit to start sending, one receiving). The bits 8 to 31 > are used to set up DMA transfers and various interrupt events like > error status and finish transfers. I haven't included them as they are > not used in the driver. If they're not used maybe we can drop them :-)
On Mon, Mar 11, 2013 at 11:32:31PM +0100, Laurent Pinchart wrote: > On Thursday 07 March 2013 11:26:29 Bastian Hecht wrote: > > >> +- 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 > > > > > > What is that for exactly ? > > > > What I can see from 3 different datasheets (sh7372, sh73a0, r8a7740) > > the first 2 bits differ in meaning. > > > > On r8a7740 it depends: In asynchronous mode the first 2 bits CKE0/1 > > define whether you want to use an external clock to drive the baud > > generator or if you want to use the internal SUB clock. If you use the > > SUB clock you can further define if you want to use a subscaled SUB > > clock in the SCSMR register. In synchronous mode you must rely on the > > internal clock for the baud generator and can select if the SCK pin is > > used as clock input or output. > > What about adding an optional source clock property to the bindings, that > would contain the phandle of the source clock ? If the property is absent then > the internal clock would be used. > We can't really use the internal oscillator at the moment, it's one of the things that is blocked on adoption of the common struct clk work. The baud rate generator algo will likewise differ depending on whether it's using an internally generated clock or not, which also makes a different set of baud rates available. My plan originally was to wait until the common struct clk stuff came along well enough that we could provide a clock dynamically depending on the target baud (or for avoiding cpufreq pre/post-change notifier chains), but this obviously hasn't happened yet. I think your idea for the abstraction with the phandle sounds like the right approach, but we aren't presently in a state where we can handle that. > > Bit 2 and 3 don't exist in my datasheets. > > > > The other bits define under which conditions you receive interrupts > > (send FIFO empty, receive FIFO full) and which bits you need to start > > transfers (one bit to start sending, one receiving). The bits 8 to 31 > > are used to set up DMA transfers and various interrupt events like > > error status and finish transfers. I haven't included them as they are > > not used in the driver. > > If they're not used maybe we can drop them :-) > Obviously they're used, otherwise they wouldn't be defined in the common header. Whether they are used by these specific CPUs or not isn't relevant, as there are plenty of cases where they are. grep harder. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paul, On Tuesday 12 March 2013 17:20:39 Paul Mundt wrote: > On Mon, Mar 11, 2013 at 11:32:31PM +0100, Laurent Pinchart wrote: > > On Thursday 07 March 2013 11:26:29 Bastian Hecht wrote: > > > >> +- 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 > > > > > > > > What is that for exactly ? > > > > > > What I can see from 3 different datasheets (sh7372, sh73a0, r8a7740) > > > the first 2 bits differ in meaning. > > > > > > On r8a7740 it depends: In asynchronous mode the first 2 bits CKE0/1 > > > define whether you want to use an external clock to drive the baud > > > generator or if you want to use the internal SUB clock. If you use the > > > SUB clock you can further define if you want to use a subscaled SUB > > > clock in the SCSMR register. In synchronous mode you must rely on the > > > internal clock for the baud generator and can select if the SCK pin is > > > used as clock input or output. > > > > What about adding an optional source clock property to the bindings, that > > would contain the phandle of the source clock ? If the property is absent > > then the internal clock would be used. > > We can't really use the internal oscillator at the moment, it's one of > the things that is blocked on adoption of the common struct clk work. The > baud rate generator algo will likewise differ depending on whether it's > using an internally generated clock or not, which also makes a different > set of baud rates available. > > My plan originally was to wait until the common struct clk stuff came > along well enough that we could provide a clock dynamically depending on > the target baud (or for avoiding cpufreq pre/post-change notifier > chains), but this obviously hasn't happened yet. > > I think your idea for the abstraction with the phandle sounds like the > right approach, but we aren't presently in a state where we can handle > that. Do the platforms that will be converted to DT currently need to make the clock source configurable ? If not we could just leave it out of the DT bindings until needed. Hopefully by then we'll have a common clock framework implementation. > > > Bit 2 and 3 don't exist in my datasheets. > > > > > > The other bits define under which conditions you receive interrupts > > > (send FIFO empty, receive FIFO full) and which bits you need to start > > > transfers (one bit to start sending, one receiving). The bits 8 to 31 > > > are used to set up DMA transfers and various interrupt events like > > > error status and finish transfers. I haven't included them as they are > > > not used in the driver. > > > > If they're not used maybe we can drop them :-) > > Obviously they're used, otherwise they wouldn't be defined in the common > header. Whether they are used by these specific CPUs or not isn't > relevant, as there are plenty of cases where they are. grep harder. My point is that if they're only used by arm/sh/* SoCs we don't need to support them in DT.
Hi, 2013/3/12 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Paul, > > On Tuesday 12 March 2013 17:20:39 Paul Mundt wrote: >> On Mon, Mar 11, 2013 at 11:32:31PM +0100, Laurent Pinchart wrote: >> > On Thursday 07 March 2013 11:26:29 Bastian Hecht wrote: >> > > >> +- 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 >> > > > >> > > > What is that for exactly ? >> > > >> > > What I can see from 3 different datasheets (sh7372, sh73a0, r8a7740) >> > > the first 2 bits differ in meaning. >> > > >> > > On r8a7740 it depends: In asynchronous mode the first 2 bits CKE0/1 >> > > define whether you want to use an external clock to drive the baud >> > > generator or if you want to use the internal SUB clock. If you use the >> > > SUB clock you can further define if you want to use a subscaled SUB >> > > clock in the SCSMR register. In synchronous mode you must rely on the >> > > internal clock for the baud generator and can select if the SCK pin is >> > > used as clock input or output. >> > >> > What about adding an optional source clock property to the bindings, that >> > would contain the phandle of the source clock ? If the property is absent >> > then the internal clock would be used. >> >> We can't really use the internal oscillator at the moment, it's one of >> the things that is blocked on adoption of the common struct clk work. The >> baud rate generator algo will likewise differ depending on whether it's >> using an internally generated clock or not, which also makes a different >> set of baud rates available. >> >> My plan originally was to wait until the common struct clk stuff came >> along well enough that we could provide a clock dynamically depending on >> the target baud (or for avoiding cpufreq pre/post-change notifier >> chains), but this obviously hasn't happened yet. >> >> I think your idea for the abstraction with the phandle sounds like the >> right approach, but we aren't presently in a state where we can handle >> that. > > Do the platforms that will be converted to DT currently need to make the clock > source configurable ? If not we could just leave it out of the DT bindings > until needed. Hopefully by then we'll have a common clock framework > implementation. The r8a7779 does use the external clock (according to my r8a7740 datasheet) while the other mainline SoCs use the internal one. >> > > Bit 2 and 3 don't exist in my datasheets. >> > > >> > > The other bits define under which conditions you receive interrupts >> > > (send FIFO empty, receive FIFO full) and which bits you need to start >> > > transfers (one bit to start sending, one receiving). The bits 8 to 31 >> > > are used to set up DMA transfers and various interrupt events like >> > > error status and finish transfers. I haven't included them as they are >> > > not used in the driver. >> > >> > If they're not used maybe we can drop them :-) >> >> Obviously they're used, otherwise they wouldn't be defined in the common >> header. Whether they are used by these specific CPUs or not isn't >> relevant, as there are plenty of cases where they are. grep harder. > > My point is that if they're only used by arm/sh/* SoCs we don't need to > support them in DT. Assuming that SCSCR_TE and SCSCR_RE are always used and we could handle the clock flags with a phandle and the other flags are only used in the realtime domain (where we don't want DT support?), I suppose we could indeed drop the whole property - but we would have to wait. Otherwise it's not so shiny but we could get it merged now. I don't know how to proceed now, any decision makers here? Cheers, Bastian > -- > Regards, > > Laurent Pinchart > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bastian, On Tuesday 19 March 2013 16:10:57 Bastian Hecht wrote: > 2013/3/12 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > > On Tuesday 12 March 2013 17:20:39 Paul Mundt wrote: > >> On Mon, Mar 11, 2013 at 11:32:31PM +0100, Laurent Pinchart wrote: > >> > On Thursday 07 March 2013 11:26:29 Bastian Hecht wrote: > >> > > >> +- 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 > >> > > > > >> > > > What is that for exactly ? > >> > > > >> > > What I can see from 3 different datasheets (sh7372, sh73a0, r8a7740) > >> > > the first 2 bits differ in meaning. > >> > > > >> > > On r8a7740 it depends: In asynchronous mode the first 2 bits CKE0/1 > >> > > define whether you want to use an external clock to drive the baud > >> > > generator or if you want to use the internal SUB clock. If you use > >> > > the SUB clock you can further define if you want to use a subscaled > >> > > SUB clock in the SCSMR register. In synchronous mode you must rely on > >> > > the internal clock for the baud generator and can select if the SCK > >> > > pin is used as clock input or output. > >> > > >> > What about adding an optional source clock property to the bindings, > >> > that would contain the phandle of the source clock ? If the property is > >> > absent then the internal clock would be used. > >> > >> We can't really use the internal oscillator at the moment, it's one of > >> the things that is blocked on adoption of the common struct clk work. The > >> baud rate generator algo will likewise differ depending on whether it's > >> using an internally generated clock or not, which also makes a different > >> set of baud rates available. > >> > >> My plan originally was to wait until the common struct clk stuff came > >> along well enough that we could provide a clock dynamically depending on > >> the target baud (or for avoiding cpufreq pre/post-change notifier > >> chains), but this obviously hasn't happened yet. > >> > >> I think your idea for the abstraction with the phandle sounds like the > >> right approach, but we aren't presently in a state where we can handle > >> that. > > > > Do the platforms that will be converted to DT currently need to make the > > clock source configurable ? If not we could just leave it out of the DT > > bindings until needed. Hopefully by then we'll have a common clock > > framework implementation. > > The r8a7779 does use the external clock (according to my r8a7740 > datasheet) while the other mainline SoCs use the internal one. > > >> > > Bit 2 and 3 don't exist in my datasheets. > >> > > > >> > > The other bits define under which conditions you receive interrupts > >> > > (send FIFO empty, receive FIFO full) and which bits you need to start > >> > > transfers (one bit to start sending, one receiving). The bits 8 to 31 > >> > > are used to set up DMA transfers and various interrupt events like > >> > > error status and finish transfers. I haven't included them as they > >> > > are not used in the driver. > >> > > >> > If they're not used maybe we can drop them :-) > >> > >> Obviously they're used, otherwise they wouldn't be defined in the common > >> header. Whether they are used by these specific CPUs or not isn't > >> relevant, as there are plenty of cases where they are. grep harder. > > > > My point is that if they're only used by arm/sh/* SoCs we don't need to > > support them in DT. > > Assuming that SCSCR_TE and SCSCR_RE are always used and we could handle the > clock flags with a phandle and the other flags are only used in the realtime > domain (where we don't want DT support?), I suppose we could indeed drop the > whole property - but we would have to wait. Otherwise it's not so shiny but > we could get it merged now. I don't know how to proceed now, any decision > makers here? My main concern is that DT bindings are ABIs that need to be supported forever. This is why I'd rather not add those flags to the DT bindings now only to deprecate them in a couple of kernel versions.
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..d80039e --- /dev/null +++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt @@ -0,0 +1,47 @@ +* Renesas SH-Mobile Serial Communication Interface + +Required properties: +- compatible : Should be "renesas,sci-<port type>-uart", where <port type> is + "sci", "scif", "irda", "scifa", "scifb" + or for legacy devices + "sh2_scif_fifodata", "sh3_scif", "sh4_scif", "sh4_scif_no_scsptr", + "sh4_scif_fifodata", "sh7705_scif". +- reg : Address and length of the register set for the device +- interrupts : Should contain the following IRQs in this order: + ERI: receive-error interrupt + RXI: receive-FIFO-data-full interrupt + TXI: transmit-FIFO-data-empty interrupt + BRI: break reception interrupt +- interrupt-names: The IRQ names "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-algorithm : Choose an algorithm ID for the baud rate generator. + 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. + +Example: + serial@e6c50000 { + compatible = "renesas,sci-scifa-uart"; + interrupt-parent = <&intca>; + reg = <0xe6c50000 0x100>; + interrupts = <0x0c20>, <0x0c20>, <0x0c20>, <0x0c20>; + interrupt-names = "eri", "rxi", "txi", "bri"; + cell-index = <1>; + renesas,scscr = <0x30>; + renesas,scbrr-algorithm = <4>; + renesas,autoconf; + }; diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 6147756..03bb740 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,131 @@ static int sci_remove(struct platform_device *dev) return 0; } +static const struct of_device_id of_sci_match[] = { + { .compatible = "renesas,sci-sci-uart", + .data = (void *)SCIx_SCI_REGTYPE }, + { .compatible = "renesas,sci-scif-uart", + .data = (void *)SCIx_SH4_SCIF_REGTYPE, }, + { .compatible = "renesas,sci-irda-uart", + .data = (void *)SCIx_IRDA_REGTYPE }, + { .compatible = "renesas,sci-scifa-uart", + .data = (void *)SCIx_SCIFA_REGTYPE }, + { .compatible = "renesas,sci-scifb-uart", + .data = (void *)SCIx_SCIFB_REGTYPE }, + { .compatible = "renesas,sci-sh2_scif_fifodata-uart", + .data = (void *)SCIx_SH2_SCIF_FIFODATA_REGTYPE }, + { .compatible = "renesas,sci-sh3_scif-uart", + .data = (void *)SCIx_SH3_SCIF_REGTYPE }, + { .compatible = "renesas,sci-sh4_scif-uart", + .data = (void *)SCIx_SH4_SCIF_REGTYPE }, + { .compatible = "renesas,sci-sh4_scif_no_scsptr-uart", + .data = (void *)SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE }, + { .compatible = "renesas,sci-sh4_scif_fifodata-uart", + .data = (void *)SCIx_SH4_SCIF_FIFODATA_REGTYPE }, + { .compatible = "renesas,sci-sh7705_scif-uart", + .data = (void *)SCIx_SH7705_SCIF_REGTYPE }, + {}, +}; +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; + + if (!IS_ENABLED(CONFIG_OF) || !np) + return NULL; + + 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-algorithm", NULL); + if (!prop) { + dev_err(&pdev->dev, "required DT prop scbrr-algorithm missing\n"); + return NULL; + } + val = be32_to_cpup(prop); + if (val <= SCBRR_ALGO_INVALID || val >= SCBRR_NR_ALGOS) { + dev_err(&pdev->dev, "DT prop clock-algorithm 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; + + p->regtype = (unsigned int)match->data; + + switch (p->regtype) { + case SCIx_SCI_REGTYPE: + p->type = PORT_SCI; + break; + case SCIx_SH4_SCIF_REGTYPE: + p->type = PORT_SCIF; + break; + case SCIx_IRDA_REGTYPE: + p->type = PORT_IRDA; + break; + case SCIx_SCIFA_REGTYPE: + p->type = PORT_SCIFA; + break; + case SCIx_SCIFB_REGTYPE: + p->type = PORT_SCIFB; + break; + default: + /* legacy register sets default to PORT_SCIF */ + p->type = PORT_SCIF; + break; + } + + return p; +} + static int sci_probe_single(struct platform_device *dev, unsigned int index, struct plat_sci_port *p, @@ -2385,9 +2511,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 +2523,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 +2588,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)