[PATCH 14/16] x86/amd_smn, platform/x86/amd/hsmp: Have HSMP use SMN

Yazen Ghannam posted 16 patches 1 month ago
[PATCH 14/16] x86/amd_smn, platform/x86/amd/hsmp: Have HSMP use SMN
Posted by Yazen Ghannam 1 month ago
The HSMP interface is just an SMN interface with different offsets.

Define an HSMP wrapper in the SMN code and have the HSMP platform driver
use that rather than a local solution.

Also, remove the "root" member from AMD_NB, since there are no more
users of it.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/include/asm/amd_nb.h    |  1 -
 arch/x86/include/asm/amd_smn.h   |  3 +++
 arch/x86/kernel/amd_nb.c         |  1 -
 arch/x86/kernel/amd_smn.c        |  9 +++++++++
 drivers/platform/x86/amd/Kconfig |  2 +-
 drivers/platform/x86/amd/hsmp.c  | 32 +++++---------------------------
 6 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index 55c03d3495bc..cbe31e316e39 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -27,7 +27,6 @@ struct amd_l3_cache {
 };
 
 struct amd_northbridge {
-	struct pci_dev *root;
 	struct pci_dev *misc;
 	struct pci_dev *link;
 	struct amd_l3_cache l3_cache;
diff --git a/arch/x86/include/asm/amd_smn.h b/arch/x86/include/asm/amd_smn.h
index 6850de69f863..f0eb12859c42 100644
--- a/arch/x86/include/asm/amd_smn.h
+++ b/arch/x86/include/asm/amd_smn.h
@@ -8,4 +8,7 @@
 int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
 int __must_check amd_smn_write(u16 node, u32 address, u32 value);
 
+/* Should only be used by the HSMP driver. */
+int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write);
+
 #endif /* _ASM_X86_AMD_SMN_H */
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 10cdeddeda02..4c22317a6dfe 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -73,7 +73,6 @@ static int amd_cache_northbridges(void)
 	amd_northbridges.nb = nb;
 
 	for (i = 0; i < amd_northbridges.num; i++) {
-		node_to_amd_nb(i)->root = amd_node_get_root(i);
 		node_to_amd_nb(i)->misc = amd_node_get_func(i, 3);
 		node_to_amd_nb(i)->link = amd_node_get_func(i, 4);
 	}
diff --git a/arch/x86/kernel/amd_smn.c b/arch/x86/kernel/amd_smn.c
index 997fd3edd9c0..527dda8e3a2b 100644
--- a/arch/x86/kernel/amd_smn.c
+++ b/arch/x86/kernel/amd_smn.c
@@ -18,6 +18,9 @@ static DEFINE_MUTEX(smn_mutex);
 #define SMN_INDEX_OFFSET	0x60
 #define SMN_DATA_OFFSET		0x64
 
+#define HSMP_INDEX_OFFSET	0xc4
+#define HSMP_DATA_OFFSET	0xc8
+
 /*
  * SMN accesses may fail in ways that are difficult to detect here in the called
  * functions amd_smn_read() and amd_smn_write(). Therefore, callers must do
@@ -100,6 +103,12 @@ int __must_check amd_smn_write(u16 node, u32 address, u32 value)
 }
 EXPORT_SYMBOL_GPL(amd_smn_write);
 
+int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write)
+{
+	return __amd_smn_rw(HSMP_INDEX_OFFSET, HSMP_DATA_OFFSET, node, address, value, write);
+}
+EXPORT_SYMBOL_GPL(amd_smn_hsmp_rdwr);
+
 static int amd_cache_roots(void)
 {
 	u16 node, num_nodes = amd_num_nodes();
diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
index f88682d36447..e100b315c62b 100644
--- a/drivers/platform/x86/amd/Kconfig
+++ b/drivers/platform/x86/amd/Kconfig
@@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
 
 config AMD_HSMP
 	tristate "AMD HSMP Driver"
-	depends on AMD_NB && X86_64 && ACPI
+	depends on AMD_SMN && X86_64 && ACPI
 	help
 	  The driver provides a way for user space tools to monitor and manage
 	  system management functionality on EPYC server CPUs from AMD.
diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 8fcf38eed7f0..544efb0255c0 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -10,7 +10,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <asm/amd_hsmp.h>
-#include <asm/amd_nb.h>
+#include <asm/amd_smn.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/miscdevice.h>
@@ -48,9 +48,6 @@
 #define SMN_HSMP_MSG_RESP	0x0010980
 #define SMN_HSMP_MSG_DATA	0x00109E0
 
-#define HSMP_INDEX_REG		0xc4
-#define HSMP_DATA_REG		0xc8
-
 #define HSMP_CDEV_NAME		"hsmp_cdev"
 #define HSMP_DEVNODE_NAME	"hsmp"
 #define HSMP_METRICS_TABLE_NAME	"metrics_bin"
@@ -62,8 +59,6 @@
 #define MSG_ARGOFF_STR		"MsgArgOffset"
 #define MSG_RESPOFF_STR		"MsgRspOffset"
 
-#define MAX_AMD_SOCKETS 8
-
 struct hsmp_mbaddr_info {
 	u32 base_addr;
 	u32 msg_id_off;
@@ -79,7 +74,6 @@ struct hsmp_socket {
 	void __iomem *virt_base_addr;
 	struct semaphore hsmp_sem;
 	char name[HSMP_ATTR_GRP_NAME_SIZE];
-	struct pci_dev *root;
 	struct device *dev;
 	u16 sock_ind;
 };
@@ -98,20 +92,7 @@ static struct hsmp_plat_device plat_dev;
 static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
 			     u32 *value, bool write)
 {
-	int ret;
-
-	if (!sock->root)
-		return -ENODEV;
-
-	ret = pci_write_config_dword(sock->root, HSMP_INDEX_REG,
-				     sock->mbinfo.base_addr + offset);
-	if (ret)
-		return ret;
-
-	ret = (write ? pci_write_config_dword(sock->root, HSMP_DATA_REG, *value)
-		     : pci_read_config_dword(sock->root, HSMP_DATA_REG, value));
-
-	return ret;
+	return amd_smn_hsmp_rdwr(sock->sock_ind, sock->mbinfo.base_addr + offset, value, write);
 }
 
 static void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
@@ -749,10 +730,7 @@ static int init_platform_device(struct device *dev)
 	int ret, i;
 
 	for (i = 0; i < plat_dev.num_sockets; i++) {
-		if (!node_to_amd_nb(i))
-			return -ENODEV;
 		sock = &plat_dev.sock[i];
-		sock->root			= node_to_amd_nb(i)->root;
 		sock->sock_ind			= i;
 		sock->dev			= dev;
 		sock->mbinfo.base_addr		= SMN_HSMP_BASE;
@@ -946,11 +924,11 @@ static int __init hsmp_plt_init(void)
 	int ret = -ENODEV;
 
 	/*
-	 * amd_nb_num() returns number of SMN/DF interfaces present in the system
+	 * amd_num_nodes() returns number of SMN/DF interfaces present in the system
 	 * if we have N SMN/DF interfaces that ideally means N sockets
 	 */
-	plat_dev.num_sockets = amd_nb_num();
-	if (plat_dev.num_sockets == 0 || plat_dev.num_sockets > MAX_AMD_SOCKETS)
+	plat_dev.num_sockets = amd_num_nodes();
+	if (plat_dev.num_sockets == 0 || plat_dev.num_sockets > MAX_AMD_NUM_NODES)
 		return ret;
 
 	ret = platform_driver_register(&amd_hsmp_driver);
-- 
2.43.0
Re: [PATCH 14/16] x86/amd_smn, platform/x86/amd/hsmp: Have HSMP use SMN
Posted by Ilpo Järvinen 1 month ago
On Wed, 23 Oct 2024, Yazen Ghannam wrote:

> The HSMP interface is just an SMN interface with different offsets.
> 
> Define an HSMP wrapper in the SMN code and have the HSMP platform driver
> use that rather than a local solution.
> 
> Also, remove the "root" member from AMD_NB, since there are no more
> users of it.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/include/asm/amd_nb.h    |  1 -
>  arch/x86/include/asm/amd_smn.h   |  3 +++
>  arch/x86/kernel/amd_nb.c         |  1 -
>  arch/x86/kernel/amd_smn.c        |  9 +++++++++
>  drivers/platform/x86/amd/Kconfig |  2 +-
>  drivers/platform/x86/amd/hsmp.c  | 32 +++++---------------------------
>  6 files changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> index 55c03d3495bc..cbe31e316e39 100644
> --- a/arch/x86/include/asm/amd_nb.h
> +++ b/arch/x86/include/asm/amd_nb.h
> @@ -27,7 +27,6 @@ struct amd_l3_cache {
>  };
>  
>  struct amd_northbridge {
> -	struct pci_dev *root;
>  	struct pci_dev *misc;
>  	struct pci_dev *link;
>  	struct amd_l3_cache l3_cache;
> diff --git a/arch/x86/include/asm/amd_smn.h b/arch/x86/include/asm/amd_smn.h
> index 6850de69f863..f0eb12859c42 100644
> --- a/arch/x86/include/asm/amd_smn.h
> +++ b/arch/x86/include/asm/amd_smn.h
> @@ -8,4 +8,7 @@
>  int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
>  int __must_check amd_smn_write(u16 node, u32 address, u32 value);
>  
> +/* Should only be used by the HSMP driver. */
> +int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write);
> +
>  #endif /* _ASM_X86_AMD_SMN_H */
> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> index 10cdeddeda02..4c22317a6dfe 100644
> --- a/arch/x86/kernel/amd_nb.c
> +++ b/arch/x86/kernel/amd_nb.c
> @@ -73,7 +73,6 @@ static int amd_cache_northbridges(void)
>  	amd_northbridges.nb = nb;
>  
>  	for (i = 0; i < amd_northbridges.num; i++) {
> -		node_to_amd_nb(i)->root = amd_node_get_root(i);
>  		node_to_amd_nb(i)->misc = amd_node_get_func(i, 3);
>  		node_to_amd_nb(i)->link = amd_node_get_func(i, 4);
>  	}
> diff --git a/arch/x86/kernel/amd_smn.c b/arch/x86/kernel/amd_smn.c
> index 997fd3edd9c0..527dda8e3a2b 100644
> --- a/arch/x86/kernel/amd_smn.c
> +++ b/arch/x86/kernel/amd_smn.c
> @@ -18,6 +18,9 @@ static DEFINE_MUTEX(smn_mutex);
>  #define SMN_INDEX_OFFSET	0x60
>  #define SMN_DATA_OFFSET		0x64
>  
> +#define HSMP_INDEX_OFFSET	0xc4
> +#define HSMP_DATA_OFFSET	0xc8
> +
>  /*
>   * SMN accesses may fail in ways that are difficult to detect here in the called
>   * functions amd_smn_read() and amd_smn_write(). Therefore, callers must do
> @@ -100,6 +103,12 @@ int __must_check amd_smn_write(u16 node, u32 address, u32 value)
>  }
>  EXPORT_SYMBOL_GPL(amd_smn_write);
>  
> +int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write)
> +{
> +	return __amd_smn_rw(HSMP_INDEX_OFFSET, HSMP_DATA_OFFSET, node, address, value, write);
> +}
> +EXPORT_SYMBOL_GPL(amd_smn_hsmp_rdwr);
> +
>  static int amd_cache_roots(void)
>  {
>  	u16 node, num_nodes = amd_num_nodes();
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index f88682d36447..e100b315c62b 100644
> --- a/drivers/platform/x86/amd/Kconfig
> +++ b/drivers/platform/x86/amd/Kconfig
> @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
>  
>  config AMD_HSMP
>  	tristate "AMD HSMP Driver"
> -	depends on AMD_NB && X86_64 && ACPI
> +	depends on AMD_SMN && X86_64 && ACPI
>  	help
>  	  The driver provides a way for user space tools to monitor and manage
>  	  system management functionality on EPYC server CPUs from AMD.
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 8fcf38eed7f0..544efb0255c0 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c

FYI, there has been major restructuring done for this driver in 
pdx86/for-next.

-- 
 i.

> @@ -10,7 +10,7 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <asm/amd_hsmp.h>
> -#include <asm/amd_nb.h>
> +#include <asm/amd_smn.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/miscdevice.h>
> @@ -48,9 +48,6 @@
>  #define SMN_HSMP_MSG_RESP	0x0010980
>  #define SMN_HSMP_MSG_DATA	0x00109E0
>  
> -#define HSMP_INDEX_REG		0xc4
> -#define HSMP_DATA_REG		0xc8
> -
>  #define HSMP_CDEV_NAME		"hsmp_cdev"
>  #define HSMP_DEVNODE_NAME	"hsmp"
>  #define HSMP_METRICS_TABLE_NAME	"metrics_bin"
> @@ -62,8 +59,6 @@
>  #define MSG_ARGOFF_STR		"MsgArgOffset"
>  #define MSG_RESPOFF_STR		"MsgRspOffset"
>  
> -#define MAX_AMD_SOCKETS 8
> -
>  struct hsmp_mbaddr_info {
>  	u32 base_addr;
>  	u32 msg_id_off;
> @@ -79,7 +74,6 @@ struct hsmp_socket {
>  	void __iomem *virt_base_addr;
>  	struct semaphore hsmp_sem;
>  	char name[HSMP_ATTR_GRP_NAME_SIZE];
> -	struct pci_dev *root;
>  	struct device *dev;
>  	u16 sock_ind;
>  };
> @@ -98,20 +92,7 @@ static struct hsmp_plat_device plat_dev;
>  static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
>  			     u32 *value, bool write)
>  {
> -	int ret;
> -
> -	if (!sock->root)
> -		return -ENODEV;
> -
> -	ret = pci_write_config_dword(sock->root, HSMP_INDEX_REG,
> -				     sock->mbinfo.base_addr + offset);
> -	if (ret)
> -		return ret;
> -
> -	ret = (write ? pci_write_config_dword(sock->root, HSMP_DATA_REG, *value)
> -		     : pci_read_config_dword(sock->root, HSMP_DATA_REG, value));
> -
> -	return ret;
> +	return amd_smn_hsmp_rdwr(sock->sock_ind, sock->mbinfo.base_addr + offset, value, write);
>  }
>  
>  static void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
> @@ -749,10 +730,7 @@ static int init_platform_device(struct device *dev)
>  	int ret, i;
>  
>  	for (i = 0; i < plat_dev.num_sockets; i++) {
> -		if (!node_to_amd_nb(i))
> -			return -ENODEV;
>  		sock = &plat_dev.sock[i];
> -		sock->root			= node_to_amd_nb(i)->root;
>  		sock->sock_ind			= i;
>  		sock->dev			= dev;
>  		sock->mbinfo.base_addr		= SMN_HSMP_BASE;
> @@ -946,11 +924,11 @@ static int __init hsmp_plt_init(void)
>  	int ret = -ENODEV;
>  
>  	/*
> -	 * amd_nb_num() returns number of SMN/DF interfaces present in the system
> +	 * amd_num_nodes() returns number of SMN/DF interfaces present in the system
>  	 * if we have N SMN/DF interfaces that ideally means N sockets
>  	 */
> -	plat_dev.num_sockets = amd_nb_num();
> -	if (plat_dev.num_sockets == 0 || plat_dev.num_sockets > MAX_AMD_SOCKETS)
> +	plat_dev.num_sockets = amd_num_nodes();
> +	if (plat_dev.num_sockets == 0 || plat_dev.num_sockets > MAX_AMD_NUM_NODES)
>  		return ret;
>  
>  	ret = platform_driver_register(&amd_hsmp_driver);
>
Re: [PATCH 14/16] x86/amd_smn, platform/x86/amd/hsmp: Have HSMP use SMN
Posted by Yazen Ghannam 1 month ago
On Thu, Oct 24, 2024 at 04:23:55PM +0300, Ilpo Järvinen wrote:
> On Wed, 23 Oct 2024, Yazen Ghannam wrote:
> 
> > The HSMP interface is just an SMN interface with different offsets.
> > 
> > Define an HSMP wrapper in the SMN code and have the HSMP platform driver
> > use that rather than a local solution.
> > 
> > Also, remove the "root" member from AMD_NB, since there are no more
> > users of it.
> > 
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> >  arch/x86/include/asm/amd_nb.h    |  1 -
> >  arch/x86/include/asm/amd_smn.h   |  3 +++
> >  arch/x86/kernel/amd_nb.c         |  1 -
> >  arch/x86/kernel/amd_smn.c        |  9 +++++++++
> >  drivers/platform/x86/amd/Kconfig |  2 +-
> >  drivers/platform/x86/amd/hsmp.c  | 32 +++++---------------------------
> >  6 files changed, 18 insertions(+), 30 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> > index 55c03d3495bc..cbe31e316e39 100644
> > --- a/arch/x86/include/asm/amd_nb.h
> > +++ b/arch/x86/include/asm/amd_nb.h
> > @@ -27,7 +27,6 @@ struct amd_l3_cache {
> >  };
> >  
> >  struct amd_northbridge {
> > -	struct pci_dev *root;
> >  	struct pci_dev *misc;
> >  	struct pci_dev *link;
> >  	struct amd_l3_cache l3_cache;
> > diff --git a/arch/x86/include/asm/amd_smn.h b/arch/x86/include/asm/amd_smn.h
> > index 6850de69f863..f0eb12859c42 100644
> > --- a/arch/x86/include/asm/amd_smn.h
> > +++ b/arch/x86/include/asm/amd_smn.h
> > @@ -8,4 +8,7 @@
> >  int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
> >  int __must_check amd_smn_write(u16 node, u32 address, u32 value);
> >  
> > +/* Should only be used by the HSMP driver. */
> > +int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write);
> > +
> >  #endif /* _ASM_X86_AMD_SMN_H */
> > diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> > index 10cdeddeda02..4c22317a6dfe 100644
> > --- a/arch/x86/kernel/amd_nb.c
> > +++ b/arch/x86/kernel/amd_nb.c
> > @@ -73,7 +73,6 @@ static int amd_cache_northbridges(void)
> >  	amd_northbridges.nb = nb;
> >  
> >  	for (i = 0; i < amd_northbridges.num; i++) {
> > -		node_to_amd_nb(i)->root = amd_node_get_root(i);
> >  		node_to_amd_nb(i)->misc = amd_node_get_func(i, 3);
> >  		node_to_amd_nb(i)->link = amd_node_get_func(i, 4);
> >  	}
> > diff --git a/arch/x86/kernel/amd_smn.c b/arch/x86/kernel/amd_smn.c
> > index 997fd3edd9c0..527dda8e3a2b 100644
> > --- a/arch/x86/kernel/amd_smn.c
> > +++ b/arch/x86/kernel/amd_smn.c
> > @@ -18,6 +18,9 @@ static DEFINE_MUTEX(smn_mutex);
> >  #define SMN_INDEX_OFFSET	0x60
> >  #define SMN_DATA_OFFSET		0x64
> >  
> > +#define HSMP_INDEX_OFFSET	0xc4
> > +#define HSMP_DATA_OFFSET	0xc8
> > +
> >  /*
> >   * SMN accesses may fail in ways that are difficult to detect here in the called
> >   * functions amd_smn_read() and amd_smn_write(). Therefore, callers must do
> > @@ -100,6 +103,12 @@ int __must_check amd_smn_write(u16 node, u32 address, u32 value)
> >  }
> >  EXPORT_SYMBOL_GPL(amd_smn_write);
> >  
> > +int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write)
> > +{
> > +	return __amd_smn_rw(HSMP_INDEX_OFFSET, HSMP_DATA_OFFSET, node, address, value, write);
> > +}
> > +EXPORT_SYMBOL_GPL(amd_smn_hsmp_rdwr);
> > +
> >  static int amd_cache_roots(void)
> >  {
> >  	u16 node, num_nodes = amd_num_nodes();
> > diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> > index f88682d36447..e100b315c62b 100644
> > --- a/drivers/platform/x86/amd/Kconfig
> > +++ b/drivers/platform/x86/amd/Kconfig
> > @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
> >  
> >  config AMD_HSMP
> >  	tristate "AMD HSMP Driver"
> > -	depends on AMD_NB && X86_64 && ACPI
> > +	depends on AMD_SMN && X86_64 && ACPI
> >  	help
> >  	  The driver provides a way for user space tools to monitor and manage
> >  	  system management functionality on EPYC server CPUs from AMD.
> > diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> > index 8fcf38eed7f0..544efb0255c0 100644
> > --- a/drivers/platform/x86/amd/hsmp.c
> > +++ b/drivers/platform/x86/amd/hsmp.c
> 
> FYI, there has been major restructuring done for this driver in 
> pdx86/for-next.
>

Yep, no problem. I can rebase these changes on top of those.

Any comments on the general approach?

Thanks,
Yazen
Re: [PATCH 14/16] x86/amd_smn, platform/x86/amd/hsmp: Have HSMP use SMN
Posted by Ilpo Järvinen 1 month ago
On Thu, 24 Oct 2024, Yazen Ghannam wrote:

> On Thu, Oct 24, 2024 at 04:23:55PM +0300, Ilpo Järvinen wrote:
> > On Wed, 23 Oct 2024, Yazen Ghannam wrote:
> > 
> > > The HSMP interface is just an SMN interface with different offsets.
> > > 
> > > Define an HSMP wrapper in the SMN code and have the HSMP platform driver
> > > use that rather than a local solution.
> > > 
> > > Also, remove the "root" member from AMD_NB, since there are no more
> > > users of it.
> > > 
> > > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > > ---
> > >  arch/x86/include/asm/amd_nb.h    |  1 -
> > >  arch/x86/include/asm/amd_smn.h   |  3 +++
> > >  arch/x86/kernel/amd_nb.c         |  1 -
> > >  arch/x86/kernel/amd_smn.c        |  9 +++++++++
> > >  drivers/platform/x86/amd/Kconfig |  2 +-
> > >  drivers/platform/x86/amd/hsmp.c  | 32 +++++---------------------------
> > >  6 files changed, 18 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> > > index 55c03d3495bc..cbe31e316e39 100644
> > > --- a/arch/x86/include/asm/amd_nb.h
> > > +++ b/arch/x86/include/asm/amd_nb.h
> > > @@ -27,7 +27,6 @@ struct amd_l3_cache {
> > >  };
> > >  
> > >  struct amd_northbridge {
> > > -	struct pci_dev *root;
> > >  	struct pci_dev *misc;
> > >  	struct pci_dev *link;
> > >  	struct amd_l3_cache l3_cache;
> > > diff --git a/arch/x86/include/asm/amd_smn.h b/arch/x86/include/asm/amd_smn.h
> > > index 6850de69f863..f0eb12859c42 100644
> > > --- a/arch/x86/include/asm/amd_smn.h
> > > +++ b/arch/x86/include/asm/amd_smn.h
> > > @@ -8,4 +8,7 @@
> > >  int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
> > >  int __must_check amd_smn_write(u16 node, u32 address, u32 value);
> > >  
> > > +/* Should only be used by the HSMP driver. */
> > > +int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write);
> > > +
> > >  #endif /* _ASM_X86_AMD_SMN_H */
> > > diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
> > > index 10cdeddeda02..4c22317a6dfe 100644
> > > --- a/arch/x86/kernel/amd_nb.c
> > > +++ b/arch/x86/kernel/amd_nb.c
> > > @@ -73,7 +73,6 @@ static int amd_cache_northbridges(void)
> > >  	amd_northbridges.nb = nb;
> > >  
> > >  	for (i = 0; i < amd_northbridges.num; i++) {
> > > -		node_to_amd_nb(i)->root = amd_node_get_root(i);
> > >  		node_to_amd_nb(i)->misc = amd_node_get_func(i, 3);
> > >  		node_to_amd_nb(i)->link = amd_node_get_func(i, 4);
> > >  	}
> > > diff --git a/arch/x86/kernel/amd_smn.c b/arch/x86/kernel/amd_smn.c
> > > index 997fd3edd9c0..527dda8e3a2b 100644
> > > --- a/arch/x86/kernel/amd_smn.c
> > > +++ b/arch/x86/kernel/amd_smn.c
> > > @@ -18,6 +18,9 @@ static DEFINE_MUTEX(smn_mutex);
> > >  #define SMN_INDEX_OFFSET	0x60
> > >  #define SMN_DATA_OFFSET		0x64
> > >  
> > > +#define HSMP_INDEX_OFFSET	0xc4
> > > +#define HSMP_DATA_OFFSET	0xc8
> > > +
> > >  /*
> > >   * SMN accesses may fail in ways that are difficult to detect here in the called
> > >   * functions amd_smn_read() and amd_smn_write(). Therefore, callers must do
> > > @@ -100,6 +103,12 @@ int __must_check amd_smn_write(u16 node, u32 address, u32 value)
> > >  }
> > >  EXPORT_SYMBOL_GPL(amd_smn_write);
> > >  
> > > +int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write)
> > > +{
> > > +	return __amd_smn_rw(HSMP_INDEX_OFFSET, HSMP_DATA_OFFSET, node, address, value, write);
> > > +}
> > > +EXPORT_SYMBOL_GPL(amd_smn_hsmp_rdwr);
> > > +
> > >  static int amd_cache_roots(void)
> > >  {
> > >  	u16 node, num_nodes = amd_num_nodes();
> > > diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> > > index f88682d36447..e100b315c62b 100644
> > > --- a/drivers/platform/x86/amd/Kconfig
> > > +++ b/drivers/platform/x86/amd/Kconfig
> > > @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
> > >  
> > >  config AMD_HSMP
> > >  	tristate "AMD HSMP Driver"
> > > -	depends on AMD_NB && X86_64 && ACPI
> > > +	depends on AMD_SMN && X86_64 && ACPI
> > >  	help
> > >  	  The driver provides a way for user space tools to monitor and manage
> > >  	  system management functionality on EPYC server CPUs from AMD.
> > > diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> > > index 8fcf38eed7f0..544efb0255c0 100644
> > > --- a/drivers/platform/x86/amd/hsmp.c
> > > +++ b/drivers/platform/x86/amd/hsmp.c
> > 
> > FYI, there has been major restructuring done for this driver in 
> > pdx86/for-next.
> >
> 
> Yep, no problem. I can rebase these changes on top of those.
> 
> Any comments on the general approach?

I deemed looking deeper into the patch waste of my time due to the 
expected changes, so no comments at this time.

-- 
 i.