diff mbox series

[7/8] serial: 8250/ingenic: Add support for the JZ4750/JZ4755 SoCs

Message ID 20221009181338.2896660-8-lis8215@gmail.com
State New
Headers show
Series MIPS: ingenic: Add support for the JZ4755 SoC | expand

Commit Message

Siarhei Volkau Oct. 9, 2022, 6:13 p.m. UTC
These SoCs are close to others but they have a clock divisor /2 for low
clock peripherals, thus to set up a proper baud rate we need to take
this into account.

The divisor bit is located in CGU area, unfortunately the clk framework
can't be used at early boot steps, so it's checked by direct readl()
call.

Signed-off-by: Siarhei Volkau <lis8215@gmail.com>
---
 drivers/tty/serial/8250/8250_ingenic.c | 39 ++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 5 deletions(-)

Comments

kernel test robot Oct. 9, 2022, 10:28 p.m. UTC | #1
Hi Siarhei,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next vkoul-dmaengine/next linus/master v6.0 next-20221007]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Siarhei-Volkau/dt-bindings-ingenic-Add-support-for-the-JZ4755-SoC/20221010-031428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: ia64-allyesconfig
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d0252164e0fa9fdb0493eead109a4b24bd873a03
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Siarhei-Volkau/dt-bindings-ingenic-Add-support-for-the-JZ4755-SoC/20221010-031428
        git checkout d0252164e0fa9fdb0493eead109a4b24bd873a03
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/tty/serial/8250/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/tty/serial/8250/8250_ingenic.c: In function 'jz4750_early_console_setup':
   drivers/tty/serial/8250/8250_ingenic.c:142:34: error: implicit declaration of function 'CKSEG1ADDR' [-Werror=implicit-function-declaration]
     142 | #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))
         |                                  ^~~~~~~~~~
   drivers/tty/serial/8250/8250_ingenic.c:144:27: note: in expansion of macro 'CGU_REG_CPCCR'
     144 |         u32 cpccr = readl(CGU_REG_CPCCR);
         |                           ^~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_ingenic.c:142:26: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     142 | #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))
         |                          ^
   drivers/tty/serial/8250/8250_ingenic.c:144:27: note: in expansion of macro 'CGU_REG_CPCCR'
     144 |         u32 cpccr = readl(CGU_REG_CPCCR);
         |                           ^~~~~~~~~~~~~
   {standard input}: Assembler messages:
   {standard input}:790: Error: Register number out of range 0..4
   {standard input}:791: Error: Register number out of range 0..4
   {standard input}:791: Warning: Use of 'mov' violates WAW dependency 'GR%, % in 1 - 127' (impliedf), specific resource number is 42
   {standard input}:791: Warning: Only the first path encountering the conflict is reported
   {standard input}:790: Warning: This is the location of the conflicting usage
   {standard input}:795: Error: Register number out of range 0..4
   cc1: some warnings being treated as errors


vim +142 drivers/tty/serial/8250/8250_ingenic.c

   138	
   139	static int __init jz4750_early_console_setup(struct earlycon_device *dev,
   140						     const char *opt)
   141	{
 > 142	#define CGU_REG_CPCCR	((void *)CKSEG1ADDR(0x10000000))
   143	#define CPCCR_ECS	BIT(30)
   144		u32 cpccr = readl(CGU_REG_CPCCR);
   145		int clk_div = (cpccr & CPCCR_ECS) ? 2 : 1;
   146	#undef CGU_REG_CPCCR
   147	#undef CPCCR_ECS
   148	
   149		return ingenic_earlycon_setup_common(dev, opt, clk_div);
   150	}
   151
kernel test robot Oct. 10, 2022, 4:12 p.m. UTC | #2
Hi Siarhei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on robh/for-next vkoul-dmaengine/next linus/master v6.0 next-20221010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Siarhei-Volkau/dt-bindings-ingenic-Add-support-for-the-JZ4755-SoC/20221010-031428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: arm64-randconfig-r035-20221010
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/d0252164e0fa9fdb0493eead109a4b24bd873a03
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Siarhei-Volkau/dt-bindings-ingenic-Add-support-for-the-JZ4755-SoC/20221010-031428
        git checkout d0252164e0fa9fdb0493eead109a4b24bd873a03
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/tty/serial/8250/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_ingenic.c:144:20: error: call to undeclared function 'CKSEG1ADDR'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           u32 cpccr = readl(CGU_REG_CPCCR);
                             ^
   drivers/tty/serial/8250/8250_ingenic.c:142:32: note: expanded from macro 'CGU_REG_CPCCR'
   #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))
                                    ^
>> drivers/tty/serial/8250/8250_ingenic.c:144:20: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]
           u32 cpccr = readl(CGU_REG_CPCCR);
                             ^~~~~~~~~~~~~
   drivers/tty/serial/8250/8250_ingenic.c:142:24: note: expanded from macro 'CGU_REG_CPCCR'
   #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))
                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +/CKSEG1ADDR +144 drivers/tty/serial/8250/8250_ingenic.c

   138	
   139	static int __init jz4750_early_console_setup(struct earlycon_device *dev,
   140						     const char *opt)
   141	{
   142	#define CGU_REG_CPCCR	((void *)CKSEG1ADDR(0x10000000))
   143	#define CPCCR_ECS	BIT(30)
 > 144		u32 cpccr = readl(CGU_REG_CPCCR);
   145		int clk_div = (cpccr & CPCCR_ECS) ? 2 : 1;
   146	#undef CGU_REG_CPCCR
   147	#undef CPCCR_ECS
   148	
   149		return ingenic_earlycon_setup_common(dev, opt, clk_div);
   150	}
   151
Greg KH Oct. 10, 2022, 8:20 p.m. UTC | #3
On Sun, Oct 09, 2022 at 09:13:36PM +0300, Siarhei Volkau wrote:
> These SoCs are close to others but they have a clock divisor /2 for low
> clock peripherals, thus to set up a proper baud rate we need to take
> this into account.
> 
> The divisor bit is located in CGU area, unfortunately the clk framework
> can't be used at early boot steps, so it's checked by direct readl()
> call.
> 
> Signed-off-by: Siarhei Volkau <lis8215@gmail.com>
> ---
>  drivers/tty/serial/8250/8250_ingenic.c | 39 ++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
> index 2b2f5d8d2..f2662720d 100644
> --- a/drivers/tty/serial/8250/8250_ingenic.c
> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> @@ -70,7 +70,8 @@ static void ingenic_early_console_write(struct console *console,
>  			   ingenic_early_console_putc);
>  }
>  
> -static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev)
> +static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev,
> +						     int clkdiv)

What does "clkdiv" mean here?

And this function is rough, adding a random integer to a function
requires you to look it up every time you see this call.

If you only have 1 or 2 as an option, just have 2 functions instead
please.

thanks,

greg k-h
kernel test robot Oct. 10, 2022, 9:39 p.m. UTC | #4
Hi Siarhei,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next vkoul-dmaengine/next linus/master v6.0 next-20221010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Siarhei-Volkau/dt-bindings-ingenic-Add-support-for-the-JZ4755-SoC/20221010-031428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: mips-randconfig-s042-20221010
compiler: mips64el-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d0252164e0fa9fdb0493eead109a4b24bd873a03
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Siarhei-Volkau/dt-bindings-ingenic-Add-support-for-the-JZ4755-SoC/20221010-031428
        git checkout d0252164e0fa9fdb0493eead109a4b24bd873a03
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=mips SHELL=/bin/bash drivers/tty/serial/8250/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

sparse warnings: (new ones prefixed by >>)
>> drivers/tty/serial/8250/8250_ingenic.c:144:27: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const volatile [noderef] __iomem *mem @@     got void * @@
   drivers/tty/serial/8250/8250_ingenic.c:144:27: sparse:     expected void const volatile [noderef] __iomem *mem
   drivers/tty/serial/8250/8250_ingenic.c:144:27: sparse:     got void *

vim +144 drivers/tty/serial/8250/8250_ingenic.c

   138	
   139	static int __init jz4750_early_console_setup(struct earlycon_device *dev,
   140						     const char *opt)
   141	{
   142	#define CGU_REG_CPCCR	((void *)CKSEG1ADDR(0x10000000))
   143	#define CPCCR_ECS	BIT(30)
 > 144		u32 cpccr = readl(CGU_REG_CPCCR);
   145		int clk_div = (cpccr & CPCCR_ECS) ? 2 : 1;
   146	#undef CGU_REG_CPCCR
   147	#undef CPCCR_ECS
   148	
   149		return ingenic_earlycon_setup_common(dev, opt, clk_div);
   150	}
   151
Siarhei Volkau Oct. 11, 2022, 6:38 p.m. UTC | #5
пн, 10 окт. 2022 г. в 23:20, Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> What does "clkdiv" mean here?

That means a clock divisor between the input oscillator and UART
peripheral clock source. Most Ingenic SoCs don't have that divisor,
so 1 is always in effect for them.
However, the JZ4750 and JZ4755 have switchable /2 clock divisor.

> If you only have 1 or 2 as an option

Yes, it is.

> just have 2 functions instead please.

Got it, will do that.

Thank you.
Siarhei Volkau Oct. 13, 2022, 6:37 a.m. UTC | #6
пн, 10 окт. 2022 г. в 01:29, kernel test robot <lkp@intel.com>:
> config: ia64-allyesconfig
> config: arm64-randconfig-r035-20221010

>  > 142  #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))

> 0-DAY CI Kernel Test Service

I know CKSEG1ADDR is MIPS specific, might be it needed to disable COMPILE_TEST
on the driver?
Since early syscon isn't mainlined yet I don't see any other way at the moment.

Any suggestions on that, folks?
Arnd Bergmann Oct. 13, 2022, 6:46 a.m. UTC | #7
On Thu, Oct 13, 2022, at 8:37 AM, Siarhei Volkau wrote:
> пн, 10 окт. 2022 г. в 01:29, kernel test robot <lkp@intel.com>:
>> config: ia64-allyesconfig
>> config: arm64-randconfig-r035-20221010
>
>>  > 142  #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))
>
>> 0-DAY CI Kernel Test Service
>
> I know CKSEG1ADDR is MIPS specific, might be it needed to disable COMPILE_TEST
> on the driver?
> Since early syscon isn't mainlined yet I don't see any other way at the moment.
>
> Any suggestions on that, folks?

This looks like some setup that belongs into the bootloader. If you are
handing over the console from bootloader to kernel, the hardware should
already be in a working state, with no need to touch it during early
boot.

If you are dealing with broken bootloaders that are not under your control,
having this code in the architecture specific early boot as a fixup
would be better than putting it into the driver.

      Arnd
Paul Cercueil Oct. 13, 2022, 9:17 a.m. UTC | #8
Hi,

Le jeu., oct. 13 2022 at 08:46:39 +0200, Arnd Bergmann <arnd@arndb.de> 
a écrit :
> On Thu, Oct 13, 2022, at 8:37 AM, Siarhei Volkau wrote:
>>  пн, 10 окт. 2022 г. в 01:29, kernel test robot 
>> <lkp@intel.com>:
>>>  config: ia64-allyesconfig
>>>  config: arm64-randconfig-r035-20221010
>> 
>>>   > 142  #define CGU_REG_CPCCR   ((void *)CKSEG1ADDR(0x10000000))
>> 
>>>  0-DAY CI Kernel Test Service
>> 
>>  I know CKSEG1ADDR is MIPS specific, might be it needed to disable 
>> COMPILE_TEST
>>  on the driver?
>>  Since early syscon isn't mainlined yet I don't see any other way at 
>> the moment.
>> 
>>  Any suggestions on that, folks?
> 
> This looks like some setup that belongs into the bootloader. If you 
> are
> handing over the console from bootloader to kernel, the hardware 
> should
> already be in a working state, with no need to touch it during early
> boot.
> 
> If you are dealing with broken bootloaders that are not under your 
> control,
> having this code in the architecture specific early boot as a fixup
> would be better than putting it into the driver.

Agreed. I am not fond of having a driver poking into an unrelated 
subsystem's memory area.

Just disable the divider in ingenic_fixup_fdt() in 
arch/mips/generic/board-ingenic.c.

Cheers,
-Paul
Siarhei Volkau Oct. 13, 2022, 6:56 p.m. UTC | #9
чт, 13 окт. 2022 г. в 12:17, Paul Cercueil <paul@crapouillou.net>:
>
> Just disable the divider in ingenic_fixup_fdt() in
> arch/mips/generic/board-ingenic.c.
>
> Cheers,
> -Paul
>

Looks reasonable, I hope the bootloader initialized peripherals can handle
doubled frequency, till re-initialization completes. I'll check that.

Thank you all, guys.
Paul Cercueil Oct. 17, 2022, 9:31 a.m. UTC | #10
Hi Siarhei,

Le dim., oct. 16 2022 at 21:39:48 +0300, Siarhei Volkau 
<lis8215@gmail.com> a écrit :
> чт, 13 окт. 2022 г. в 21:56, Siarhei Volkau 
> <lis8215@gmail.com>:
> 
>>  > Just disable the divider in ingenic_fixup_fdt() in
> 
>>  I'll check that.
> 
> I checked that approach: serial seems to be working as expected,
> but not all the time: there's a time period when the CGU driver
> started but serial console driver is still early one.
> In my case UART produces garbage at that period since CGU
> needs to enable clock divider back: ext is 24MHz but 12MHz
> required for audio codec and USB to function properly.

What I'd do, is just force-enable it to 12 MHz in ingenic_fixup_fdt(), 
since the programming manual basically says that 24 MHz does not work 
properly.

Then in the earlycon setup code hardcode the /2 divider with a big fat 
comment about why it's there.

Cheers,
-Paul

> So I think Arnd's approach:
> 
>>  the hardware should already be in a working state,
>>  with no need to touch it during early boot.
> 
> shall resolve the problem, although I can't check it on all supported
> hardware.
> 
> BR,
> Siarhei
Siarhei Volkau Oct. 19, 2022, 3:19 p.m. UTC | #11
пн, 17 окт. 2022 г. в 12:32, Paul Cercueil <paul@crapouillou.net>:
> > I checked that approach: serial seems to be working as expected,
> > but not all the time: there's a time period when the CGU driver
> > started but serial console driver is still early one.
> > In my case UART produces garbage at that period since CGU
> > needs to enable clock divider back: ext is 24MHz but 12MHz
> > required for audio codec and USB to function properly.
>
> What I'd do, is just force-enable it to 12 MHz in ingenic_fixup_fdt(),
> since the programming manual basically says that 24 MHz does not work
> properly.
>
> Then in the earlycon setup code hardcode the /2 divider with a big fat
> comment about why it's there.

Agree, the vendor's kernel does that as well.

Also I found that:
1. Many other drivers compile the early console only when
CONFIG_SERIAL_8250_CONSOLE is set.
2. All the early ingenic_ functions can be labeled as __init.
Shall I fix that while I'm already here?
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
index 2b2f5d8d2..f2662720d 100644
--- a/drivers/tty/serial/8250/8250_ingenic.c
+++ b/drivers/tty/serial/8250/8250_ingenic.c
@@ -70,7 +70,8 @@  static void ingenic_early_console_write(struct console *console,
 			   ingenic_early_console_putc);
 }
 
-static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev)
+static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev,
+						     int clkdiv)
 {
 	void *fdt = initial_boot_params;
 	const __be32 *prop;
@@ -84,11 +85,11 @@  static void __init ingenic_early_console_setup_clock(struct earlycon_device *dev
 	if (!prop)
 		return;
 
-	dev->port.uartclk = be32_to_cpup(prop);
+	dev->port.uartclk = be32_to_cpup(prop) / clkdiv;
 }
 
-static int __init ingenic_early_console_setup(struct earlycon_device *dev,
-					      const char *opt)
+static int __init ingenic_earlycon_setup_common(struct earlycon_device *dev,
+						const char *opt, int clkdiv)
 {
 	struct uart_port *port = &dev->port;
 	unsigned int divisor;
@@ -103,7 +104,7 @@  static int __init ingenic_early_console_setup(struct earlycon_device *dev,
 		uart_parse_options(opt, &baud, &parity, &bits, &flow);
 	}
 
-	ingenic_early_console_setup_clock(dev);
+	ingenic_early_console_setup_clock(dev, clkdiv);
 
 	if (dev->baud)
 		baud = dev->baud;
@@ -129,9 +130,31 @@  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)
+{
+	return ingenic_earlycon_setup_common(dev, opt, 1);
+}
+
+static int __init jz4750_early_console_setup(struct earlycon_device *dev,
+					     const char *opt)
+{
+#define CGU_REG_CPCCR	((void *)CKSEG1ADDR(0x10000000))
+#define CPCCR_ECS	BIT(30)
+	u32 cpccr = readl(CGU_REG_CPCCR);
+	int clk_div = (cpccr & CPCCR_ECS) ? 2 : 1;
+#undef CGU_REG_CPCCR
+#undef CPCCR_ECS
+
+	return ingenic_earlycon_setup_common(dev, opt, clk_div);
+}
+
 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);
 
@@ -311,6 +334,11 @@  static const struct ingenic_uart_config jz4740_uart_config = {
 	.fifosize = 16,
 };
 
+static const struct ingenic_uart_config jz4750_uart_config = {
+	.tx_loadsz = 16,
+	.fifosize = 32,
+};
+
 static const struct ingenic_uart_config jz4760_uart_config = {
 	.tx_loadsz = 16,
 	.fifosize = 32,
@@ -328,6 +356,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 = &jz4750_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 },