| Message ID | 20201229145130.2680442-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] clk: imx: enable the earlycon uart clocks by parsing from dt | expand |
Hi Adam,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on shawnguo/for-next]
[also build test ERROR on clk/clk-next rockchip/for-next soc/for-next v5.11-rc1 next-20201223]
[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]
url: https://github.com/0day-ci/linux/commits/Adam-Ford/clk-imx-enable-the-earlycon-uart-clocks-by-parsing-from-dt/20201229-225330
base: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
config: i386-randconfig-a016-20201230 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/dbc1139f0f9718caba0f0091cdf3ece65ef47f3f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Adam-Ford/clk-imx-enable-the-earlycon-uart-clocks-by-parsing-from-dt/20201229-225330
git checkout dbc1139f0f9718caba0f0091cdf3ece65ef47f3f
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: drivers/clk/imx/clk.o: in function `imx_earlycon_uart_clks_onoff':
>> clk.c:(.text+0x1bd): undefined reference to `of_stdout'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Adam, On Tue, Dec 29, 2020 at 08:51:28AM -0600, Adam Ford wrote: > Remove the earlycon uart clocks that are hard cord in platforms > clock driver, instead of parsing the earlycon uart port from dt "instead parse the earlycon uart..." Otherwise it's confusing what you mean here. > and enable these clocks from clock property in dt node. > > Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection") > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > Based on NXP's code base and adapted for 5.11-rc1. > https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b > > The original signed-off was retained. > Added the fixes tag. > --- > drivers/clk/imx/clk.c | 43 +++++++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c > index 47882c51cb85..c32b46890945 100644 > --- a/drivers/clk/imx/clk.c > +++ b/drivers/clk/imx/clk.c > @@ -148,7 +148,7 @@ void imx_cscmr1_fixup(u32 *val) > > #ifndef MODULE > static int imx_keep_uart_clocks; > -static struct clk ** const *imx_uart_clocks; > +static bool imx_uart_clks_on; > > static int __init imx_keep_uart_clocks_param(char *str) > { > @@ -161,25 +161,40 @@ __setup_param("earlycon", imx_keep_uart_earlycon, > __setup_param("earlyprintk", imx_keep_uart_earlyprintk, > imx_keep_uart_clocks_param, 0); > > -void imx_register_uart_clocks(struct clk ** const clks[]) > +static void imx_earlycon_uart_clks_onoff(bool is_on) "is_on" sounds like it's the current state of the clock, but actually the variable is used for the desired state, so I suggest using plain "on" as name. > { > - if (imx_keep_uart_clocks) { > - int i; > + struct clk *uart_clk; > + int i = 0; > > - imx_uart_clocks = clks; > - for (i = 0; imx_uart_clocks[i]; i++) > - clk_prepare_enable(*imx_uart_clocks[i]); > - } > + if (!imx_keep_uart_clocks || (!is_on && !imx_uart_clks_on)) > + return; > + > + /* only support dt */ > + if (!of_stdout) > + return; > + > + do { > + uart_clk = of_clk_get(of_stdout, i++); of_clk_get() allocates memory and gets you a reference to the clock. You have to release the clock with clk_put(). I think what you have to do here is to fill an array with clks when called from imx_register_uart_clocks() and when called from imx_clk_disable_uart() use that array to clk_disable_unprepare()/clk_put() the clocks. Sascha > + if (IS_ERR(uart_clk)) > + break; > + > + if (is_on) > + clk_prepare_enable(uart_clk); > + else > + clk_disable_unprepare(uart_clk); > + } while (true); > + > + if (is_on) > + imx_uart_clks_on = true; > +} > +void imx_register_uart_clocks(struct clk ** const clks[]) > +{ > + imx_earlycon_uart_clks_onoff(true); > } > > static int __init imx_clk_disable_uart(void) > { > - if (imx_keep_uart_clocks && imx_uart_clocks) { > - int i; > - > - for (i = 0; imx_uart_clocks[i]; i++) > - clk_disable_unprepare(*imx_uart_clocks[i]); > - } > + imx_earlycon_uart_clks_onoff(false); > > return 0; > } > -- > 2.25.1 > >
On Mon, Jan 4, 2021 at 1:12 AM Sascha Hauer <s.hauer@pengutronix.de> wrote: > > Hi Adam, > > On Tue, Dec 29, 2020 at 08:51:28AM -0600, Adam Ford wrote: > > Remove the earlycon uart clocks that are hard cord in platforms > > clock driver, instead of parsing the earlycon uart port from dt > > "instead parse the earlycon uart..." > > Otherwise it's confusing what you mean here. > > > and enable these clocks from clock property in dt node. > > > > Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection") > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > Based on NXP's code base and adapted for 5.11-rc1. > > https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b > > > > The original signed-off was retained. > > Added the fixes tag. > > --- > > drivers/clk/imx/clk.c | 43 +++++++++++++++++++++++++++++-------------- > > 1 file changed, 29 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c > > index 47882c51cb85..c32b46890945 100644 > > --- a/drivers/clk/imx/clk.c > > +++ b/drivers/clk/imx/clk.c > > @@ -148,7 +148,7 @@ void imx_cscmr1_fixup(u32 *val) > > > > #ifndef MODULE > > static int imx_keep_uart_clocks; > > -static struct clk ** const *imx_uart_clocks; > > +static bool imx_uart_clks_on; > > > > static int __init imx_keep_uart_clocks_param(char *str) > > { > > @@ -161,25 +161,40 @@ __setup_param("earlycon", imx_keep_uart_earlycon, > > __setup_param("earlyprintk", imx_keep_uart_earlyprintk, > > imx_keep_uart_clocks_param, 0); > > > > -void imx_register_uart_clocks(struct clk ** const clks[]) > > +static void imx_earlycon_uart_clks_onoff(bool is_on) > > "is_on" sounds like it's the current state of the clock, but actually > the variable is used for the desired state, so I suggest using plain > "on" as name. Sascha, I think I'll try to keep the existing structure of imx/clk.c in place so this function won't be needed. It was part of NXP's custom kernel, but I have a different idea that I'll explain below. > > > { > > - if (imx_keep_uart_clocks) { > > - int i; > > + struct clk *uart_clk; > > + int i = 0; > > > > - imx_uart_clocks = clks; > > - for (i = 0; imx_uart_clocks[i]; i++) > > - clk_prepare_enable(*imx_uart_clocks[i]); > > - } > > + if (!imx_keep_uart_clocks || (!is_on && !imx_uart_clks_on)) > > + return; > > + > > + /* only support dt */ > > + if (!of_stdout) > > + return; > > + > > + do { > > + uart_clk = of_clk_get(of_stdout, i++); > > of_clk_get() allocates memory and gets you a reference to the clock. You > have to release the clock with clk_put(). I think what you have to do > here is to fill an array with clks when called from > imx_register_uart_clocks() and when called from imx_clk_disable_uart() > use that array to clk_disable_unprepare()/clk_put() the clocks. I have another revision pending which modifies imx_register_uart_clocks() to receive the number of available UART clocks from the calling SoC. It will then allocate an array of clock structures equal to that size. Instead of enabling all UART clocks, it will then go through of_clk_get(of_stdout, ...) to fill the array and keep track of the number of devices it's assigned to the array. Most likely the array will be larger than the number of of_stdout entries. Because it keeps track of the number of enabled UART's, it will use that to go through the array and only try to unprepare/disable and put that many clocks. Once all the clocks have been disabled and put, the entire clock array will be freed. It will be more closely related to how the current imx/clk.c file is now instead of using NXP's custom kernel, but it will also allow me to remove the static arrays setting up the UART clocks for each SoC. Does that sound OK to you? I need to run some tests on my i.MX6Q board before I submit it, but tests on my i.MX8MM are looking promising. I can re-parent the UART that I need reparented, and it fails if I try to reparent when that UART is assigned to stdout. adam > > Sascha > > > + if (IS_ERR(uart_clk)) > > + break; > > + > > + if (is_on) > > + clk_prepare_enable(uart_clk); > > + else > > + clk_disable_unprepare(uart_clk); > > + } while (true); > > + > > + if (is_on) > > + imx_uart_clks_on = true; > > +} > > +void imx_register_uart_clocks(struct clk ** const clks[]) > > +{ > > + imx_earlycon_uart_clks_onoff(true); > > } > > > > static int __init imx_clk_disable_uart(void) > > { > > - if (imx_keep_uart_clocks && imx_uart_clocks) { > > - int i; > > - > > - for (i = 0; imx_uart_clocks[i]; i++) > > - clk_disable_unprepare(*imx_uart_clocks[i]); > > - } > > + imx_earlycon_uart_clks_onoff(false); > > > > return 0; > > } > > -- > > 2.25.1 > > > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c index 47882c51cb85..c32b46890945 100644 --- a/drivers/clk/imx/clk.c +++ b/drivers/clk/imx/clk.c @@ -148,7 +148,7 @@ void imx_cscmr1_fixup(u32 *val) #ifndef MODULE static int imx_keep_uart_clocks; -static struct clk ** const *imx_uart_clocks; +static bool imx_uart_clks_on; static int __init imx_keep_uart_clocks_param(char *str) { @@ -161,25 +161,40 @@ __setup_param("earlycon", imx_keep_uart_earlycon, __setup_param("earlyprintk", imx_keep_uart_earlyprintk, imx_keep_uart_clocks_param, 0); -void imx_register_uart_clocks(struct clk ** const clks[]) +static void imx_earlycon_uart_clks_onoff(bool is_on) { - if (imx_keep_uart_clocks) { - int i; + struct clk *uart_clk; + int i = 0; - imx_uart_clocks = clks; - for (i = 0; imx_uart_clocks[i]; i++) - clk_prepare_enable(*imx_uart_clocks[i]); - } + if (!imx_keep_uart_clocks || (!is_on && !imx_uart_clks_on)) + return; + + /* only support dt */ + if (!of_stdout) + return; + + do { + uart_clk = of_clk_get(of_stdout, i++); + if (IS_ERR(uart_clk)) + break; + + if (is_on) + clk_prepare_enable(uart_clk); + else + clk_disable_unprepare(uart_clk); + } while (true); + + if (is_on) + imx_uart_clks_on = true; +} +void imx_register_uart_clocks(struct clk ** const clks[]) +{ + imx_earlycon_uart_clks_onoff(true); } static int __init imx_clk_disable_uart(void) { - if (imx_keep_uart_clocks && imx_uart_clocks) { - int i; - - for (i = 0; imx_uart_clocks[i]; i++) - clk_disable_unprepare(*imx_uart_clocks[i]); - } + imx_earlycon_uart_clks_onoff(false); return 0; }
Remove the earlycon uart clocks that are hard cord in platforms clock driver, instead of parsing the earlycon uart port from dt and enable these clocks from clock property in dt node. Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection") Signed-off-by: Fugang Duan <fugang.duan@nxp.com> Signed-off-by: Adam Ford <aford173@gmail.com> --- Based on NXP's code base and adapted for 5.11-rc1. https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b The original signed-off was retained. Added the fixes tag. --- drivers/clk/imx/clk.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-)