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

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

[Siarhei Volkau]

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

[PATCH v3 1/2] dt-bindings: serial: ingenic: Add support for the JZ4750/55 SoCs

[Siarhei Volkau]

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 <krzysztof.kozlowski@linaro.org>
Signed-off-by: Siarhei Volkau <lis8215@gmail.com>
---
 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

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

[Siarhei Volkau]

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;
+
+	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

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

[Paul Cercueil]

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

> +
> +	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
> 

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

[Siarhei Volkau]

сб, 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
> >
>
>

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@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
>>  >
>> 
>> 

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

[Siarhei Volkau]

вс, 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

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

[Paul Cercueil]

Le dim. 23 oct. 2022 à 17:04:49 +0300, Siarhei Volkau 
<lis8215@gmail.com> a écrit :
> вс, 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

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

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

[Paul Cercueil]

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;

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
>