| Message ID | 20221009181338.2896660-8-lis8215@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | MIPS: ingenic: Add support for the JZ4755 SoC | expand |
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
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
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
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
пн, 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.
пн, 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?
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
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
чт, 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.
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
пн, 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 --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 },
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(-)