| Message ID | 20221022165047.4020785-3-lis8215@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: 8250/ingenic: Add support for the JZ4750 | expand |
сб, 22 окт. 2022 г. в 23:07, Paul Cercueil <paul@crapouillou.net>: > > Hi Siarhei, > > Le sam. 22 oct. 2022 à 19:50:47 +0300, Siarhei Volkau > <lis8215@gmail.com> a écrit : > > JZ4750/55/60 (but not JZ4760b) have an extra divisor in between extclk > > and peripheral clock, called CPCCR.ECS, the driver can't figure out > > the > > real state of the divisor without dirty hack - peek CGU CPCCR > > register. > > However, we can rely on a vendor's bootloader (u-boot 1.1.6) behavior: > > if (extclk > 16MHz) > > the divisor is enabled, so the UART driving clock is extclk/2. > > > > This behavior relies on hardware differences: most boards (if not all) > > with those SoCs have 12 or 24 MHz oscillators but many peripherals > > want > > 12Mhz to operate properly (AIC and USB-PHY at least). > > > > The patch doesn't affect JZ4760's behavior as it is subject for > > another > > patchset with re-classification of all supported ingenic UARTs. > > > > Link: > > https://github.com/carlos-wong/uboot_jz4755/blob/master/cpu/mips/jz_serial.c#L158 > > Signed-off-by: Siarhei Volkau <lis8215@gmail.com> > > --- > > drivers/tty/serial/8250/8250_ingenic.c | 48 > > ++++++++++++++++++++++---- > > 1 file changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/tty/serial/8250/8250_ingenic.c > > b/drivers/tty/serial/8250/8250_ingenic.c > > index 2b2f5d8d2..744705467 100644 > > --- a/drivers/tty/serial/8250/8250_ingenic.c > > +++ b/drivers/tty/serial/8250/8250_ingenic.c > > @@ -87,24 +87,19 @@ static void __init > > ingenic_early_console_setup_clock(struct earlycon_device *dev > > dev->port.uartclk = be32_to_cpup(prop); > > } > > > > -static int __init ingenic_early_console_setup(struct earlycon_device > > *dev, > > +static int __init ingenic_earlycon_setup_tail(struct earlycon_device > > *dev, > > const char *opt) > > { > > struct uart_port *port = &dev->port; > > unsigned int divisor; > > int baud = 115200; > > > > - if (!dev->port.membase) > > - return -ENODEV; > > - > > if (opt) { > > unsigned int parity, bits, flow; /* unused for now */ > > > > uart_parse_options(opt, &baud, &parity, &bits, &flow); > > } > > > > - ingenic_early_console_setup_clock(dev); > > - > > if (dev->baud) > > baud = dev->baud; > > divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud); > > @@ -129,9 +124,49 @@ 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) > > +{ > > + if (!dev->port.membase) > > + return -ENODEV; > > + > > + ingenic_early_console_setup_clock(dev); > > + > > + return ingenic_earlycon_setup_tail(dev, opt); > > +} > > + > > +static int __init jz4750_early_console_setup(struct earlycon_device > > *dev, > > + const char *opt) > > +{ > > + if (!dev->port.membase) > > + return -ENODEV; > > + > > + /* > > + * JZ4750/55/60 (not JZ4760b) have an extra divisor > > + * between extclk and peripheral clock, the > > + * driver can't figure out the real state of the > > + * divisor without dirty hacks (peek CGU register). > > + * However, we can rely on a vendor's behavior: > > + * if (extclk > 16MHz) > > + * the divisor is enabled. > > + * This behavior relies on hardware differences: > > + * most boards with those SoCs have 12 or 24 MHz > > + * oscillators but many peripherals want 12Mhz > > + * to operate properly (AIC and USB-phy at least). > > + */ > > + ingenic_early_console_setup_clock(dev); > > + if (dev->port.uartclk > 16000000) > > + dev->port.uartclk /= 2; > > I don't understand, didn't we came up to the conclusion in your V1 that > it was better to force-enable the EXT/2 divider in the ingenic init > code? > > -Paul > That was better at that moment. I dived deeper in the situation and found a more simple and universal solution, as I think. Your proposal doesn't cover following situations: - JZ475x with 12MHz crystal - JZ4760 with 24MHz crystal which are legal and might appear in some hardware. > > + > > + return ingenic_earlycon_setup_tail(dev, opt); > > +} > > + > > 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); > > > > @@ -328,6 +363,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 = &jz4760_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 > > }, > > -- > > 2.36.1 > > > >
Hi, Le dim. 23 oct. 2022 à 08:29:45 +0300, Siarhei Volkau <lis8215@gmail.com> a écrit : > сб, 22 окт. 2022 г. в 23:07, Paul Cercueil > <paul@crapouillou.net>: >> >> Hi Siarhei, >> >> Le sam. 22 oct. 2022 à 19:50:47 +0300, Siarhei Volkau >> <lis8215@gmail.com> a écrit : >> > JZ4750/55/60 (but not JZ4760b) have an extra divisor in between >> extclk >> > and peripheral clock, called CPCCR.ECS, the driver can't figure >> out >> > the >> > real state of the divisor without dirty hack - peek CGU CPCCR >> > register. >> > However, we can rely on a vendor's bootloader (u-boot 1.1.6) >> behavior: >> > if (extclk > 16MHz) >> > the divisor is enabled, so the UART driving clock is extclk/2. >> > >> > This behavior relies on hardware differences: most boards (if not >> all) >> > with those SoCs have 12 or 24 MHz oscillators but many peripherals >> > want >> > 12Mhz to operate properly (AIC and USB-PHY at least). >> > >> > The patch doesn't affect JZ4760's behavior as it is subject for >> > another >> > patchset with re-classification of all supported ingenic UARTs. >> > >> > Link: >> > >> https://github.com/carlos-wong/uboot_jz4755/blob/master/cpu/mips/jz_serial.c#L158 >> > Signed-off-by: Siarhei Volkau <lis8215@gmail.com> >> > --- >> > drivers/tty/serial/8250/8250_ingenic.c | 48 >> > ++++++++++++++++++++++---- >> > 1 file changed, 42 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/tty/serial/8250/8250_ingenic.c >> > b/drivers/tty/serial/8250/8250_ingenic.c >> > index 2b2f5d8d2..744705467 100644 >> > --- a/drivers/tty/serial/8250/8250_ingenic.c >> > +++ b/drivers/tty/serial/8250/8250_ingenic.c >> > @@ -87,24 +87,19 @@ static void __init >> > ingenic_early_console_setup_clock(struct earlycon_device *dev >> > dev->port.uartclk = be32_to_cpup(prop); >> > } >> > >> > -static int __init ingenic_early_console_setup(struct >> earlycon_device >> > *dev, >> > +static int __init ingenic_earlycon_setup_tail(struct >> earlycon_device >> > *dev, >> > const char *opt) >> > { >> > struct uart_port *port = &dev->port; >> > unsigned int divisor; >> > int baud = 115200; >> > >> > - if (!dev->port.membase) >> > - return -ENODEV; >> > - >> > if (opt) { >> > unsigned int parity, bits, flow; /* unused for now >> */ >> > >> > uart_parse_options(opt, &baud, &parity, &bits, >> &flow); >> > } >> > >> > - ingenic_early_console_setup_clock(dev); >> > - >> > if (dev->baud) >> > baud = dev->baud; >> > divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud); >> > @@ -129,9 +124,49 @@ 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) >> > +{ >> > + if (!dev->port.membase) >> > + return -ENODEV; >> > + >> > + ingenic_early_console_setup_clock(dev); >> > + >> > + return ingenic_earlycon_setup_tail(dev, opt); >> > +} >> > + >> > +static int __init jz4750_early_console_setup(struct >> earlycon_device >> > *dev, >> > + const char *opt) >> > +{ >> > + if (!dev->port.membase) >> > + return -ENODEV; >> > + >> > + /* >> > + * JZ4750/55/60 (not JZ4760b) have an extra divisor >> > + * between extclk and peripheral clock, the >> > + * driver can't figure out the real state of the >> > + * divisor without dirty hacks (peek CGU register). >> > + * However, we can rely on a vendor's behavior: >> > + * if (extclk > 16MHz) >> > + * the divisor is enabled. >> > + * This behavior relies on hardware differences: >> > + * most boards with those SoCs have 12 or 24 MHz >> > + * oscillators but many peripherals want 12Mhz >> > + * to operate properly (AIC and USB-phy at least). >> > + */ >> > + ingenic_early_console_setup_clock(dev); >> > + if (dev->port.uartclk > 16000000) >> > + dev->port.uartclk /= 2; >> >> I don't understand, didn't we came up to the conclusion in your V1 >> that >> it was better to force-enable the EXT/2 divider in the ingenic init >> code? >> >> -Paul >> > > That was better at that moment. I dived deeper in the situation and > found > a more simple and universal solution, as I think. > Your proposal doesn't cover following situations: > - JZ475x with 12MHz crystal > - JZ4760 with 24MHz crystal > which are legal and might appear in some hardware. Do you have such hardware? Don't add support for cases you can't test. For what we know - all JZ475x use a 24 MHz crystal and all JZ4760(B) use a 12 MHz crystal, until proven otherwise. -Paul >> > + >> > + return ingenic_earlycon_setup_tail(dev, opt); >> > +} >> > + >> > 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); >> > >> > @@ -328,6 +363,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 = >> &jz4760_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 >> > }, >> > -- >> > 2.36.1 >> > >> >>
вс, 23 окт. 2022 г. в 12:16, Paul Cercueil <paul@crapouillou.net>: > Do you have such hardware? No > Don't add support for cases you can't test. It's just a side effect of that approach. > For what we know - all JZ475x use a 24 MHz crystal and all JZ4760(B) > use a 12 MHz crystal, until proven otherwise. Ouf course it just confirms the rule but I found one exception: JZ4750 & 12MHz Link: https://github.com/carlos-wong/uboot_jz4755/blob/master/include/configs/lib4750.h Regarding your proposal: In my opinion enabling the divisor unconditionally is a bad practice, as it's already enabled (or not) by the bootloader, with respect to the hardware capabilities.I think it's better to keep the driver as it is than adding such things. BR, Siarhei
diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c index 2b2f5d8d2..744705467 100644 --- a/drivers/tty/serial/8250/8250_ingenic.c +++ b/drivers/tty/serial/8250/8250_ingenic.c @@ -87,24 +87,19 @@ static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev dev->port.uartclk = be32_to_cpup(prop); } -static int __init ingenic_early_console_setup(struct earlycon_device *dev, +static int __init ingenic_earlycon_setup_tail(struct earlycon_device *dev, const char *opt) { struct uart_port *port = &dev->port; unsigned int divisor; int baud = 115200; - if (!dev->port.membase) - return -ENODEV; - if (opt) { unsigned int parity, bits, flow; /* unused for now */ uart_parse_options(opt, &baud, &parity, &bits, &flow); } - ingenic_early_console_setup_clock(dev); - if (dev->baud) baud = dev->baud; divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud); @@ -129,9 +124,49 @@ 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) +{ + if (!dev->port.membase) + return -ENODEV; + + ingenic_early_console_setup_clock(dev); + + return ingenic_earlycon_setup_tail(dev, opt); +} + +static int __init jz4750_early_console_setup(struct earlycon_device *dev, + const char *opt) +{ + if (!dev->port.membase) + return -ENODEV; + + /* + * JZ4750/55/60 (not JZ4760b) have an extra divisor + * between extclk and peripheral clock, the + * driver can't figure out the real state of the + * divisor without dirty hacks (peek CGU register). + * However, we can rely on a vendor's behavior: + * if (extclk > 16MHz) + * the divisor is enabled. + * This behavior relies on hardware differences: + * most boards with those SoCs have 12 or 24 MHz + * oscillators but many peripherals want 12Mhz + * to operate properly (AIC and USB-phy at least). + */ + ingenic_early_console_setup_clock(dev); + if (dev->port.uartclk > 16000000) + dev->port.uartclk /= 2; + + return ingenic_earlycon_setup_tail(dev, opt); +} + 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); @@ -328,6 +363,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 = &jz4760_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 },
JZ4750/55/60 (but not JZ4760b) have an extra divisor in between extclk and peripheral clock, called CPCCR.ECS, the driver can't figure out the real state of the divisor without dirty hack - peek CGU CPCCR register. However, we can rely on a vendor's bootloader (u-boot 1.1.6) behavior: if (extclk > 16MHz) the divisor is enabled, so the UART driving clock is extclk/2. This behavior relies on hardware differences: most boards (if not all) with those SoCs have 12 or 24 MHz oscillators but many peripherals want 12Mhz to operate properly (AIC and USB-PHY at least). The patch doesn't affect JZ4760's behavior as it is subject for another patchset with re-classification of all supported ingenic UARTs. Link: https://github.com/carlos-wong/uboot_jz4755/blob/master/cpu/mips/jz_serial.c#L158 Signed-off-by: Siarhei Volkau <lis8215@gmail.com> --- drivers/tty/serial/8250/8250_ingenic.c | 48 ++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 6 deletions(-)