| Message ID | 1384054421-13357-29-git-send-email-laurent.pinchart+renesas@ideasonboard.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 10-11-2013 7:33, Laurent Pinchart wrote: > Document the device tree bindings for the sci serial port devices. > Cc: devicetree@vger.kernel.org Hm, why are you not CCing a bunch of people responsible for the DT bindings maintenance that scripts/get_maintainer.pl should yield? > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Acked-by: Simon Horman <horms+renesas@verge.net.au> > --- > .../bindings/serial/renesas,sci-serial.txt | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > new file mode 100644 > index 0000000..66d3bca > --- /dev/null > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > @@ -0,0 +1,42 @@ > +* Renesas SH-Mobile Serial Communication Interface > + > +Required properties: > + > + - compatible: one of the following types (scif, scifa, scifb, hscif). > + > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible UART. > + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB compatible UART. > + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) HSCIF compatible UART. > + - "renesas,scif-generic" for generic SCIF compatible UART. > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > + - "renesas,hscif-generic" for generic HSCIF compatible UART. > + > + When compatible with the generic version, nodes must also list the > + SoC-specific version corresponding to the platform. Which are not all specified here? > + > + - reg: Base address and length of the memory resource used by the UART. > + > + - interrupt-parent: Reference to the parent interrupt controller. This is not a required property. It can be specified in the parent node. > + - interrupts: Interrupt number. s/number/specifier/ > + > + - clocks: Reference to the SCIx UART interface clock. > + - clock-names: Should be "sci_ick". > + > +Note: Each enabled SCIx UART should have an alias correctly numbered in the > +"aliases" node. > + > +Example: > + aliases { > + serial0 = &scifa0; > + }; > + > + scifa0: serial@e6c40000 { > + compatible = "renesas,scifa-r8a7790", "renesas,scifa-generic"; > + reg = <0 0xe6c40000 0 64>; > + interrupt-parent = <&gic>; > + interrupts = <0 144 4>; > + clocks = <&mstp2_clks 4>; > + clock-names = "sci_ick"; > + }; WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Sunday 10 November 2013 22:07:16 Sergei Shtylyov wrote: > On 10-11-2013 7:33, Laurent Pinchart wrote: > > Document the device tree bindings for the sci serial port devices. > > > > Cc: devicetree@vger.kernel.org > > Hm, why are you not CCing a bunch of people responsible for the DT bindings > maintenance that scripts/get_maintainer.pl should yield? Because they're subscribed to the devicetree list, and they're already overwhelmed with e-mails. > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > Acked-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > > > .../bindings/serial/renesas,sci-serial.txt | 42 ++++++++++++++++ > > 1 file changed, 42 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/serial/renesas,sci-serial.txt> > > diff --git > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt new > > file mode 100644 > > index 0000000..66d3bca > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > @@ -0,0 +1,42 @@ > > +* Renesas SH-Mobile Serial Communication Interface > > + > > +Required properties: > > + > > + - compatible: one of the following types (scif, scifa, scifb, hscif). > > + > > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. > > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible > > UART. > > + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB compatible > > UART. > > + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) HSCIF compatible > > UART. > > + - "renesas,scif-generic" for generic SCIF compatible UART. > > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > > + - "renesas,hscif-generic" for generic HSCIF compatible UART. > > + > > + When compatible with the generic version, nodes must also list the > > + SoC-specific version corresponding to the platform. > > Which are not all specified here? The r8a7790-specific versions are. Other SoCs will be added later when ported to SCIF DT. > > + > > + - reg: Base address and length of the memory resource used by the UART. > > + > > + - interrupt-parent: Reference to the parent interrupt controller. > > This is not a required property. It can be specified in the parent node. But in practice it isn't for the SCIF devices. Given that we'll soon get a new way to specify interrupts, I think I'll just reference the corresponding DT bindings document when it will be updated instead of documenting all possibilities here. > > + - interrupts: Interrupt number. > > s/number/specifier/ I'll fix that. > > > + > > + - clocks: Reference to the SCIx UART interface clock. > > + - clock-names: Should be "sci_ick". > > + > > +Note: Each enabled SCIx UART should have an alias correctly numbered in > > the +"aliases" node. > > + > > +Example: > > + aliases { > > + serial0 = &scifa0; > > + }; > > + > > + scifa0: serial@e6c40000 { > > + compatible = "renesas,scifa-r8a7790", "renesas,scifa-generic"; > > + reg = <0 0xe6c40000 0 64>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 144 4>; > > + clocks = <&mstp2_clks 4>; > > + clock-names = "sci_ick"; > > + };
Hi Laurent, I have a few comments. On Sun, Nov 10, 2013 at 03:33:40AM +0000, Laurent Pinchart wrote: > Document the device tree bindings for the sci serial port devices. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Acked-by: Simon Horman <horms+renesas@verge.net.au> > --- > .../bindings/serial/renesas,sci-serial.txt | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > new file mode 100644 > index 0000000..66d3bca > --- /dev/null > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > @@ -0,0 +1,42 @@ > +* Renesas SH-Mobile Serial Communication Interface > + > +Required properties: > + > + - compatible: one of the following types (scif, scifa, scifb, hscif). Minor nit, but would be nice to have "Should contain one of the following:". We might have future variants whereby the compatible string will actually be a string list. > + > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible UART. > + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB compatible UART. > + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) HSCIF compatible UART. > + - "renesas,scif-generic" for generic SCIF compatible UART. > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > + - "renesas,hscif-generic" for generic HSCIF compatible UART. Is the "-generic" suffix necessary? Why not just "renesas,scif" etc? > + > + When compatible with the generic version, nodes must also list the > + SoC-specific version corresponding to the platform. Presumably the SoC-specific version should come first; it would be nice to note that. > + > + - reg: Base address and length of the memory resource used by the UART. I assume by memory resource you mean the memory-mapped registers? Resource sounds like Linux-internal nomenclature leaking. > + > + - interrupt-parent: Reference to the parent interrupt controller. I don't think this is strictly necessary. It's implicit by default and can be added optionally when a system is wired in a complex way. As it's a completely standard optional property I'm not even sure it needs to be documented. > + - interrupts: Interrupt number. Minor nit: "Should contain an interrupt-specifier for ..." I saw the cover mentioned multiple interrupts. Which logical interrupt output of the device are you expecting here? > + > + - clocks: Reference to the SCIx UART interface clock. Minor nit: s/Reference to/A phandle + clock-specifier pair for/ > + - clock-names: Should be "sci_ick". As we're using clock-names, it would be nicer still to define clocks in terms of clock-names so as to make it easier to update the document in future and make the expected use of clock-names clearer: - clocks: Should contain a phandle + clock-specifier pair for each entry in clock-names. - clock-names: Should contain "sci_ick". > + > +Note: Each enabled SCIx UART should have an alias correctly numbered in the > +"aliases" node. > + > +Example: > + aliases { > + serial0 = &scifa0; > + }; > + > + scifa0: serial@e6c40000 { > + compatible = "renesas,scifa-r8a7790", "renesas,scifa-generic"; > + reg = <0 0xe6c40000 0 64>; > + interrupt-parent = <&gic>; > + interrupts = <0 144 4>; > + clocks = <&mstp2_clks 4>; > + clock-names = "sci_ick"; > + }; Otherwise this looks good to me. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, Thank you for the quick and detailed review. On Monday 11 November 2013 09:40:39 Mark Rutland wrote: > Hi Laurent, > > I have a few comments. > > On Sun, Nov 10, 2013 at 03:33:40AM +0000, Laurent Pinchart wrote: > > Document the device tree bindings for the sci serial port devices. > > > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > Acked-by: Simon Horman <horms+renesas@verge.net.au> > > --- > > > > .../bindings/serial/renesas,sci-serial.txt | 42 +++++++++++++++++ > > 1 file changed, 42 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/serial/renesas,sci-serial.txt> > > diff --git > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt new > > file mode 100644 > > index 0000000..66d3bca > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > @@ -0,0 +1,42 @@ > > +* Renesas SH-Mobile Serial Communication Interface > > + > > +Required properties: > > + > > + - compatible: one of the following types (scif, scifa, scifb, hscif). > > Minor nit, but would be nice to have "Should contain one of the > following:". We might have future variants whereby the compatible string > will actually be a string list. Sure, I can do that. > > + > > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. > > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible > > UART. + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB > > compatible UART. + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) > > HSCIF compatible UART. + - "renesas,scif-generic" for generic SCIF > > compatible UART. > > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > > + - "renesas,hscif-generic" for generic HSCIF compatible UART. > > Is the "-generic" suffix necessary? Why not just "renesas,scif" etc? You're right, I'll remove the suffix. > > + > > + When compatible with the generic version, nodes must also list the > > + SoC-specific version corresponding to the platform. > > Presumably the SoC-specific version should come first; it would be nice > to note that. Indeed, I'll do so. > > + > > + - reg: Base address and length of the memory resource used by the UART. > > I assume by memory resource you mean the memory-mapped registers? > Resource sounds like Linux-internal nomenclature leaking. I'll fix it as well, although we could decide on a definition of "resource" that isn't Linux-specific :-) > > + > > + - interrupt-parent: Reference to the parent interrupt controller. > > I don't think this is strictly necessary. It's implicit by default and > can be added optionally when a system is wired in a complex way. As it's > a completely standard optional property I'm not even sure it needs to be > documented. > > > + - interrupts: Interrupt number. > > Minor nit: "Should contain an interrupt-specifier for ..." > > I saw the cover mentioned multiple interrupts. Which logical interrupt > output of the device are you expecting here? SCI devices come in several flavours, with individual interrupt sources wired up to different interrupt lines, or multiplexed all together. As the DT bindings support the later only at the moment, there's only a single interrupt to handle. Should we come up with a standard way of saying in the bindings that a device uses the interrupt bindings defined in interrupt-controller/interrupts.txt ? This will become even more important with the interrupts-extended property, we don't want every DT bindings document to detail all possible way to specify interrupts. > > + > > + - clocks: Reference to the SCIx UART interface clock. > > Minor nit: s/Reference to/A phandle + clock-specifier pair for/ > > > + - clock-names: Should be "sci_ick". > > As we're using clock-names, it would be nicer still to define clocks in > terms of clock-names so as to make it easier to update the document in > future and make the expected use of clock-names clearer: > > - clocks: Should "Should", or "must" ? > contain a phandle + clock-specifier pair for each entry > in clock-names. > > - clock-names: Should contain "sci_ick". Shouldn't that be "Should contain "sci_ick" for the SCIx UART interface clock." ? Otherwise the bindings wouldn't tell which clock the clocks property should reference. > > + > > +Note: Each enabled SCIx UART should have an alias correctly numbered in > > the +"aliases" node. > > + > > +Example: > > + aliases { > > + serial0 = &scifa0; > > + }; > > + > > + scifa0: serial@e6c40000 { > > + compatible = "renesas,scifa-r8a7790", "renesas,scifa-generic"; > > + reg = <0 0xe6c40000 0 64>; > > + interrupt-parent = <&gic>; > > + interrupts = <0 144 4>; > > + clocks = <&mstp2_clks 4>; > > + clock-names = "sci_ick"; > > + }; > > Otherwise this looks good to me.
On Mon, Nov 11, 2013 at 01:26:11PM +0000, Laurent Pinchart wrote: > Hi Mark, > > Thank you for the quick and detailed review. > > On Monday 11 November 2013 09:40:39 Mark Rutland wrote: > > Hi Laurent, > > > > I have a few comments. > > > > On Sun, Nov 10, 2013 at 03:33:40AM +0000, Laurent Pinchart wrote: > > > Document the device tree bindings for the sci serial port devices. > > > > > > Cc: devicetree@vger.kernel.org > > > Signed-off-by: Laurent Pinchart > > > <laurent.pinchart+renesas@ideasonboard.com> > > > Acked-by: Simon Horman <horms+renesas@verge.net.au> > > > --- > > > > > > .../bindings/serial/renesas,sci-serial.txt | 42 +++++++++++++++++ > > > 1 file changed, 42 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/serial/renesas,sci-serial.txt> > > > diff --git > > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt new > > > file mode 100644 > > > index 0000000..66d3bca > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > > @@ -0,0 +1,42 @@ > > > +* Renesas SH-Mobile Serial Communication Interface > > > + > > > +Required properties: > > > + > > > + - compatible: one of the following types (scif, scifa, scifb, hscif). > > > > Minor nit, but would be nice to have "Should contain one of the > > following:". We might have future variants whereby the compatible string > > will actually be a string list. > > Sure, I can do that. > > > > + > > > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. > > > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible > > > UART. + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB > > > compatible UART. + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) > > > HSCIF compatible UART. + - "renesas,scif-generic" for generic SCIF > > > compatible UART. > > > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > > > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > > > + - "renesas,hscif-generic" for generic HSCIF compatible UART. > > > > Is the "-generic" suffix necessary? Why not just "renesas,scif" etc? > > You're right, I'll remove the suffix. > > > > + > > > + When compatible with the generic version, nodes must also list the > > > + SoC-specific version corresponding to the platform. > > > > Presumably the SoC-specific version should come first; it would be nice > > to note that. > > Indeed, I'll do so. > > > > + > > > + - reg: Base address and length of the memory resource used by the UART. > > > > I assume by memory resource you mean the memory-mapped registers? > > Resource sounds like Linux-internal nomenclature leaking. > > I'll fix it as well, although we could decide on a definition of "resource" > that isn't Linux-specific :-) We could, I guess. For the moment I'd prefer to point out that these are MMIO registers :) > > > > + > > > + - interrupt-parent: Reference to the parent interrupt controller. > > > > I don't think this is strictly necessary. It's implicit by default and > > can be added optionally when a system is wired in a complex way. As it's > > a completely standard optional property I'm not even sure it needs to be > > documented. > > > > > + - interrupts: Interrupt number. > > > > Minor nit: "Should contain an interrupt-specifier for ..." > > > > I saw the cover mentioned multiple interrupts. Which logical interrupt > > output of the device are you expecting here? > > SCI devices come in several flavours, with individual interrupt sources wired > up to different interrupt lines, or multiplexed all together. As the DT > bindings support the later only at the moment, there's only a single interrupt > to handle. Ok. If the binding just covers the muxed interrupt for now that's fine. We can add interrupt-names later when we support multiple interrupts (and fall back to assuming muxed when no interrupt-names property is present). > > Should we come up with a standard way of saying in the bindings that a device > uses the interrupt bindings defined in interrupt-controller/interrupts.txt ? > This will become even more important with the interrupts-extended property, we > don't want every DT bindings document to detail all possible way to specify > interrupts. It would certainly be nice to have a standard way of doing just that. I'd hope that with the schema work going on that would fall into place automatically, so perhaps it's not worth doing now. > > > > + > > > + - clocks: Reference to the SCIx UART interface clock. > > > > Minor nit: s/Reference to/A phandle + clock-specifier pair for/ > > > > > + - clock-names: Should be "sci_ick". > > > > As we're using clock-names, it would be nicer still to define clocks in > > terms of clock-names so as to make it easier to update the document in > > future and make the expected use of clock-names clearer: > > > > - clocks: Should > > "Should", or "must" ? I don't expect the format of clocks and the relationship to clock-names to change, so "must" is likely more appropriate. I'm just too used to writing "should"... > > > contain a phandle + clock-specifier pair for each entry > > in clock-names. > > > > - clock-names: Should contain "sci_ick". > > Shouldn't that be > > "Should contain "sci_ick" for the SCIx UART interface clock." > > ? Otherwise the bindings wouldn't tell which clock the clocks property should > reference. Yes, that sounds much better. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Monday 11 November 2013 15:48:49 Mark Rutland wrote: > On Mon, Nov 11, 2013 at 01:26:11PM +0000, Laurent Pinchart wrote: > > Hi Mark, > > > > Thank you for the quick and detailed review. > > > > On Monday 11 November 2013 09:40:39 Mark Rutland wrote: > > > Hi Laurent, > > > > > > I have a few comments. > > > > > > On Sun, Nov 10, 2013 at 03:33:40AM +0000, Laurent Pinchart wrote: > > > > Document the device tree bindings for the sci serial port devices. > > > > > > > > Cc: devicetree@vger.kernel.org > > > > Signed-off-by: Laurent Pinchart > > > > <laurent.pinchart+renesas@ideasonboard.com> > > > > Acked-by: Simon Horman <horms+renesas@verge.net.au> > > > > --- > > > > > > > > .../bindings/serial/renesas,sci-serial.txt | 42 +++++++++++++ > > > > 1 file changed, 42 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/serial/renesas,sci-serial.txt> > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt new > > > > file mode 100644 > > > > index 0000000..66d3bca > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > > > @@ -0,0 +1,42 @@ > > > > +* Renesas SH-Mobile Serial Communication Interface > > > > + > > > > +Required properties: > > > > + > > > > + - compatible: one of the following types (scif, scifa, scifb, > > > > hscif). > > > > > > Minor nit, but would be nice to have "Should contain one of the > > > following:". We might have future variants whereby the compatible string > > > will actually be a string list. > > > > Sure, I can do that. > > > > > > + > > > > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible > > > > UART. > > > > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible > > > > UART. + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB > > > > compatible UART. + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) > > > > HSCIF compatible UART. + - "renesas,scif-generic" for generic SCIF > > > > compatible UART. > > > > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > > > > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > > > > + - "renesas,hscif-generic" for generic HSCIF compatible UART. > > > > > > Is the "-generic" suffix necessary? Why not just "renesas,scif" etc? > > > > You're right, I'll remove the suffix. > > > > > > + > > > > + When compatible with the generic version, nodes must also list > > > > the > > > > + SoC-specific version corresponding to the platform. > > > > > > Presumably the SoC-specific version should come first; it would be nice > > > to note that. > > > > Indeed, I'll do so. > > > > > > + > > > > + - reg: Base address and length of the memory resource used by the > > > > UART. > > > > > > I assume by memory resource you mean the memory-mapped registers? > > > Resource sounds like Linux-internal nomenclature leaking. > > > > I'll fix it as well, although we could decide on a definition of > > "resource" that isn't Linux-specific :-) > > We could, I guess. For the moment I'd prefer to point out that these are > MMIO registers :) > > > > > + > > > > + - interrupt-parent: Reference to the parent interrupt controller. > > > > > > I don't think this is strictly necessary. It's implicit by default and > > > can be added optionally when a system is wired in a complex way. As it's > > > a completely standard optional property I'm not even sure it needs to be > > > documented. > > > > > > > + - interrupts: Interrupt number. > > > > > > Minor nit: "Should contain an interrupt-specifier for ..." > > > > > > I saw the cover mentioned multiple interrupts. Which logical interrupt > > > output of the device are you expecting here? > > > > SCI devices come in several flavours, with individual interrupt sources > > wired up to different interrupt lines, or multiplexed all together. As > > the DT bindings support the later only at the moment, there's only a > > single interrupt to handle. > > Ok. If the binding just covers the muxed interrupt for now that's fine. > We can add interrupt-names later when we support multiple interrupts > (and fall back to assuming muxed when no interrupt-names property is > present). That was my plan, good. > > Should we come up with a standard way of saying in the bindings that a > > device uses the interrupt bindings defined in > > interrupt-controller/interrupts.txt ? This will become even more > > important with the interrupts-extended property, we don't want every DT > > bindings document to detail all possible way to specify interrupts. > > It would certainly be nice to have a standard way of doing just that. > I'd hope that with the schema work going on that would fall into place > automatically, so perhaps it's not worth doing now. Fine with me. > > > > + > > > > + - clocks: Reference to the SCIx UART interface clock. > > > > > > Minor nit: s/Reference to/A phandle + clock-specifier pair for/ > > > > > > > + - clock-names: Should be "sci_ick". > > > > > > As we're using clock-names, it would be nicer still to define clocks in > > > terms of clock-names so as to make it easier to update the document in > > > future and make the expected use of clock-names clearer: > > > > > > - clocks: Should > > > > "Should", or "must" ? > > I don't expect the format of clocks and the relationship to clock-names > to change, so "must" is likely more appropriate. I'm just too used to > writing "should"... Are "Should" and "Must" to be interpreted as described in http://tools.ietf.org/html/rfc2119 ? 1. MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification. 3. SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. If so, shouldn't we use "Must" in most places ? > > > contain a phandle + clock-specifier pair for each entry > > > > > > in clock-names. > > > > > > - clock-names: Should contain "sci_ick". > > > > Shouldn't that be > > > > "Should contain "sci_ick" for the SCIx UART interface clock." > > > > ? Otherwise the bindings wouldn't tell which clock the clocks property > > should reference. > > Yes, that sounds much better.
diff --git a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt new file mode 100644 index 0000000..66d3bca --- /dev/null +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt @@ -0,0 +1,42 @@ +* Renesas SH-Mobile Serial Communication Interface + +Required properties: + + - compatible: one of the following types (scif, scifa, scifb, hscif). + + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible UART. + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB compatible UART. + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) HSCIF compatible UART. + - "renesas,scif-generic" for generic SCIF compatible UART. + - "renesas,scifa-generic" for generic SCIFA compatible UART. + - "renesas,scifb-generic" for generic SCIFB compatible UART. + - "renesas,hscif-generic" for generic HSCIF compatible UART. + + When compatible with the generic version, nodes must also list the + SoC-specific version corresponding to the platform. + + - reg: Base address and length of the memory resource used by the UART. + + - interrupt-parent: Reference to the parent interrupt controller. + - interrupts: Interrupt number. + + - clocks: Reference to the SCIx UART interface clock. + - clock-names: Should be "sci_ick". + +Note: Each enabled SCIx UART should have an alias correctly numbered in the +"aliases" node. + +Example: + aliases { + serial0 = &scifa0; + }; + + scifa0: serial@e6c40000 { + compatible = "renesas,scifa-r8a7790", "renesas,scifa-generic"; + reg = <0 0xe6c40000 0 64>; + interrupt-parent = <&gic>; + interrupts = <0 144 4>; + clocks = <&mstp2_clks 4>; + clock-names = "sci_ick"; + };