| Message ID | 20221009181338.2896660-8-lis8215@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: ingenic: Add support for the JZ4755 SoC | expand |
On Sun, Oct 09, 2022 at 09:13:36PM +0300, Siarhei Volkau wrote: > These SoCs are close to others but they have a clock divisor /2 for low > clock peripherals, thus to set up a proper baud rate we need to take > this into account. > > The divisor bit is located in CGU area, unfortunately the clk framework > can't be used at early boot steps, so it's checked by direct readl() > call. > > Signed-off-by: Siarhei Volkau <lis8215@gmail.com> > --- > drivers/tty/serial/8250/8250_ingenic.c | 39 ++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c > index 2b2f5d8d2..f2662720d 100644 > --- a/drivers/tty/serial/8250/8250_ingenic.c > +++ b/drivers/tty/serial/8250/8250_ingenic.c > @@ -70,7 +70,8 @@ static void ingenic_early_console_write(struct console *console, > ingenic_early_console_putc); > } > > -static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev) > +static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev, > + int clkdiv) What does "clkdiv" mean here? And this function is rough, adding a random integer to a function requires you to look it up every time you see this call. If you only have 1 or 2 as an option, just have 2 functions instead please. thanks, greg k-h
пн, 10 окт. 2022 г. в 23:20, Greg Kroah-Hartman <gregkh@linuxfoundation.org>: > What does "clkdiv" mean here? That means a clock divisor between the input oscillator and UART peripheral clock source. Most Ingenic SoCs don't have that divisor, so 1 is always in effect for them. However, the JZ4750 and JZ4755 have switchable /2 clock divisor. > If you only have 1 or 2 as an option Yes, it is. > just have 2 functions instead please. Got it, will do that. Thank you.
пн, 10 окт. 2022 г. в 01:29, kernel test robot <lkp@intel.com>: > config: ia64-allyesconfig > config: arm64-randconfig-r035-20221010 > > 142 #define CGU_REG_CPCCR ((void *)CKSEG1ADDR(0x10000000)) > 0-DAY CI Kernel Test Service I know CKSEG1ADDR is MIPS specific, might be it needed to disable COMPILE_TEST on the driver? Since early syscon isn't mainlined yet I don't see any other way at the moment. Any suggestions on that, folks?
On Thu, Oct 13, 2022, at 8:37 AM, Siarhei Volkau wrote: > пн, 10 окт. 2022 г. в 01:29, kernel test robot <lkp@intel.com>: >> config: ia64-allyesconfig >> config: arm64-randconfig-r035-20221010 > >> > 142 #define CGU_REG_CPCCR ((void *)CKSEG1ADDR(0x10000000)) > >> 0-DAY CI Kernel Test Service > > I know CKSEG1ADDR is MIPS specific, might be it needed to disable COMPILE_TEST > on the driver? > Since early syscon isn't mainlined yet I don't see any other way at the moment. > > Any suggestions on that, folks? This looks like some setup that belongs into the bootloader. If you are handing over the console from bootloader to kernel, the hardware should already be in a working state, with no need to touch it during early boot. If you are dealing with broken bootloaders that are not under your control, having this code in the architecture specific early boot as a fixup would be better than putting it into the driver. Arnd
Hi, Le jeu., oct. 13 2022 at 08:46:39 +0200, Arnd Bergmann <arnd@arndb.de> a écrit : > On Thu, Oct 13, 2022, at 8:37 AM, Siarhei Volkau wrote: >> пн, 10 окт. 2022 г. в 01:29, kernel test robot >> <lkp@intel.com>: >>> config: ia64-allyesconfig >>> config: arm64-randconfig-r035-20221010 >> >>> > 142 #define CGU_REG_CPCCR ((void *)CKSEG1ADDR(0x10000000)) >> >>> 0-DAY CI Kernel Test Service >> >> I know CKSEG1ADDR is MIPS specific, might be it needed to disable >> COMPILE_TEST >> on the driver? >> Since early syscon isn't mainlined yet I don't see any other way at >> the moment. >> >> Any suggestions on that, folks? > > This looks like some setup that belongs into the bootloader. If you > are > handing over the console from bootloader to kernel, the hardware > should > already be in a working state, with no need to touch it during early > boot. > > If you are dealing with broken bootloaders that are not under your > control, > having this code in the architecture specific early boot as a fixup > would be better than putting it into the driver. Agreed. I am not fond of having a driver poking into an unrelated subsystem's memory area. Just disable the divider in ingenic_fixup_fdt() in arch/mips/generic/board-ingenic.c. Cheers, -Paul
чт, 13 окт. 2022 г. в 12:17, Paul Cercueil <paul@crapouillou.net>: > > Just disable the divider in ingenic_fixup_fdt() in > arch/mips/generic/board-ingenic.c. > > Cheers, > -Paul > Looks reasonable, I hope the bootloader initialized peripherals can handle doubled frequency, till re-initialization completes. I'll check that. Thank you all, guys.
чт, 13 окт. 2022 г. в 21:56, Siarhei Volkau <lis8215@gmail.com>: > > Just disable the divider in ingenic_fixup_fdt() in > I'll check that. I checked that approach: serial seems to be working as expected, but not all the time: there's a time period when the CGU driver started but serial console driver is still early one. In my case UART produces garbage at that period since CGU needs to enable clock divider back: ext is 24MHz but 12MHz required for audio codec and USB to function properly. So I think Arnd's approach: > the hardware should already be in a working state, > with no need to touch it during early boot. shall resolve the problem, although I can't check it on all supported hardware. BR, Siarhei
Hi Siarhei, Le dim., oct. 16 2022 at 21:39:48 +0300, Siarhei Volkau <lis8215@gmail.com> a écrit : > чт, 13 окт. 2022 г. в 21:56, Siarhei Volkau > <lis8215@gmail.com>: > >> > Just disable the divider in ingenic_fixup_fdt() in > >> I'll check that. > > I checked that approach: serial seems to be working as expected, > but not all the time: there's a time period when the CGU driver > started but serial console driver is still early one. > In my case UART produces garbage at that period since CGU > needs to enable clock divider back: ext is 24MHz but 12MHz > required for audio codec and USB to function properly. What I'd do, is just force-enable it to 12 MHz in ingenic_fixup_fdt(), since the programming manual basically says that 24 MHz does not work properly. Then in the earlycon setup code hardcode the /2 divider with a big fat comment about why it's there. Cheers, -Paul > So I think Arnd's approach: > >> the hardware should already be in a working state, >> with no need to touch it during early boot. > > shall resolve the problem, although I can't check it on all supported > hardware. > > BR, > Siarhei
пн, 17 окт. 2022 г. в 12:32, Paul Cercueil <paul@crapouillou.net>: > > I checked that approach: serial seems to be working as expected, > > but not all the time: there's a time period when the CGU driver > > started but serial console driver is still early one. > > In my case UART produces garbage at that period since CGU > > needs to enable clock divider back: ext is 24MHz but 12MHz > > required for audio codec and USB to function properly. > > What I'd do, is just force-enable it to 12 MHz in ingenic_fixup_fdt(), > since the programming manual basically says that 24 MHz does not work > properly. > > Then in the earlycon setup code hardcode the /2 divider with a big fat > comment about why it's there. Agree, the vendor's kernel does that as well. Also I found that: 1. Many other drivers compile the early console only when CONFIG_SERIAL_8250_CONSOLE is set. 2. All the early ingenic_ functions can be labeled as __init. Shall I fix that while I'm already here?
diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c index 2b2f5d8d2..f2662720d 100644 --- a/drivers/tty/serial/8250/8250_ingenic.c +++ b/drivers/tty/serial/8250/8250_ingenic.c @@ -70,7 +70,8 @@ static void ingenic_early_console_write(struct console *console, ingenic_early_console_putc); } -static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev) +static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev, + int clkdiv) { void *fdt = initial_boot_params; const __be32 *prop; @@ -84,11 +85,11 @@ static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev if (!prop) return; - dev->port.uartclk = be32_to_cpup(prop); + dev->port.uartclk = be32_to_cpup(prop) / clkdiv; } -static int __init ingenic_early_console_setup(struct earlycon_device *dev, - const char *opt) +static int __init ingenic_earlycon_setup_common(struct earlycon_device *dev, + const char *opt, int clkdiv) { struct uart_port *port = &dev->port; unsigned int divisor; @@ -103,7 +104,7 @@ static int __init ingenic_early_console_setup(struct earlycon_device *dev, uart_parse_options(opt, &baud, &parity, &bits, &flow); } - ingenic_early_console_setup_clock(dev); + ingenic_early_console_setup_clock(dev, clkdiv); if (dev->baud) baud = dev->baud; @@ -129,9 +130,31 @@ static int __init ingenic_early_console_setup(struct earlycon_device *dev, return 0; } +static int __init ingenic_early_console_setup(struct earlycon_device *dev, + const char *opt) +{ + return ingenic_earlycon_setup_common(dev, opt, 1); +} + +static int __init jz4750_early_console_setup(struct earlycon_device *dev, + const char *opt) +{ +#define CGU_REG_CPCCR ((void *)CKSEG1ADDR(0x10000000)) +#define CPCCR_ECS BIT(30) + u32 cpccr = readl(CGU_REG_CPCCR); + int clk_div = (cpccr & CPCCR_ECS) ? 2 : 1; +#undef CGU_REG_CPCCR +#undef CPCCR_ECS + + return ingenic_earlycon_setup_common(dev, opt, clk_div); +} + OF_EARLYCON_DECLARE(jz4740_uart, "ingenic,jz4740-uart", ingenic_early_console_setup); +OF_EARLYCON_DECLARE(jz4750_uart, "ingenic,jz4750-uart", + jz4750_early_console_setup); + OF_EARLYCON_DECLARE(jz4770_uart, "ingenic,jz4770-uart", ingenic_early_console_setup); @@ -311,6 +334,11 @@ static const struct ingenic_uart_config jz4740_uart_config = { .fifosize = 16, }; +static const struct ingenic_uart_config jz4750_uart_config = { + .tx_loadsz = 16, + .fifosize = 32, +}; + static const struct ingenic_uart_config jz4760_uart_config = { .tx_loadsz = 16, .fifosize = 32, @@ -328,6 +356,7 @@ static const struct ingenic_uart_config x1000_uart_config = { static const struct of_device_id of_match[] = { { .compatible = "ingenic,jz4740-uart", .data = &jz4740_uart_config }, + { .compatible = "ingenic,jz4750-uart", .data = &jz4750_uart_config }, { .compatible = "ingenic,jz4760-uart", .data = &jz4760_uart_config }, { .compatible = "ingenic,jz4770-uart", .data = &jz4760_uart_config }, { .compatible = "ingenic,jz4775-uart", .data = &jz4760_uart_config },
These SoCs are close to others but they have a clock divisor /2 for low clock peripherals, thus to set up a proper baud rate we need to take this into account. The divisor bit is located in CGU area, unfortunately the clk framework can't be used at early boot steps, so it's checked by direct readl() call. Signed-off-by: Siarhei Volkau <lis8215@gmail.com> --- drivers/tty/serial/8250/8250_ingenic.c | 39 ++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 5 deletions(-)