Re: [PATCH v3 2/2] serial: 8250/ingenic: Add support for the JZ4750/JZ4755

[Paul Cercueil]

Hi,

Le dim. 23 oct. 2022 à 08:29:45 +0300, Siarhei Volkau 
<lis8215@xxxxxxxxx> a écrit :
> сб, 22 окт. 2022 г. в 23:07, Paul Cercueil 
> <paul@xxxxxxxxxxxxxxx>:
> >  Hi Siarhei,
> >
> >  Le sam. 22 oct. 2022 à 19:50:47 +0300, Siarhei Volkau
> >  <lis8215@xxxxxxxxx> 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@xxxxxxxxx>
> >  > ---
> >  >  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
> >  >