[PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Rob Herring (Arm)]

The PPC64 specific MMIO setup open codes DT address functions rather
than using standard address parsing functions. The open-coded version
fails to handle any address translation and is not endian safe.

I haven't found any evidence of what platform used this. The only thing
that turned up was a PPC405 platform, but that is 32-bit and PPC405
support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
any powerpc config and never was. The support was added in 2005 and
hasn't been touched since.

Rather than try to modernize and fix this code, just remove it.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/char/tpm/Kconfig     |   2 +-
 drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
 drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
 3 files changed, 62 insertions(+), 144 deletions(-)
 delete mode 100644 drivers/char/tpm/tpm_atmel.h

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index e63a6a17793c..9b655e9fc7ab 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -162,7 +162,7 @@ config TCG_NSC
 
 config TCG_ATMEL
 	tristate "Atmel TPM Interface"
-	depends on PPC64 || HAS_IOPORT_MAP
+	depends on HAS_IOPORT_MAP
 	depends on HAS_IOPORT
 	help
 	  If you have a TPM security chip from Atmel say Yes and it 
diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
index 9fb2defa9dc4..622c4abe8cb3 100644
--- a/drivers/char/tpm/tpm_atmel.c
+++ b/drivers/char/tpm/tpm_atmel.c
@@ -15,7 +15,67 @@
  */
 
 #include "tpm.h"
-#include "tpm_atmel.h"
+
+struct tpm_atmel_priv {
+	int region_size;
+	int have_region;
+	unsigned long base;
+	void __iomem *iobase;
+};
+
+#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
+#define atmel_putb(val, chip, offset) \
+	outb(val, atmel_get_priv(chip)->base + offset)
+#define atmel_request_region request_region
+#define atmel_release_region release_region
+/* Atmel definitions */
+enum tpm_atmel_addr {
+	TPM_ATMEL_BASE_ADDR_LO = 0x08,
+	TPM_ATMEL_BASE_ADDR_HI = 0x09
+};
+
+static inline int tpm_read_index(int base, int index)
+{
+	outb(index, base);
+	return inb(base+1) & 0xFF;
+}
+
+/* Verify this is a 1.1 Atmel TPM */
+static int atmel_verify_tpm11(void)
+{
+
+	/* verify that it is an Atmel part */
+	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
+	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
+	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
+	    tpm_read_index(TPM_ADDR, 7) != 'L')
+		return 1;
+
+	/* query chip for its version number */
+	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
+	    tpm_read_index(TPM_ADDR, 0x01) != 1)
+		return 1;
+
+	/* This is an atmel supported part */
+	return 0;
+}
+
+/* Determine where to talk to device */
+static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
+{
+	int lo, hi;
+
+	if (atmel_verify_tpm11() != 0)
+		return NULL;
+
+	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
+	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
+
+	*base = (hi << 8) | lo;
+	*region_size = 2;
+
+	return ioport_map(*base, *region_size);
+}
 
 /* write status bits */
 enum tpm_atmel_write_status {
@@ -142,7 +202,6 @@ static void atml_plat_remove(void)
 	tpm_chip_unregister(chip);
 	if (priv->have_region)
 		atmel_release_region(priv->base, priv->region_size);
-	atmel_put_base_addr(priv->iobase);
 	platform_device_unregister(pdev);
 }
 
@@ -211,7 +270,6 @@ static int __init init_atmel(void)
 err_unreg_dev:
 	platform_device_unregister(pdev);
 err_rel_reg:
-	atmel_put_base_addr(iobase);
 	if (have_region)
 		atmel_release_region(base,
 				     region_size);
diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
deleted file mode 100644
index 7ac3f69dcf0f..000000000000
--- a/drivers/char/tpm/tpm_atmel.h
+++ /dev/null
@@ -1,140 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright (C) 2005 IBM Corporation
- *
- * Authors:
- * Kylene Hall <kjhall@us.ibm.com>
- *
- * Maintained by: <tpmdd-devel@lists.sourceforge.net>
- *
- * Device driver for TCG/TCPA TPM (trusted platform module).
- * Specifications at www.trustedcomputinggroup.org
- *
- * These difference are required on power because the device must be
- * discovered through the device tree and iomap must be used to get
- * around the need for holes in the io_page_mask.  This does not happen
- * automatically because the tpm is not a normal pci device and lives
- * under the root node.
- */
-
-struct tpm_atmel_priv {
-	int region_size;
-	int have_region;
-	unsigned long base;
-	void __iomem *iobase;
-};
-
-#ifdef CONFIG_PPC64
-
-#include <linux/of.h>
-
-#define atmel_getb(priv, offset) readb(priv->iobase + offset)
-#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
-#define atmel_request_region request_mem_region
-#define atmel_release_region release_mem_region
-
-static inline void atmel_put_base_addr(void __iomem *iobase)
-{
-	iounmap(iobase);
-}
-
-static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
-{
-	struct device_node *dn;
-	unsigned long address, size;
-	const unsigned int *reg;
-	int reglen;
-	int naddrc;
-	int nsizec;
-
-	dn = of_find_node_by_name(NULL, "tpm");
-
-	if (!dn)
-		return NULL;
-
-	if (!of_device_is_compatible(dn, "AT97SC3201")) {
-		of_node_put(dn);
-		return NULL;
-	}
-
-	reg = of_get_property(dn, "reg", &reglen);
-	naddrc = of_n_addr_cells(dn);
-	nsizec = of_n_size_cells(dn);
-
-	of_node_put(dn);
-
-
-	if (naddrc == 2)
-		address = ((unsigned long) reg[0] << 32) | reg[1];
-	else
-		address = reg[0];
-
-	if (nsizec == 2)
-		size =
-		    ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
-	else
-		size = reg[naddrc];
-
-	*base = address;
-	*region_size = size;
-	return ioremap(*base, *region_size);
-}
-#else
-#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
-#define atmel_putb(val, chip, offset) \
-	outb(val, atmel_get_priv(chip)->base + offset)
-#define atmel_request_region request_region
-#define atmel_release_region release_region
-/* Atmel definitions */
-enum tpm_atmel_addr {
-	TPM_ATMEL_BASE_ADDR_LO = 0x08,
-	TPM_ATMEL_BASE_ADDR_HI = 0x09
-};
-
-static inline int tpm_read_index(int base, int index)
-{
-	outb(index, base);
-	return inb(base+1) & 0xFF;
-}
-
-/* Verify this is a 1.1 Atmel TPM */
-static int atmel_verify_tpm11(void)
-{
-
-	/* verify that it is an Atmel part */
-	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
-	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
-	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
-	    tpm_read_index(TPM_ADDR, 7) != 'L')
-		return 1;
-
-	/* query chip for its version number */
-	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
-	    tpm_read_index(TPM_ADDR, 0x01) != 1)
-		return 1;
-
-	/* This is an atmel supported part */
-	return 0;
-}
-
-static inline void atmel_put_base_addr(void __iomem *iobase)
-{
-}
-
-/* Determine where to talk to device */
-static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
-{
-	int lo, hi;
-
-	if (atmel_verify_tpm11() != 0)
-		return NULL;
-
-	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
-	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
-
-	*base = (hi << 8) | lo;
-	*region_size = 2;
-
-	return ioport_map(*base, *region_size);
-}
-#endif
-- 
2.43.0

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Tue, 2024-07-02 at 10:10 -0600, Rob Herring (Arm) wrote:
> The PPC64 specific MMIO setup open codes DT address functions rather
> than using standard address parsing functions. The open-coded version
> fails to handle any address translation and is not endian safe.
> 
> I haven't found any evidence of what platform used this. The only thing
> that turned up was a PPC405 platform, but that is 32-bit and PPC405
> support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> any powerpc config and never was. The support was added in 2005 and
> hasn't been touched since.
> 
> Rather than try to modernize and fix this code, just remove it.
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/char/tpm/Kconfig     |   2 +-
>  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
>  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
>  3 files changed, 62 insertions(+), 144 deletions(-)
>  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index e63a6a17793c..9b655e9fc7ab 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -162,7 +162,7 @@ config TCG_NSC
>  
>  config TCG_ATMEL
>  	tristate "Atmel TPM Interface"
> -	depends on PPC64 || HAS_IOPORT_MAP
> +	depends on HAS_IOPORT_MAP
>  	depends on HAS_IOPORT
>  	help
>  	  If you have a TPM security chip from Atmel say Yes and it 
> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 9fb2defa9dc4..622c4abe8cb3 100644
> --- a/drivers/char/tpm/tpm_atmel.c
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -15,7 +15,67 @@
>   */
>  
>  #include "tpm.h"
> -#include "tpm_atmel.h"
> +
> +struct tpm_atmel_priv {
> +	int region_size;
> +	int have_region;
> +	unsigned long base;
> +	void __iomem *iobase;
> +};
> +
> +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> +#define atmel_putb(val, chip, offset) \
> +	outb(val, atmel_get_priv(chip)->base + offset)
> +#define atmel_request_region request_region
> +#define atmel_release_region release_region
> +/* Atmel definitions */
> +enum tpm_atmel_addr {
> +	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> +	TPM_ATMEL_BASE_ADDR_HI = 0x09
> +};
> +
> +static inline int tpm_read_index(int base, int index)
> +{
> +	outb(index, base);
> +	return inb(base+1) & 0xFF;
> +}
> +
> +/* Verify this is a 1.1 Atmel TPM */
> +static int atmel_verify_tpm11(void)
> +{
> +
> +	/* verify that it is an Atmel part */
> +	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> +	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> +	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> +	    tpm_read_index(TPM_ADDR, 7) != 'L')
> +		return 1;
> +
> +	/* query chip for its version number */
> +	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> +	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> +		return 1;
> +
> +	/* This is an atmel supported part */
> +	return 0;
> +}
> +
> +/* Determine where to talk to device */
> +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> +{
> +	int lo, hi;
> +
> +	if (atmel_verify_tpm11() != 0)
> +		return NULL;
> +
> +	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> +	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> +
> +	*base = (hi << 8) | lo;
> +	*region_size = 2;
> +
> +	return ioport_map(*base, *region_size);
> +}
>  
>  /* write status bits */
>  enum tpm_atmel_write_status {
> @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
>  	tpm_chip_unregister(chip);
>  	if (priv->have_region)
>  		atmel_release_region(priv->base, priv->region_size);
> -	atmel_put_base_addr(priv->iobase);
>  	platform_device_unregister(pdev);
>  }
>  
> @@ -211,7 +270,6 @@ static int __init init_atmel(void)
>  err_unreg_dev:
>  	platform_device_unregister(pdev);
>  err_rel_reg:
> -	atmel_put_base_addr(iobase);
>  	if (have_region)
>  		atmel_release_region(base,
>  				     region_size);
> diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> deleted file mode 100644
> index 7ac3f69dcf0f..000000000000
> --- a/drivers/char/tpm/tpm_atmel.h
> +++ /dev/null
> @@ -1,140 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) 2005 IBM Corporation
> - *
> - * Authors:
> - * Kylene Hall <kjhall@us.ibm.com>
> - *
> - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> - *
> - * Device driver for TCG/TCPA TPM (trusted platform module).
> - * Specifications at www.trustedcomputinggroup.org
> - *
> - * These difference are required on power because the device must be
> - * discovered through the device tree and iomap must be used to get
> - * around the need for holes in the io_page_mask.  This does not happen
> - * automatically because the tpm is not a normal pci device and lives
> - * under the root node.
> - */
> -
> -struct tpm_atmel_priv {
> -	int region_size;
> -	int have_region;
> -	unsigned long base;
> -	void __iomem *iobase;
> -};
> -
> -#ifdef CONFIG_PPC64
> -
> -#include <linux/of.h>
> -
> -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> -#define atmel_request_region request_mem_region
> -#define atmel_release_region release_mem_region
> -
> -static inline void atmel_put_base_addr(void __iomem *iobase)
> -{
> -	iounmap(iobase);
> -}
> -
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> -{
> -	struct device_node *dn;
> -	unsigned long address, size;
> -	const unsigned int *reg;
> -	int reglen;
> -	int naddrc;
> -	int nsizec;
> -
> -	dn = of_find_node_by_name(NULL, "tpm");
> -
> -	if (!dn)
> -		return NULL;
> -
> -	if (!of_device_is_compatible(dn, "AT97SC3201")) {
> -		of_node_put(dn);
> -		return NULL;
> -	}
> -
> -	reg = of_get_property(dn, "reg", &reglen);
> -	naddrc = of_n_addr_cells(dn);
> -	nsizec = of_n_size_cells(dn);
> -
> -	of_node_put(dn);
> -
> -
> -	if (naddrc == 2)
> -		address = ((unsigned long) reg[0] << 32) | reg[1];
> -	else
> -		address = reg[0];
> -
> -	if (nsizec == 2)
> -		size =
> -		    ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> -	else
> -		size = reg[naddrc];
> -
> -	*base = address;
> -	*region_size = size;
> -	return ioremap(*base, *region_size);
> -}
> -#else
> -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> -#define atmel_putb(val, chip, offset) \
> -	outb(val, atmel_get_priv(chip)->base + offset)
> -#define atmel_request_region request_region
> -#define atmel_release_region release_region
> -/* Atmel definitions */
> -enum tpm_atmel_addr {
> -	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> -	TPM_ATMEL_BASE_ADDR_HI = 0x09
> -};
> -
> -static inline int tpm_read_index(int base, int index)
> -{
> -	outb(index, base);
> -	return inb(base+1) & 0xFF;
> -}
> -
> -/* Verify this is a 1.1 Atmel TPM */
> -static int atmel_verify_tpm11(void)
> -{
> -
> -	/* verify that it is an Atmel part */
> -	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> -	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> -	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> -	    tpm_read_index(TPM_ADDR, 7) != 'L')
> -		return 1;
> -
> -	/* query chip for its version number */
> -	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> -	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> -		return 1;
> -
> -	/* This is an atmel supported part */
> -	return 0;
> -}
> -
> -static inline void atmel_put_base_addr(void __iomem *iobase)
> -{
> -}
> -
> -/* Determine where to talk to device */
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> -{
> -	int lo, hi;
> -
> -	if (atmel_verify_tpm11() != 0)
> -		return NULL;
> -
> -	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> -	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> -
> -	*base = (hi << 8) | lo;
> -	*region_size = 2;
> -
> -	return ioport_map(*base, *region_size);
> -}
> -#endif


Acknowledging that I noticed this but won't test
before week 31, when I'm back from holiday.

BR, Jarkko

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Michael Ellerman]

"Rob Herring (Arm)" <robh@kernel.org> writes:
> The PPC64 specific MMIO setup open codes DT address functions rather
> than using standard address parsing functions. The open-coded version
> fails to handle any address translation and is not endian safe.
>
> I haven't found any evidence of what platform used this. The only thing
> that turned up was a PPC405 platform, but that is 32-bit and PPC405
> support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> any powerpc config and never was. The support was added in 2005 and
> hasn't been touched since.

I found a post on the tpm list which says it was used in JS21, which
would make sense given the time frame:

  https://lore.kernel.org/all/526EA6FF.2080401@linux.vnet.ibm.com/

  As near as I can tell this was on a single machine type (the js21 circa
  2006).  The firmware on the machine didn't support establishing a root
  of trust, so use of the TPM was as a practical matter effective only for
  the other functions like random number generation and key management.
  The number of users who used the TPM for this on this machine was likely
  very small 7 years ago.  The number of those machines still in service
  today is likely smaller still.  The cross section of those two small
  numbers combined with those who want to run on a shiny new kernel has to
  be quickly approaching zero.
  
The SLOF (firmware) code does confirm that, and includes the same
compatible value as below, so I think that's pretty definitive:

  https://github.com/qemu/SLOF/blob/master/board-js2x/slof/tpm.fs

  \ Atmel TPM.
  
  new-device   500 1 set-unit
  s" tpm" 2dup device-name device-type
  s" AT97SC3201" compatible
  
I used to have a JS21, but it got scrapped in a cleanup a few years
back. I strongly doubt any others still exist, I couldn't find any on
ebay :)

> Rather than try to modernize and fix this code, just remove it.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index e63a6a17793c..9b655e9fc7ab 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -162,7 +162,7 @@ config TCG_NSC
>  
>  config TCG_ATMEL
>  	tristate "Atmel TPM Interface"
> -	depends on PPC64 || HAS_IOPORT_MAP
> +	depends on HAS_IOPORT_MAP
>  	depends on HAS_IOPORT
>  	help
>  	  If you have a TPM security chip from Atmel say Yes and it 
> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 9fb2defa9dc4..622c4abe8cb3 100644
> --- a/drivers/char/tpm/tpm_atmel.c
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -15,7 +15,67 @@
>   */
>  
>  #include "tpm.h"
> -#include "tpm_atmel.h"
> +
> +struct tpm_atmel_priv {
> +	int region_size;
> +	int have_region;
> +	unsigned long base;
> +	void __iomem *iobase;
> +};
> +
> +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> +#define atmel_putb(val, chip, offset) \
> +	outb(val, atmel_get_priv(chip)->base + offset)
> +#define atmel_request_region request_region
> +#define atmel_release_region release_region
> +/* Atmel definitions */
> +enum tpm_atmel_addr {
> +	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> +	TPM_ATMEL_BASE_ADDR_HI = 0x09
> +};
> +
> +static inline int tpm_read_index(int base, int index)
> +{
> +	outb(index, base);
> +	return inb(base+1) & 0xFF;
> +}
> +
> +/* Verify this is a 1.1 Atmel TPM */
> +static int atmel_verify_tpm11(void)
> +{
> +
> +	/* verify that it is an Atmel part */
> +	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> +	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> +	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> +	    tpm_read_index(TPM_ADDR, 7) != 'L')
> +		return 1;
> +
> +	/* query chip for its version number */
> +	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> +	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> +		return 1;
> +
> +	/* This is an atmel supported part */
> +	return 0;
> +}
> +
> +/* Determine where to talk to device */
> +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> +{
> +	int lo, hi;
> +
> +	if (atmel_verify_tpm11() != 0)
> +		return NULL;
> +
> +	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> +	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> +
> +	*base = (hi << 8) | lo;
> +	*region_size = 2;
> +
> +	return ioport_map(*base, *region_size);
> +}
>  
>  /* write status bits */
>  enum tpm_atmel_write_status {
> @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
>  	tpm_chip_unregister(chip);
>  	if (priv->have_region)
>  		atmel_release_region(priv->base, priv->region_size);
> -	atmel_put_base_addr(priv->iobase);
>  	platform_device_unregister(pdev);
>  }
>  
> @@ -211,7 +270,6 @@ static int __init init_atmel(void)
>  err_unreg_dev:
>  	platform_device_unregister(pdev);
>  err_rel_reg:
> -	atmel_put_base_addr(iobase);
>  	if (have_region)
>  		atmel_release_region(base,
>  				     region_size);
> diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> deleted file mode 100644
> index 7ac3f69dcf0f..000000000000
> --- a/drivers/char/tpm/tpm_atmel.h
> +++ /dev/null
> @@ -1,140 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) 2005 IBM Corporation
> - *
> - * Authors:
> - * Kylene Hall <kjhall@us.ibm.com>
> - *
> - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> - *
> - * Device driver for TCG/TCPA TPM (trusted platform module).
> - * Specifications at www.trustedcomputinggroup.org
> - *
> - * These difference are required on power because the device must be
> - * discovered through the device tree and iomap must be used to get
> - * around the need for holes in the io_page_mask.  This does not happen
> - * automatically because the tpm is not a normal pci device and lives
> - * under the root node.
> - */
> -
> -struct tpm_atmel_priv {
> -	int region_size;
> -	int have_region;
> -	unsigned long base;
> -	void __iomem *iobase;
> -};
> -
> -#ifdef CONFIG_PPC64
> -
> -#include <linux/of.h>
> -
> -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> -#define atmel_request_region request_mem_region
> -#define atmel_release_region release_mem_region
> -
> -static inline void atmel_put_base_addr(void __iomem *iobase)
> -{
> -	iounmap(iobase);
> -}
> -
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> -{
> -	struct device_node *dn;
> -	unsigned long address, size;
> -	const unsigned int *reg;
> -	int reglen;
> -	int naddrc;
> -	int nsizec;
> -
> -	dn = of_find_node_by_name(NULL, "tpm");
> -
> -	if (!dn)
> -		return NULL;
> -
> -	if (!of_device_is_compatible(dn, "AT97SC3201")) {
> -		of_node_put(dn);
> -		return NULL;
> -	}
> -
> -	reg = of_get_property(dn, "reg", &reglen);
> -	naddrc = of_n_addr_cells(dn);
> -	nsizec = of_n_size_cells(dn);
> -
> -	of_node_put(dn);
> -
> -
> -	if (naddrc == 2)
> -		address = ((unsigned long) reg[0] << 32) | reg[1];
> -	else
> -		address = reg[0];
> -
> -	if (nsizec == 2)
> -		size =
> -		    ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> -	else
> -		size = reg[naddrc];
> -
> -	*base = address;
> -	*region_size = size;
> -	return ioremap(*base, *region_size);
> -}
> -#else
> -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> -#define atmel_putb(val, chip, offset) \
> -	outb(val, atmel_get_priv(chip)->base + offset)
> -#define atmel_request_region request_region
> -#define atmel_release_region release_region
> -/* Atmel definitions */
> -enum tpm_atmel_addr {
> -	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> -	TPM_ATMEL_BASE_ADDR_HI = 0x09
> -};
> -
> -static inline int tpm_read_index(int base, int index)
> -{
> -	outb(index, base);
> -	return inb(base+1) & 0xFF;
> -}
> -
> -/* Verify this is a 1.1 Atmel TPM */
> -static int atmel_verify_tpm11(void)
> -{
> -
> -	/* verify that it is an Atmel part */
> -	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> -	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> -	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> -	    tpm_read_index(TPM_ADDR, 7) != 'L')
> -		return 1;
> -
> -	/* query chip for its version number */
> -	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> -	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> -		return 1;
> -
> -	/* This is an atmel supported part */
> -	return 0;
> -}
> -
> -static inline void atmel_put_base_addr(void __iomem *iobase)
> -{
> -}
> -
> -/* Determine where to talk to device */
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> -{
> -	int lo, hi;
> -
> -	if (atmel_verify_tpm11() != 0)
> -		return NULL;
> -
> -	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> -	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> -
> -	*base = (hi << 8) | lo;
> -	*region_size = 2;
> -
> -	return ioport_map(*base, *region_size);
> -}
> -#endif
> -- 
> 2.43.0

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> The PPC64 specific MMIO setup open codes DT address functions rather
> than using standard address parsing functions. The open-coded version
> fails to handle any address translation and is not endian safe.
>
> I haven't found any evidence of what platform used this. The only thing
> that turned up was a PPC405 platform, but that is 32-bit and PPC405
> support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> any powerpc config and never was. The support was added in 2005 and
> hasn't been touched since.
>
> Rather than try to modernize and fix this code, just remove it.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  drivers/char/tpm/Kconfig     |   2 +-
>  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
>  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
>  3 files changed, 62 insertions(+), 144 deletions(-)
>  delete mode 100644 drivers/char/tpm/tpm_atmel.h
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index e63a6a17793c..9b655e9fc7ab 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -162,7 +162,7 @@ config TCG_NSC
>  
>  config TCG_ATMEL
>  	tristate "Atmel TPM Interface"
> -	depends on PPC64 || HAS_IOPORT_MAP
> +	depends on HAS_IOPORT_MAP
>  	depends on HAS_IOPORT
>  	help
>  	  If you have a TPM security chip from Atmel say Yes and it 
> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 9fb2defa9dc4..622c4abe8cb3 100644
> --- a/drivers/char/tpm/tpm_atmel.c
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -15,7 +15,67 @@
>   */
>  
>  #include "tpm.h"
> -#include "tpm_atmel.h"
> +
> +struct tpm_atmel_priv {
> +	int region_size;
> +	int have_region;
> +	unsigned long base;
> +	void __iomem *iobase;
> +};
> +
> +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> +#define atmel_putb(val, chip, offset) \
> +	outb(val, atmel_get_priv(chip)->base + offset)
> +#define atmel_request_region request_region
> +#define atmel_release_region release_region
> +/* Atmel definitions */
> +enum tpm_atmel_addr {
> +	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> +	TPM_ATMEL_BASE_ADDR_HI = 0x09
> +};
> +
> +static inline int tpm_read_index(int base, int index)
> +{
> +	outb(index, base);
> +	return inb(base+1) & 0xFF;
> +}
> +
> +/* Verify this is a 1.1 Atmel TPM */
> +static int atmel_verify_tpm11(void)
> +{
> +
> +	/* verify that it is an Atmel part */
> +	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> +	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> +	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> +	    tpm_read_index(TPM_ADDR, 7) != 'L')
> +		return 1;
> +
> +	/* query chip for its version number */
> +	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> +	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> +		return 1;
> +
> +	/* This is an atmel supported part */
> +	return 0;
> +}
> +
> +/* Determine where to talk to device */
> +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> +{
> +	int lo, hi;
> +
> +	if (atmel_verify_tpm11() != 0)
> +		return NULL;
> +
> +	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> +	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> +
> +	*base = (hi << 8) | lo;
> +	*region_size = 2;
> +
> +	return ioport_map(*base, *region_size);
> +}
>  
>  /* write status bits */
>  enum tpm_atmel_write_status {
> @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
>  	tpm_chip_unregister(chip);
>  	if (priv->have_region)
>  		atmel_release_region(priv->base, priv->region_size);
> -	atmel_put_base_addr(priv->iobase);
>  	platform_device_unregister(pdev);
>  }
>  
> @@ -211,7 +270,6 @@ static int __init init_atmel(void)
>  err_unreg_dev:
>  	platform_device_unregister(pdev);
>  err_rel_reg:
> -	atmel_put_base_addr(iobase);
>  	if (have_region)
>  		atmel_release_region(base,
>  				     region_size);
> diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> deleted file mode 100644
> index 7ac3f69dcf0f..000000000000
> --- a/drivers/char/tpm/tpm_atmel.h
> +++ /dev/null
> @@ -1,140 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * Copyright (C) 2005 IBM Corporation
> - *
> - * Authors:
> - * Kylene Hall <kjhall@us.ibm.com>
> - *
> - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> - *
> - * Device driver for TCG/TCPA TPM (trusted platform module).
> - * Specifications at www.trustedcomputinggroup.org
> - *
> - * These difference are required on power because the device must be
> - * discovered through the device tree and iomap must be used to get
> - * around the need for holes in the io_page_mask.  This does not happen
> - * automatically because the tpm is not a normal pci device and lives
> - * under the root node.
> - */
> -
> -struct tpm_atmel_priv {
> -	int region_size;
> -	int have_region;
> -	unsigned long base;
> -	void __iomem *iobase;
> -};
> -
> -#ifdef CONFIG_PPC64
> -
> -#include <linux/of.h>
> -
> -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> -#define atmel_request_region request_mem_region
> -#define atmel_release_region release_mem_region
> -
> -static inline void atmel_put_base_addr(void __iomem *iobase)
> -{
> -	iounmap(iobase);
> -}
> -
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> -{
> -	struct device_node *dn;
> -	unsigned long address, size;
> -	const unsigned int *reg;
> -	int reglen;
> -	int naddrc;
> -	int nsizec;
> -
> -	dn = of_find_node_by_name(NULL, "tpm");
> -
> -	if (!dn)
> -		return NULL;
> -
> -	if (!of_device_is_compatible(dn, "AT97SC3201")) {
> -		of_node_put(dn);
> -		return NULL;
> -	}
> -
> -	reg = of_get_property(dn, "reg", &reglen);
> -	naddrc = of_n_addr_cells(dn);
> -	nsizec = of_n_size_cells(dn);
> -
> -	of_node_put(dn);
> -
> -
> -	if (naddrc == 2)
> -		address = ((unsigned long) reg[0] << 32) | reg[1];
> -	else
> -		address = reg[0];
> -
> -	if (nsizec == 2)
> -		size =
> -		    ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> -	else
> -		size = reg[naddrc];
> -
> -	*base = address;
> -	*region_size = size;
> -	return ioremap(*base, *region_size);
> -}
> -#else
> -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> -#define atmel_putb(val, chip, offset) \
> -	outb(val, atmel_get_priv(chip)->base + offset)
> -#define atmel_request_region request_region
> -#define atmel_release_region release_region
> -/* Atmel definitions */
> -enum tpm_atmel_addr {
> -	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> -	TPM_ATMEL_BASE_ADDR_HI = 0x09
> -};
> -
> -static inline int tpm_read_index(int base, int index)
> -{
> -	outb(index, base);
> -	return inb(base+1) & 0xFF;
> -}
> -
> -/* Verify this is a 1.1 Atmel TPM */
> -static int atmel_verify_tpm11(void)
> -{
> -
> -	/* verify that it is an Atmel part */
> -	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> -	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> -	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> -	    tpm_read_index(TPM_ADDR, 7) != 'L')
> -		return 1;
> -
> -	/* query chip for its version number */
> -	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> -	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> -		return 1;
> -
> -	/* This is an atmel supported part */
> -	return 0;
> -}
> -
> -static inline void atmel_put_base_addr(void __iomem *iobase)
> -{
> -}
> -
> -/* Determine where to talk to device */
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> -{
> -	int lo, hi;
> -
> -	if (atmel_verify_tpm11() != 0)
> -		return NULL;
> -
> -	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> -	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> -
> -	*base = (hi << 8) | lo;
> -	*region_size = 2;
> -
> -	return ioport_map(*base, *region_size);
> -}
> -#endif

Responding from holidays but:

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

[still away for couple of weeks]

BR, Jarkko

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Wed Jul 17, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> > The PPC64 specific MMIO setup open codes DT address functions rather
> > than using standard address parsing functions. The open-coded version
> > fails to handle any address translation and is not endian safe.
> >
> > I haven't found any evidence of what platform used this. The only thing
> > that turned up was a PPC405 platform, but that is 32-bit and PPC405
> > support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> > any powerpc config and never was. The support was added in 2005 and
> > hasn't been touched since.
> >
> > Rather than try to modernize and fix this code, just remove it.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  drivers/char/tpm/Kconfig     |   2 +-
> >  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
> >  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
> >  3 files changed, 62 insertions(+), 144 deletions(-)
> >  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> >
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index e63a6a17793c..9b655e9fc7ab 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -162,7 +162,7 @@ config TCG_NSC
> >  
> >  config TCG_ATMEL
> >  	tristate "Atmel TPM Interface"
> > -	depends on PPC64 || HAS_IOPORT_MAP
> > +	depends on HAS_IOPORT_MAP
> >  	depends on HAS_IOPORT
> >  	help
> >  	  If you have a TPM security chip from Atmel say Yes and it 
> > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > index 9fb2defa9dc4..622c4abe8cb3 100644
> > --- a/drivers/char/tpm/tpm_atmel.c
> > +++ b/drivers/char/tpm/tpm_atmel.c
> > @@ -15,7 +15,67 @@
> >   */
> >  
> >  #include "tpm.h"
> > -#include "tpm_atmel.h"
> > +
> > +struct tpm_atmel_priv {
> > +	int region_size;
> > +	int have_region;
> > +	unsigned long base;
> > +	void __iomem *iobase;
> > +};
> > +
> > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > +#define atmel_putb(val, chip, offset) \
> > +	outb(val, atmel_get_priv(chip)->base + offset)
> > +#define atmel_request_region request_region
> > +#define atmel_release_region release_region
> > +/* Atmel definitions */
> > +enum tpm_atmel_addr {
> > +	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > +	TPM_ATMEL_BASE_ADDR_HI = 0x09
> > +};
> > +
> > +static inline int tpm_read_index(int base, int index)
> > +{
> > +	outb(index, base);
> > +	return inb(base+1) & 0xFF;
> > +}
> > +
> > +/* Verify this is a 1.1 Atmel TPM */
> > +static int atmel_verify_tpm11(void)
> > +{
> > +
> > +	/* verify that it is an Atmel part */
> > +	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > +	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > +	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > +	    tpm_read_index(TPM_ADDR, 7) != 'L')
> > +		return 1;
> > +
> > +	/* query chip for its version number */
> > +	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > +	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> > +		return 1;
> > +
> > +	/* This is an atmel supported part */
> > +	return 0;
> > +}
> > +
> > +/* Determine where to talk to device */
> > +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > +{
> > +	int lo, hi;
> > +
> > +	if (atmel_verify_tpm11() != 0)
> > +		return NULL;
> > +
> > +	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > +	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > +
> > +	*base = (hi << 8) | lo;
> > +	*region_size = 2;
> > +
> > +	return ioport_map(*base, *region_size);
> > +}
> >  
> >  /* write status bits */
> >  enum tpm_atmel_write_status {
> > @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
> >  	tpm_chip_unregister(chip);
> >  	if (priv->have_region)
> >  		atmel_release_region(priv->base, priv->region_size);
> > -	atmel_put_base_addr(priv->iobase);
> >  	platform_device_unregister(pdev);
> >  }
> >  
> > @@ -211,7 +270,6 @@ static int __init init_atmel(void)
> >  err_unreg_dev:
> >  	platform_device_unregister(pdev);
> >  err_rel_reg:
> > -	atmel_put_base_addr(iobase);
> >  	if (have_region)
> >  		atmel_release_region(base,
> >  				     region_size);
> > diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> > deleted file mode 100644
> > index 7ac3f69dcf0f..000000000000
> > --- a/drivers/char/tpm/tpm_atmel.h
> > +++ /dev/null
> > @@ -1,140 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -/*
> > - * Copyright (C) 2005 IBM Corporation
> > - *
> > - * Authors:
> > - * Kylene Hall <kjhall@us.ibm.com>
> > - *
> > - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> > - *
> > - * Device driver for TCG/TCPA TPM (trusted platform module).
> > - * Specifications at www.trustedcomputinggroup.org
> > - *
> > - * These difference are required on power because the device must be
> > - * discovered through the device tree and iomap must be used to get
> > - * around the need for holes in the io_page_mask.  This does not happen
> > - * automatically because the tpm is not a normal pci device and lives
> > - * under the root node.
> > - */
> > -
> > -struct tpm_atmel_priv {
> > -	int region_size;
> > -	int have_region;
> > -	unsigned long base;
> > -	void __iomem *iobase;
> > -};
> > -
> > -#ifdef CONFIG_PPC64
> > -
> > -#include <linux/of.h>
> > -
> > -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> > -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> > -#define atmel_request_region request_mem_region
> > -#define atmel_release_region release_mem_region
> > -
> > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > -{
> > -	iounmap(iobase);
> > -}
> > -
> > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > -{
> > -	struct device_node *dn;
> > -	unsigned long address, size;
> > -	const unsigned int *reg;
> > -	int reglen;
> > -	int naddrc;
> > -	int nsizec;
> > -
> > -	dn = of_find_node_by_name(NULL, "tpm");
> > -
> > -	if (!dn)
> > -		return NULL;
> > -
> > -	if (!of_device_is_compatible(dn, "AT97SC3201")) {
> > -		of_node_put(dn);
> > -		return NULL;
> > -	}
> > -
> > -	reg = of_get_property(dn, "reg", &reglen);
> > -	naddrc = of_n_addr_cells(dn);
> > -	nsizec = of_n_size_cells(dn);
> > -
> > -	of_node_put(dn);
> > -
> > -
> > -	if (naddrc == 2)
> > -		address = ((unsigned long) reg[0] << 32) | reg[1];
> > -	else
> > -		address = reg[0];
> > -
> > -	if (nsizec == 2)
> > -		size =
> > -		    ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> > -	else
> > -		size = reg[naddrc];
> > -
> > -	*base = address;
> > -	*region_size = size;
> > -	return ioremap(*base, *region_size);
> > -}
> > -#else
> > -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > -#define atmel_putb(val, chip, offset) \
> > -	outb(val, atmel_get_priv(chip)->base + offset)
> > -#define atmel_request_region request_region
> > -#define atmel_release_region release_region
> > -/* Atmel definitions */
> > -enum tpm_atmel_addr {
> > -	TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > -	TPM_ATMEL_BASE_ADDR_HI = 0x09
> > -};
> > -
> > -static inline int tpm_read_index(int base, int index)
> > -{
> > -	outb(index, base);
> > -	return inb(base+1) & 0xFF;
> > -}
> > -
> > -/* Verify this is a 1.1 Atmel TPM */
> > -static int atmel_verify_tpm11(void)
> > -{
> > -
> > -	/* verify that it is an Atmel part */
> > -	if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > -	    tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > -	    tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > -	    tpm_read_index(TPM_ADDR, 7) != 'L')
> > -		return 1;
> > -
> > -	/* query chip for its version number */
> > -	if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > -	    tpm_read_index(TPM_ADDR, 0x01) != 1)
> > -		return 1;
> > -
> > -	/* This is an atmel supported part */
> > -	return 0;
> > -}
> > -
> > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > -{
> > -}
> > -
> > -/* Determine where to talk to device */
> > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > -{
> > -	int lo, hi;
> > -
> > -	if (atmel_verify_tpm11() != 0)
> > -		return NULL;
> > -
> > -	lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > -	hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > -
> > -	*base = (hi << 8) | lo;
> > -	*region_size = 2;
> > -
> > -	return ioport_map(*base, *region_size);
> > -}
> > -#endif
>
> Responding from holidays but:
>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> [still away for couple of weeks]

I got these in with checkpatch.pl --strict:

CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
#59: FILE: drivers/char/tpm/tpm_atmel.c:26:
+#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)

CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
#60: FILE: drivers/char/tpm/tpm_atmel.c:27:
+#define atmel_putb(val, chip, offset) \
+	outb(val, atmel_get_priv(chip)->base + offset)

CHECK: spaces preferred around that '+' (ctx:VxV)
#73: FILE: drivers/char/tpm/tpm_atmel.c:40:
+	return inb(base+1) & 0xFF;
 	              ^

CHECK: Blank lines aren't necessary after an open brace '{'
#79: FILE: drivers/char/tpm/tpm_atmel.c:46:
+{
+

Can you address them and I'll tag the next version, once I've
revisited checkpatch. Otherwise, nothing against the code change.

BR, Jarkko

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Rob Herring]

On Wed, Jul 17, 2024 at 6:14 AM Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
>
> On Wed Jul 17, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> > > The PPC64 specific MMIO setup open codes DT address functions rather
> > > than using standard address parsing functions. The open-coded version
> > > fails to handle any address translation and is not endian safe.
> > >
> > > I haven't found any evidence of what platform used this. The only thing
> > > that turned up was a PPC405 platform, but that is 32-bit and PPC405
> > > support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> > > any powerpc config and never was. The support was added in 2005 and
> > > hasn't been touched since.
> > >
> > > Rather than try to modernize and fix this code, just remove it.
> > >
> > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > ---
> > >  drivers/char/tpm/Kconfig     |   2 +-
> > >  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
> > >  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
> > >  3 files changed, 62 insertions(+), 144 deletions(-)
> > >  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> > >
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index e63a6a17793c..9b655e9fc7ab 100644
> > > --- a/drivers/char/tpm/Kconfig
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -162,7 +162,7 @@ config TCG_NSC
> > >
> > >  config TCG_ATMEL
> > >     tristate "Atmel TPM Interface"
> > > -   depends on PPC64 || HAS_IOPORT_MAP
> > > +   depends on HAS_IOPORT_MAP
> > >     depends on HAS_IOPORT
> > >     help
> > >       If you have a TPM security chip from Atmel say Yes and it
> > > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > > index 9fb2defa9dc4..622c4abe8cb3 100644
> > > --- a/drivers/char/tpm/tpm_atmel.c
> > > +++ b/drivers/char/tpm/tpm_atmel.c
> > > @@ -15,7 +15,67 @@
> > >   */
> > >
> > >  #include "tpm.h"
> > > -#include "tpm_atmel.h"
> > > +
> > > +struct tpm_atmel_priv {
> > > +   int region_size;
> > > +   int have_region;
> > > +   unsigned long base;
> > > +   void __iomem *iobase;
> > > +};
> > > +
> > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > +#define atmel_putb(val, chip, offset) \
> > > +   outb(val, atmel_get_priv(chip)->base + offset)
> > > +#define atmel_request_region request_region
> > > +#define atmel_release_region release_region
> > > +/* Atmel definitions */
> > > +enum tpm_atmel_addr {
> > > +   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > +   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > +};
> > > +
> > > +static inline int tpm_read_index(int base, int index)
> > > +{
> > > +   outb(index, base);
> > > +   return inb(base+1) & 0xFF;
> > > +}
> > > +
> > > +/* Verify this is a 1.1 Atmel TPM */
> > > +static int atmel_verify_tpm11(void)
> > > +{
> > > +
> > > +   /* verify that it is an Atmel part */
> > > +   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > +       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > +       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > +       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > +           return 1;
> > > +
> > > +   /* query chip for its version number */
> > > +   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > +       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > +           return 1;
> > > +
> > > +   /* This is an atmel supported part */
> > > +   return 0;
> > > +}
> > > +
> > > +/* Determine where to talk to device */
> > > +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > +{
> > > +   int lo, hi;
> > > +
> > > +   if (atmel_verify_tpm11() != 0)
> > > +           return NULL;
> > > +
> > > +   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > +   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > +
> > > +   *base = (hi << 8) | lo;
> > > +   *region_size = 2;
> > > +
> > > +   return ioport_map(*base, *region_size);
> > > +}
> > >
> > >  /* write status bits */
> > >  enum tpm_atmel_write_status {
> > > @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
> > >     tpm_chip_unregister(chip);
> > >     if (priv->have_region)
> > >             atmel_release_region(priv->base, priv->region_size);
> > > -   atmel_put_base_addr(priv->iobase);
> > >     platform_device_unregister(pdev);
> > >  }
> > >
> > > @@ -211,7 +270,6 @@ static int __init init_atmel(void)
> > >  err_unreg_dev:
> > >     platform_device_unregister(pdev);
> > >  err_rel_reg:
> > > -   atmel_put_base_addr(iobase);
> > >     if (have_region)
> > >             atmel_release_region(base,
> > >                                  region_size);
> > > diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> > > deleted file mode 100644
> > > index 7ac3f69dcf0f..000000000000
> > > --- a/drivers/char/tpm/tpm_atmel.h
> > > +++ /dev/null
> > > @@ -1,140 +0,0 @@
> > > -/* SPDX-License-Identifier: GPL-2.0-only */
> > > -/*
> > > - * Copyright (C) 2005 IBM Corporation
> > > - *
> > > - * Authors:
> > > - * Kylene Hall <kjhall@us.ibm.com>
> > > - *
> > > - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> > > - *
> > > - * Device driver for TCG/TCPA TPM (trusted platform module).
> > > - * Specifications at www.trustedcomputinggroup.org
> > > - *
> > > - * These difference are required on power because the device must be
> > > - * discovered through the device tree and iomap must be used to get
> > > - * around the need for holes in the io_page_mask.  This does not happen
> > > - * automatically because the tpm is not a normal pci device and lives
> > > - * under the root node.
> > > - */
> > > -
> > > -struct tpm_atmel_priv {
> > > -   int region_size;
> > > -   int have_region;
> > > -   unsigned long base;
> > > -   void __iomem *iobase;
> > > -};
> > > -
> > > -#ifdef CONFIG_PPC64
> > > -
> > > -#include <linux/of.h>
> > > -
> > > -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> > > -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> > > -#define atmel_request_region request_mem_region
> > > -#define atmel_release_region release_mem_region
> > > -
> > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > -{
> > > -   iounmap(iobase);
> > > -}
> > > -
> > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > -{
> > > -   struct device_node *dn;
> > > -   unsigned long address, size;
> > > -   const unsigned int *reg;
> > > -   int reglen;
> > > -   int naddrc;
> > > -   int nsizec;
> > > -
> > > -   dn = of_find_node_by_name(NULL, "tpm");
> > > -
> > > -   if (!dn)
> > > -           return NULL;
> > > -
> > > -   if (!of_device_is_compatible(dn, "AT97SC3201")) {
> > > -           of_node_put(dn);
> > > -           return NULL;
> > > -   }
> > > -
> > > -   reg = of_get_property(dn, "reg", &reglen);
> > > -   naddrc = of_n_addr_cells(dn);
> > > -   nsizec = of_n_size_cells(dn);
> > > -
> > > -   of_node_put(dn);
> > > -
> > > -
> > > -   if (naddrc == 2)
> > > -           address = ((unsigned long) reg[0] << 32) | reg[1];
> > > -   else
> > > -           address = reg[0];
> > > -
> > > -   if (nsizec == 2)
> > > -           size =
> > > -               ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> > > -   else
> > > -           size = reg[naddrc];
> > > -
> > > -   *base = address;
> > > -   *region_size = size;
> > > -   return ioremap(*base, *region_size);
> > > -}
> > > -#else
> > > -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > -#define atmel_putb(val, chip, offset) \
> > > -   outb(val, atmel_get_priv(chip)->base + offset)
> > > -#define atmel_request_region request_region
> > > -#define atmel_release_region release_region
> > > -/* Atmel definitions */
> > > -enum tpm_atmel_addr {
> > > -   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > -   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > -};
> > > -
> > > -static inline int tpm_read_index(int base, int index)
> > > -{
> > > -   outb(index, base);
> > > -   return inb(base+1) & 0xFF;
> > > -}
> > > -
> > > -/* Verify this is a 1.1 Atmel TPM */
> > > -static int atmel_verify_tpm11(void)
> > > -{
> > > -
> > > -   /* verify that it is an Atmel part */
> > > -   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > -       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > -       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > -       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > -           return 1;
> > > -
> > > -   /* query chip for its version number */
> > > -   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > -       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > -           return 1;
> > > -
> > > -   /* This is an atmel supported part */
> > > -   return 0;
> > > -}
> > > -
> > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > -{
> > > -}
> > > -
> > > -/* Determine where to talk to device */
> > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > -{
> > > -   int lo, hi;
> > > -
> > > -   if (atmel_verify_tpm11() != 0)
> > > -           return NULL;
> > > -
> > > -   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > -   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > -
> > > -   *base = (hi << 8) | lo;
> > > -   *region_size = 2;
> > > -
> > > -   return ioport_map(*base, *region_size);
> > > -}
> > > -#endif
> >
> > Responding from holidays but:
> >
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > [still away for couple of weeks]
>
> I got these in with checkpatch.pl --strict:
>
> CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> #59: FILE: drivers/char/tpm/tpm_atmel.c:26:
> +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
>
> CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> #60: FILE: drivers/char/tpm/tpm_atmel.c:27:
> +#define atmel_putb(val, chip, offset) \
> +       outb(val, atmel_get_priv(chip)->base + offset)
>
> CHECK: spaces preferred around that '+' (ctx:VxV)
> #73: FILE: drivers/char/tpm/tpm_atmel.c:40:
> +       return inb(base+1) & 0xFF;
>                       ^
>
> CHECK: Blank lines aren't necessary after an open brace '{'
> #79: FILE: drivers/char/tpm/tpm_atmel.c:46:
> +{
> +
>
> Can you address them and I'll tag the next version, once I've
> revisited checkpatch. Otherwise, nothing against the code change.

Those all existed before because I just moved what was left of the
header contents into the .c file. Fixing them seems like a separate
change to me. I can just leave the header in place and avoid the
warnings if you prefer. Otherwise, those warnings are the least of the
clean-up this driver needs. For starters, I would make those defines
static inlines instead.

Rob

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Thu Jul 18, 2024 at 5:57 PM EEST, Rob Herring wrote:
> On Wed, Jul 17, 2024 at 6:14 AM Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
> >
> > On Wed Jul 17, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > > On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> > > > The PPC64 specific MMIO setup open codes DT address functions rather
> > > > than using standard address parsing functions. The open-coded version
> > > > fails to handle any address translation and is not endian safe.
> > > >
> > > > I haven't found any evidence of what platform used this. The only thing
> > > > that turned up was a PPC405 platform, but that is 32-bit and PPC405
> > > > support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> > > > any powerpc config and never was. The support was added in 2005 and
> > > > hasn't been touched since.
> > > >
> > > > Rather than try to modernize and fix this code, just remove it.
> > > >
> > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > > ---
> > > >  drivers/char/tpm/Kconfig     |   2 +-
> > > >  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
> > > >  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
> > > >  3 files changed, 62 insertions(+), 144 deletions(-)
> > > >  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> > > >
> > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > index e63a6a17793c..9b655e9fc7ab 100644
> > > > --- a/drivers/char/tpm/Kconfig
> > > > +++ b/drivers/char/tpm/Kconfig
> > > > @@ -162,7 +162,7 @@ config TCG_NSC
> > > >
> > > >  config TCG_ATMEL
> > > >     tristate "Atmel TPM Interface"
> > > > -   depends on PPC64 || HAS_IOPORT_MAP
> > > > +   depends on HAS_IOPORT_MAP
> > > >     depends on HAS_IOPORT
> > > >     help
> > > >       If you have a TPM security chip from Atmel say Yes and it
> > > > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > > > index 9fb2defa9dc4..622c4abe8cb3 100644
> > > > --- a/drivers/char/tpm/tpm_atmel.c
> > > > +++ b/drivers/char/tpm/tpm_atmel.c
> > > > @@ -15,7 +15,67 @@
> > > >   */
> > > >
> > > >  #include "tpm.h"
> > > > -#include "tpm_atmel.h"
> > > > +
> > > > +struct tpm_atmel_priv {
> > > > +   int region_size;
> > > > +   int have_region;
> > > > +   unsigned long base;
> > > > +   void __iomem *iobase;
> > > > +};
> > > > +
> > > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > +#define atmel_putb(val, chip, offset) \
> > > > +   outb(val, atmel_get_priv(chip)->base + offset)
> > > > +#define atmel_request_region request_region
> > > > +#define atmel_release_region release_region
> > > > +/* Atmel definitions */
> > > > +enum tpm_atmel_addr {
> > > > +   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > +   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > +};
> > > > +
> > > > +static inline int tpm_read_index(int base, int index)
> > > > +{
> > > > +   outb(index, base);
> > > > +   return inb(base+1) & 0xFF;
> > > > +}
> > > > +
> > > > +/* Verify this is a 1.1 Atmel TPM */
> > > > +static int atmel_verify_tpm11(void)
> > > > +{
> > > > +
> > > > +   /* verify that it is an Atmel part */
> > > > +   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > +       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > +       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > +       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > +           return 1;
> > > > +
> > > > +   /* query chip for its version number */
> > > > +   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > +       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > +           return 1;
> > > > +
> > > > +   /* This is an atmel supported part */
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +/* Determine where to talk to device */
> > > > +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > +{
> > > > +   int lo, hi;
> > > > +
> > > > +   if (atmel_verify_tpm11() != 0)
> > > > +           return NULL;
> > > > +
> > > > +   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > +   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > +
> > > > +   *base = (hi << 8) | lo;
> > > > +   *region_size = 2;
> > > > +
> > > > +   return ioport_map(*base, *region_size);
> > > > +}
> > > >
> > > >  /* write status bits */
> > > >  enum tpm_atmel_write_status {
> > > > @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
> > > >     tpm_chip_unregister(chip);
> > > >     if (priv->have_region)
> > > >             atmel_release_region(priv->base, priv->region_size);
> > > > -   atmel_put_base_addr(priv->iobase);
> > > >     platform_device_unregister(pdev);
> > > >  }
> > > >
> > > > @@ -211,7 +270,6 @@ static int __init init_atmel(void)
> > > >  err_unreg_dev:
> > > >     platform_device_unregister(pdev);
> > > >  err_rel_reg:
> > > > -   atmel_put_base_addr(iobase);
> > > >     if (have_region)
> > > >             atmel_release_region(base,
> > > >                                  region_size);
> > > > diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> > > > deleted file mode 100644
> > > > index 7ac3f69dcf0f..000000000000
> > > > --- a/drivers/char/tpm/tpm_atmel.h
> > > > +++ /dev/null
> > > > @@ -1,140 +0,0 @@
> > > > -/* SPDX-License-Identifier: GPL-2.0-only */
> > > > -/*
> > > > - * Copyright (C) 2005 IBM Corporation
> > > > - *
> > > > - * Authors:
> > > > - * Kylene Hall <kjhall@us.ibm.com>
> > > > - *
> > > > - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> > > > - *
> > > > - * Device driver for TCG/TCPA TPM (trusted platform module).
> > > > - * Specifications at www.trustedcomputinggroup.org
> > > > - *
> > > > - * These difference are required on power because the device must be
> > > > - * discovered through the device tree and iomap must be used to get
> > > > - * around the need for holes in the io_page_mask.  This does not happen
> > > > - * automatically because the tpm is not a normal pci device and lives
> > > > - * under the root node.
> > > > - */
> > > > -
> > > > -struct tpm_atmel_priv {
> > > > -   int region_size;
> > > > -   int have_region;
> > > > -   unsigned long base;
> > > > -   void __iomem *iobase;
> > > > -};
> > > > -
> > > > -#ifdef CONFIG_PPC64
> > > > -
> > > > -#include <linux/of.h>
> > > > -
> > > > -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> > > > -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> > > > -#define atmel_request_region request_mem_region
> > > > -#define atmel_release_region release_mem_region
> > > > -
> > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > -{
> > > > -   iounmap(iobase);
> > > > -}
> > > > -
> > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > -{
> > > > -   struct device_node *dn;
> > > > -   unsigned long address, size;
> > > > -   const unsigned int *reg;
> > > > -   int reglen;
> > > > -   int naddrc;
> > > > -   int nsizec;
> > > > -
> > > > -   dn = of_find_node_by_name(NULL, "tpm");
> > > > -
> > > > -   if (!dn)
> > > > -           return NULL;
> > > > -
> > > > -   if (!of_device_is_compatible(dn, "AT97SC3201")) {
> > > > -           of_node_put(dn);
> > > > -           return NULL;
> > > > -   }
> > > > -
> > > > -   reg = of_get_property(dn, "reg", &reglen);
> > > > -   naddrc = of_n_addr_cells(dn);
> > > > -   nsizec = of_n_size_cells(dn);
> > > > -
> > > > -   of_node_put(dn);
> > > > -
> > > > -
> > > > -   if (naddrc == 2)
> > > > -           address = ((unsigned long) reg[0] << 32) | reg[1];
> > > > -   else
> > > > -           address = reg[0];
> > > > -
> > > > -   if (nsizec == 2)
> > > > -           size =
> > > > -               ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> > > > -   else
> > > > -           size = reg[naddrc];
> > > > -
> > > > -   *base = address;
> > > > -   *region_size = size;
> > > > -   return ioremap(*base, *region_size);
> > > > -}
> > > > -#else
> > > > -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > -#define atmel_putb(val, chip, offset) \
> > > > -   outb(val, atmel_get_priv(chip)->base + offset)
> > > > -#define atmel_request_region request_region
> > > > -#define atmel_release_region release_region
> > > > -/* Atmel definitions */
> > > > -enum tpm_atmel_addr {
> > > > -   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > -   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > -};
> > > > -
> > > > -static inline int tpm_read_index(int base, int index)
> > > > -{
> > > > -   outb(index, base);
> > > > -   return inb(base+1) & 0xFF;
> > > > -}
> > > > -
> > > > -/* Verify this is a 1.1 Atmel TPM */
> > > > -static int atmel_verify_tpm11(void)
> > > > -{
> > > > -
> > > > -   /* verify that it is an Atmel part */
> > > > -   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > -       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > -       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > -       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > -           return 1;
> > > > -
> > > > -   /* query chip for its version number */
> > > > -   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > -       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > -           return 1;
> > > > -
> > > > -   /* This is an atmel supported part */
> > > > -   return 0;
> > > > -}
> > > > -
> > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > -{
> > > > -}
> > > > -
> > > > -/* Determine where to talk to device */
> > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > -{
> > > > -   int lo, hi;
> > > > -
> > > > -   if (atmel_verify_tpm11() != 0)
> > > > -           return NULL;
> > > > -
> > > > -   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > -   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > -
> > > > -   *base = (hi << 8) | lo;
> > > > -   *region_size = 2;
> > > > -
> > > > -   return ioport_map(*base, *region_size);
> > > > -}
> > > > -#endif
> > >
> > > Responding from holidays but:
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > >
> > > [still away for couple of weeks]
> >
> > I got these in with checkpatch.pl --strict:
> >
> > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > #59: FILE: drivers/char/tpm/tpm_atmel.c:26:
> > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> >
> > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > #60: FILE: drivers/char/tpm/tpm_atmel.c:27:
> > +#define atmel_putb(val, chip, offset) \
> > +       outb(val, atmel_get_priv(chip)->base + offset)
> >
> > CHECK: spaces preferred around that '+' (ctx:VxV)
> > #73: FILE: drivers/char/tpm/tpm_atmel.c:40:
> > +       return inb(base+1) & 0xFF;
> >                       ^
> >
> > CHECK: Blank lines aren't necessary after an open brace '{'
> > #79: FILE: drivers/char/tpm/tpm_atmel.c:46:
> > +{
> > +
> >
> > Can you address them and I'll tag the next version, once I've
> > revisited checkpatch. Otherwise, nothing against the code change.
>
> Those all existed before because I just moved what was left of the
> header contents into the .c file. Fixing them seems like a separate
> change to me. I can just leave the header in place and avoid the
> warnings if you prefer. Otherwise, those warnings are the least of the
> clean-up this driver needs. For starters, I would make those defines
> static inlines instead.

Ah, ok sorry. It was fairly mechanical check as I'm just quickly
going critical stuff from holidays.

I'm back after next week, so I'd really like to hold with this
up until that and prepare when I'm back a patch set, which includes
a supplemental patch fixing those issues (if you don't mind).

BR, Jarkko

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Rob Herring]

On Thu, Jul 18, 2024 at 11:01 AM Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
>
> On Thu Jul 18, 2024 at 5:57 PM EEST, Rob Herring wrote:
> > On Wed, Jul 17, 2024 at 6:14 AM Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
> > >
> > > On Wed Jul 17, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > > > On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> > > > > The PPC64 specific MMIO setup open codes DT address functions rather
> > > > > than using standard address parsing functions. The open-coded version
> > > > > fails to handle any address translation and is not endian safe.
> > > > >
> > > > > I haven't found any evidence of what platform used this. The only thing
> > > > > that turned up was a PPC405 platform, but that is 32-bit and PPC405
> > > > > support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> > > > > any powerpc config and never was. The support was added in 2005 and
> > > > > hasn't been touched since.
> > > > >
> > > > > Rather than try to modernize and fix this code, just remove it.
> > > > >
> > > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > ---
> > > > >  drivers/char/tpm/Kconfig     |   2 +-
> > > > >  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
> > > > >  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
> > > > >  3 files changed, 62 insertions(+), 144 deletions(-)
> > > > >  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> > > > >
> > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > index e63a6a17793c..9b655e9fc7ab 100644
> > > > > --- a/drivers/char/tpm/Kconfig
> > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > @@ -162,7 +162,7 @@ config TCG_NSC
> > > > >
> > > > >  config TCG_ATMEL
> > > > >     tristate "Atmel TPM Interface"
> > > > > -   depends on PPC64 || HAS_IOPORT_MAP
> > > > > +   depends on HAS_IOPORT_MAP
> > > > >     depends on HAS_IOPORT
> > > > >     help
> > > > >       If you have a TPM security chip from Atmel say Yes and it
> > > > > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > > > > index 9fb2defa9dc4..622c4abe8cb3 100644
> > > > > --- a/drivers/char/tpm/tpm_atmel.c
> > > > > +++ b/drivers/char/tpm/tpm_atmel.c
> > > > > @@ -15,7 +15,67 @@
> > > > >   */
> > > > >
> > > > >  #include "tpm.h"
> > > > > -#include "tpm_atmel.h"
> > > > > +
> > > > > +struct tpm_atmel_priv {
> > > > > +   int region_size;
> > > > > +   int have_region;
> > > > > +   unsigned long base;
> > > > > +   void __iomem *iobase;
> > > > > +};
> > > > > +
> > > > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > > +#define atmel_putb(val, chip, offset) \
> > > > > +   outb(val, atmel_get_priv(chip)->base + offset)
> > > > > +#define atmel_request_region request_region
> > > > > +#define atmel_release_region release_region
> > > > > +/* Atmel definitions */
> > > > > +enum tpm_atmel_addr {
> > > > > +   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > > +   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > > +};
> > > > > +
> > > > > +static inline int tpm_read_index(int base, int index)
> > > > > +{
> > > > > +   outb(index, base);
> > > > > +   return inb(base+1) & 0xFF;
> > > > > +}
> > > > > +
> > > > > +/* Verify this is a 1.1 Atmel TPM */
> > > > > +static int atmel_verify_tpm11(void)
> > > > > +{
> > > > > +
> > > > > +   /* verify that it is an Atmel part */
> > > > > +   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > > +       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > > +       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > > +       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > > +           return 1;
> > > > > +
> > > > > +   /* query chip for its version number */
> > > > > +   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > > +       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > > +           return 1;
> > > > > +
> > > > > +   /* This is an atmel supported part */
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +/* Determine where to talk to device */
> > > > > +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > +{
> > > > > +   int lo, hi;
> > > > > +
> > > > > +   if (atmel_verify_tpm11() != 0)
> > > > > +           return NULL;
> > > > > +
> > > > > +   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > > +   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > > +
> > > > > +   *base = (hi << 8) | lo;
> > > > > +   *region_size = 2;
> > > > > +
> > > > > +   return ioport_map(*base, *region_size);
> > > > > +}
> > > > >
> > > > >  /* write status bits */
> > > > >  enum tpm_atmel_write_status {
> > > > > @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
> > > > >     tpm_chip_unregister(chip);
> > > > >     if (priv->have_region)
> > > > >             atmel_release_region(priv->base, priv->region_size);
> > > > > -   atmel_put_base_addr(priv->iobase);
> > > > >     platform_device_unregister(pdev);
> > > > >  }
> > > > >
> > > > > @@ -211,7 +270,6 @@ static int __init init_atmel(void)
> > > > >  err_unreg_dev:
> > > > >     platform_device_unregister(pdev);
> > > > >  err_rel_reg:
> > > > > -   atmel_put_base_addr(iobase);
> > > > >     if (have_region)
> > > > >             atmel_release_region(base,
> > > > >                                  region_size);
> > > > > diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> > > > > deleted file mode 100644
> > > > > index 7ac3f69dcf0f..000000000000
> > > > > --- a/drivers/char/tpm/tpm_atmel.h
> > > > > +++ /dev/null
> > > > > @@ -1,140 +0,0 @@
> > > > > -/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > -/*
> > > > > - * Copyright (C) 2005 IBM Corporation
> > > > > - *
> > > > > - * Authors:
> > > > > - * Kylene Hall <kjhall@us.ibm.com>
> > > > > - *
> > > > > - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> > > > > - *
> > > > > - * Device driver for TCG/TCPA TPM (trusted platform module).
> > > > > - * Specifications at www.trustedcomputinggroup.org
> > > > > - *
> > > > > - * These difference are required on power because the device must be
> > > > > - * discovered through the device tree and iomap must be used to get
> > > > > - * around the need for holes in the io_page_mask.  This does not happen
> > > > > - * automatically because the tpm is not a normal pci device and lives
> > > > > - * under the root node.
> > > > > - */
> > > > > -
> > > > > -struct tpm_atmel_priv {
> > > > > -   int region_size;
> > > > > -   int have_region;
> > > > > -   unsigned long base;
> > > > > -   void __iomem *iobase;
> > > > > -};
> > > > > -
> > > > > -#ifdef CONFIG_PPC64
> > > > > -
> > > > > -#include <linux/of.h>
> > > > > -
> > > > > -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> > > > > -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> > > > > -#define atmel_request_region request_mem_region
> > > > > -#define atmel_release_region release_mem_region
> > > > > -
> > > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > > -{
> > > > > -   iounmap(iobase);
> > > > > -}
> > > > > -
> > > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > -{
> > > > > -   struct device_node *dn;
> > > > > -   unsigned long address, size;
> > > > > -   const unsigned int *reg;
> > > > > -   int reglen;
> > > > > -   int naddrc;
> > > > > -   int nsizec;
> > > > > -
> > > > > -   dn = of_find_node_by_name(NULL, "tpm");
> > > > > -
> > > > > -   if (!dn)
> > > > > -           return NULL;
> > > > > -
> > > > > -   if (!of_device_is_compatible(dn, "AT97SC3201")) {
> > > > > -           of_node_put(dn);
> > > > > -           return NULL;
> > > > > -   }
> > > > > -
> > > > > -   reg = of_get_property(dn, "reg", &reglen);
> > > > > -   naddrc = of_n_addr_cells(dn);
> > > > > -   nsizec = of_n_size_cells(dn);
> > > > > -
> > > > > -   of_node_put(dn);
> > > > > -
> > > > > -
> > > > > -   if (naddrc == 2)
> > > > > -           address = ((unsigned long) reg[0] << 32) | reg[1];
> > > > > -   else
> > > > > -           address = reg[0];
> > > > > -
> > > > > -   if (nsizec == 2)
> > > > > -           size =
> > > > > -               ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> > > > > -   else
> > > > > -           size = reg[naddrc];
> > > > > -
> > > > > -   *base = address;
> > > > > -   *region_size = size;
> > > > > -   return ioremap(*base, *region_size);
> > > > > -}
> > > > > -#else
> > > > > -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > > -#define atmel_putb(val, chip, offset) \
> > > > > -   outb(val, atmel_get_priv(chip)->base + offset)
> > > > > -#define atmel_request_region request_region
> > > > > -#define atmel_release_region release_region
> > > > > -/* Atmel definitions */
> > > > > -enum tpm_atmel_addr {
> > > > > -   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > > -   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > > -};
> > > > > -
> > > > > -static inline int tpm_read_index(int base, int index)
> > > > > -{
> > > > > -   outb(index, base);
> > > > > -   return inb(base+1) & 0xFF;
> > > > > -}
> > > > > -
> > > > > -/* Verify this is a 1.1 Atmel TPM */
> > > > > -static int atmel_verify_tpm11(void)
> > > > > -{
> > > > > -
> > > > > -   /* verify that it is an Atmel part */
> > > > > -   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > > -       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > > -       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > > -       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > > -           return 1;
> > > > > -
> > > > > -   /* query chip for its version number */
> > > > > -   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > > -       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > > -           return 1;
> > > > > -
> > > > > -   /* This is an atmel supported part */
> > > > > -   return 0;
> > > > > -}
> > > > > -
> > > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > > -{
> > > > > -}
> > > > > -
> > > > > -/* Determine where to talk to device */
> > > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > -{
> > > > > -   int lo, hi;
> > > > > -
> > > > > -   if (atmel_verify_tpm11() != 0)
> > > > > -           return NULL;
> > > > > -
> > > > > -   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > > -   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > > -
> > > > > -   *base = (hi << 8) | lo;
> > > > > -   *region_size = 2;
> > > > > -
> > > > > -   return ioport_map(*base, *region_size);
> > > > > -}
> > > > > -#endif
> > > >
> > > > Responding from holidays but:
> > > >
> > > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > >
> > > > [still away for couple of weeks]
> > >
> > > I got these in with checkpatch.pl --strict:
> > >
> > > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > > #59: FILE: drivers/char/tpm/tpm_atmel.c:26:
> > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > >
> > > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > > #60: FILE: drivers/char/tpm/tpm_atmel.c:27:
> > > +#define atmel_putb(val, chip, offset) \
> > > +       outb(val, atmel_get_priv(chip)->base + offset)
> > >
> > > CHECK: spaces preferred around that '+' (ctx:VxV)
> > > #73: FILE: drivers/char/tpm/tpm_atmel.c:40:
> > > +       return inb(base+1) & 0xFF;
> > >                       ^
> > >
> > > CHECK: Blank lines aren't necessary after an open brace '{'
> > > #79: FILE: drivers/char/tpm/tpm_atmel.c:46:
> > > +{
> > > +
> > >
> > > Can you address them and I'll tag the next version, once I've
> > > revisited checkpatch. Otherwise, nothing against the code change.
> >
> > Those all existed before because I just moved what was left of the
> > header contents into the .c file. Fixing them seems like a separate
> > change to me. I can just leave the header in place and avoid the
> > warnings if you prefer. Otherwise, those warnings are the least of the
> > clean-up this driver needs. For starters, I would make those defines
> > static inlines instead.
>
> Ah, ok sorry. It was fairly mechanical check as I'm just quickly
> going critical stuff from holidays.
>
> I'm back after next week, so I'd really like to hold with this
> up until that and prepare when I'm back a patch set, which includes
> a supplemental patch fixing those issues (if you don't mind).

Whatever happened to this? Can you please apply my patch if you don't
have the time for further rework.

Rob

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Wed Nov 6, 2024 at 4:50 PM EET, Rob Herring wrote:
> On Thu, Jul 18, 2024 at 11:01 AM Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
> >
> > On Thu Jul 18, 2024 at 5:57 PM EEST, Rob Herring wrote:
> > > On Wed, Jul 17, 2024 at 6:14 AM Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
> > > >
> > > > On Wed Jul 17, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > > > > On Tue Jul 2, 2024 at 7:10 PM EEST, Rob Herring (Arm) wrote:
> > > > > > The PPC64 specific MMIO setup open codes DT address functions rather
> > > > > > than using standard address parsing functions. The open-coded version
> > > > > > fails to handle any address translation and is not endian safe.
> > > > > >
> > > > > > I haven't found any evidence of what platform used this. The only thing
> > > > > > that turned up was a PPC405 platform, but that is 32-bit and PPC405
> > > > > > support is being removed as well. CONFIG_TCG_ATMEL is not enabled for
> > > > > > any powerpc config and never was. The support was added in 2005 and
> > > > > > hasn't been touched since.
> > > > > >
> > > > > > Rather than try to modernize and fix this code, just remove it.
> > > > > >
> > > > > > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > ---
> > > > > >  drivers/char/tpm/Kconfig     |   2 +-
> > > > > >  drivers/char/tpm/tpm_atmel.c |  64 +++++++++++++++-
> > > > > >  drivers/char/tpm/tpm_atmel.h | 140 -----------------------------------
> > > > > >  3 files changed, 62 insertions(+), 144 deletions(-)
> > > > > >  delete mode 100644 drivers/char/tpm/tpm_atmel.h
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > index e63a6a17793c..9b655e9fc7ab 100644
> > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > @@ -162,7 +162,7 @@ config TCG_NSC
> > > > > >
> > > > > >  config TCG_ATMEL
> > > > > >     tristate "Atmel TPM Interface"
> > > > > > -   depends on PPC64 || HAS_IOPORT_MAP
> > > > > > +   depends on HAS_IOPORT_MAP
> > > > > >     depends on HAS_IOPORT
> > > > > >     help
> > > > > >       If you have a TPM security chip from Atmel say Yes and it
> > > > > > diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> > > > > > index 9fb2defa9dc4..622c4abe8cb3 100644
> > > > > > --- a/drivers/char/tpm/tpm_atmel.c
> > > > > > +++ b/drivers/char/tpm/tpm_atmel.c
> > > > > > @@ -15,7 +15,67 @@
> > > > > >   */
> > > > > >
> > > > > >  #include "tpm.h"
> > > > > > -#include "tpm_atmel.h"
> > > > > > +
> > > > > > +struct tpm_atmel_priv {
> > > > > > +   int region_size;
> > > > > > +   int have_region;
> > > > > > +   unsigned long base;
> > > > > > +   void __iomem *iobase;
> > > > > > +};
> > > > > > +
> > > > > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > > > +#define atmel_putb(val, chip, offset) \
> > > > > > +   outb(val, atmel_get_priv(chip)->base + offset)
> > > > > > +#define atmel_request_region request_region
> > > > > > +#define atmel_release_region release_region
> > > > > > +/* Atmel definitions */
> > > > > > +enum tpm_atmel_addr {
> > > > > > +   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > > > +   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > > > +};
> > > > > > +
> > > > > > +static inline int tpm_read_index(int base, int index)
> > > > > > +{
> > > > > > +   outb(index, base);
> > > > > > +   return inb(base+1) & 0xFF;
> > > > > > +}
> > > > > > +
> > > > > > +/* Verify this is a 1.1 Atmel TPM */
> > > > > > +static int atmel_verify_tpm11(void)
> > > > > > +{
> > > > > > +
> > > > > > +   /* verify that it is an Atmel part */
> > > > > > +   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > > > +       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > > > +       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > > > +       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > > > +           return 1;
> > > > > > +
> > > > > > +   /* query chip for its version number */
> > > > > > +   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > > > +       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > > > +           return 1;
> > > > > > +
> > > > > > +   /* This is an atmel supported part */
> > > > > > +   return 0;
> > > > > > +}
> > > > > > +
> > > > > > +/* Determine where to talk to device */
> > > > > > +static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > > +{
> > > > > > +   int lo, hi;
> > > > > > +
> > > > > > +   if (atmel_verify_tpm11() != 0)
> > > > > > +           return NULL;
> > > > > > +
> > > > > > +   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > > > +   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > > > +
> > > > > > +   *base = (hi << 8) | lo;
> > > > > > +   *region_size = 2;
> > > > > > +
> > > > > > +   return ioport_map(*base, *region_size);
> > > > > > +}
> > > > > >
> > > > > >  /* write status bits */
> > > > > >  enum tpm_atmel_write_status {
> > > > > > @@ -142,7 +202,6 @@ static void atml_plat_remove(void)
> > > > > >     tpm_chip_unregister(chip);
> > > > > >     if (priv->have_region)
> > > > > >             atmel_release_region(priv->base, priv->region_size);
> > > > > > -   atmel_put_base_addr(priv->iobase);
> > > > > >     platform_device_unregister(pdev);
> > > > > >  }
> > > > > >
> > > > > > @@ -211,7 +270,6 @@ static int __init init_atmel(void)
> > > > > >  err_unreg_dev:
> > > > > >     platform_device_unregister(pdev);
> > > > > >  err_rel_reg:
> > > > > > -   atmel_put_base_addr(iobase);
> > > > > >     if (have_region)
> > > > > >             atmel_release_region(base,
> > > > > >                                  region_size);
> > > > > > diff --git a/drivers/char/tpm/tpm_atmel.h b/drivers/char/tpm/tpm_atmel.h
> > > > > > deleted file mode 100644
> > > > > > index 7ac3f69dcf0f..000000000000
> > > > > > --- a/drivers/char/tpm/tpm_atmel.h
> > > > > > +++ /dev/null
> > > > > > @@ -1,140 +0,0 @@
> > > > > > -/* SPDX-License-Identifier: GPL-2.0-only */
> > > > > > -/*
> > > > > > - * Copyright (C) 2005 IBM Corporation
> > > > > > - *
> > > > > > - * Authors:
> > > > > > - * Kylene Hall <kjhall@us.ibm.com>
> > > > > > - *
> > > > > > - * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> > > > > > - *
> > > > > > - * Device driver for TCG/TCPA TPM (trusted platform module).
> > > > > > - * Specifications at www.trustedcomputinggroup.org
> > > > > > - *
> > > > > > - * These difference are required on power because the device must be
> > > > > > - * discovered through the device tree and iomap must be used to get
> > > > > > - * around the need for holes in the io_page_mask.  This does not happen
> > > > > > - * automatically because the tpm is not a normal pci device and lives
> > > > > > - * under the root node.
> > > > > > - */
> > > > > > -
> > > > > > -struct tpm_atmel_priv {
> > > > > > -   int region_size;
> > > > > > -   int have_region;
> > > > > > -   unsigned long base;
> > > > > > -   void __iomem *iobase;
> > > > > > -};
> > > > > > -
> > > > > > -#ifdef CONFIG_PPC64
> > > > > > -
> > > > > > -#include <linux/of.h>
> > > > > > -
> > > > > > -#define atmel_getb(priv, offset) readb(priv->iobase + offset)
> > > > > > -#define atmel_putb(val, priv, offset) writeb(val, priv->iobase + offset)
> > > > > > -#define atmel_request_region request_mem_region
> > > > > > -#define atmel_release_region release_mem_region
> > > > > > -
> > > > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > > > -{
> > > > > > -   iounmap(iobase);
> > > > > > -}
> > > > > > -
> > > > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > > -{
> > > > > > -   struct device_node *dn;
> > > > > > -   unsigned long address, size;
> > > > > > -   const unsigned int *reg;
> > > > > > -   int reglen;
> > > > > > -   int naddrc;
> > > > > > -   int nsizec;
> > > > > > -
> > > > > > -   dn = of_find_node_by_name(NULL, "tpm");
> > > > > > -
> > > > > > -   if (!dn)
> > > > > > -           return NULL;
> > > > > > -
> > > > > > -   if (!of_device_is_compatible(dn, "AT97SC3201")) {
> > > > > > -           of_node_put(dn);
> > > > > > -           return NULL;
> > > > > > -   }
> > > > > > -
> > > > > > -   reg = of_get_property(dn, "reg", &reglen);
> > > > > > -   naddrc = of_n_addr_cells(dn);
> > > > > > -   nsizec = of_n_size_cells(dn);
> > > > > > -
> > > > > > -   of_node_put(dn);
> > > > > > -
> > > > > > -
> > > > > > -   if (naddrc == 2)
> > > > > > -           address = ((unsigned long) reg[0] << 32) | reg[1];
> > > > > > -   else
> > > > > > -           address = reg[0];
> > > > > > -
> > > > > > -   if (nsizec == 2)
> > > > > > -           size =
> > > > > > -               ((unsigned long) reg[naddrc] << 32) | reg[naddrc + 1];
> > > > > > -   else
> > > > > > -           size = reg[naddrc];
> > > > > > -
> > > > > > -   *base = address;
> > > > > > -   *region_size = size;
> > > > > > -   return ioremap(*base, *region_size);
> > > > > > -}
> > > > > > -#else
> > > > > > -#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > > > > -#define atmel_putb(val, chip, offset) \
> > > > > > -   outb(val, atmel_get_priv(chip)->base + offset)
> > > > > > -#define atmel_request_region request_region
> > > > > > -#define atmel_release_region release_region
> > > > > > -/* Atmel definitions */
> > > > > > -enum tpm_atmel_addr {
> > > > > > -   TPM_ATMEL_BASE_ADDR_LO = 0x08,
> > > > > > -   TPM_ATMEL_BASE_ADDR_HI = 0x09
> > > > > > -};
> > > > > > -
> > > > > > -static inline int tpm_read_index(int base, int index)
> > > > > > -{
> > > > > > -   outb(index, base);
> > > > > > -   return inb(base+1) & 0xFF;
> > > > > > -}
> > > > > > -
> > > > > > -/* Verify this is a 1.1 Atmel TPM */
> > > > > > -static int atmel_verify_tpm11(void)
> > > > > > -{
> > > > > > -
> > > > > > -   /* verify that it is an Atmel part */
> > > > > > -   if (tpm_read_index(TPM_ADDR, 4) != 'A' ||
> > > > > > -       tpm_read_index(TPM_ADDR, 5) != 'T' ||
> > > > > > -       tpm_read_index(TPM_ADDR, 6) != 'M' ||
> > > > > > -       tpm_read_index(TPM_ADDR, 7) != 'L')
> > > > > > -           return 1;
> > > > > > -
> > > > > > -   /* query chip for its version number */
> > > > > > -   if (tpm_read_index(TPM_ADDR, 0x00) != 1 ||
> > > > > > -       tpm_read_index(TPM_ADDR, 0x01) != 1)
> > > > > > -           return 1;
> > > > > > -
> > > > > > -   /* This is an atmel supported part */
> > > > > > -   return 0;
> > > > > > -}
> > > > > > -
> > > > > > -static inline void atmel_put_base_addr(void __iomem *iobase)
> > > > > > -{
> > > > > > -}
> > > > > > -
> > > > > > -/* Determine where to talk to device */
> > > > > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > > > > -{
> > > > > > -   int lo, hi;
> > > > > > -
> > > > > > -   if (atmel_verify_tpm11() != 0)
> > > > > > -           return NULL;
> > > > > > -
> > > > > > -   lo = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_LO);
> > > > > > -   hi = tpm_read_index(TPM_ADDR, TPM_ATMEL_BASE_ADDR_HI);
> > > > > > -
> > > > > > -   *base = (hi << 8) | lo;
> > > > > > -   *region_size = 2;
> > > > > > -
> > > > > > -   return ioport_map(*base, *region_size);
> > > > > > -}
> > > > > > -#endif
> > > > >
> > > > > Responding from holidays but:
> > > > >
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > >
> > > > > [still away for couple of weeks]
> > > >
> > > > I got these in with checkpatch.pl --strict:
> > > >
> > > > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > > > #59: FILE: drivers/char/tpm/tpm_atmel.c:26:
> > > > +#define atmel_getb(chip, offset) inb(atmel_get_priv(chip)->base + offset)
> > > >
> > > > CHECK: Macro argument 'offset' may be better as '(offset)' to avoid precedence issues
> > > > #60: FILE: drivers/char/tpm/tpm_atmel.c:27:
> > > > +#define atmel_putb(val, chip, offset) \
> > > > +       outb(val, atmel_get_priv(chip)->base + offset)
> > > >
> > > > CHECK: spaces preferred around that '+' (ctx:VxV)
> > > > #73: FILE: drivers/char/tpm/tpm_atmel.c:40:
> > > > +       return inb(base+1) & 0xFF;
> > > >                       ^
> > > >
> > > > CHECK: Blank lines aren't necessary after an open brace '{'
> > > > #79: FILE: drivers/char/tpm/tpm_atmel.c:46:
> > > > +{
> > > > +
> > > >
> > > > Can you address them and I'll tag the next version, once I've
> > > > revisited checkpatch. Otherwise, nothing against the code change.
> > >
> > > Those all existed before because I just moved what was left of the
> > > header contents into the .c file. Fixing them seems like a separate
> > > change to me. I can just leave the header in place and avoid the
> > > warnings if you prefer. Otherwise, those warnings are the least of the
> > > clean-up this driver needs. For starters, I would make those defines
> > > static inlines instead.
> >
> > Ah, ok sorry. It was fairly mechanical check as I'm just quickly
> > going critical stuff from holidays.
> >
> > I'm back after next week, so I'd really like to hold with this
> > up until that and prepare when I'm back a patch set, which includes
> > a supplemental patch fixing those issues (if you don't mind).
>
> Whatever happened to this? Can you please apply my patch if you don't
> have the time for further rework.

Sorry unintentional.

I applied with

-static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
+static void __iomem *atmel_get_base_addr(unsigned long *base, int *region_size)
 
as this gives checkpatch error.

> Rob

BR, Jarkko

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Wed Nov 6, 2024 at 8:17 PM EET, Jarkko Sakkinen wrote:
> > Whatever happened to this? Can you please apply my patch if you don't
> > have the time for further rework.
>
> Sorry unintentional.
>
> I applied with
>
> -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> +static void __iomem *atmel_get_base_addr(unsigned long *base, int *region_size)
>  
> as this gives checkpatch error.

See: 
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b18ffd5e0faaa02bffda61e19a013573451008d4

If that looks good to you, I can mirror it to -next.

BR, Jarkko

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Rob Herring]

On Wed, Nov 6, 2024 at 12:19 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed Nov 6, 2024 at 8:17 PM EET, Jarkko Sakkinen wrote:
> > > Whatever happened to this? Can you please apply my patch if you don't
> > > have the time for further rework.
> >
> > Sorry unintentional.
> >
> > I applied with
> >
> > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > +static void __iomem *atmel_get_base_addr(unsigned long *base, int *region_size)
> >
> > as this gives checkpatch error.
>
> See:
> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b18ffd5e0faaa02bffda61e19a013573451008d4
>
> If that looks good to you, I can mirror it to -next.

Looks good. Thanks.

Rob

Re: [PATCH] tpm: atmel: Drop PPC64 specific MMIO setup

[Jarkko Sakkinen]

On Wed Nov 6, 2024 at 8:32 PM EET, Rob Herring wrote:
> On Wed, Nov 6, 2024 at 12:19 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Wed Nov 6, 2024 at 8:17 PM EET, Jarkko Sakkinen wrote:
> > > > Whatever happened to this? Can you please apply my patch if you don't
> > > > have the time for further rework.
> > >
> > > Sorry unintentional.
> > >
> > > I applied with
> > >
> > > -static void __iomem * atmel_get_base_addr(unsigned long *base, int *region_size)
> > > +static void __iomem *atmel_get_base_addr(unsigned long *base, int *region_size)
> > >
> > > as this gives checkpatch error.
> >
> > See:
> > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=b18ffd5e0faaa02bffda61e19a013573451008d4
> >
> > If that looks good to you, I can mirror it to -next.
>
> Looks good. Thanks.

Ya, an my sincere apologies!

>
> Rob


BR, Jarkko