| Message ID | 20180806140755.24087-5-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh-sci : Do not derive regshift from regsize | expand |
Hi Geert, On Monday, August 06, 2018 1, linux-sh-owner@vger.kernel.org wrote: > Deriving the proper regshift value from the register block size is > fragile (it may have been rounded up in DT, and the mapping granularity > is usually PAGE_SIZE anyway), and turned out to be inappropriate for > earlycon support (the size is not easily available). > > On DT systems, derive it from the compatible value instead. > This requires adding an entry for RZ/A2 serial ports, which use an > atypical regshift value. I had a simple patch to add support for CONFIG_DEBUG_LL for RZ/A2 because earlycon never worked because of RZ/A2's different register locations. I'll have to see how things change (better?) with this patch. Chris -- 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 Chris, On Mon, Aug 6, 2018 at 4:18 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Monday, August 06, 2018 1, linux-sh-owner@vger.kernel.org wrote: > > Deriving the proper regshift value from the register block size is > > fragile (it may have been rounded up in DT, and the mapping granularity > > is usually PAGE_SIZE anyway), and turned out to be inappropriate for > > earlycon support (the size is not easily available). > > > > On DT systems, derive it from the compatible value instead. > > This requires adding an entry for RZ/A2 serial ports, which use an > > atypical regshift value. > > I had a simple patch to add support for CONFIG_DEBUG_LL for RZ/A2 > because earlycon never worked because of RZ/A2's different register locations. Yeah, sci_probe_regmap() assumed the wrong regtype for your TYPE_SCIF port. You needed an OF_EARLYCON_DECLARE() line that also filled in the correct regtype. BTW, it would have been very valuable to know that earlycon didn't work, as that would have helped in avoiding the earlycon breakage on other parts. Gr{oetje,eeting}s, Geert
Hi Geert, On Monday, August 06, 2018 1, Geert Uytterhoeven wrote: > BTW, it would have been very valuable to know that earlycon didn't work, > as that > would have helped in avoiding the earlycon breakage on other parts. My apologies. When I had first looked at this months ago, I quickly realized that earlycon was not going to work because the RZ/A2 SCIF was not like all the other SCIF devices. So instead I went with a simple 2 line change to DEBUG_LL. After that point, I forgot all about earlycon until I saw you mention it this morning. Chris
Hi Chris, On Tue, Aug 7, 2018 at 9:24 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Monday, August 06, 2018 1, Geert Uytterhoeven wrote: > > > I had a simple patch to add support for CONFIG_DEBUG_LL for RZ/A2 > > > because earlycon never worked because of RZ/A2's different register > > locations. > > > > Yeah, sci_probe_regmap() assumed the wrong regtype for your TYPE_SCIF > > port. You needed an OF_EARLYCON_DECLARE() line that also filled in > > the correct regtype. > > > I gave your patch a try. > When earlycon is enabled, on RZ/A2, it gets stuck in here: > > static void sci_poll_put_char(struct uart_port *port, unsigned char c) > { > unsigned short status; > > do { > status = serial_port_in(port, SCxSR); > } while (!(status & SCxSR_TDxE(port))); > > serial_port_out(port, SCxTDR, c); > sci_clear_SCxSR(port, SCxSR_TDxE_CLEAR(port) & ~SCxSR_TEND(port)); > } > > > I see that you added this: > > OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rza2_early_console_setup); > > and "renesas,scif-r7s9210" matches what I have in my .dtsi. > > But, when I run the code, I end up in function scif_early_console_setup, > not rza2_early_console_setup Hmm, perhaps it doesn't pick the first/most specialized match. > I'm assuming I'm just supposed to use this on my bootargs: > earlycon=scif,0xE8009000 Just "earlycon" should be sufficient. It'll find the right port from chosen/stdout-path in DT. Can you please retry using that? Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On Tuesday, August 07, 2018, Geert Uytterhoeven wrote: > > But, when I run the code, I end up in function scif_early_console_setup, > > not rza2_early_console_setup > > Hmm, perhaps it doesn't pick the first/most specialized match. > > > I'm assuming I'm just supposed to use this on my bootargs: > > earlycon=scif,0xE8009000 > > Just "earlycon" should be sufficient. It'll find the right port from > chosen/stdout-path > in DT. Can you please retry using that? With just using "earlycon", I get the same result. I think the code that does the matching is in drivers/of/fdt.c Function "early_init_dt_scan_chosen_stdout". I'll step through the code a bit to see if I can figure out why. Chris
Hi Geert, On Tuesday, August 07, 2018, Geert Uytterhoeven wrote: > > I see that you added this: > > > > OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", > rza2_early_console_setup); > > > > and "renesas,scif-r7s9210" matches what I have in my .dtsi. > > > > But, when I run the code, I end up in function scif_early_console_setup, > > not rza2_early_console_setup > > Hmm, perhaps it doesn't pick the first/most specialized match. It seems it picks the first match. But, the table is built in the opposite order in which they are declared in the file. So "renesas,scif" was coming before "renesas,scif-r7s9210". So, by just swapping the order of "renesas,scif" and "renesas,scif-r7s9210" made it work! --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -3423,8 +3423,8 @@ static int __init hscif_early_console_setup(struct earlycon_device *device, } OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup); -OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rza2_early_console_setup); OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup); +OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rza2_early_console_setup); OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup); OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup); OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup); -- log -- Booting Linux... [ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Linux version 4.18.0-rc7-00017-g70cdb4f243c6-dirty (chris@ubuntu) (gcc version 5.4.1 20161213 (Linaro GCC 5.4-2017.01)) #23 Tue Aug 7 19:04:25 EST 2018 [ 0.000000] CPU: ARMv7 Processor [414fc091] revision 1 (ARMv7), cr=50c53c7d [ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache [ 0.000000] OF: fdt: Machine model: RZA2MEVB [ 0.000000] debug: ignoring loglevel setting. [ 0.000000] earlycon: scif0 at MMIO 0xe8009000 (options '115200n8') [ 0.000000] bootconsole [scif256] enabled Chris
Hi Chris, On Wed, Aug 8, 2018 at 2:16 AM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tuesday, August 07, 2018, Geert Uytterhoeven wrote: > > > I see that you added this: > > > > > > OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", > > rza2_early_console_setup); > > > > > > and "renesas,scif-r7s9210" matches what I have in my .dtsi. > > > > > > But, when I run the code, I end up in function scif_early_console_setup, > > > not rza2_early_console_setup > > > > Hmm, perhaps it doesn't pick the first/most specialized match. > > It seems it picks the first match. > > But, the table is built in the opposite order in which they are declared > in the file. So "renesas,scif" was coming before > "renesas,scif-r7s9210". > > So, by just swapping the order of "renesas,scif" and > "renesas,scif-r7s9210" made it work! Right, and since the RZ/A2 SCIF is not really compatible with "renesas,scif" (see my conclusion as a reply to the cover letter), the other order doesn't work. BTW, I guess earlycon also works on RZ/A2 with current tty-next or latest renesas-drivers? Gr{oetje,eeting}s, Geert
Hi Geert, On Wednesday, August 08, 2018, Geert Uytterhoeven wrote: > BTW, I guess earlycon also works on RZ/A2 with current tty-next or > latest renesas-drivers? I was using your RFC patches on top of the current renesas-drivers. Chris
Hi Chris, On Wed, Aug 8, 2018 at 12:39 PM Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Wednesday, August 08, 2018, Geert Uytterhoeven wrote: > > BTW, I guess earlycon also works on RZ/A2 with current tty-next or > > latest renesas-drivers? > > I was using your RFC patches on top of the current renesas-drivers. I meant without the RFC patch series. Gr{oetje,eeting}s, Geert
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 955c057dff6e8c78..c4342e61b8db72c3 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2898,9 +2898,10 @@ static int sci_init_single(struct platform_device *dev, port->regshift = p->regshift; port->fifosize = sci_port->params->fifosize; - if (regtype == SCIx_SH4_SCIF_REGTYPE) + if (!dev->dev.of_node && regtype == SCIx_SH4_SCIF_REGTYPE) { if (sci_port->reg_size >= 0x20) port->regshift = 1; + } /* * The UART port needs an IRQ value, so we peg this to the RX IRQ @@ -3100,43 +3101,49 @@ static int sci_remove(struct platform_device *dev) } -#define SCI_OF_DATA(type, regtype) (void *)((type) << 16 | (regtype)) +#define SCI_OF_DATA(type, regtype, regshift) \ + (void *)((type) << 16 | (regtype) << 8 | (regshift)) #define SCI_OF_TYPE(data) ((unsigned long)(data) >> 16) -#define SCI_OF_REGTYPE(data) ((unsigned long)(data) & 0xffff) +#define SCI_OF_REGTYPE(data) (((unsigned long)(data) >> 8) & 0xff) +#define SCI_OF_REGSHIFT(data) ((unsigned long)(data) & 0xff) static const struct of_device_id of_sci_match[] = { /* SoC-specific types */ { .compatible = "renesas,scif-r7s72100", - .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH2_SCIF_FIFODATA_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH2_SCIF_FIFODATA_REGTYPE, + 0), + }, { + .compatible = "renesas,scif-r7s9210", + .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_REGTYPE, 0), }, /* Family-specific types */ { .compatible = "renesas,rcar-gen1-scif", - .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_BRG_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_BRG_REGTYPE, 0), }, { .compatible = "renesas,rcar-gen2-scif", - .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_BRG_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_BRG_REGTYPE, 0), }, { .compatible = "renesas,rcar-gen3-scif", - .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_BRG_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_BRG_REGTYPE, 0), }, /* Generic types */ { .compatible = "renesas,scif", - .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIF, SCIx_SH4_SCIF_REGTYPE, 1), }, { .compatible = "renesas,scifa", - .data = SCI_OF_DATA(PORT_SCIFA, SCIx_SCIFA_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIFA, SCIx_SCIFA_REGTYPE, 0), }, { .compatible = "renesas,scifb", - .data = SCI_OF_DATA(PORT_SCIFB, SCIx_SCIFB_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIFB, SCIx_SCIFB_REGTYPE, 0), }, { .compatible = "renesas,hscif", - .data = SCI_OF_DATA(PORT_HSCIF, SCIx_HSCIF_REGTYPE), + .data = SCI_OF_DATA(PORT_HSCIF, SCIx_HSCIF_REGTYPE, 0), }, { .compatible = "renesas,sci", - .data = SCI_OF_DATA(PORT_SCI, SCIx_SCI_REGTYPE), + .data = SCI_OF_DATA(PORT_SCI, SCIx_SCI_REGTYPE, 0), }, { /* Terminator */ }, @@ -3179,6 +3186,7 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev, p->type = SCI_OF_TYPE(data); p->regtype = SCI_OF_REGTYPE(data); + p->regshift = SCI_OF_REGSHIFT(data); sp->has_rtscts = of_property_read_bool(np, "uart-has-rtscts");
Deriving the proper regshift value from the register block size is fragile (it may have been rounded up in DT, and the mapping granularity is usually PAGE_SIZE anyway), and turned out to be inappropriate for earlycon support (the size is not easily available). On DT systems, derive it from the compatible value instead. This requires adding an entry for RZ/A2 serial ports, which use an atypical regshift value. On non-DT systems the regshift value is still derived from the register block size, as before. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- - Sato-san: I assume this fixes SCI on H8/300, too? Cfr. your patch "serial: sh-sci: byte allocated register support" (https://www.spinics.net/lists/linux-sh/msg53175.html). - Getting rid of the regshift setup for non-DT systems probably means we'll have to add ".regshift = 1" to each and every SH board file describing SCIF serial ports :-( Any other suggestions? --- drivers/tty/serial/sh-sci.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)