| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 23 Oct 2022 10:16:10 +0100
From: Paul Cercueil <paul@...pouillou.net>
To: Siarhei Volkau <lis8215@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jiri Slaby <jirislaby@...nel.org>,
linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org
Subject: Re: [PATCH v3 2/2] serial: 8250/ingenic: Add support for the
JZ4750/JZ4755
Hi,
Le dim. 23 oct. 2022 à 08:29:45 +0300, Siarhei Volkau
<lis8215@...il.com> a écrit :
> сб, 22 окт. 2022 г. в 23:07, Paul Cercueil
> <paul@...pouillou.net>:
>>
>> Hi Siarhei,
>>
>> Le sam. 22 oct. 2022 à 19:50:47 +0300, Siarhei Volkau
>> <lis8215@...il.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@...il.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
>> >
>>
>>
Powered by blists - more mailing lists