| Message ID | 1361298586-30357-2-git-send-email-hechtb+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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. -- 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, 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 -- 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 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. -- 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/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 -- 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 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. -- 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 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 -- 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! 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 -- 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
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), }, };
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