| Message ID | 20180806140755.24087-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| Headers | show |
| Series | sh-sci : Do not derive regshift from regsize | expand |
Hi Geert, On Monday, 6 August 2018 17:07:51 EEST Geert Uytterhoeven wrote: > Hi all, > > This RFC patch series was sparked by noticing that commit 2d4dd0da45401c7a Where can that commit be found ? > ("serial: sh-sci: Allow for compressed SCIF address") broke earlycon > support on most Renesas ARM SoCs using SCIF ports, and by the fragility of > deriving regshift from the register block size (which may be rounded up): Why should it be rounded up ? > 1. The first patch is an old patch from Sato-san, which I never really > understood. But it turned out to be a dependency for patch 2. > 2. Patch 2 makes sure regshift is initialized when using earlycon, > unbreaking the serial console on e.g. R-Car Gen2 and Gen3. > 3. Patch 3 reverts the patch that started deriving regshift from the > register block size, and that removed the plat_sci_port.regshift > field. Which is a field I needed again in patch 4. > 4. Patch 4 removes the remaining regshift derivations on DT platforms. > (5. I didn't bother writing patch 5, which involves adding .regshift > initializations to all SH board files that need it.) > > However, I'm not happy with the end result, so please DO NOT apply this! > As I spent almost a full day on this, and would still like to know the > story about "sh-sci: Use a separate sci_port for earlycon", I decided to > post it anyway. > > As earlycon will be broken in v4.19-rc1 on RZ/A1, RZ/G, and R-Car, assuming > no other actions are taken, an alternative solution would be to: > 1. Revert commit 7acece71a517cad8 ("serial: sh-sci: Remove > SCIx_RZ_SCIFA_REGTYPE"), > 2. Revert commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed > SCIF address") alternative, > 3. Add an OF_EARLYCON_DECLARE() for RZ/A2, to fix earlycon on RZ/A2. > > What do you think? > Thanks for your comments! > > P.S. Apparently SCIx_SH4_SCIF_REGTYPE and SCIx_SH2_SCIF_FIFODATA_REGTYPE > are identical? > > Geert Uytterhoeven (3): > [RFC] sh-sci: Take into account regshift to fix earlycon breakage > [RFC] Revert "serial: sh-sci: Compute the regshift value for SCI > ports" > [RFC] sh-sci: Derive regshift value from DT compatible value > > Yoshinori Sato (1): > [RFC] sh-sci: Use a separate sci_port for earlycon > > arch/sh/kernel/cpu/sh3/setup-sh770x.c | 1 + > arch/sh/kernel/cpu/sh4/setup-sh7750.c | 3 +- > arch/sh/kernel/cpu/sh4/setup-sh7760.c | 10 +--- > drivers/tty/serial/sh-sci.c | 68 +++++++++++++++++---------- > include/linux/serial_sci.h | 1 + > 5 files changed, 49 insertions(+), 34 deletions(-)
On Monday, 6 August 2018 17:37:45 EEST Laurent Pinchart wrote: > On Monday, 6 August 2018 17:07:51 EEST Geert Uytterhoeven wrote: > > Hi all, > > > > This RFC patch series was sparked by noticing that commit 2d4dd0da45401c7a > > Where can that commit be found ? I found it in -next, sorry for the noise. > > ("serial: sh-sci: Allow for compressed SCIF address") broke earlycon > > support on most Renesas ARM SoCs using SCIF ports, and by the fragility of > > > deriving regshift from the register block size (which may be rounded up): > > Why should it be rounded up ? > > > 1. The first patch is an old patch from Sato-san, which I never really > > understood. But it turned out to be a dependency for patch 2. > > 2. Patch 2 makes sure regshift is initialized when using earlycon, > > unbreaking the serial console on e.g. R-Car Gen2 and Gen3. > > 3. Patch 3 reverts the patch that started deriving regshift from the > > register block size, and that removed the plat_sci_port.regshift > > field. Which is a field I needed again in patch 4. > > 4. Patch 4 removes the remaining regshift derivations on DT platforms. > > (5. I didn't bother writing patch 5, which involves adding .regshift > > initializations to all SH board files that need it.) > > > > However, I'm not happy with the end result, so please DO NOT apply this! > > As I spent almost a full day on this, and would still like to know the > > story about "sh-sci: Use a separate sci_port for earlycon", I decided to > > post it anyway. > > > > As earlycon will be broken in v4.19-rc1 on RZ/A1, RZ/G, and R-Car, > > assuming > > > > no other actions are taken, an alternative solution would be to: > > 1. Revert commit 7acece71a517cad8 ("serial: sh-sci: Remove > > SCIx_RZ_SCIFA_REGTYPE"), > > 2. Revert commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed > > SCIF address") alternative, > > 3. Add an OF_EARLYCON_DECLARE() for RZ/A2, to fix earlycon on RZ/A2. > > > > What do you think? > > Thanks for your comments! > > > > P.S. Apparently SCIx_SH4_SCIF_REGTYPE and SCIx_SH2_SCIF_FIFODATA_REGTYPE > > > > are identical? > > > > Geert Uytterhoeven (3): > > [RFC] sh-sci: Take into account regshift to fix earlycon breakage > > [RFC] Revert "serial: sh-sci: Compute the regshift value for SCI > > ports" > > [RFC] sh-sci: Derive regshift value from DT compatible value > > > > Yoshinori Sato (1): > > [RFC] sh-sci: Use a separate sci_port for earlycon > > > > arch/sh/kernel/cpu/sh3/setup-sh770x.c | 1 + > > arch/sh/kernel/cpu/sh4/setup-sh7750.c | 3 +- > > arch/sh/kernel/cpu/sh4/setup-sh7760.c | 10 +--- > > drivers/tty/serial/sh-sci.c | 68 +++++++++++++++++---------- > > include/linux/serial_sci.h | 1 + > > 5 files changed, 49 insertions(+), 34 deletions(-)
Hi Laurent, On Mon, Aug 6, 2018 at 4:37 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Monday, 6 August 2018 17:07:51 EEST Geert Uytterhoeven wrote: > > This RFC patch series was sparked by noticing that commit 2d4dd0da45401c7a > > Where can that commit be found ? tty-next > > ("serial: sh-sci: Allow for compressed SCIF address") broke earlycon > > support on most Renesas ARM SoCs using SCIF ports, and by the fragility of > > deriving regshift from the register block size (which may be rounded up): > > Why should it be rounded up ? There's no requirement to round it up. But it's not uncommon to round up register block sizes to the next power of two, or more. In hindsight, perhaps we should have adopted the "reg-shift" DT property, which is used by other serial port drivers, and by of_setup_earlycon(). Gr{oetje,eeting}s, Geert
On Mon, Aug 6, 2018 at 4:08 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote: > This RFC patch series was sparked by noticing that commit 2d4dd0da45401c7a > ("serial: sh-sci: Allow for compressed SCIF address") broke earlycon > support on most Renesas ARM SoCs using SCIF ports, and by the fragility of > deriving regshift from the register block size (which may be rounded up): > 1. The first patch is an old patch from Sato-san, which I never really > understood. But it turned out to be a dependency for patch 2. > 2. Patch 2 makes sure regshift is initialized when using earlycon, > unbreaking the serial console on e.g. R-Car Gen2 and Gen3. > 3. Patch 3 reverts the patch that started deriving regshift from the > register block size, and that removed the plat_sci_port.regshift > field. Which is a field I needed again in patch 4. > 4. Patch 4 removes the remaining regshift derivations on DT platforms. > (5. I didn't bother writing patch 5, which involves adding .regshift > initializations to all SH board files that need it.) > > However, I'm not happy with the end result, so please DO NOT apply this! > As I spent almost a full day on this, and would still like to know the > story about "sh-sci: Use a separate sci_port for earlycon", I decided to > post it anyway. > > As earlycon will be broken in v4.19-rc1 on RZ/A1, RZ/G, and R-Car, assuming > no other actions are taken, an alternative solution would be to: > 1. Revert commit 7acece71a517cad8 ("serial: sh-sci: Remove > SCIx_RZ_SCIFA_REGTYPE"), > 2. Revert commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed > SCIF address") alternative, > 3. Add an OF_EARLYCON_DECLARE() for RZ/A2, to fix earlycon on RZ/A2. > > What do you think? > Thanks for your comments! I gave this a bit more thought, and these are my conclusions: 1. RZ/A2 SCIF is not really compatible with SCIF on RZ/A1, RZ/G, and R-Car, due to the absence of support for the DT "reg-shift" property. Given RZ/A1 and R-Car would need "reg-shift = <1>", we cannot retroactively add support for that for ports declared compatible with "renesas,scif" (assuming regshift == 1 in case the "reg-shift" property is not present may work for the main serial driver, but not for earlycon, where of_setup_earlycon() is in charge of all register block parsing). Sorry for suggesting this in the first place. Based on the regshift handling for SCI, I assumed it was fine, but apparently that particular code isn't without problems on SCI ports neither. 2. SCI ports are the only ports where a DT "reg-shift" property makes sense, as it is the only port type with fixed (8-bit) register sizes, with registers spaced 1, 2 or 4 bytes apart (regshift = 0, 1, or 2). All other types use a mix of 8-bit and 16-bit registers, 2 or 4 bytes apart. - On H8/300 (DT only), regshift is always zero. AFAIK this is handled correctly for earlycon, but not for the main serial driver (presumably broken since commit dfc80387aefb78161f83732804c6d01c89c24595, cfr. Sato-san's patch "serial: sh-sci: byte allocated register support" (https://www.spinics.net/lists/linux-sh/msg53175.html)). - On SuperH (non-DT), regshift is 1 or 2. If/when DT support is ever added, the "reg-shift" DT property can be used to indicate that. Suggestions for actions: 1. Revert commit 7acece71a517cad8 ("serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE"), as this is a dependency for step 2, 2. Revert commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed SCIF address"), to unbreak earlycon on RZ/A1, RZ/G, and R-Car SCIF ports, 3. Restrict the regshift setup (1 or 2) for PORT_SCI to the !dev->dev.of_node case, to fix SCI ports instantiated from DT on H8/300. 4. Drop "renesas,scif" from the compatible value in the (not yet submitted) r7s9210.dtsi (this may have to be clarified in the DT bindings, although they already say "renesas,scif" is only meant for ports compatible with the generic version, whatever that means ;-), 5. Add an OF_EARLYCON_DECLARE() for RZ/A2, setting port_cfg.regtype to SCIx_RZ_SCIFA_REGTYPE, to (presumably) make earlycon work on RZ/A2, 1-2 are regression fixes that can be done immediately, 3 is a bug fix to be submitted, 5 is an enhancement to be submitted. Any other opinions or comments? As both Greg and I will be on holidays until after the v4.18 release, you have a bit more time to polish all of this for the serial pull request for v4.19... Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Wednesday, August 08, 2018, Geert Uytterhoeven wrote: > Suggestions for actions: > 1. Revert commit 7acece71a517cad8 ("serial: sh-sci: Remove > SCIx_RZ_SCIFA_REGTYPE"), as this is a dependency for step 2, > 2. Revert commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed > SCIF address"), to unbreak earlycon on RZ/A1, RZ/G, and R-Car SCIF > ports, Oh well. The idea of a shared SCIF regtype was a good idea...just too much baggage to deal with. > 4. Drop "renesas,scif" from the compatible value in the (not yet > submitted) r7s9210.dtsi (this may have to be clarified in the DT > bindings, although they already say "renesas,scif" is only meant for > ports compatible with the generic version, whatever that means ;-), As soon as I update to the new clock driver style and get that mainlined, then I can submit the r7s9210.dtsi. > 5. Add an OF_EARLYCON_DECLARE() for RZ/A2, setting port_cfg.regtype to > SCIx_RZ_SCIFA_REGTYPE, to (presumably) make earlycon work on RZ/A2, This is the one that I was happy to learn how to do it since originally I wasn't sure how I could get earlycon to work with the oddball RZ/A2 SCIF. Thanks, Chris
Hi Geert, Just FYI... On Wednesday, August 08, 2018, Geert Uytterhoeven wrote: > 4. Drop "renesas,scif" from the compatible value in the (not yet > submitted) r7s9210.dtsi (this may have to be clarified in the DT > bindings, although they already say "renesas,scif" is only meant for > ports compatible with the generic version, whatever that means ;-), Just as you would expect, if I drop "renesas,scif" from r7s9210.dtsi, then earlycon works with your RFC patches since now there is only one compatible it can possibly match against. Chris
Hi all, This RFC patch series was sparked by noticing that commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed SCIF address") broke earlycon support on most Renesas ARM SoCs using SCIF ports, and by the fragility of deriving regshift from the register block size (which may be rounded up): 1. The first patch is an old patch from Sato-san, which I never really understood. But it turned out to be a dependency for patch 2. 2. Patch 2 makes sure regshift is initialized when using earlycon, unbreaking the serial console on e.g. R-Car Gen2 and Gen3. 3. Patch 3 reverts the patch that started deriving regshift from the register block size, and that removed the plat_sci_port.regshift field. Which is a field I needed again in patch 4. 4. Patch 4 removes the remaining regshift derivations on DT platforms. (5. I didn't bother writing patch 5, which involves adding .regshift initializations to all SH board files that need it.) However, I'm not happy with the end result, so please DO NOT apply this! As I spent almost a full day on this, and would still like to know the story about "sh-sci: Use a separate sci_port for earlycon", I decided to post it anyway. As earlycon will be broken in v4.19-rc1 on RZ/A1, RZ/G, and R-Car, assuming no other actions are taken, an alternative solution would be to: 1. Revert commit 7acece71a517cad8 ("serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE"), 2. Revert commit 2d4dd0da45401c7a ("serial: sh-sci: Allow for compressed SCIF address") alternative, 3. Add an OF_EARLYCON_DECLARE() for RZ/A2, to fix earlycon on RZ/A2. What do you think? Thanks for your comments! P.S. Apparently SCIx_SH4_SCIF_REGTYPE and SCIx_SH2_SCIF_FIFODATA_REGTYPE are identical? Geert Uytterhoeven (3): [RFC] sh-sci: Take into account regshift to fix earlycon breakage [RFC] Revert "serial: sh-sci: Compute the regshift value for SCI ports" [RFC] sh-sci: Derive regshift value from DT compatible value Yoshinori Sato (1): [RFC] sh-sci: Use a separate sci_port for earlycon arch/sh/kernel/cpu/sh3/setup-sh770x.c | 1 + arch/sh/kernel/cpu/sh4/setup-sh7750.c | 3 +- arch/sh/kernel/cpu/sh4/setup-sh7760.c | 10 +--- drivers/tty/serial/sh-sci.c | 68 +++++++++++++++++---------- include/linux/serial_sci.h | 1 + 5 files changed, 49 insertions(+), 34 deletions(-)