[PATCH 0/6] soc/tegra: fuse: Add ACPI support

[PATCH 0/6] soc/tegra: fuse: Add ACPI support

[Kartik]

This series of patches add ACPI support for Tegra194 and Tegra234 in
Tegra fuse and apbmisc drivers. It also adds support for Tegra241
which uses ACPI boot

Kartik (6):
  soc/tegra: fuse: Add tegra_acpi_init_apbmisc()
  soc/tegra: fuse: Add function to register nvmem
  soc/tegra: fuse: Add function to add lookups
  soc/tegra: fuse: Add function to print SKU info
  soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234
  soc/tegra: fuse: Add support for Tegra241

 drivers/soc/tegra/Kconfig              |   5 +
 drivers/soc/tegra/fuse/fuse-tegra.c    | 180 +++++++++++++++++++------
 drivers/soc/tegra/fuse/fuse-tegra30.c  |  25 ++++
 drivers/soc/tegra/fuse/fuse.h          |   5 +
 drivers/soc/tegra/fuse/tegra-apbmisc.c |  74 +++++++---
 include/soc/tegra/fuse.h               |   1 +
 6 files changed, 235 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[Kartik]

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(-)

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);
+}
-- 
2.34.1

Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[Andy Shevchenko]

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).

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[Kartik]

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

Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[kernel test robot]

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);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/6] soc/tegra: fuse: Add tegra_acpi_init_apbmisc()

[kernel test robot]

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);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/6] soc/tegra: fuse: Add function to register nvmem

[Kartik]

Add helper function tegra_fuse_nvmem_register() to register
Tegra Fuse nvmem and use it in tegra_fuse_probe(). So, this can be
shared between device-tree and ACPI probe, which is to be introduced
later.

Signed-off-by: Kartik <kkartik@nvidia.com>
---
 drivers/soc/tegra/fuse/fuse-tegra.c | 62 +++++++++++++++++------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index a2c28f493a75..45784ac6393d 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -113,10 +113,44 @@ static void tegra_fuse_restore(void *base)
 	fuse->clk = NULL;
 }
 
+static int tegra_fuse_nvmem_register(struct tegra_fuse *fuse,
+				     struct device *dev)
+{
+	struct nvmem_config nvmem;
+	int err;
+
+	memset(&nvmem, 0, sizeof(nvmem));
+	nvmem.dev = dev;
+	nvmem.name = "fuse";
+	nvmem.id = -1;
+	nvmem.owner = THIS_MODULE;
+	nvmem.cells = fuse->soc->cells;
+	nvmem.ncells = fuse->soc->num_cells;
+	nvmem.keepout = fuse->soc->keepouts;
+	nvmem.nkeepout = fuse->soc->num_keepouts;
+	nvmem.type = NVMEM_TYPE_OTP;
+	nvmem.read_only = true;
+	nvmem.root_only = false;
+	nvmem.reg_read = tegra_fuse_read;
+	nvmem.size = fuse->soc->info->size;
+	nvmem.word_size = 4;
+	nvmem.stride = 4;
+	nvmem.priv = fuse;
+
+	fuse->nvmem = devm_nvmem_register(dev, &nvmem);
+	if (IS_ERR(fuse->nvmem)) {
+		err = PTR_ERR(fuse->nvmem);
+		dev_err(dev, "failed to register NVMEM device: %d\n",
+			err);
+		return err;
+	}
+
+	return 0;
+}
+
 static int tegra_fuse_probe(struct platform_device *pdev)
 {
 	void __iomem *base = fuse->base;
-	struct nvmem_config nvmem;
 	struct resource *res;
 	int err;
 
@@ -152,31 +186,9 @@ static int tegra_fuse_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	memset(&nvmem, 0, sizeof(nvmem));
-	nvmem.dev = &pdev->dev;
-	nvmem.name = "fuse";
-	nvmem.id = -1;
-	nvmem.owner = THIS_MODULE;
-	nvmem.cells = fuse->soc->cells;
-	nvmem.ncells = fuse->soc->num_cells;
-	nvmem.keepout = fuse->soc->keepouts;
-	nvmem.nkeepout = fuse->soc->num_keepouts;
-	nvmem.type = NVMEM_TYPE_OTP;
-	nvmem.read_only = true;
-	nvmem.root_only = false;
-	nvmem.reg_read = tegra_fuse_read;
-	nvmem.size = fuse->soc->info->size;
-	nvmem.word_size = 4;
-	nvmem.stride = 4;
-	nvmem.priv = fuse;
-
-	fuse->nvmem = devm_nvmem_register(&pdev->dev, &nvmem);
-	if (IS_ERR(fuse->nvmem)) {
-		err = PTR_ERR(fuse->nvmem);
-		dev_err(&pdev->dev, "failed to register NVMEM device: %d\n",
-			err);
+	err = tegra_fuse_nvmem_register(fuse, &pdev->dev);
+	if (err)
 		return err;
-	}
 
 	fuse->rst = devm_reset_control_get_optional(&pdev->dev, "fuse");
 	if (IS_ERR(fuse->rst)) {
-- 
2.34.1

Re: [PATCH 2/6] soc/tegra: fuse: Add function to register nvmem

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 03:00:24PM +0530, Kartik wrote:
> Add helper function tegra_fuse_nvmem_register() to register
> Tegra Fuse nvmem and use it in tegra_fuse_probe(). So, this can be
> shared between device-tree and ACPI probe, which is to be introduced
> later.

...

> +	fuse->nvmem = devm_nvmem_register(dev, &nvmem);
> +	if (IS_ERR(fuse->nvmem)) {

> +		err = PTR_ERR(fuse->nvmem);
> +		dev_err(dev, "failed to register NVMEM device: %d\n",
> +			err);
> +		return err;

		return dev_err_probe();

> +	}
> +
> +	return 0;
> +}

...

Seems it comes from the original code, so consider this as a suggestion for
an additional improvement.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/6] soc/tegra: fuse: Add function to register nvmem

[Kartik]

On Fri, 2023-08-18 at 16:50 +0300, Andy Shevchenko wrote:

>On Fri, Aug 18, 2023 at 03:00:24PM +0530, Kartik wrote:
>> Add helper function tegra_fuse_nvmem_register() to register
>> Tegra Fuse nvmem and use it in tegra_fuse_probe(). So, this can be
>> shared between device-tree and ACPI probe, which is to be introduced
>> later.
>
>...
>
>> +	fuse->nvmem = devm_nvmem_register(dev, &nvmem);
>> +	if (IS_ERR(fuse->nvmem)) {
>
>> +		err = PTR_ERR(fuse->nvmem);
>> +		dev_err(dev, "failed to register NVMEM device: %d\n",
>> +			err);
>> +		return err;
>
>		return dev_err_probe();
>
>> +	}
>> +
>> +	return 0;
>> +}
>
>...
>
>Seems it comes from the original code, so consider this as a suggestion for
>an additional improvement.
>
>-- 
>With Best Regards,
>Andy Shevchenko

Thanks Andy, I will update this.


Regards,
Kartik

[PATCH 3/6] soc/tegra: fuse: Add function to add lookups

[Kartik]

Add helper function tegra_fuse_add_lookups() to register Tegra fuse
nvmem lookups. So, this can be shared between tegra_fuse_init() and
ACPI probe, which is to be introduced later.

Signed-off-by: Kartik <kkartik@nvidia.com>
---
 drivers/soc/tegra/fuse/fuse-tegra.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index 45784ac6393d..bbb1a5c4823b 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -148,6 +148,24 @@ static int tegra_fuse_nvmem_register(struct tegra_fuse *fuse,
 	return 0;
 }
 
+static int tegra_fuse_add_lookups(struct tegra_fuse *fuse)
+{
+	size_t size;
+
+	if (!fuse->soc->lookups)
+		return 0;
+
+	size = sizeof(*fuse->lookups) * fuse->soc->num_lookups;
+
+	fuse->lookups = kmemdup(fuse->soc->lookups, size, GFP_KERNEL);
+	if (!fuse->lookups)
+		return -ENOMEM;
+
+	nvmem_add_cell_lookups(fuse->lookups, fuse->soc->num_lookups);
+
+	return 0;
+}
+
 static int tegra_fuse_probe(struct platform_device *pdev)
 {
 	void __iomem *base = fuse->base;
@@ -419,6 +437,7 @@ static int __init tegra_init_fuse(void)
 	const struct of_device_id *match;
 	struct device_node *np;
 	struct resource regs;
+	int err;
 
 	tegra_init_apbmisc();
 
@@ -516,12 +535,10 @@ static int __init tegra_init_fuse(void)
 	pr_debug("Tegra CPU Speedo ID %d, SoC Speedo ID %d\n",
 		 tegra_sku_info.cpu_speedo_id, tegra_sku_info.soc_speedo_id);
 
-	if (fuse->soc->lookups) {
-		size_t size = sizeof(*fuse->lookups) * fuse->soc->num_lookups;
-
-		fuse->lookups = kmemdup(fuse->soc->lookups, size, GFP_KERNEL);
-		if (fuse->lookups)
-			nvmem_add_cell_lookups(fuse->lookups, fuse->soc->num_lookups);
+	err = tegra_fuse_add_lookups(fuse);
+	if (err) {
+		pr_err("failed to add FUSE lookups\n");
+		return err;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH 3/6] soc/tegra: fuse: Add function to add lookups

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 03:00:25PM +0530, Kartik wrote:
> Add helper function tegra_fuse_add_lookups() to register Tegra fuse
> nvmem lookups. So, this can be shared between tegra_fuse_init() and
> ACPI probe, which is to be introduced later.

...

> +	size = sizeof(*fuse->lookups) * fuse->soc->num_lookups;

At least this should use size_mul().

> +	fuse->lookups = kmemdup(fuse->soc->lookups, size, GFP_KERNEL);
> +	if (!fuse->lookups)
> +		return -ENOMEM;

But ideally you need to add a patch that brings kmemdup_array().

Okay, it seems it's in the original code :-(
Can you add in your ToDo list to amend this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] soc/tegra: fuse: Add function to add lookups

[Kartik]

On Fri, 2023-08-18 at 17:06 +0300, Andy Shevchenko wrote:
>On Fri, Aug 18, 2023 at 03:00:25PM +0530, Kartik wrote:
>> Add helper function tegra_fuse_add_lookups() to register Tegra fuse
>> nvmem lookups. So, this can be shared between tegra_fuse_init() and
>> ACPI probe, which is to be introduced later.
>
>...
>
>> +	size = sizeof(*fuse->lookups) * fuse->soc->num_lookups;
>
>At least this should use size_mul().
>
>> +	fuse->lookups = kmemdup(fuse->soc->lookups, size, GFP_KERNEL);
>> +	if (!fuse->lookups)
>> +		return -ENOMEM;
>
>But ideally you need to add a patch that brings kmemdup_array().
>
>Okay, it seems it's in the original code :-(
>Can you add in your ToDo list to amend this?
>
>-- 
>With Best Regards,
>Andy Shevchenko

I will update this in the next patch.

Regards,
Kartik

[PATCH 4/6] soc/tegra: fuse: Add function to print SKU info

[Kartik]

Add helper function tegra_fuse_pr_sku_info() to print Tegra SKU
information. So, it can be shared between tegra_fuse_init() and
ACPI probe which is to be introduced later.

Signed-off-by: Kartik <kkartik@nvidia.com>
---
 drivers/soc/tegra/fuse/fuse-tegra.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index bbb1a5c4823b..70e8eeddcbd9 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -166,6 +166,16 @@ static int tegra_fuse_add_lookups(struct tegra_fuse *fuse)
 	return 0;
 }
 
+static void tegra_fuse_pr_sku_info(struct tegra_sku_info *tegra_sku_info)
+{
+	pr_info("Tegra Revision: %s SKU: %d CPU Process: %d SoC Process: %d\n",
+		tegra_revision_name[tegra_sku_info->revision],
+		tegra_sku_info->sku_id, tegra_sku_info->cpu_process_id,
+		tegra_sku_info->soc_process_id);
+	pr_debug("Tegra CPU Speedo ID %d, SoC Speedo ID %d\n",
+		tegra_sku_info->cpu_speedo_id, tegra_sku_info->soc_speedo_id);
+}
+
 static int tegra_fuse_probe(struct platform_device *pdev)
 {
 	void __iomem *base = fuse->base;
@@ -528,12 +538,7 @@ static int __init tegra_init_fuse(void)
 
 	fuse->soc->init(fuse);
 
-	pr_info("Tegra Revision: %s SKU: %d CPU Process: %d SoC Process: %d\n",
-		tegra_revision_name[tegra_sku_info.revision],
-		tegra_sku_info.sku_id, tegra_sku_info.cpu_process_id,
-		tegra_sku_info.soc_process_id);
-	pr_debug("Tegra CPU Speedo ID %d, SoC Speedo ID %d\n",
-		 tegra_sku_info.cpu_speedo_id, tegra_sku_info.soc_speedo_id);
+	tegra_fuse_pr_sku_info(&tegra_sku_info);
 
 	err = tegra_fuse_add_lookups(fuse);
 	if (err) {
-- 
2.34.1

Re: [PATCH 4/6] soc/tegra: fuse: Add function to print SKU info

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

On Fri, Aug 18, 2023 at 03:00:26PM +0530, Kartik wrote:
> Add helper function tegra_fuse_pr_sku_info() to print Tegra SKU

Nit: I'd prefer tegra_fuse_print_sku_info(). This is already quite long,
so the extra 3 characters aren't going to make things a lot worse, but
they improve readability quite a bit.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/6] soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234

[Kartik]

Add tegra_fuse_acpi_probe() to initialize Tegra fuse while using ACPI.
Also, drop '__init' keyword for tegra_soc_device_register() as this is also
used by tegra_fuse_acpi_probe().

Note that as ACPI subsystem initialize at subsys init, function
tegra_fuse_acpi_probe() also contains the necessary initialization
that we are currently doing for device-tree boot as a part of
early init.

Signed-off-by: Kartik <kkartik@nvidia.com>
---
 drivers/soc/tegra/fuse/fuse-tegra.c | 67 ++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index 70e8eeddcbd9..9c5596d968a2 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2013-2023, NVIDIA CORPORATION.  All rights reserved.
  */
 
+#include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/kobject.h>
@@ -94,6 +95,13 @@ static const struct of_device_id tegra_fuse_match[] = {
 	{ /* sentinel */ }
 };
 
+static const struct acpi_device_id tegra_fuse_acpi_match[] = {
+	{
+		.id = "NVDA200F",
+	},
+	{ /* sentinel */ },
+};
+
 static int tegra_fuse_read(void *priv, unsigned int offset, void *value,
 			   size_t bytes)
 {
@@ -176,12 +184,65 @@ static void tegra_fuse_pr_sku_info(struct tegra_sku_info *tegra_sku_info)
 		tegra_sku_info->cpu_speedo_id, tegra_sku_info->soc_speedo_id);
 }
 
+static int tegra_fuse_acpi_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	u8 chip;
+	int err;
+
+	tegra_acpi_init_apbmisc();
+
+	chip = tegra_get_chip_id();
+	switch (chip) {
+#if defined(CONFIG_ARCH_TEGRA_194_SOC)
+	case TEGRA194:
+		fuse->soc = &tegra194_fuse_soc;
+		break;
+#endif
+#if defined(CONFIG_ARCH_TEGRA_234_SOC)
+	case TEGRA234:
+		fuse->soc = &tegra234_fuse_soc;
+		break;
+#endif
+	default:
+		dev_err(&pdev->dev, "Unsupported SoC: %02x\n", chip);
+		return -EINVAL;
+	}
+
+	fuse->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(fuse->base))
+		return PTR_ERR(fuse->base);
+	fuse->phys = res->start;
+
+	platform_set_drvdata(pdev, fuse);
+	fuse->dev = &pdev->dev;
+
+	err = tegra_fuse_nvmem_register(fuse, &pdev->dev);
+	if (err)
+		return err;
+
+	fuse->soc->init(fuse);
+	tegra_soc_device_register();
+	tegra_fuse_pr_sku_info(&tegra_sku_info);
+
+	err = tegra_fuse_add_lookups(fuse);
+	if (err) {
+		dev_err(&pdev->dev, "failed to add FUSE lookups\n");
+		return err;
+	}
+
+	return 0;
+}
+
 static int tegra_fuse_probe(struct platform_device *pdev)
 {
 	void __iomem *base = fuse->base;
 	struct resource *res;
 	int err;
 
+	if (has_acpi_companion(&pdev->dev))
+		return tegra_fuse_acpi_probe(pdev);
+
 	err = devm_add_action(&pdev->dev, tegra_fuse_restore, (void __force *)base);
 	if (err)
 		return err;
@@ -306,6 +367,7 @@ static struct platform_driver tegra_fuse_driver = {
 	.driver = {
 		.name = "tegra-fuse",
 		.of_match_table = tegra_fuse_match,
+		.acpi_match_table = tegra_fuse_acpi_match,
 		.pm = &tegra_fuse_pm,
 		.suppress_bind_attrs = true,
 	},
@@ -327,7 +389,8 @@ u32 __init tegra_fuse_read_early(unsigned int offset)
 
 int tegra_fuse_readl(unsigned long offset, u32 *value)
 {
-	if (!fuse->read || !fuse->clk)
+	/* fuse->clk is not required when ACPI is used. */
+	if (!fuse->read || (!fuse->clk && !has_acpi_companion(fuse->dev)))
 		return -EPROBE_DEFER;
 
 	if (IS_ERR(fuse->clk))
@@ -410,7 +473,7 @@ const struct attribute_group tegra194_soc_attr_group = {
 };
 #endif
 
-struct device * __init tegra_soc_device_register(void)
+struct device *tegra_soc_device_register(void)
 {
 	struct soc_device_attribute *attr;
 	struct soc_device *dev;
-- 
2.34.1

Re: [PATCH 5/6] soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 03:00:27PM +0530, Kartik wrote:
> Add tegra_fuse_acpi_probe() to initialize Tegra fuse while using ACPI.
> Also, drop '__init' keyword for tegra_soc_device_register() as this is also
> used by tegra_fuse_acpi_probe().
> 
> Note that as ACPI subsystem initialize at subsys init, function
> tegra_fuse_acpi_probe() also contains the necessary initialization
> that we are currently doing for device-tree boot as a part of
> early init.

...

> +#include <linux/acpi.h>

You meed mod_devicetable.h and possibly property.h, not this header
(see below).

...

> +static const struct acpi_device_id tegra_fuse_acpi_match[] = {
> +	{
> +		.id = "NVDA200F",
> +	},

Single line, no inner comma.

> +	{ /* sentinel */ },

The idea of sentinel is to guard, the trailing comma ruins this contract.

> +};

...

> +static int tegra_fuse_acpi_probe(struct platform_device *pdev)
> +{

Why you need a separate function?

> +	struct resource *res;
> +	u8 chip;
> +	int err;
> +
> +	tegra_acpi_init_apbmisc();
> +
> +	chip = tegra_get_chip_id();
> +	switch (chip) {
> +#if defined(CONFIG_ARCH_TEGRA_194_SOC)

Can we avoid ugly ifdeffery?

> +	case TEGRA194:
> +		fuse->soc = &tegra194_fuse_soc;
> +		break;
> +#endif
> +#if defined(CONFIG_ARCH_TEGRA_234_SOC)

Ditto.

> +	case TEGRA234:
> +		fuse->soc = &tegra234_fuse_soc;
> +		break;
> +#endif
> +	default:
> +		dev_err(&pdev->dev, "Unsupported SoC: %02x\n", chip);
> +		return -EINVAL;

		return dev_err_probe(...);

> +	}
> +
> +	fuse->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(fuse->base))
> +		return PTR_ERR(fuse->base);
> +	fuse->phys = res->start;

> +	platform_set_drvdata(pdev, fuse);

Is it being used?

> +	fuse->dev = &pdev->dev;
> +
> +	err = tegra_fuse_nvmem_register(fuse, &pdev->dev);
> +	if (err)
> +		return err;
> +
> +	fuse->soc->init(fuse);
> +	tegra_soc_device_register();
> +	tegra_fuse_pr_sku_info(&tegra_sku_info);
> +
> +	err = tegra_fuse_add_lookups(fuse);
> +	if (err) {

> +		dev_err(&pdev->dev, "failed to add FUSE lookups\n");
> +		return err;

		return dev_err_probe(...);

> +	}
> +
> +	return 0;
> +}

...

> +	if (has_acpi_companion(&pdev->dev))
> +		return tegra_fuse_acpi_probe(pdev);

Why is the ACPI so special here? Why you can't go same flow?

...

> +	/* fuse->clk is not required when ACPI is used. */
> +	if (!fuse->read || (!fuse->clk && !has_acpi_companion(fuse->dev)))

No, just make CLK optional and that's it.

>  		return -EPROBE_DEFER;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234

[Kartik]

On Fri, 2023-08-18 at 18:07 +0300, Andy Shevchenko wrote:
>On Fri, Aug 18, 2023 at 03:00:27PM +0530, Kartik wrote:
>> Add tegra_fuse_acpi_probe() to initialize Tegra fuse while using ACPI.
>> Also, drop '__init' keyword for tegra_soc_device_register() as this is also
>> used by tegra_fuse_acpi_probe().
>> 
>> Note that as ACPI subsystem initialize at subsys init, function
>> tegra_fuse_acpi_probe() also contains the necessary initialization
>> that we are currently doing for device-tree boot as a part of
>> early init.
>
>...
>
>> +#include <linux/acpi.h>
>
>You meed mod_devicetable.h and possibly property.h, not this header
>(see below).
>
>...
>
>> +static const struct acpi_device_id tegra_fuse_acpi_match[] = {
>> +	{
>> +		.id = "NVDA200F",
>> +	},
>
>Single line, no inner comma.

ACK.

>
>> +	{ /* sentinel */ },
>
>The idea of sentinel is to guard, the trailing comma ruins this contract.
>
>> +};

ACK.

>
>...
>
>> +static int tegra_fuse_acpi_probe(struct platform_device *pdev)
>> +{
>
>Why you need a separate function?
>
>> +	struct resource *res;
>> +	u8 chip;
>> +	int err;
>> +
>> +	tegra_acpi_init_apbmisc();
>> +
>> +	chip = tegra_get_chip_id();
>> +	switch (chip) {
>> +#if defined(CONFIG_ARCH_TEGRA_194_SOC)
>
>Can we avoid ugly ifdeffery?
>

No, the SoC data is defined only when the SoC specific config is
enabled. So, guarding these with ifdef's is required here.

>> +	case TEGRA194:
>> +		fuse->soc = &tegra194_fuse_soc;
>> +		break;
>> +#endif
>> +#if defined(CONFIG_ARCH_TEGRA_234_SOC)
>
>Ditto.
>
>> +	case TEGRA234:
>> +		fuse->soc = &tegra234_fuse_soc;
>> +		break;
>> +#endif
>> +	default:
>> +		dev_err(&pdev->dev, "Unsupported SoC: %02x\n", chip);
>> +		return -EINVAL;
>
>		return dev_err_probe(...);
>

ACK.

>> +	}
>> +
>> +	fuse->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> +	if (IS_ERR(fuse->base))
>> +		return PTR_ERR(fuse->base);
>> +	fuse->phys = res->start;
>
>> +	platform_set_drvdata(pdev, fuse);
>
>Is it being used?
>

Looks like this is not being used.

>> +	fuse->dev = &pdev->dev;
>> +
>> +	err = tegra_fuse_nvmem_register(fuse, &pdev->dev);
>> +	if (err)
>> +		return err;
>> +
>> +	fuse->soc->init(fuse);
>> +	tegra_soc_device_register();
>> +	tegra_fuse_pr_sku_info(&tegra_sku_info);
>> +
>> +	err = tegra_fuse_add_lookups(fuse);
>> +	if (err) {
>
>> +		dev_err(&pdev->dev, "failed to add FUSE lookups\n");
>> +		return err;
>
>		return dev_err_probe(...);
>
>> +	}
>> +
>> +	return 0;
>> +}
>
>...
>
>> +	if (has_acpi_companion(&pdev->dev))
>> +		return tegra_fuse_acpi_probe(pdev);
>
>Why is the ACPI so special here? Why you can't go same flow?
>
>...
>

We need to initialize the soc data before we continue the probe.
I guess we can have a conditional here for this initialization.
and re-use the same function. I will update this in the next patch.

>> +	/* fuse->clk is not required when ACPI is used. */
>> +	if (!fuse->read || (!fuse->clk && !has_acpi_companion(fuse->dev)))
>
>No, just make CLK optional and that's it.
>
>>  		return -EPROBE_DEFER;
>
>-- 

If the Fuse driver is probed using device-tree. Then we need to make
sure that fuse->clk has been initilaized.

>With Best Regards,
>Andy Shevchenko

Regards,
Kartik

Re: [PATCH 5/6] soc/tegra: fuse: Add ACPI support for Tegra194 and Tegra234

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 05:08:19PM +0530, Kartik wrote:
> On Fri, 2023-08-18 at 18:07 +0300, Andy Shevchenko wrote:
> >On Fri, Aug 18, 2023 at 03:00:27PM +0530, Kartik wrote:

...

> >> +	/* fuse->clk is not required when ACPI is used. */
> >> +	if (!fuse->read || (!fuse->clk && !has_acpi_companion(fuse->dev)))
> >
> >No, just make CLK optional and that's it.
> >
> >>  		return -EPROBE_DEFER;
> >
> If the Fuse driver is probed using device-tree. Then we need to make
> sure that fuse->clk has been initilaized.

So, I recommend to think about it more, maybe you can find better solution
than above.

And actually don't use has_acpi_companion() in this case. What you need is
is_acpi_node() or is_of_node() depending on the case you want to check for.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 6/6] soc/tegra: fuse: Add support for Tegra241

[Kartik]

Add support for Tegra241 which use ACPI boot.

Signed-off-by: Kartik <kkartik@nvidia.com>
---
 drivers/soc/tegra/Kconfig              |  5 +++++
 drivers/soc/tegra/fuse/fuse-tegra.c    |  5 +++++
 drivers/soc/tegra/fuse/fuse-tegra30.c  | 25 +++++++++++++++++++++++++
 drivers/soc/tegra/fuse/fuse.h          |  4 ++++
 drivers/soc/tegra/fuse/tegra-apbmisc.c |  1 +
 include/soc/tegra/fuse.h               |  1 +
 6 files changed, 41 insertions(+)

diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
index 6f3098822969..5f5d9d663fef 100644
--- a/drivers/soc/tegra/Kconfig
+++ b/drivers/soc/tegra/Kconfig
@@ -133,6 +133,11 @@ config ARCH_TEGRA_234_SOC
 	help
 	  Enable support for the NVIDIA Tegra234 SoC.
 
+config ARCH_TEGRA_241_SOC
+	bool "NVIDIA Tegra241 SoC"
+	help
+	  Enable support for the NVIDIA Tegra241 SoC.
+
 endif
 endif
 
diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index 9c5596d968a2..8750450438c3 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -203,6 +203,11 @@ static int tegra_fuse_acpi_probe(struct platform_device *pdev)
 	case TEGRA234:
 		fuse->soc = &tegra234_fuse_soc;
 		break;
+#endif
+#if defined(CONFIG_ARCH_TEGRA_241_SOC)
+	case TEGRA241:
+		fuse->soc = &tegra241_fuse_soc;
+		break;
 #endif
 	default:
 		dev_err(&pdev->dev, "Unsupported SoC: %02x\n", chip);
diff --git a/drivers/soc/tegra/fuse/fuse-tegra30.c b/drivers/soc/tegra/fuse/fuse-tegra30.c
index e94d46372a63..757bcf5ee82e 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra30.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra30.c
@@ -678,3 +678,28 @@ const struct tegra_fuse_soc tegra234_fuse_soc = {
 	.clk_suspend_on = false,
 };
 #endif
+
+#if defined(CONFIG_ARCH_TEGRA_241_SOC)
+static const struct tegra_fuse_info tegra241_fuse_info = {
+	.read = tegra30_fuse_read,
+	.size = 0x16008,
+	.spare = 0xcf0,
+};
+
+static const struct nvmem_keepout tegra241_fuse_keepouts[] = {
+	{ .start = 0xc, .end = 0x1600c }
+};
+
+const struct tegra_fuse_soc tegra241_fuse_soc = {
+	.init = tegra30_fuse_init,
+	.info = &tegra241_fuse_info,
+	.lookups = NULL,
+	.num_lookups = 0,
+	.cells = NULL,
+	.num_cells = 0,
+	.keepouts = tegra241_fuse_keepouts,
+	.num_keepouts = ARRAY_SIZE(tegra241_fuse_keepouts),
+	.soc_attr_group = &tegra194_soc_attr_group,
+	.clk_suspend_on = false,
+};
+#endif
diff --git a/drivers/soc/tegra/fuse/fuse.h b/drivers/soc/tegra/fuse/fuse.h
index a41e9f85281a..f3b705327c20 100644
--- a/drivers/soc/tegra/fuse/fuse.h
+++ b/drivers/soc/tegra/fuse/fuse.h
@@ -136,4 +136,8 @@ extern const struct tegra_fuse_soc tegra194_fuse_soc;
 extern const struct tegra_fuse_soc tegra234_fuse_soc;
 #endif
 
+#ifdef CONFIG_ARCH_TEGRA_241_SOC
+extern const struct tegra_fuse_soc tegra241_fuse_soc;
+#endif
+
 #endif
diff --git a/drivers/soc/tegra/fuse/tegra-apbmisc.c b/drivers/soc/tegra/fuse/tegra-apbmisc.c
index 79db12076d56..f830aaf3d92d 100644
--- a/drivers/soc/tegra/fuse/tegra-apbmisc.c
+++ b/drivers/soc/tegra/fuse/tegra-apbmisc.c
@@ -63,6 +63,7 @@ bool tegra_is_silicon(void)
 	switch (tegra_get_chip_id()) {
 	case TEGRA194:
 	case TEGRA234:
+	case TEGRA241:
 	case TEGRA264:
 		if (tegra_get_platform() == 0)
 			return true;
diff --git a/include/soc/tegra/fuse.h b/include/soc/tegra/fuse.h
index 3a513be50243..8f421b9f7585 100644
--- a/include/soc/tegra/fuse.h
+++ b/include/soc/tegra/fuse.h
@@ -17,6 +17,7 @@
 #define TEGRA186	0x18
 #define TEGRA194	0x19
 #define TEGRA234	0x23
+#define TEGRA241	0x24
 #define TEGRA264	0x26
 
 #define TEGRA_FUSE_SKU_CALIB_0	0xf0
-- 
2.34.1

Re: [PATCH 6/6] soc/tegra: fuse: Add support for Tegra241

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 03:00:28PM +0530, Kartik wrote:
> Add support for Tegra241 which use ACPI boot.

...

>  	case TEGRA234:
>  		fuse->soc = &tegra234_fuse_soc;
>  		break;
> +#endif
> +#if defined(CONFIG_ARCH_TEGRA_241_SOC)
> +	case TEGRA241:
> +		fuse->soc = &tegra241_fuse_soc;
> +		break;
>  #endif

Have you tried --patience when formatting patches?
Does it help them to look better?

...

> +const struct tegra_fuse_soc tegra241_fuse_soc = {
> +	.init = tegra30_fuse_init,
> +	.info = &tegra241_fuse_info,

> +	.lookups = NULL,
> +	.num_lookups = 0,
> +	.cells = NULL,
> +	.num_cells = 0,

Isn't it the default?

> +	.keepouts = tegra241_fuse_keepouts,
> +	.num_keepouts = ARRAY_SIZE(tegra241_fuse_keepouts),
> +	.soc_attr_group = &tegra194_soc_attr_group,

> +	.clk_suspend_on = false,

Ditto.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] soc/tegra: fuse: Add support for Tegra241

[Kartik]

On Fri, 2023-08-18 at 18:10 +0300, Andy Shevchenko wrote:
>On Fri, Aug 18, 2023 at 03:00:28PM +0530, Kartik wrote:
>> Add support for Tegra241 which use ACPI boot.
>
>...
>
>>  	case TEGRA234:
>>  		fuse->soc = &tegra234_fuse_soc;
>>  		break;
>> +#endif
>> +#if defined(CONFIG_ARCH_TEGRA_241_SOC)
>> +	case TEGRA241:
>> +		fuse->soc = &tegra241_fuse_soc;
>> +		break;
>>  #endif
>
>Have you tried --patience when formatting patches?
>Does it help them to look better?
>

No, I did not use --patience flag while formatting the patches.
Shall I post the next patch set using it?

>...
>
>> +const struct tegra_fuse_soc tegra241_fuse_soc = {
>> +	.init = tegra30_fuse_init,
>> +	.info = &tegra241_fuse_info,
>
>> +	.lookups = NULL,
>> +	.num_lookups = 0,
>> +	.cells = NULL,
>> +	.num_cells = 0,
>
>Isn't it the default?
>
>> +	.keepouts = tegra241_fuse_keepouts,
>> +	.num_keepouts = ARRAY_SIZE(tegra241_fuse_keepouts),
>> +	.soc_attr_group = &tegra194_soc_attr_group,
>
>> +	.clk_suspend_on = false,
>
>Ditto.
>

Yes, I will trim this list in the next patch.

>> +};

Regards,
Kartik

Re: [PATCH 6/6] soc/tegra: fuse: Add support for Tegra241

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 05:10:33PM +0530, Kartik wrote:
> On Fri, 2023-08-18 at 18:10 +0300, Andy Shevchenko wrote:
> >On Fri, Aug 18, 2023 at 03:00:28PM +0530, Kartik wrote:

...

> >>  	case TEGRA234:
> >>  		fuse->soc = &tegra234_fuse_soc;
> >>  		break;
> >> +#endif
> >> +#if defined(CONFIG_ARCH_TEGRA_241_SOC)
> >> +	case TEGRA241:
> >> +		fuse->soc = &tegra241_fuse_soc;
> >> +		break;
> >>  #endif
> >
> >Have you tried --patience when formatting patches?
> >Does it help them to look better?
> 
> No, I did not use --patience flag while formatting the patches.
> Shall I post the next patch set using it?

Do you see the difference in the output? If yes, definitely use it.
Otherwise it's recommended to use anyway.

-- 
With Best Regards,
Andy Shevchenko