Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver
Hi Alice,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on v5.10-rc6 next-20201201]
[cannot apply to shawnguo/for-next krzk/for-next]
[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/Alice-Guo/dt-bindings-soc-imx8m-add-DT-Binding-doc-for-soc-unique-ID/20201124-100402
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: mips-randconfig-r026-20201201 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project
2671fccf0381769276ca8246ec0499adcb9b0355)
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 mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
#
https://github.com/0day-ci/linux/commit/410e3191d62767ba3d347a48d38ad3bd00bee387
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Alice-Guo/dt-bindings-soc-imx8m-add-DT-Binding-doc-for-soc-unique-ID/20201124-100402
git checkout 410e3191d62767ba3d347a48d38ad3bd00bee387
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/soc/imx/soc-imx8m.c:174:34: warning: unused variable
>> 'imx8_machine_match' [-Wunused-const-variable]
static const struct of_device_id imx8_machine_match[] = {
^
1 warning generated.
vim +/imx8_machine_match +174 drivers/soc/imx/soc-imx8m.c
173
> 174 static const struct of_device_id imx8_machine_match[] = {
175 { .compatible = "fsl,imx8mq", .data = _soc_data, },
176 { .compatible = "fsl,imx8mm", .data = _soc_data, },
177 { .compatible = "fsl,imx8mn", .data = _soc_data, },
178 { .compatible = "fsl,imx8mp", .data = _soc_data, },
179 { }
180 };
181
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver
On Thu, Nov 26, 2020 at 02:15:35AM +, Alice Guo wrote: > > > > -Original Message- > > From: linux-arm-kernel On > > Behalf Of Adam Ford > > Sent: 2020年11月25日 8:45 > > To: Alice Guo > > Cc: devicetree ; Peng Fan ; > > Sascha Hauer ; Linux Kernel Mailing List > > ; Krzysztof Kozlowski ; Rob > > Herring ; dl-linux-imx ; Shawn Guo > > ; arm-soc > > Subject: Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver > > > > On Mon, Nov 23, 2020 at 8:04 PM Alice Guo wrote: > > > > > > Directly reading ocotp register depends on that bootloader enables > > > ocotp clk, which is not always effective, so change to use nvmem API. > > > Using nvmem API requires to support driver defer probe and thus change > > > soc-imx8m.c to use platform driver. > > > > > > The other reason is that directly reading ocotp register causes kexec > > > kernel hang because the 1st kernel running will disable unused clks > > > after kernel boots up, and then ocotp clk will be disabled even if > > > bootloader enables it. When kexec kernel, ocotp clk needs to be > > > enabled before reading ocotp registers, and nvmem API with platform > > > driver supported can accomplish this. > > > > > > Signed-off-by: Alice Guo > > > --- > > > > > The patch reads V6, but the change log only shows V2. Can you elaborate on > > what has changed between V2 and V6? > > > > adam > > Hi, > > Sorry. The order of changlog is reversed. Thank Adam for pointing out the > problem, and thank Krzysztof for reviewing my patch. > Do I need to resend the patchset with the correct changelog order? No, no need. Best regards, Krzysztof
RE: [PATCH v6 4/4] soc: imx8m: change to use platform driver
> -Original Message-
> From: linux-arm-kernel On
> Behalf Of Adam Ford
> Sent: 2020年11月25日 8:45
> To: Alice Guo
> Cc: devicetree ; Peng Fan ;
> Sascha Hauer ; Linux Kernel Mailing List
> ; Krzysztof Kozlowski ; Rob
> Herring ; dl-linux-imx ; Shawn Guo
> ; arm-soc
> Subject: Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver
>
> On Mon, Nov 23, 2020 at 8:04 PM Alice Guo wrote:
> >
> > Directly reading ocotp register depends on that bootloader enables
> > ocotp clk, which is not always effective, so change to use nvmem API.
> > Using nvmem API requires to support driver defer probe and thus change
> > soc-imx8m.c to use platform driver.
> >
> > The other reason is that directly reading ocotp register causes kexec
> > kernel hang because the 1st kernel running will disable unused clks
> > after kernel boots up, and then ocotp clk will be disabled even if
> > bootloader enables it. When kexec kernel, ocotp clk needs to be
> > enabled before reading ocotp registers, and nvmem API with platform
> > driver supported can accomplish this.
> >
> > Signed-off-by: Alice Guo
> > ---
> >
> The patch reads V6, but the change log only shows V2. Can you elaborate on
> what has changed between V2 and V6?
>
> adam
Hi,
Sorry. The order of changlog is reversed. Thank Adam for pointing out the
problem, and thank Krzysztof for reviewing my patch.
Do I need to resend the patchset with the correct changelog order?
Best regards,
Alice
> > v2: remove the subject prefix "LF-2571-4"
> > v3: Keep the original way which uses device_initcall to read soc unique
> > ID, and add the other way which uses module_platform_driver and
> > nvmem API, so that it will not break the old version DTBs.
> > v4: delete "__maybe_unused"
> > delete MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> > rename match table, "fsl,imx8mm/n/q/p" is actually a machine
> > compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0
> > delete "flag" and change to determine whether the pointer is NULL
> > ues of_find_matching_node_and_match()
> > delete of_match_ptr()
> > v5: add cleanup part "of_node_put"
> > add note to explain that why device_initcall still exists
> > v6: none
> >
> > drivers/soc/imx/soc-imx8m.c | 87
> > -
> > 1 file changed, 75 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> > index cc57a384d74d..250530177920 100644
> > --- a/drivers/soc/imx/soc-imx8m.c
> > +++ b/drivers/soc/imx/soc-imx8m.c
> > @@ -5,6 +5,8 @@
> >
> > #include
> > #include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > @@ -29,7 +31,7 @@
> >
> > struct imx8_soc_data {
> > char *name;
> > - u32 (*soc_revision)(void);
> > + u32 (*soc_revision)(struct device *dev);
> > };
> >
> > static u64 soc_uid;
> > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
> > static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
> > #endif
> >
> > -static u32 __init imx8mq_soc_revision(void)
> > +static u32 __init imx8mq_soc_revision(struct device *dev)
> > {
> > struct device_node *np;
> > void __iomem *ocotp_base;
> > @@ -75,9 +77,20 @@ static u32 __init imx8mq_soc_revision(void)
> > rev = REV_B1;
> > }
> >
> > - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > - soc_uid <<= 32;
> > - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > + if (dev) {
> > + int ret = 0;
> > +
> > + ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> _uid);
> > + if (ret) {
> > + iounmap(ocotp_base);
> > + of_node_put(np);
> > + return ret;
> > + }
> > + } else {
> > + soc_uid = readl_relaxed(ocotp_base +
> OCOTP_UID_HIGH);
> > + soc_uid <<= 32;
> > + soc_uid |= readl_relaxed(ocotp_base +
> OCOTP_UID_LOW);
> > + }
> >
> > iounmap(ocotp_base);
> > of_node_put(np);
> > @@ -107,7 +120,7 @@ static void __init imx8mm_soc_uid(void)
> > of_node_put(np);
> > }
> >
> > -static u32 __init imx8mm_soc_rev
Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver
On Wed, 25 Nov 2020 at 01:44, Adam Ford wrote: > > On Mon, Nov 23, 2020 at 8:04 PM Alice Guo wrote: > > > > Directly reading ocotp register depends on that bootloader enables ocotp > > clk, which is not always effective, so change to use nvmem API. Using > > nvmem API requires to support driver defer probe and thus change > > soc-imx8m.c to use platform driver. > > > > The other reason is that directly reading ocotp register causes kexec > > kernel hang because the 1st kernel running will disable unused clks > > after kernel boots up, and then ocotp clk will be disabled even if > > bootloader enables it. When kexec kernel, ocotp clk needs to be enabled > > before reading ocotp registers, and nvmem API with platform driver > > supported can accomplish this. > > > > Signed-off-by: Alice Guo > > --- > > > The patch reads V6, but the change log only shows V2. Can you > elaborate on what has changed between V2 and V6? > > adam > > > v2: remove the subject prefix "LF-2571-4" > > v3: Keep the original way which uses device_initcall to read soc unique > > ID, and add the other way which uses module_platform_driver and > > nvmem API, so that it will not break the old version DTBs. > > v4: delete "__maybe_unused" > > delete MODULE_DEVICE_TABLE(of, imx8m_soc_match); > > rename match table, "fsl,imx8mm/n/q/p" is actually a machine > > compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0 > > delete "flag" and change to determine whether the pointer is NULL > > ues of_find_matching_node_and_match() > > delete of_match_ptr() > > v5: add cleanup part "of_node_put" > > add note to explain that why device_initcall still exists > > v6: none Hi Adam, It says up to v6, just in unnatural order... I was also surprised. Best regards, Krzysztof
Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver
On Mon, Nov 23, 2020 at 8:04 PM Alice Guo wrote:
>
> Directly reading ocotp register depends on that bootloader enables ocotp
> clk, which is not always effective, so change to use nvmem API. Using
> nvmem API requires to support driver defer probe and thus change
> soc-imx8m.c to use platform driver.
>
> The other reason is that directly reading ocotp register causes kexec
> kernel hang because the 1st kernel running will disable unused clks
> after kernel boots up, and then ocotp clk will be disabled even if
> bootloader enables it. When kexec kernel, ocotp clk needs to be enabled
> before reading ocotp registers, and nvmem API with platform driver
> supported can accomplish this.
>
> Signed-off-by: Alice Guo
> ---
>
The patch reads V6, but the change log only shows V2. Can you
elaborate on what has changed between V2 and V6?
adam
> v2: remove the subject prefix "LF-2571-4"
> v3: Keep the original way which uses device_initcall to read soc unique
> ID, and add the other way which uses module_platform_driver and
> nvmem API, so that it will not break the old version DTBs.
> v4: delete "__maybe_unused"
> delete MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> rename match table, "fsl,imx8mm/n/q/p" is actually a machine
> compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0
> delete "flag" and change to determine whether the pointer is NULL
> ues of_find_matching_node_and_match()
> delete of_match_ptr()
> v5: add cleanup part "of_node_put"
> add note to explain that why device_initcall still exists
> v6: none
>
> drivers/soc/imx/soc-imx8m.c | 87 -
> 1 file changed, 75 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> index cc57a384d74d..250530177920 100644
> --- a/drivers/soc/imx/soc-imx8m.c
> +++ b/drivers/soc/imx/soc-imx8m.c
> @@ -5,6 +5,8 @@
>
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -29,7 +31,7 @@
>
> struct imx8_soc_data {
> char *name;
> - u32 (*soc_revision)(void);
> + u32 (*soc_revision)(struct device *dev);
> };
>
> static u64 soc_uid;
> @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
> static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
> #endif
>
> -static u32 __init imx8mq_soc_revision(void)
> +static u32 __init imx8mq_soc_revision(struct device *dev)
> {
> struct device_node *np;
> void __iomem *ocotp_base;
> @@ -75,9 +77,20 @@ static u32 __init imx8mq_soc_revision(void)
> rev = REV_B1;
> }
>
> - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> - soc_uid <<= 32;
> - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> + if (dev) {
> + int ret = 0;
> +
> + ret = nvmem_cell_read_u64(dev, "soc_unique_id", _uid);
> + if (ret) {
> + iounmap(ocotp_base);
> + of_node_put(np);
> + return ret;
> + }
> + } else {
> + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> + soc_uid <<= 32;
> + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> + }
>
> iounmap(ocotp_base);
> of_node_put(np);
> @@ -107,7 +120,7 @@ static void __init imx8mm_soc_uid(void)
> of_node_put(np);
> }
>
> -static u32 __init imx8mm_soc_revision(void)
> +static u32 __init imx8mm_soc_revision(struct device *dev)
> {
> struct device_node *np;
> void __iomem *anatop_base;
> @@ -125,7 +138,15 @@ static u32 __init imx8mm_soc_revision(void)
> iounmap(anatop_base);
> of_node_put(np);
>
> - imx8mm_soc_uid();
> + if (dev) {
> + int ret = 0;
> +
> + ret = nvmem_cell_read_u64(dev, "soc_unique_id", _uid);
> + if (ret)
> + return ret;
> + } else {
> + imx8mm_soc_uid();
> + }
>
> return rev;
> }
> @@ -150,7 +171,7 @@ static const struct imx8_soc_data imx8mp_soc_data = {
> .soc_revision = imx8mm_soc_revision,
> };
>
> -static __maybe_unused const struct of_device_id imx8_soc_match[] = {
> +static const struct of_device_id imx8_machine_match[] = {
> { .compatible = "fsl,imx8mq", .data = _soc_data, },
> { .compatible = "fsl,imx8mm", .data = _soc_data, },
> { .compatible = "fsl,imx8mn", .data = _soc_data, },
> @@ -158,12 +179,20 @@ static __maybe_unused const struct of_device_id
> imx8_soc_match[] = {
> { }
> };
>
> +static const struct of_device_id imx8_soc_match[] = {
> + { .compatible = "fsl,imx8mq-soc", .data = _soc_data, },
> + { .compatible = "fsl,imx8mm-soc", .data = _soc_data, },
> + { .compatible = "fsl,imx8mn-soc", .data = _soc_data, },
> + { .compatible = "fsl,imx8mp-soc", .data = _soc_data, },
> + {
Re: [PATCH v6 4/4] soc: imx8m: change to use platform driver
On Tue, Nov 24, 2020 at 09:59:49AM +0800, Alice Guo wrote: > Directly reading ocotp register depends on that bootloader enables ocotp > clk, which is not always effective, so change to use nvmem API. Using > nvmem API requires to support driver defer probe and thus change > soc-imx8m.c to use platform driver. > > The other reason is that directly reading ocotp register causes kexec > kernel hang because the 1st kernel running will disable unused clks > after kernel boots up, and then ocotp clk will be disabled even if > bootloader enables it. When kexec kernel, ocotp clk needs to be enabled > before reading ocotp registers, and nvmem API with platform driver > supported can accomplish this. > > Signed-off-by: Alice Guo I already reviewed it. You skipped all my review tags from v5. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof > --- > > v2: remove the subject prefix "LF-2571-4" > v3: Keep the original way which uses device_initcall to read soc unique > ID, and add the other way which uses module_platform_driver and > nvmem API, so that it will not break the old version DTBs. > v4: delete "__maybe_unused" > delete MODULE_DEVICE_TABLE(of, imx8m_soc_match); > rename match table, "fsl,imx8mm/n/q/p" is actually a machine > compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0 > delete "flag" and change to determine whether the pointer is NULL > ues of_find_matching_node_and_match() > delete of_match_ptr() > v5: add cleanup part "of_node_put" > add note to explain that why device_initcall still exists > v6: none
