| Message ID | 20230818093028.7807-2-kkartik@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc/tegra: fuse: Add ACPI support | expand |
On Fri, Aug 18, 2023 at 03:00:23PM +0530, Kartik wrote: > In preparation to ACPI support in Tegra fuse driver add function > tegra_acpi_init_apbmisc() and move common code used for both ACPI and > device-tree into a new helper function tegra_init_apbmisc_base(). > > Note that function tegra_acpi_init_apbmisc() is not placed in the __init > section, because it will be called during probe. ... > void tegra_init_revision(void); > void tegra_init_apbmisc(void); > +void tegra_acpi_init_apbmisc(void); Why do you need a separate function? ... > +static const struct acpi_device_id apbmisc_acpi_match[] = { > + { .id = "NVDA2010", 0 }, We rarely use C99 initializers for ACPI ID table. Also 0 is not needed. > + { /* sentinel */ } > +}; ... > + if (!apbmisc_base) > + pr_err("failed to map APBMISC registers\n"); > + else > + chipid = readl_relaxed(apbmisc_base + 4); Why not positive conditional as you have two branches anyway? ... > + if (!strapping_base) { > + pr_err("failed to map strapping options registers\n"); > + } else { > + strapping = readl_relaxed(strapping_base); > + iounmap(strapping_base); > + } Ditto. ... > - apbmisc_base = ioremap(apbmisc.start, resource_size(&apbmisc)); > - if (!apbmisc_base) { > - pr_err("failed to map APBMISC registers\n"); > - } else { > - chipid = readl_relaxed(apbmisc_base + 4); > - } > - > - strapping_base = ioremap(straps.start, resource_size(&straps)); > - if (!strapping_base) { > - pr_err("failed to map strapping options registers\n"); > - } else { > - strapping = readl_relaxed(strapping_base); > - iounmap(strapping_base); > - } > - > + tegra_init_apbmisc_base(&apbmisc, &straps); This split can be done in a separate precursor patch. ... > +void tegra_acpi_init_apbmisc(void) > +{ > + struct acpi_device *adev = NULL; > + struct resource *apbmisc, *straps; > + struct list_head resource_list; > + struct resource_entry *rentry; > + int rcount; > + > + adev = acpi_dev_get_first_match_dev(apbmisc_acpi_match[0].id, NULL, -1); > + if (!adev) > + return; > + > + INIT_LIST_HEAD(&resource_list); > + > + rcount = acpi_dev_get_memory_resources(adev, &resource_list); > + if (rcount != 2) { > + pr_err("failed to get APBMISC memory resources"); (1) > + return; > + } > + rentry = list_first_entry(&resource_list, struct resource_entry, node); > + apbmisc = rentry->res; > + rentry = list_next_entry(rentry, node); > + straps = rentry->res; We don't do like this, we walk through them and depending on the type and index do something. The above if error prone and not scalable code. > + tegra_init_apbmisc_base(apbmisc, straps); > + acpi_dev_free_resource_list(&resource_list); Not okay in (1). > + acpi_dev_put(adev); Not okay in (1). > +}
Thanks for reviewing the patches Andy! On Fri, 2023-08-18 at 16:21 +0300, Andy Shevchenko wrote: >> void tegra_init_revision(void); >> void tegra_init_apbmisc(void); >> +void tegra_acpi_init_apbmisc(void); > >Why do you need a separate function? Function tegra_init_apbmisc() is called from tegra_init_fuse() which is invoked at early init and it also has `__init` keyword. If we use the same function for both ACPI/DT, then we will get init section mismatches when the Tegra Fuse driver probes using ACPI. We can use the same function by dropping the `init` keyword. But the way we are getting the resources for device-tree and on ACPI is slightly different. Hence, I kept a separate function for ACPI and move the common bits to a function shared between tegra_init_apbmisc() and tegra_acpi_init_apbmisc(). > > >> +static const struct acpi_device_id apbmisc_acpi_match[] = { >> + { .id = "NVDA2010", 0 }, > >We rarely use C99 initializers for ACPI ID table. >Also 0 is not needed. > I will update this in the next patch. ... >> + if (!apbmisc_base) >> + pr_err("failed to map APBMISC registers\n"); >> + else >> + chipid = readl_relaxed(apbmisc_base + 4); > >Why not positive conditional as you have two branches anyway? > >... > >> + if (!strapping_base) { >> + pr_err("failed to map strapping options registers\n"); >> + } else { >> + strapping = readl_relaxed(strapping_base); >> + iounmap(strapping_base); >> + } > >Ditto. > >... > Sure, I will update these in the next patch. ... >> - apbmisc_base = ioremap(apbmisc.start, resource_size(&apbmisc)); >> - if (!apbmisc_base) { >> - pr_err("failed to map APBMISC registers\n"); >> - } else { >> - chipid = readl_relaxed(apbmisc_base + 4); >> - } >> - >> - strapping_base = ioremap(straps.start, resource_size(&straps)); >> - if (!strapping_base) { >> - pr_err("failed to map strapping options registers\n"); >> - } else { >> - strapping = readl_relaxed(strapping_base); >> - iounmap(strapping_base); >> - } >> - >> + tegra_init_apbmisc_base(&apbmisc, &straps); > >This split can be done in a separate precursor patch. ACK. ... >> +void tegra_acpi_init_apbmisc(void) >> +{ >> + struct acpi_device *adev = NULL; >> + struct resource *apbmisc, *straps; >> + struct list_head resource_list; >> + struct resource_entry *rentry; >> + int rcount; >> + >> + adev = acpi_dev_get_first_match_dev(apbmisc_acpi_match[0].id, NULL, -1); >> + if (!adev) >> + return; >> + >> + INIT_LIST_HEAD(&resource_list); >> + >> + rcount = acpi_dev_get_memory_resources(adev, &resource_list); >> + if (rcount != 2) { >> + pr_err("failed to get APBMISC memory resources"); > >(1) > >> + return; >> + } > >> + rentry = list_first_entry(&resource_list, struct resource_entry, node); >> + apbmisc = rentry->res; >> + rentry = list_next_entry(rentry, node); >> + straps = rentry->res; > >We don't do like this, we walk through them and depending on the type and index >do something. The above if error prone and not scalable code. I will fix this logic in next patch. > >> + tegra_init_apbmisc_base(apbmisc, straps); > >> + acpi_dev_free_resource_list(&resource_list); > >Not okay in (1). > >> + acpi_dev_put(adev); > >Not okay in (1). Yes, these won't be called when rcount is `1`. I will update this in the next patch set. > +} Regards, Kartik
On Mon, Aug 21, 2023 at 05:02:20PM +0530, Kartik wrote: > On Fri, 2023-08-18 at 16:21 +0300, Andy Shevchenko wrote: ... > >> void tegra_init_revision(void); > >> void tegra_init_apbmisc(void); > >> +void tegra_acpi_init_apbmisc(void); > > > >Why do you need a separate function? > > Function tegra_init_apbmisc() is called from tegra_init_fuse() which > is invoked at early init and it also has `__init` keyword. If we use > the same function for both ACPI/DT, then we will get init section > mismatches when the Tegra Fuse driver probes using ACPI. > > We can use the same function by dropping the `init` keyword. But > the way we are getting the resources for device-tree and on ACPI is > slightly different. Hence, I kept a separate function for ACPI > and move the common bits to a function shared between > tegra_init_apbmisc() and tegra_acpi_init_apbmisc(). So, you mean that behaviour is different for ACPI and DT cases. Then obvious question why DT case can't be delayed to not so early stage to be run? This requires some explanations, more than given in the commit message and here.
Hi Kartik, kernel test robot noticed the following build errors: [auto build test ERROR on tegra/for-next] [also build test ERROR on soc/for-next linus/master v6.5-rc7 next-20230821] [cannot apply to tegra-drm/drm/tegra/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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Kartik/soc-tegra-fuse-Add-tegra_acpi_init_apbmisc/20230821-095539 base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next patch link: https://lore.kernel.org/r/20230818093028.7807-2-kkartik%40nvidia.com patch subject: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc() config: arm-randconfig-r004-20230822 (https://download.01.org/0day-ci/archive/20230822/202308221133.L1WzlvN7-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230822/202308221133.L1WzlvN7-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308221133.L1WzlvN7-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/soc/tegra/fuse/tegra-apbmisc.c:268:11: error: call to undeclared function 'acpi_dev_get_memory_resources'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 268 | rcount = acpi_dev_get_memory_resources(adev, &resource_list); | ^ >> drivers/soc/tegra/fuse/tegra-apbmisc.c:280:2: error: call to undeclared function 'acpi_dev_free_resource_list'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 280 | acpi_dev_free_resource_list(&resource_list); | ^ 2 errors generated. vim +/acpi_dev_get_memory_resources +268 drivers/soc/tegra/fuse/tegra-apbmisc.c 253 254 void tegra_acpi_init_apbmisc(void) 255 { 256 struct acpi_device *adev = NULL; 257 struct resource *apbmisc, *straps; 258 struct list_head resource_list; 259 struct resource_entry *rentry; 260 int rcount; 261 262 adev = acpi_dev_get_first_match_dev(apbmisc_acpi_match[0].id, NULL, -1); 263 if (!adev) 264 return; 265 266 INIT_LIST_HEAD(&resource_list); 267 > 268 rcount = acpi_dev_get_memory_resources(adev, &resource_list); 269 if (rcount != 2) { 270 pr_err("failed to get APBMISC memory resources"); 271 return; 272 } 273 274 rentry = list_first_entry(&resource_list, struct resource_entry, node); 275 apbmisc = rentry->res; 276 rentry = list_next_entry(rentry, node); 277 straps = rentry->res; 278 279 tegra_init_apbmisc_base(apbmisc, straps); > 280 acpi_dev_free_resource_list(&resource_list);
Hi Kartik, kernel test robot noticed the following build errors: [auto build test ERROR on tegra/for-next] [also build test ERROR on soc/for-next linus/master v6.5-rc7 next-20230821] [cannot apply to tegra-drm/drm/tegra/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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Kartik/soc-tegra-fuse-Add-tegra_acpi_init_apbmisc/20230821-095539 base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next patch link: https://lore.kernel.org/r/20230818093028.7807-2-kkartik%40nvidia.com patch subject: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc() config: arm-defconfig (https://download.01.org/0day-ci/archive/20230822/202308221124.bP9Do9ir-lkp@intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230822/202308221124.bP9Do9ir-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308221124.bP9Do9ir-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/soc/tegra/fuse/tegra-apbmisc.c: In function 'tegra_acpi_init_apbmisc': >> drivers/soc/tegra/fuse/tegra-apbmisc.c:268:18: error: implicit declaration of function 'acpi_dev_get_memory_resources'; did you mean 'acpi_get_event_resources'? [-Werror=implicit-function-declaration] 268 | rcount = acpi_dev_get_memory_resources(adev, &resource_list); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | acpi_get_event_resources >> drivers/soc/tegra/fuse/tegra-apbmisc.c:280:9: error: implicit declaration of function 'acpi_dev_free_resource_list' [-Werror=implicit-function-declaration] 280 | acpi_dev_free_resource_list(&resource_list); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +268 drivers/soc/tegra/fuse/tegra-apbmisc.c 253 254 void tegra_acpi_init_apbmisc(void) 255 { 256 struct acpi_device *adev = NULL; 257 struct resource *apbmisc, *straps; 258 struct list_head resource_list; 259 struct resource_entry *rentry; 260 int rcount; 261 262 adev = acpi_dev_get_first_match_dev(apbmisc_acpi_match[0].id, NULL, -1); 263 if (!adev) 264 return; 265 266 INIT_LIST_HEAD(&resource_list); 267 > 268 rcount = acpi_dev_get_memory_resources(adev, &resource_list); 269 if (rcount != 2) { 270 pr_err("failed to get APBMISC memory resources"); 271 return; 272 } 273 274 rentry = list_first_entry(&resource_list, struct resource_entry, node); 275 apbmisc = rentry->res; 276 rentry = list_next_entry(rentry, node); 277 straps = rentry->res; 278 279 tegra_init_apbmisc_base(apbmisc, straps); > 280 acpi_dev_free_resource_list(&resource_list);
On Mon, Aug 21, 2023 at 03:32:41PM +0300, Andy Shevchenko wrote: > On Mon, Aug 21, 2023 at 05:02:20PM +0530, Kartik wrote: > > On Fri, 2023-08-18 at 16:21 +0300, Andy Shevchenko wrote: > > ... > > > >> void tegra_init_revision(void); > > >> void tegra_init_apbmisc(void); > > >> +void tegra_acpi_init_apbmisc(void); > > > > > >Why do you need a separate function? > > > > Function tegra_init_apbmisc() is called from tegra_init_fuse() which > > is invoked at early init and it also has `__init` keyword. If we use > > the same function for both ACPI/DT, then we will get init section > > mismatches when the Tegra Fuse driver probes using ACPI. > > > > We can use the same function by dropping the `init` keyword. But > > the way we are getting the resources for device-tree and on ACPI is > > slightly different. Hence, I kept a separate function for ACPI > > and move the common bits to a function shared between > > tegra_init_apbmisc() and tegra_acpi_init_apbmisc(). > > So, you mean that behaviour is different for ACPI and DT cases. > Then obvious question why DT case can't be delayed to not so early > stage to be run? This requires some explanations, more than given > in the commit message and here. We've done some experiments in the past to unify this and unfortunately we can't. The reason is that some of the old 32-bit ARM code needs some information from the APBMISC registers very early during boot (among other things for SMP support), so delaying this doesn't work. I agree that it would be good to put this into some comment for later reference. Thierry
diff --git a/drivers/soc/tegra/fuse/fuse.h b/drivers/soc/tegra/fuse/fuse.h index 90f23be73894..a41e9f85281a 100644 --- a/drivers/soc/tegra/fuse/fuse.h +++ b/drivers/soc/tegra/fuse/fuse.h @@ -69,6 +69,7 @@ struct tegra_fuse { void tegra_init_revision(void); void tegra_init_apbmisc(void); +void tegra_acpi_init_apbmisc(void); u32 __init tegra_fuse_read_spare(unsigned int spare); u32 __init tegra_fuse_read_early(unsigned int offset); diff --git a/drivers/soc/tegra/fuse/tegra-apbmisc.c b/drivers/soc/tegra/fuse/tegra-apbmisc.c index da970f3dbf35..79db12076d56 100644 --- a/drivers/soc/tegra/fuse/tegra-apbmisc.c +++ b/drivers/soc/tegra/fuse/tegra-apbmisc.c @@ -3,6 +3,7 @@ * Copyright (c) 2014-2023, NVIDIA CORPORATION. All rights reserved. */ +#include <linux/acpi.h> #include <linux/export.h> #include <linux/io.h> #include <linux/kernel.h> @@ -120,6 +121,11 @@ int tegra194_miscreg_mask_serror(void) } EXPORT_SYMBOL(tegra194_miscreg_mask_serror); +static const struct acpi_device_id apbmisc_acpi_match[] = { + { .id = "NVDA2010", 0 }, + { /* sentinel */ } +}; + static const struct of_device_id apbmisc_match[] __initconst = { { .compatible = "nvidia,tegra20-apbmisc", }, { .compatible = "nvidia,tegra186-misc", }, @@ -160,9 +166,28 @@ void __init tegra_init_revision(void) tegra_sku_info.platform = tegra_get_platform(); } -void __init tegra_init_apbmisc(void) +static void tegra_init_apbmisc_base(struct resource *apbmisc, + struct resource *straps) { void __iomem *strapping_base; + + apbmisc_base = ioremap(apbmisc->start, resource_size(apbmisc)); + if (!apbmisc_base) + pr_err("failed to map APBMISC registers\n"); + else + chipid = readl_relaxed(apbmisc_base + 4); + + strapping_base = ioremap(straps->start, resource_size(straps)); + if (!strapping_base) { + pr_err("failed to map strapping options registers\n"); + } else { + strapping = readl_relaxed(strapping_base); + iounmap(strapping_base); + } +} + +void __init tegra_init_apbmisc(void) +{ struct resource apbmisc, straps; struct device_node *np; @@ -219,23 +244,39 @@ void __init tegra_init_apbmisc(void) } } - apbmisc_base = ioremap(apbmisc.start, resource_size(&apbmisc)); - if (!apbmisc_base) { - pr_err("failed to map APBMISC registers\n"); - } else { - chipid = readl_relaxed(apbmisc_base + 4); - } - - strapping_base = ioremap(straps.start, resource_size(&straps)); - if (!strapping_base) { - pr_err("failed to map strapping options registers\n"); - } else { - strapping = readl_relaxed(strapping_base); - iounmap(strapping_base); - } - + tegra_init_apbmisc_base(&apbmisc, &straps); long_ram_code = of_property_read_bool(np, "nvidia,long-ram-code"); put: of_node_put(np); } + +void tegra_acpi_init_apbmisc(void) +{ + struct acpi_device *adev = NULL; + struct resource *apbmisc, *straps; + struct list_head resource_list; + struct resource_entry *rentry; + int rcount; + + adev = acpi_dev_get_first_match_dev(apbmisc_acpi_match[0].id, NULL, -1); + if (!adev) + return; + + INIT_LIST_HEAD(&resource_list); + + rcount = acpi_dev_get_memory_resources(adev, &resource_list); + if (rcount != 2) { + pr_err("failed to get APBMISC memory resources"); + return; + } + + rentry = list_first_entry(&resource_list, struct resource_entry, node); + apbmisc = rentry->res; + rentry = list_next_entry(rentry, node); + straps = rentry->res; + + tegra_init_apbmisc_base(apbmisc, straps); + acpi_dev_free_resource_list(&resource_list); + acpi_dev_put(adev); +}
In preparation to ACPI support in Tegra fuse driver add function tegra_acpi_init_apbmisc() and move common code used for both ACPI and device-tree into a new helper function tegra_init_apbmisc_base(). Note that function tegra_acpi_init_apbmisc() is not placed in the __init section, because it will be called during probe. Signed-off-by: Kartik <kkartik@nvidia.com> --- drivers/soc/tegra/fuse/fuse.h | 1 + drivers/soc/tegra/fuse/tegra-apbmisc.c | 73 ++++++++++++++++++++------ 2 files changed, 58 insertions(+), 16 deletions(-)