JZ4750 and JZ4755 have an extra clock divisor in CGU called CPCCR.ECS.
It needs to be handled properly in the early console driver.
v3:
- fix build errors
v2:
- serial moved into separate patchset
- code refactored to avoid peek in CGU register
- Krzysztof's ack picked
v1:
- big patchset for the whole JZ4755 support
Siarhei Volkau (2):
dt-bindings: serial: ingenic: Add support for the JZ4750/55 SoCs
serial: 8250/ingenic: Add support for the JZ4750/JZ4755
.../bindings/serial/ingenic,uart.yaml | 4 ++
drivers/tty/serial/8250/8250_ingenic.c | 48 ++++++++++++++++---
2 files changed, 46 insertions(+), 6 deletions(-)
--
2.36.1
These SoCs UART block are the same as JZ4725b' one, the difference is
outside of the block - it is in the clock generation unit (CGU).
The difference requires to make a quirk for early console init.
Acked-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Siarhei Volkau <[email protected]>
---
Documentation/devicetree/bindings/serial/ingenic,uart.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/ingenic,uart.yaml b/Documentation/devicetree/bindings/serial/ingenic,uart.yaml
index 9ca7a18ec..315ceb722 100644
--- a/Documentation/devicetree/bindings/serial/ingenic,uart.yaml
+++ b/Documentation/devicetree/bindings/serial/ingenic,uart.yaml
@@ -20,6 +20,7 @@ properties:
oneOf:
- enum:
- ingenic,jz4740-uart
+ - ingenic,jz4750-uart
- ingenic,jz4760-uart
- ingenic,jz4780-uart
- ingenic,x1000-uart
@@ -31,6 +32,9 @@ properties:
- items:
- const: ingenic,jz4725b-uart
- const: ingenic,jz4740-uart
+ - items:
+ - const: ingenic,jz4755-uart
+ - const: ingenic,jz4750-uart
reg:
maxItems: 1
--
2.36.1
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 <[email protected]>
---
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;
+
+ 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 Siarhei,
Le sam. 22 oct. 2022 ? 19:50:47 +0300, Siarhei Volkau
<[email protected]> 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 <[email protected]>
> ---
> 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
> +
> + 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
>
сб, 22 окт. 2022 г. в 23:07, Paul Cercueil <[email protected]>:
>
> Hi Siarhei,
>
> Le sam. 22 oct. 2022 à 19:50:47 +0300, Siarhei Volkau
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
<[email protected]> a écrit :
> сб, 22 окт. 2022 г. в 23:07, Paul Cercueil
> <[email protected]>:
>>
>> Hi Siarhei,
>>
>> Le sam. 22 oct. 2022 à 19:50:47 +0300, Siarhei Volkau
>> <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 <[email protected]>:
> 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
Le dim. 23 oct. 2022 à 17:04:49 +0300, Siarhei Volkau
<[email protected]> a écrit :
> вс, 23 окт. 2022 г. в 12:16, Paul Cercueil
> <[email protected]>:
>> 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
Then when this board is upstreamed it will declare a 12 MHz oscillator
in its DT, and the ingenic init code won't have to enable the /2
divider for that particular board.
> 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.
Well, I disagree. Linux should not depend on whatever the bootloader
configures.
-Paul
Hi Siarhei,
Le sam. 22 oct. 2022 ? 19:50:47 +0300, Siarhei Volkau
<[email protected]> 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 <[email protected]>
> ---
> 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;
Again, as I said on your v2, you can keep this here. Then you won't
have to duplicate code.
> -
> 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'm OK with this code, but the comment is not very clear.
What about:
"JZ4750/55/60 have an optional /2 divider between the EXT oscillator
and some peripherals including UART, which will be enabled if using a
24 MHz oscillator, and disabled when using a 12 MHz oscillator."
Cheers,
-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
>