[PATCH v5 0/8] Introduce initial AMD ASF Controller driver support

[PATCH v5 0/8] Introduce initial AMD ASF Controller driver support

[Shyam Sundar S K]

The AMD ASF (Alert Standard Format) function block is essentially an SMBus
controller with built-in ASF functionality. It features two pins SCL1 and
SDA1 that facilitate communication with other SMBus devices. This dual
capability allows the ASF controller to issue generic SMBus packets and
communicate with the DASH controller using MCTP over ASF. Additionally,
the ASF controller supports remote commands defined by the ASF
specification, such as shutdown, reset, power-up, and power-down, without
requiring any software interaction.

The concept is to enable a remote system to communicate with the target
system over the network. The local network controller, such as an Ethernet
MAC, receives remote packets and relays the commands to the FCH
(Fusion Controller Hub) through the ASF. Examples of these commands
include shutdown and reset. Since ASF uses the SMBus protocol, this
controller can be configured as a secondary SMBus controller.

This series of updates focuses on extending the i2c-piix4 driver to
support the ASF driver by exporting several functions from the i2c-piix4
driver, allowing the AMD ASF driver to leverage existing functionalities.
Additionally, this change incorporates core ASF functionality, including
ACPI integration and the implementation of i2c_algorithm callbacks for ASF
operations.

v5:
----
 - use platform_get_resource to the ACPI resources of ASF device
 - add relavant headers
 - remove unnecessary headers
 - use devm_* wherever applicable
 - update commit messages to patch 1 and 3 in series v4

v4:
----
 - Carve out a separate _HID driver for ASF
 - Export i2c_piix4 driver functions as library
 - Make function signature changes within i2c-pixx4 driver
 - Use dev_err_probe() in probe()
 - Address other remarks from Andy.

v3:
----
 - Fix LKP reported issue by adding 'depends on X86'
 - Drop callback when using acpi_dev_get_resources()
 - Address other remarks from Andy on v2.

v2:
----
 - Change function signature from u8 to enum
 - Use default case in switch
 - Use acpi_dev_get_resources() and drop devm_kzalloc() usage
 - Fix LKP reported issues
 - Address other minor remarks from Andy and Andi Shyti

Shyam Sundar S K (8):
  i2c: piix4: Change the parameter list of piix4_transaction function
  i2c: piix4: Move i2c_piix4 macros and structures to common header
  i2c: piix4: Export i2c_piix4 driver functions as library
  i2c: amd-asf: Add ACPI support for AMD ASF Controller
  i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with
    SMBus
  i2c: amd-asf: Add routine to handle the ASF slave process
  i2c: amd-asf: Clear remote IRR bit to get successive interrupt
  MAINTAINERS: Add AMD ASF driver entry

 MAINTAINERS                           |   8 +-
 drivers/i2c/busses/Kconfig            |  17 ++
 drivers/i2c/busses/Makefile           |   1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 372 ++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-piix4.c        |  56 ++--
 drivers/i2c/busses/i2c-piix4.h        |  45 ++++
 6 files changed, 465 insertions(+), 34 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-amd-asf-plat.c
 create mode 100644 drivers/i2c/busses/i2c-piix4.h

-- 
2.25.1

[PATCH v5 1/8] i2c: piix4: Change the parameter list of piix4_transaction function

[Shyam Sundar S K]

Currently, `piix4_transaction()` accepts only one parameter, which is the
`i2c_adapter` information. This approach works well as long as SB800 SMBus
port accesses are confined to the piix4 driver. However, with the
implementation of a separate ASF driver and the varying address spaces
across drivers, it is necessary to change the function parameter list of
`piix4_transaction()` to include the port address. This modification
allows other drivers that use piix4 to pass the specific port details they
need to operate on.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 4e32d57ae0bf..69b362db6d0c 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -536,10 +536,8 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
-static int piix4_transaction(struct i2c_adapter *piix4_adapter)
+static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
 {
-	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
-	unsigned short piix4_smba = adapdata->smba;
 	int temp;
 	int result = 0;
 	int timeout = 0;
@@ -675,7 +673,7 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	status = piix4_transaction(adap);
+	status = piix4_transaction(adap, piix4_smba);
 	if (status)
 		return status;
 
-- 
2.25.1

Re: [PATCH v5 1/8] i2c: piix4: Change the parameter list of piix4_transaction function

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:03PM +0530, Shyam Sundar S K wrote:
> Currently, `piix4_transaction()` accepts only one parameter, which is the
> `i2c_adapter` information. This approach works well as long as SB800 SMBus
> port accesses are confined to the piix4 driver. However, with the
> implementation of a separate ASF driver and the varying address spaces
> across drivers, it is necessary to change the function parameter list of
> `piix4_transaction()` to include the port address. This modification
> allows other drivers that use piix4 to pass the specific port details they
> need to operate on.

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 2/8] i2c: piix4: Move i2c_piix4 macros and structures to common header

[Shyam Sundar S K]

Add a separate header file to relocate the common code from the i2c_piix4
driver, allowing the AMD ASF driver to utilize the same code.

Update the MAINTAINERS file to include information about the new common
header file.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS                    |  2 +-
 drivers/i2c/busses/i2c-piix4.c | 24 +-------------------
 drivers/i2c/busses/i2c-piix4.h | 40 ++++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 24 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-piix4.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 10430778c998..3f348b71b9c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10611,7 +10611,7 @@ F:	drivers/i2c/busses/i2c-i801.c
 F:	drivers/i2c/busses/i2c-isch.c
 F:	drivers/i2c/busses/i2c-nforce2-s4985.c
 F:	drivers/i2c/busses/i2c-nforce2.c
-F:	drivers/i2c/busses/i2c-piix4.c
+F:	drivers/i2c/busses/i2c-piix4.*
 F:	drivers/i2c/busses/i2c-sis5595.c
 F:	drivers/i2c/busses/i2c-sis630.c
 F:	drivers/i2c/busses/i2c-sis96x.c
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 69b362db6d0c..2c2a466e2f85 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -35,23 +35,7 @@
 #include <linux/acpi.h>
 #include <linux/io.h>
 
-
-/* PIIX4 SMBus address offsets */
-#define SMBHSTSTS	(0 + piix4_smba)
-#define SMBHSLVSTS	(1 + piix4_smba)
-#define SMBHSTCNT	(2 + piix4_smba)
-#define SMBHSTCMD	(3 + piix4_smba)
-#define SMBHSTADD	(4 + piix4_smba)
-#define SMBHSTDAT0	(5 + piix4_smba)
-#define SMBHSTDAT1	(6 + piix4_smba)
-#define SMBBLKDAT	(7 + piix4_smba)
-#define SMBSLVCNT	(8 + piix4_smba)
-#define SMBSHDWCMD	(9 + piix4_smba)
-#define SMBSLVEVT	(0xA + piix4_smba)
-#define SMBSLVDAT	(0xC + piix4_smba)
-
-/* count for request_region */
-#define SMBIOSIZE	9
+#include "i2c-piix4.h"
 
 /* PCI Address Constants */
 #define SMBBA		0x090
@@ -70,7 +54,6 @@
 #define PIIX4_BYTE		0x04
 #define PIIX4_BYTE_DATA		0x08
 #define PIIX4_WORD_DATA		0x0C
-#define PIIX4_BLOCK_DATA	0x14
 
 /* Multi-port constants */
 #define PIIX4_MAX_ADAPTERS	4
@@ -160,11 +143,6 @@ static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 };
 static const char *piix4_aux_port_name_sb800 = " port 1";
 
-struct sb800_mmio_cfg {
-	void __iomem *addr;
-	bool use_mmio;
-};
-
 struct i2c_piix4_adapdata {
 	unsigned short smba;
 
diff --git a/drivers/i2c/busses/i2c-piix4.h b/drivers/i2c/busses/i2c-piix4.h
new file mode 100644
index 000000000000..c4c20edacb00
--- /dev/null
+++ b/drivers/i2c/busses/i2c-piix4.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * PIIX4/SB800 SMBus Interfaces
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *	    Sanket Goswami <Sanket.Goswami@amd.com>
+ */
+
+#ifndef I2C_PIIX4_H
+#define I2C_PIIX4_H
+
+/* PIIX4 SMBus address offsets */
+#define SMBHSTSTS	(0 + piix4_smba)
+#define SMBHSLVSTS	(1 + piix4_smba)
+#define SMBHSTCNT	(2 + piix4_smba)
+#define SMBHSTCMD	(3 + piix4_smba)
+#define SMBHSTADD	(4 + piix4_smba)
+#define SMBHSTDAT0	(5 + piix4_smba)
+#define SMBHSTDAT1	(6 + piix4_smba)
+#define SMBBLKDAT	(7 + piix4_smba)
+#define SMBSLVCNT	(8 + piix4_smba)
+#define SMBSHDWCMD	(9 + piix4_smba)
+#define SMBSLVEVT	(0xA + piix4_smba)
+#define SMBSLVDAT	(0xC + piix4_smba)
+
+/* Count for request_region */
+#define SMBIOSIZE	9
+
+/* PIIX4 constants */
+#define PIIX4_BLOCK_DATA	0x14
+
+struct sb800_mmio_cfg {
+	void __iomem *addr;
+	bool use_mmio;
+};
+
+#endif /* I2C_PIIX4_H */
-- 
2.25.1

Re: [PATCH v5 2/8] i2c: piix4: Move i2c_piix4 macros and structures to common header

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:04PM +0530, Shyam Sundar S K wrote:
> Add a separate header file to relocate the common code from the i2c_piix4
> driver, allowing the AMD ASF driver to utilize the same code.
> 
> Update the MAINTAINERS file to include information about the new common
> header file.

...

> +/* PIIX4 SMBus address offsets */
> +#define SMBHSTSTS	(0 + piix4_smba)
> +#define SMBHSLVSTS	(1 + piix4_smba)
> +#define SMBHSTCNT	(2 + piix4_smba)
> +#define SMBHSTCMD	(3 + piix4_smba)
> +#define SMBHSTADD	(4 + piix4_smba)
> +#define SMBHSTDAT0	(5 + piix4_smba)
> +#define SMBHSTDAT1	(6 + piix4_smba)
> +#define SMBBLKDAT	(7 + piix4_smba)
> +#define SMBSLVCNT	(8 + piix4_smba)
> +#define SMBSHDWCMD	(9 + piix4_smba)

While at it, I would suggest to use fixed-width values for all of them, e.g.,

#define SMBHSTADD	(0x4 + piix4_smba)

> +#define SMBSLVEVT	(0xA + piix4_smba)
> +#define SMBSLVDAT	(0xC + piix4_smba)

...

> +struct sb800_mmio_cfg {
> +	void __iomem *addr;
> +	bool use_mmio;

These two need types.h.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Shyam Sundar S K]

Export the following i2c_piix4 driver functions as a library so that the
AMD ASF driver can utilize these core functionalities from the i2c_piix4
driver:

- piix4_sb800_region_request(): Request access to a specific SMBus region
on the SB800 chipset.

- piix4_sb800_region_release(): Release the previously requested SMBus
region on the SB800 chipset.

- piix4_transaction(): Handle SMBus transactions between the SMBus
controller and connected devices.

- piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
chipset.

By making these functions available as a library, enable the AMD ASF
driver to leverage the established mechanisms in the i2c_piix4 driver,
promoting code reuse and consistency across different drivers.

Note that the git diff view is presented in two separate lines in order to
suppress the checkpatch.pl "CHECKS".

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 16 ++++++++++------
 drivers/i2c/busses/i2c-piix4.h |  5 +++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 2c2a466e2f85..7e0fb51ce532 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -153,8 +153,8 @@ struct i2c_piix4_adapdata {
 	struct sb800_mmio_cfg mmio_cfg;
 };
 
-static int piix4_sb800_region_request(struct device *dev,
-				      struct sb800_mmio_cfg *mmio_cfg)
+int piix4_sb800_region_request(struct device *dev,
+			       struct sb800_mmio_cfg *mmio_cfg)
 {
 	if (mmio_cfg->use_mmio) {
 		void __iomem *addr;
@@ -192,9 +192,10 @@ static int piix4_sb800_region_request(struct device *dev,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(piix4_sb800_region_request);
 
-static void piix4_sb800_region_release(struct device *dev,
-				       struct sb800_mmio_cfg *mmio_cfg)
+void piix4_sb800_region_release(struct device *dev,
+				struct sb800_mmio_cfg *mmio_cfg)
 {
 	if (mmio_cfg->use_mmio) {
 		iounmap(mmio_cfg->addr);
@@ -205,6 +206,7 @@ static void piix4_sb800_region_release(struct device *dev,
 
 	release_region(SB800_PIIX4_SMB_IDX, SB800_PIIX4_SMB_MAP_SIZE);
 }
+EXPORT_SYMBOL_GPL(piix4_sb800_region_release);
 
 static bool piix4_sb800_use_mmio(struct pci_dev *PIIX4_dev)
 {
@@ -514,7 +516,7 @@ static int piix4_setup_aux(struct pci_dev *PIIX4_dev,
 	return piix4_smba;
 }
 
-static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
+int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba)
 {
 	int temp;
 	int result = 0;
@@ -587,6 +589,7 @@ static int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short p
 		inb_p(SMBHSTDAT1));
 	return result;
 }
+EXPORT_SYMBOL_GPL(piix4_transaction);
 
 /* Return negative errno on error. */
 static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
@@ -740,7 +743,7 @@ static void piix4_imc_wakeup(void)
 	release_region(KERNCZ_IMC_IDX, 2);
 }
 
-static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
+int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
 {
 	u8 smba_en_lo, val;
 
@@ -762,6 +765,7 @@ static int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg)
 
 	return (smba_en_lo & piix4_port_mask_sb800);
 }
+EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);
 
 /*
  * Handles access to multiple SMBus ports on the SB800.
diff --git a/drivers/i2c/busses/i2c-piix4.h b/drivers/i2c/busses/i2c-piix4.h
index c4c20edacb00..9a5faac3eedd 100644
--- a/drivers/i2c/busses/i2c-piix4.h
+++ b/drivers/i2c/busses/i2c-piix4.h
@@ -37,4 +37,9 @@ struct sb800_mmio_cfg {
 	bool use_mmio;
 };
 
+int piix4_sb800_port_sel(u8 port, struct sb800_mmio_cfg *mmio_cfg);
+int piix4_transaction(struct i2c_adapter *piix4_adapter, unsigned short piix4_smba);
+int piix4_sb800_region_request(struct device *dev, struct sb800_mmio_cfg *mmio_cfg);
+void piix4_sb800_region_release(struct device *dev, struct sb800_mmio_cfg *mmio_cfg);
+
 #endif /* I2C_PIIX4_H */
-- 
2.25.1

Re: [PATCH v5 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:05PM +0530, Shyam Sundar S K wrote:
> Export the following i2c_piix4 driver functions as a library so that the
> AMD ASF driver can utilize these core functionalities from the i2c_piix4
> driver:
> 
> - piix4_sb800_region_request(): Request access to a specific SMBus region
> on the SB800 chipset.
> 
> - piix4_sb800_region_release(): Release the previously requested SMBus
> region on the SB800 chipset.
> 
> - piix4_transaction(): Handle SMBus transactions between the SMBus
> controller and connected devices.
> 
> - piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
> chipset.
> 
> By making these functions available as a library, enable the AMD ASF
> driver to leverage the established mechanisms in the i2c_piix4 driver,
> promoting code reuse and consistency across different drivers.

> Note that the git diff view is presented in two separate lines in order to
> suppress the checkpatch.pl "CHECKS".

This paragraph should be in comment block rather than commit message body...

> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---

...somewhere here.

...

> +int piix4_sb800_region_request(struct device *dev,
> +			       struct sb800_mmio_cfg *mmio_cfg)

One line?

...

> +EXPORT_SYMBOL_GPL(piix4_sb800_region_request);

Use namespaced exports (with _NS) from day 1.

...

> +void piix4_sb800_region_release(struct device *dev,
> +				struct sb800_mmio_cfg *mmio_cfg)

> +EXPORT_SYMBOL_GPL(piix4_sb800_region_release);

Same comments as per above.

...

> +EXPORT_SYMBOL_GPL(piix4_transaction);
> +EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);

_NS

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Shyam Sundar S K]

On 9/14/2024 00:24, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:05PM +0530, Shyam Sundar S K wrote:
>> Export the following i2c_piix4 driver functions as a library so that the
>> AMD ASF driver can utilize these core functionalities from the i2c_piix4
>> driver:
>>
>> - piix4_sb800_region_request(): Request access to a specific SMBus region
>> on the SB800 chipset.
>>
>> - piix4_sb800_region_release(): Release the previously requested SMBus
>> region on the SB800 chipset.
>>
>> - piix4_transaction(): Handle SMBus transactions between the SMBus
>> controller and connected devices.
>>
>> - piix4_sb800_port_sel(): Select the appropriate SMBus port on the SB800
>> chipset.
>>
>> By making these functions available as a library, enable the AMD ASF
>> driver to leverage the established mechanisms in the i2c_piix4 driver,
>> promoting code reuse and consistency across different drivers.
> 
>> Note that the git diff view is presented in two separate lines in order to
>> suppress the checkpatch.pl "CHECKS".
> 
> This paragraph should be in comment block rather than commit message body...
> 

I can move it to comment block but in the last version Andi mentioned
that I have to leave a note about the function within one line.

>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
> 
> ...somewhere here.
> 
> ...
> 
>> +int piix4_sb800_region_request(struct device *dev,
>> +			       struct sb800_mmio_cfg *mmio_cfg)
> 
> One line?
> 

I am OK to do it, but Andi has a preference to stay within 80
character wide length.

Andi, what are you thoughts?

Thanks,
Shyam

> ...
> 
>> +EXPORT_SYMBOL_GPL(piix4_sb800_region_request);
> 
> Use namespaced exports (with _NS) from day 1.
> 
> ...
> 
>> +void piix4_sb800_region_release(struct device *dev,
>> +				struct sb800_mmio_cfg *mmio_cfg)
> 
>> +EXPORT_SYMBOL_GPL(piix4_sb800_region_release);
> 
> Same comments as per above.
> 
> ...
> 
>> +EXPORT_SYMBOL_GPL(piix4_transaction);
>> +EXPORT_SYMBOL_GPL(piix4_sb800_port_sel);
> 
> _NS
> 

Re: [PATCH v5 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Andy Shevchenko]

On Tue, Sep 17, 2024 at 11:44:04PM +0530, Shyam Sundar S K wrote:
> On 9/14/2024 00:24, Andy Shevchenko wrote:
> > On Fri, Sep 13, 2024 at 05:41:05PM +0530, Shyam Sundar S K wrote:

...

> >> Note that the git diff view is presented in two separate lines in order to
> >> suppress the checkpatch.pl "CHECKS".

(1) ^^^

> > This paragraph should be in comment block rather than commit message body...
> 
> I can move it to comment block but in the last version Andi mentioned
> that I have to leave a note about the function within one line.

What function? I'm talking about (1) which refers to Git and checkpatch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Shyam Sundar S K]

On 9/18/2024 15:26, Andy Shevchenko wrote:
> On Tue, Sep 17, 2024 at 11:44:04PM +0530, Shyam Sundar S K wrote:
>> On 9/14/2024 00:24, Andy Shevchenko wrote:
>>> On Fri, Sep 13, 2024 at 05:41:05PM +0530, Shyam Sundar S K wrote:
> 
> ...
> 
>>>> Note that the git diff view is presented in two separate lines in order to
>>>> suppress the checkpatch.pl "CHECKS".
> 
> (1) ^^^
> 
>>> This paragraph should be in comment block rather than commit message body...
>>
>> I can move it to comment block but in the last version Andi mentioned
>> that I have to leave a note about the function within one line.
> 
> What function? I'm talking about (1) which refers to Git and checkpatch.
> 

There was a remark on this function to make it into a single line.

+int piix4_sb800_region_request(struct device *dev,
+			       struct sb800_mmio_cfg *mmio_cfg)


Anyways, let me not confuse more. I will put the paragraph what you
pointed in the comment block.

Thanks,
Shyam

Re: [PATCH v5 3/8] i2c: piix4: Export i2c_piix4 driver functions as library

[Andi Shyti]

Hi Shyam,

...

> >> Note that the git diff view is presented in two separate lines in order to
> >> suppress the checkpatch.pl "CHECKS".
> > 
> > This paragraph should be in comment block rather than commit message body...
> > 
> 
> I can move it to comment block but in the last version Andi mentioned
> that I have to leave a note about the function within one line.
> 
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> > 
> > ...somewhere here.
> > 
> > ...
> > 
> >> +int piix4_sb800_region_request(struct device *dev,
> >> +			       struct sb800_mmio_cfg *mmio_cfg)
> > 
> > One line?
> > 
> 
> I am OK to do it, but Andi has a preference to stay within 80
> character wide length.
> 
> Andi, what are you thoughts?

I apologize for my earlier review of v4. I failed to notice that
you had removed the "static" (which you had properly described).
I mistakenly thought you had made an unrelated line adjustment.

I'm sorry for requesting a new version. Please feel free to use
the format you prefer.

Andi

PS: I still prefer 80 characters per line, but this is just a
personal, non-binding preference.

[PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Shyam Sundar S K]

The AMD ASF controller is presented to the operating system as an ACPI
device. The AMD ASF driver can use ACPI to obtain information about the
ASF controller's attributes, such as the ASF address space and interrupt
number, and to handle ASF interrupts.

Currently, the piix4 driver assumes that a specific port address is
designated for AUX operations. However, with the introduction of ASF, the
same port address may also be used by the ASF controller. Therefore, a
check needs to be added to ensure that if ASF is advertised and enabled in
ACPI, the AUX port should not be configured.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/Kconfig            | 16 ++++++
 drivers/i2c/busses/Makefile           |  1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 73 +++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-piix4.c        | 12 ++++-
 4 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i2c/busses/i2c-amd-asf-plat.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..03afcdbff209 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -95,6 +95,22 @@ config I2C_AMD_MP2
 	  This driver can also be built as modules.  If so, the modules will
 	  be called i2c-amd-mp2-pci and i2c-amd-mp2-plat.
 
+config I2C_AMD_ASF
+	tristate "AMD ASF I2C Controller Support"
+	depends on I2C_PIIX4
+	help
+	  This option enables support for the AMD ASF (Alert Standard Format)
+	  I2C controller. The AMD ASF controller is an SMBus controller with
+	  built-in ASF functionality, allowing it to issue generic SMBus
+	  packets and communicate with the DASH controller using MCTP over
+	  ASF.
+
+	  If you have an AMD system with ASF support and want to enable this
+	  functionality, say Y or M here. If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called i2c_amd_asf_plat.
+
 config I2C_HIX5HD2
 	tristate "Hix5hd2 high-speed I2C driver"
 	depends on ARCH_HISI || ARCH_HIX5HD2 || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..74920380a337 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 # Embedded system I2C/SMBus host controller drivers
 obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
 obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
+obj-$(CONFIG_I2C_AMD_ASF)	+= i2c-amd-asf-plat.o
 obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 i2c-at91-objs			:= i2c-at91-core.o i2c-at91-master.o
diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
new file mode 100644
index 000000000000..3d80805a7a3e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * AMD Alert Standard Format Platform Driver
+ *
+ * Copyright (c) 2024, Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
+ *	    Sanket Goswami <Sanket.Goswami@amd.com>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/sprintf.h>
+#include "i2c-piix4.h"
+
+static const char *amd_asf_port_name = " port 1";
+
+struct amd_asf_dev {
+	struct i2c_adapter adap;
+	struct device *dev;
+	struct sb800_mmio_cfg mmio_cfg;
+	struct resource *port_addr;
+};
+
+static int amd_asf_probe(struct platform_device *pdev)
+{
+	struct amd_asf_dev *asf_dev;
+
+	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
+	if (!asf_dev)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
+
+	asf_dev->dev = &pdev->dev;
+	asf_dev->mmio_cfg.use_mmio = true;
+	platform_set_drvdata(pdev, asf_dev);
+
+	asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!asf_dev->port_addr)
+		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
+
+	asf_dev->adap.owner = THIS_MODULE;
+	asf_dev->adap.dev.parent = &pdev->dev;
+
+	i2c_set_adapdata(&asf_dev->adap, asf_dev);
+	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
+		 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
+
+	return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
+}
+
+static const struct acpi_device_id amd_asf_acpi_ids[] = {
+	{ "AMDI001A" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, amd_asf_acpi_ids);
+
+static struct platform_driver amd_asf_driver = {
+	.driver = {
+		.name = "i2c-amd-asf",
+		.acpi_match_table = amd_asf_acpi_ids,
+	},
+	.probe = amd_asf_probe,
+};
+module_platform_driver(amd_asf_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD Alert Standard Format Driver");
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 7e0fb51ce532..f4a65344b481 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -84,6 +84,7 @@
 
 #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE			8
+#define SB800_ASF_ACPI_PATH			"\\_SB.ASFC"
 
 /* insmod parameters */
 
@@ -1023,6 +1024,9 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
 	bool is_sb800 = false;
+	bool is_asf = false;
+	acpi_status status;
+	acpi_handle handle;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
@@ -1085,10 +1089,16 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		}
 	}
 
+	status = acpi_get_handle(NULL, (acpi_string)SB800_ASF_ACPI_PATH, &handle);
+	if (ACPI_SUCCESS(status))
+		is_asf = true;
+
 	if (dev->vendor == PCI_VENDOR_ID_AMD &&
 	    (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
 	     dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)) {
-		retval = piix4_setup_sb800(dev, id, 1);
+		/* Do not setup AUX port if ASF is enabled */
+		if (!is_asf)
+			retval = piix4_setup_sb800(dev, id, 1);
 	}
 
 	if (retval > 0) {
-- 
2.25.1

Re: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:06PM +0530, Shyam Sundar S K wrote:
> The AMD ASF controller is presented to the operating system as an ACPI
> device. The AMD ASF driver can use ACPI to obtain information about the
> ASF controller's attributes, such as the ASF address space and interrupt
> number, and to handle ASF interrupts.
> 
> Currently, the piix4 driver assumes that a specific port address is
> designated for AUX operations. However, with the introduction of ASF, the
> same port address may also be used by the ASF controller. Therefore, a
> check needs to be added to ensure that if ASF is advertised and enabled in
> ACPI, the AUX port should not be configured.

...

> +#include <linux/device.h>
> +#include <linux/errno.h>

+ gfp_types.h

> +#include <linux/io.h>
> +#include <linux/i2c.h>

Keep them ordered.

+ ioport.h // you use definitions from there

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/sprintf.h>

...

> +static const char *amd_asf_port_name = " port 1";

It's a bit counter intuitive to have a leading space in the constants like
this...

...

> +struct amd_asf_dev {
> +	struct i2c_adapter adap;

> +	struct device *dev;

So, this is a dup of adap.dev.parent. Do you really need this shortcut?

> +	struct sb800_mmio_cfg mmio_cfg;
> +	struct resource *port_addr;
> +};

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

With

	struct device *dev = &pdev->dev;

the following lines can be made shorter.

> +	struct amd_asf_dev *asf_dev;
> +
> +	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
> +	if (!asf_dev)
> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
> +
> +	asf_dev->dev = &pdev->dev;
> +	asf_dev->mmio_cfg.use_mmio = true;

> +	platform_set_drvdata(pdev, asf_dev);

I believe this won't be needed, see the respective email reply in the series.

> +	asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +	if (!asf_dev->port_addr)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
> +
> +	asf_dev->adap.owner = THIS_MODULE;
> +	asf_dev->adap.dev.parent = &pdev->dev;
> +
> +	i2c_set_adapdata(&asf_dev->adap, asf_dev);
> +	snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
> +		 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
> +
> +	return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[kernel test robot]

Hi Shyam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.11-rc7 next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shyam-Sundar-S-K/i2c-piix4-Change-the-parameter-list-of-piix4_transaction-function/20240913-201353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240913121110.1611340-5-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240914/202409140624.TOshFT39-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140624.TOshFT39-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409140624.TOshFT39-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-amd-asf-plat.c: In function 'amd_asf_probe':
>> drivers/i2c/busses/i2c-amd-asf-plat.c:52:47: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
      52 |                  "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
         |                                            ~~~^                      ~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                               |                                        |
         |                                               long long unsigned int                   resource_size_t {aka unsigned int}
         |                                            %x


vim +52 drivers/i2c/busses/i2c-amd-asf-plat.c

    30	
    31	static int amd_asf_probe(struct platform_device *pdev)
    32	{
    33		struct amd_asf_dev *asf_dev;
    34	
    35		asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
    36		if (!asf_dev)
    37			return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
    38	
    39		asf_dev->dev = &pdev->dev;
    40		asf_dev->mmio_cfg.use_mmio = true;
    41		platform_set_drvdata(pdev, asf_dev);
    42	
    43		asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
    44		if (!asf_dev->port_addr)
    45			return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
    46	
    47		asf_dev->adap.owner = THIS_MODULE;
    48		asf_dev->adap.dev.parent = &pdev->dev;
    49	
    50		i2c_set_adapdata(&asf_dev->adap, asf_dev);
    51		snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
  > 52			 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
    53	
    54		return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
    55	}
    56	

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

Re: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller

[kernel test robot]

Hi Shyam,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.11 next-20240919]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shyam-Sundar-S-K/i2c-piix4-Change-the-parameter-list-of-piix4_transaction-function/20240913-201353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240913121110.1611340-5-Shyam-sundar.S-k%40amd.com
patch subject: [PATCH v5 4/8] i2c: amd-asf: Add ACPI support for AMD ASF Controller
config: arm-randconfig-003-20240919 (https://download.01.org/0day-ci/archive/20240919/202409192333.P0Nve307-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240919/202409192333.P0Nve307-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409192333.P0Nve307-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/i2c/busses/i2c-amd-asf-plat.c:15:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2232:
   include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     517 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/i2c/busses/i2c-amd-asf-plat.c:52:56: warning: format specifies type 'unsigned long long' but the argument has type 'resource_size_t' (aka 'unsigned int') [-Wformat]
      52 |                  "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
         |                                            ~~~~                      ^~~~~~~~~~~~~~~~~~~~~~~~~
         |                                            %x
   2 warnings generated.


vim +52 drivers/i2c/busses/i2c-amd-asf-plat.c

    30	
    31	static int amd_asf_probe(struct platform_device *pdev)
    32	{
    33		struct amd_asf_dev *asf_dev;
    34	
    35		asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
    36		if (!asf_dev)
    37			return dev_err_probe(&pdev->dev, -ENOMEM, "Failed to allocate memory\n");
    38	
    39		asf_dev->dev = &pdev->dev;
    40		asf_dev->mmio_cfg.use_mmio = true;
    41		platform_set_drvdata(pdev, asf_dev);
    42	
    43		asf_dev->port_addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
    44		if (!asf_dev->port_addr)
    45			return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
    46	
    47		asf_dev->adap.owner = THIS_MODULE;
    48		asf_dev->adap.dev.parent = &pdev->dev;
    49	
    50		i2c_set_adapdata(&asf_dev->adap, asf_dev);
    51		snprintf(asf_dev->adap.name, sizeof(asf_dev->adap.name),
  > 52			 "SMBus ASF adapter%s at 0x%llx", amd_asf_port_name, asf_dev->port_addr->start);
    53	
    54		return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
    55	}
    56	

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

[PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K]

Implement the i2c_algorithm operations to enable support for AMD ASF
(Alert Standard Format) with SMBus. This enhancement includes:

- Adding functionality to identify and select the supported ASF functions.
- Implementing mechanisms for registering and deregistering I2C slave
  devices.
- Providing support for data transfer operations over ASF.

Additionally, include a 'select' Kconfig entry as the current patch
utilizes reg_slave and unreg_slave callbacks, which are controlled by
IS_ENABLED(CONFIG_I2C_SLAVE).

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/Kconfig            |   1 +
 drivers/i2c/busses/i2c-amd-asf-plat.c | 185 ++++++++++++++++++++++++++
 2 files changed, 186 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 03afcdbff209..9353946882db 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -98,6 +98,7 @@ config I2C_AMD_MP2
 config I2C_AMD_ASF
 	tristate "AMD ASF I2C Controller Support"
 	depends on I2C_PIIX4
+	select I2C_SLAVE
 	help
 	  This option enables support for the AMD ASF (Alert Standard Format)
 	  I2C controller. The AMD ASF controller is an SMBus controller with
diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
index 3d80805a7a3e..aa85e10a3927 100644
--- a/drivers/i2c/busses/i2c-amd-asf-plat.c
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -19,15 +19,199 @@
 #include <linux/sprintf.h>
 #include "i2c-piix4.h"
 
+/* ASF register bits */
+#define ASF_SLV_LISTN	0
+#define ASF_SLV_INTR	1
+#define ASF_SLV_RST	4
+#define ASF_PEC_SP	5
+#define ASF_DATA_EN	7
+#define ASF_MSTR_EN	16
+#define ASF_CLK_EN	17
+
+/* ASF address offsets */
+#define ASFLISADDR	(9 + piix4_smba)
+#define ASFSTA		(0xA + piix4_smba)
+#define ASFSLVSTA	(0xD + piix4_smba)
+#define ASFDATABNKSEL	(0x13 + piix4_smba)
+#define ASFSLVEN	(0x15 + piix4_smba)
+
+#define ASF_BLOCK_MAX_BYTES		72
+
 static const char *amd_asf_port_name = " port 1";
 
 struct amd_asf_dev {
 	struct i2c_adapter adap;
 	struct device *dev;
+	struct i2c_client *target;
 	struct sb800_mmio_cfg mmio_cfg;
 	struct resource *port_addr;
 };
 
+static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
+				unsigned long offset, bool set)
+{
+	unsigned long reg;
+
+	reg = inb_p(offset);
+	if (set)
+		set_bit(bit, &reg);
+	else
+		clear_bit(bit, &reg);
+	outb_p(reg, offset);
+}
+
+static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
+{
+	unsigned long reg;
+
+	reg = ioread32(dev->mmio_cfg.addr);
+	if (set)
+		set_bit(bit, &reg);
+	else
+		clear_bit(bit, &reg);
+	iowrite32(reg, dev->mmio_cfg.addr);
+}
+
+static void amd_asf_setup_target(struct amd_asf_dev *dev)
+{
+	unsigned short piix4_smba = dev->port_addr->start;
+
+	/* Reset both host and target before setting up */
+	outb_p(0, SMBHSTSTS);
+	outb_p(0, ASFSLVSTA);
+	outb_p(0, ASFSTA);
+
+	/* Update target address */
+	amd_asf_update_bits(piix4_smba, ASF_SLV_LISTN, ASFLISADDR, true);
+	/* Enable target and set the clock */
+	amd_asf_update_bytes(dev, ASF_MSTR_EN, false);
+	amd_asf_update_bytes(dev, ASF_CLK_EN, true);
+	/* Enable target interrupt */
+	amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, ASFSLVEN, true);
+	amd_asf_update_bits(piix4_smba, ASF_SLV_RST, ASFSLVEN, false);
+	/* Enable PEC and PEC append */
+	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, SMBHSTCNT, true);
+	amd_asf_update_bits(piix4_smba, ASF_PEC_SP, SMBHSTCNT, true);
+}
+
+static s32 amd_asf_access(struct i2c_adapter *adap, u16 addr, u8 command, u8 *data)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = dev->port_addr->start;
+	u8 i, len;
+
+	outb_p((addr << 1), SMBHSTADD);
+	outb_p(command, SMBHSTCMD);
+	len = data[0];
+	if (len == 0 || len > ASF_BLOCK_MAX_BYTES)
+		return -EINVAL;
+
+	outb_p(len, SMBHSTDAT0);
+	/* Reset SMBBLKDAT */
+	inb_p(SMBHSTCNT);
+	for (i = 1; i <= len; i++)
+		outb_p(data[i], SMBBLKDAT);
+
+	outb_p(PIIX4_BLOCK_DATA, SMBHSTCNT);
+	/* Enable PEC and PEC append */
+	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, SMBHSTCNT, true);
+	amd_asf_update_bits(piix4_smba, ASF_PEC_SP, SMBHSTCNT, true);
+
+	return piix4_transaction(adap, piix4_smba);
+}
+
+static int amd_asf_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(adap);
+	unsigned short piix4_smba = dev->port_addr->start;
+	u8 asf_data[ASF_BLOCK_MAX_BYTES];
+	struct i2c_msg *dev_msgs = msgs;
+	u8 prev_port;
+	int ret;
+
+	if (msgs->flags & I2C_M_RD) {
+		dev_err(&adap->dev, "ASF: Read not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	/* Exclude the receive header and PEC */
+	if (msgs->len > ASF_BLOCK_MAX_BYTES - 3) {
+		dev_err(&adap->dev, "ASF: max message length exceeded\n");
+		return -EOPNOTSUPP;
+	}
+
+	asf_data[0] = dev_msgs->len;
+	memcpy(asf_data + 1, dev_msgs[0].buf, dev_msgs->len);
+
+	ret = piix4_sb800_region_request(&adap->dev, &dev->mmio_cfg);
+	if (ret)
+		return ret;
+
+	amd_asf_update_bits(piix4_smba, ASF_SLV_RST, ASFSLVEN, true);
+	amd_asf_update_bits(piix4_smba, ASF_SLV_LISTN, ASFLISADDR, false);
+	/* Clear ASF target status */
+	outb_p(0, ASFSLVSTA);
+
+	/* Enable ASF SMBus controller function */
+	amd_asf_update_bytes(dev, ASF_MSTR_EN, true);
+	prev_port = piix4_sb800_port_sel(0, &dev->mmio_cfg);
+	ret = amd_asf_access(adap, msgs->addr, msgs[0].buf[0], asf_data);
+	piix4_sb800_port_sel(prev_port, &dev->mmio_cfg);
+	amd_asf_setup_target(dev);
+	piix4_sb800_region_release(&adap->dev, &dev->mmio_cfg);
+	return ret;
+}
+
+static int amd_asf_reg_target(struct i2c_client *target)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
+	unsigned short piix4_smba = dev->port_addr->start;
+	int ret;
+	u8 reg;
+
+	if (dev->target)
+		return -EBUSY;
+
+	ret = piix4_sb800_region_request(&target->dev, &dev->mmio_cfg);
+	if (ret)
+		return ret;
+
+	reg = (target->addr << 1) | BIT(0);
+	outb_p(reg, ASFLISADDR);
+
+	amd_asf_setup_target(dev);
+	dev->target = target;
+	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, ASFDATABNKSEL, false);
+	piix4_sb800_region_release(&target->dev, &dev->mmio_cfg);
+
+	return 0;
+}
+
+static int amd_asf_unreg_target(struct i2c_client *target)
+{
+	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
+	unsigned short piix4_smba = dev->port_addr->start;
+
+	amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, ASFSLVEN, false);
+	amd_asf_update_bits(piix4_smba, ASF_SLV_RST, ASFSLVEN, true);
+	dev->target = NULL;
+
+	return 0;
+}
+
+static u32 amd_asf_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SLAVE | I2C_FUNC_SMBUS_WRITE_BLOCK_DATA | I2C_FUNC_SMBUS_PEC;
+}
+
+static const struct i2c_algorithm amd_asf_smbus_algorithm = {
+	.master_xfer = amd_asf_xfer,
+	.reg_target = amd_asf_reg_target,
+	.unreg_target = amd_asf_unreg_target,
+	.functionality = amd_asf_func,
+};
+
 static int amd_asf_probe(struct platform_device *pdev)
 {
 	struct amd_asf_dev *asf_dev;
@@ -45,6 +229,7 @@ static int amd_asf_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
 
 	asf_dev->adap.owner = THIS_MODULE;
+	asf_dev->adap.algo = &amd_asf_smbus_algorithm;
 	asf_dev->adap.dev.parent = &pdev->dev;
 
 	i2c_set_adapdata(&asf_dev->adap, asf_dev);
-- 
2.25.1

Re: [PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:07PM +0530, Shyam Sundar S K wrote:
> Implement the i2c_algorithm operations to enable support for AMD ASF
> (Alert Standard Format) with SMBus. This enhancement includes:
> 
> - Adding functionality to identify and select the supported ASF functions.
> - Implementing mechanisms for registering and deregistering I2C slave
>   devices.
> - Providing support for data transfer operations over ASF.
> 
> Additionally, include a 'select' Kconfig entry as the current patch
> utilizes reg_slave and unreg_slave callbacks, which are controlled by
> IS_ENABLED(CONFIG_I2C_SLAVE).

...

> +/* ASF address offsets */
> +#define ASFLISADDR	(9 + piix4_smba)
> +#define ASFSTA		(0xA + piix4_smba)
> +#define ASFSLVSTA	(0xD + piix4_smba)
> +#define ASFDATABNKSEL	(0x13 + piix4_smba)
> +#define ASFSLVEN	(0x15 + piix4_smba)

0x09
0x0A
0x0D

...

> +static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
> +				unsigned long offset, bool set)
> +{
> +	unsigned long reg;
> +
> +	reg = inb_p(offset);
> +	if (set)
> +		set_bit(bit, &reg);
> +	else
> +		clear_bit(bit, &reg);

+ bitops.h

The above is home made assign_bit().
Moreover, why atomic version? Wouldn't __assign_bit() suffice?

> +	outb_p(reg, offset);
> +}

...

> +static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)

I didn't get the naming, the above using IO port with _bits, and this is MMIO
with _bytes. Are you sure the naming schema is correct?

> +{
> +	unsigned long reg;
> +
> +	reg = ioread32(dev->mmio_cfg.addr);
> +	if (set)
> +		set_bit(bit, &reg);
> +	else
> +		clear_bit(bit, &reg);
> +	iowrite32(reg, dev->mmio_cfg.addr);

Ditto (bitops and related things).

> +}

...

> +static int amd_asf_reg_target(struct i2c_client *target)
> +{
> +	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
> +	unsigned short piix4_smba = dev->port_addr->start;
> +	int ret;
> +	u8 reg;
> +
> +	if (dev->target)
> +		return -EBUSY;
> +
> +	ret = piix4_sb800_region_request(&target->dev, &dev->mmio_cfg);
> +	if (ret)
> +		return ret;
> +
> +	reg = (target->addr << 1) | BIT(0);

Is BIT(0) == I2C_M_RD in this case? If so, use the latter defined constant.

> +	outb_p(reg, ASFLISADDR);
> +
> +	amd_asf_setup_target(dev);
> +	dev->target = target;
> +	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, ASFDATABNKSEL, false);
> +	piix4_sb800_region_release(&target->dev, &dev->mmio_cfg);
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K]

On 9/14/2024 00:38, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:07PM +0530, Shyam Sundar S K wrote:
>> Implement the i2c_algorithm operations to enable support for AMD ASF
>> (Alert Standard Format) with SMBus. This enhancement includes:
>>
>> - Adding functionality to identify and select the supported ASF functions.
>> - Implementing mechanisms for registering and deregistering I2C slave
>>   devices.
>> - Providing support for data transfer operations over ASF.
>>
>> Additionally, include a 'select' Kconfig entry as the current patch
>> utilizes reg_slave and unreg_slave callbacks, which are controlled by
>> IS_ENABLED(CONFIG_I2C_SLAVE).
> 
> ...
> 
>> +/* ASF address offsets */
>> +#define ASFLISADDR	(9 + piix4_smba)
>> +#define ASFSTA		(0xA + piix4_smba)
>> +#define ASFSLVSTA	(0xD + piix4_smba)
>> +#define ASFDATABNKSEL	(0x13 + piix4_smba)
>> +#define ASFSLVEN	(0x15 + piix4_smba)
> 
> 0x09
> 0x0A
> 0x0D
> 
> ...
> 
>> +static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
>> +				unsigned long offset, bool set)
>> +{
>> +	unsigned long reg;
>> +
>> +	reg = inb_p(offset);
>> +	if (set)
>> +		set_bit(bit, &reg);
>> +	else
>> +		clear_bit(bit, &reg);
> 
> + bitops.h
> 
> The above is home made assign_bit().
> Moreover, why atomic version? Wouldn't __assign_bit() suffice?
> 

thanks! __assign_bit() would suffice.

>> +	outb_p(reg, offset);
>> +}
> 
> ...
> 
>> +static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
> 
> I didn't get the naming, the above using IO port with _bits, and this is MMIO
> with _bytes. Are you sure the naming schema is correct?
> 

Thinking to merge both the functions into one, something like this:

enum io_type {
    IO_PORT,
    MMIO
};

static void amd_asf_update_target(struct amd_asf_dev *dev, enum
io_type type, u8 bit, bool set)
{

...

}

>> +{
>> +	unsigned long reg;
>> +
>> +	reg = ioread32(dev->mmio_cfg.addr);
>> +	if (set)
>> +		set_bit(bit, &reg);
>> +	else
>> +		clear_bit(bit, &reg);
>> +	iowrite32(reg, dev->mmio_cfg.addr);
> 
> Ditto (bitops and related things).
> 
>> +}
> 
> ...
> 
>> +static int amd_asf_reg_target(struct i2c_client *target)
>> +{
>> +	struct amd_asf_dev *dev = i2c_get_adapdata(target->adapter);
>> +	unsigned short piix4_smba = dev->port_addr->start;
>> +	int ret;
>> +	u8 reg;
>> +
>> +	if (dev->target)
>> +		return -EBUSY;
>> +
>> +	ret = piix4_sb800_region_request(&target->dev, &dev->mmio_cfg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reg = (target->addr << 1) | BIT(0);
> 
> Is BIT(0) == I2C_M_RD in this case? If so, use the latter defined constant.
> 
>> +	outb_p(reg, ASFLISADDR);
>> +
>> +	amd_asf_setup_target(dev);
>> +	dev->target = target;
>> +	amd_asf_update_bits(piix4_smba, ASF_DATA_EN, ASFDATABNKSEL, false);
>> +	piix4_sb800_region_release(&target->dev, &dev->mmio_cfg);
>> +
>> +	return 0;
>> +}
> 

Re: [PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Andy Shevchenko]

On Tue, Sep 17, 2024 at 11:47:00PM +0530, Shyam Sundar S K wrote:
> On 9/14/2024 00:38, Andy Shevchenko wrote:
> > On Fri, Sep 13, 2024 at 05:41:07PM +0530, Shyam Sundar S K wrote:

...

> >> +static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
> > 
> > I didn't get the naming, the above using IO port with _bits, and this is MMIO
> > with _bytes. Are you sure the naming schema is correct?
> 
> Thinking to merge both the functions into one, something like this:
> 
> enum io_type {
>     IO_PORT,
>     MMIO
> };
> 
> static void amd_asf_update_target(struct amd_asf_dev *dev, enum
> io_type type, u8 bit, bool set)
> {
> 
> ...
> 
> }

I'm not talking about merging them (and merged variant seems less readable
to me), but about naming. I.o.w. it's not obvious what the difference _bits vs.
_bytes.

> >> +{
> >> +	unsigned long reg;
> >> +
> >> +	reg = ioread32(dev->mmio_cfg.addr);
> >> +	if (set)
> >> +		set_bit(bit, &reg);
> >> +	else
> >> +		clear_bit(bit, &reg);
> >> +	iowrite32(reg, dev->mmio_cfg.addr);
> > 
> > Ditto (bitops and related things).
> > 
> >> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 5/8] i2c: amd-asf: Add i2c_algorithm operations to support AMD ASF with SMBus

[Shyam Sundar S K]

On 9/18/2024 15:28, Andy Shevchenko wrote:
> On Tue, Sep 17, 2024 at 11:47:00PM +0530, Shyam Sundar S K wrote:
>> On 9/14/2024 00:38, Andy Shevchenko wrote:
>>> On Fri, Sep 13, 2024 at 05:41:07PM +0530, Shyam Sundar S K wrote:
> 
> ...
> 
>>>> +static void amd_asf_update_bytes(struct amd_asf_dev *dev, u8 bit, bool set)
>>>
>>> I didn't get the naming, the above using IO port with _bits, and this is MMIO
>>> with _bytes. Are you sure the naming schema is correct?
>>
>> Thinking to merge both the functions into one, something like this:
>>
>> enum io_type {
>>     IO_PORT,
>>     MMIO
>> };
>>
>> static void amd_asf_update_target(struct amd_asf_dev *dev, enum
>> io_type type, u8 bit, bool set)
>> {
>>
>> ...
>>
>> }
> 
> I'm not talking about merging them (and merged variant seems less readable
> to me), but about naming. I.o.w. it's not obvious what the difference _bits vs.
> _bytes.

Alright. I will stop merging them. I'll rename them as follows:


/* updates target using memory-mapped I/O */
amd_asf_update_mmio_target(...)

/* updates target using I/O port access */
amd_asf_update_ioport_target(...)

Thanks,
Shyam

> 
>>>> +{
>>>> +	unsigned long reg;
>>>> +
>>>> +	reg = ioread32(dev->mmio_cfg.addr);
>>>> +	if (set)
>>>> +		set_bit(bit, &reg);
>>>> +	else
>>>> +		clear_bit(bit, &reg);
>>>> +	iowrite32(reg, dev->mmio_cfg.addr);
>>>
>>> Ditto (bitops and related things).
>>>
>>>> +}
> 

[PATCH v5 6/8] i2c: amd-asf: Add routine to handle the ASF slave process

[Shyam Sundar S K]

Add support for handling ASF slave process events as described in the AMD
ASF databook. This involves implementing the correct programming sequence
to manage each ASF packet appropriately.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-amd-asf-plat.c | 103 ++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
index aa85e10a3927..1beef717ef40 100644
--- a/drivers/i2c/busses/i2c-amd-asf-plat.c
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -29,13 +29,17 @@
 #define ASF_CLK_EN	17
 
 /* ASF address offsets */
+#define ASFINDEX	(7 + piix4_smba)
 #define ASFLISADDR	(9 + piix4_smba)
 #define ASFSTA		(0xA + piix4_smba)
 #define ASFSLVSTA	(0xD + piix4_smba)
+#define ASFDATARWPTR	(0x11 + piix4_smba)
+#define ASFSETDATARDPTR	(0x12 + piix4_smba)
 #define ASFDATABNKSEL	(0x13 + piix4_smba)
 #define ASFSLVEN	(0x15 + piix4_smba)
 
 #define ASF_BLOCK_MAX_BYTES		72
+#define ASF_ERROR_STATUS		0xE
 
 static const char *amd_asf_port_name = " port 1";
 
@@ -43,10 +47,71 @@ struct amd_asf_dev {
 	struct i2c_adapter adap;
 	struct device *dev;
 	struct i2c_client *target;
+	struct delayed_work work_buf;
 	struct sb800_mmio_cfg mmio_cfg;
 	struct resource *port_addr;
 };
 
+static void amd_asf_process_target(struct work_struct *work)
+{
+	struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work);
+	unsigned short piix4_smba = dev->port_addr->start;
+	u8 data[ASF_BLOCK_MAX_BYTES];
+	u8 len, idx, val = 0;
+	u8 bank, reg, cmd;
+
+	/* Read target status register */
+	reg = inb_p(ASFSLVSTA);
+
+	/* Check if no error bits are set in target status register */
+	if (reg & ASF_ERROR_STATUS) {
+		/* Set bank as full */
+		cmd = 0;
+		reg = reg | GENMASK(3, 2);
+		outb_p(reg, ASFDATABNKSEL);
+	} else {
+		/* Read data bank */
+		reg = inb_p(ASFDATABNKSEL);
+		bank = (reg & BIT(3)) ? 1 : 0;
+
+		/* Set read data bank */
+		if (bank) {
+			reg = reg | BIT(4);
+			reg = reg & ~BIT(3);
+		} else {
+			reg = reg & ~BIT(4);
+			reg = reg & ~BIT(2);
+		}
+
+		/* Read command register */
+		outb_p(reg, ASFDATABNKSEL);
+		cmd = inb_p(ASFINDEX);
+		len = inb_p(ASFDATARWPTR);
+		for (idx = 0; idx < len; idx++)
+			data[idx] = inb_p(ASFINDEX);
+
+		/* Clear data bank status */
+		if (bank) {
+			reg = reg | BIT(3);
+			outb_p(reg, ASFDATABNKSEL);
+		} else {
+			reg = reg | BIT(2);
+			outb_p(reg, ASFDATABNKSEL);
+		}
+	}
+
+	outb_p(0, ASFSETDATARDPTR);
+	if (cmd & BIT(0))
+		return;
+
+	i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
+	for (idx = 0; idx < len; idx++) {
+		val = data[idx];
+		i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
+	}
+	i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val);
+}
+
 static void amd_asf_update_bits(unsigned short piix4_smba, u8 bit,
 				unsigned long offset, bool set)
 {
@@ -212,9 +277,28 @@ static const struct i2c_algorithm amd_asf_smbus_algorithm = {
 	.functionality = amd_asf_func,
 };
 
+static irqreturn_t amd_asf_irq_handler(int irq, void *ptr)
+{
+	struct amd_asf_dev *dev = ptr;
+	unsigned short piix4_smba = dev->port_addr->start;
+	u8 target_int = inb_p(ASFSTA);
+
+	if (target_int & BIT(6)) {
+		/* Target Interrupt */
+		outb_p(target_int | BIT(6), ASFSTA);
+		schedule_delayed_work(&dev->work_buf, HZ);
+	} else {
+		/* Controller Interrupt */
+		amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, SMBHSTSTS, true);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int amd_asf_probe(struct platform_device *pdev)
 {
 	struct amd_asf_dev *asf_dev;
+	int ret, irq;
 
 	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
 	if (!asf_dev)
@@ -228,6 +312,17 @@ static int amd_asf_probe(struct platform_device *pdev)
 	if (!asf_dev->port_addr)
 		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
 
+	INIT_DELAYED_WORK(&asf_dev->work_buf, amd_asf_process_target);
+
+	irq = platform_get_irq(pdev, 0);
+	if (!irq)
+		return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n");
+
+	ret = devm_request_irq(&pdev->dev, irq, amd_asf_irq_handler, IRQF_SHARED,
+			       "amd_smbus_asf", asf_dev);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Unable to request irq: %d for use\n", irq);
+
 	asf_dev->adap.owner = THIS_MODULE;
 	asf_dev->adap.algo = &amd_asf_smbus_algorithm;
 	asf_dev->adap.dev.parent = &pdev->dev;
@@ -239,6 +334,13 @@ static int amd_asf_probe(struct platform_device *pdev)
 	return devm_i2c_add_adapter(&pdev->dev, &asf_dev->adap);
 }
 
+static void amd_asf_remove(struct platform_device *pdev)
+{
+	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&dev->work_buf);
+}
+
 static const struct acpi_device_id amd_asf_acpi_ids[] = {
 	{ "AMDI001A" },
 	{ }
@@ -251,6 +353,7 @@ static struct platform_driver amd_asf_driver = {
 		.acpi_match_table = amd_asf_acpi_ids,
 	},
 	.probe = amd_asf_probe,
+	.remove_new = amd_asf_remove,
 };
 module_platform_driver(amd_asf_driver);
 
-- 
2.25.1

Re: [PATCH v5 6/8] i2c: amd-asf: Add routine to handle the ASF slave process

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote:
> Add support for handling ASF slave process events as described in the AMD
> ASF databook. This involves implementing the correct programming sequence
> to manage each ASF packet appropriately.

...

>  /* ASF address offsets */
> +#define ASFINDEX	(7 + piix4_smba)

0x07

...

> +#define ASF_ERROR_STATUS		0xE

So, according to the usage this seems to be a mask, then perhaps GENMASK(3, 1) ?

...

> +static void amd_asf_process_target(struct work_struct *work)
> +{
> +	struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work);
> +	unsigned short piix4_smba = dev->port_addr->start;
> +	u8 data[ASF_BLOCK_MAX_BYTES];

> +	u8 len, idx, val = 0;

Hmm... Does val = 0 assignment is due to false positive (or missing error check)?

> +	u8 bank, reg, cmd;
> +
> +	/* Read target status register */
> +	reg = inb_p(ASFSLVSTA);
> +
> +	/* Check if no error bits are set in target status register */
> +	if (reg & ASF_ERROR_STATUS) {
> +		/* Set bank as full */
> +		cmd = 0;
> +		reg = reg | GENMASK(3, 2);
> +		outb_p(reg, ASFDATABNKSEL);
> +	} else {
> +		/* Read data bank */
> +		reg = inb_p(ASFDATABNKSEL);
> +		bank = (reg & BIT(3)) ? 1 : 0;
> +
> +		/* Set read data bank */
> +		if (bank) {
> +			reg = reg | BIT(4);
> +			reg = reg & ~BIT(3);
> +		} else {
> +			reg = reg & ~BIT(4);
> +			reg = reg & ~BIT(2);
> +		}
> +
> +		/* Read command register */
> +		outb_p(reg, ASFDATABNKSEL);
> +		cmd = inb_p(ASFINDEX);
> +		len = inb_p(ASFDATARWPTR);
> +		for (idx = 0; idx < len; idx++)
> +			data[idx] = inb_p(ASFINDEX);
> +
> +		/* Clear data bank status */
> +		if (bank) {
> +			reg = reg | BIT(3);
> +			outb_p(reg, ASFDATABNKSEL);
> +		} else {
> +			reg = reg | BIT(2);
> +			outb_p(reg, ASFDATABNKSEL);
> +		}
> +	}
> +
> +	outb_p(0, ASFSETDATARDPTR);
> +	if (cmd & BIT(0))
> +		return;
> +
> +	i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);

Can this fail / return an error code? (I haven't checked)

> +	for (idx = 0; idx < len; idx++) {
> +		val = data[idx];
> +		i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
> +	}
> +	i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val);
> +}

...

> +	irq = platform_get_irq(pdev, 0);
> +	if (!irq)

Incorrect check.

> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n");

Shadower real error code.

...

> +static void amd_asf_remove(struct platform_device *pdev)
> +{
> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&dev->work_buf);
> +}

Wouldn't devm-helpers.h APIs help with avoiding ->remove() creation?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 6/8] i2c: amd-asf: Add routine to handle the ASF slave process

[Shyam Sundar S K]

On 9/14/2024 00:47, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote:
>> Add support for handling ASF slave process events as described in the AMD
>> ASF databook. This involves implementing the correct programming sequence
>> to manage each ASF packet appropriately.
> 
> ...
> 
>>  /* ASF address offsets */
>> +#define ASFINDEX	(7 + piix4_smba)
> 
> 0x07
> 
> ...
> 
>> +#define ASF_ERROR_STATUS		0xE
> 
> So, according to the usage this seems to be a mask, then perhaps GENMASK(3, 1) ?
> 

GENMASK() works here.

> ...
> 
>> +static void amd_asf_process_target(struct work_struct *work)
>> +{
>> +	struct amd_asf_dev *dev = container_of(work, struct amd_asf_dev, work_buf.work);
>> +	unsigned short piix4_smba = dev->port_addr->start;
>> +	u8 data[ASF_BLOCK_MAX_BYTES];
> 
>> +	u8 len, idx, val = 0;
> 
> Hmm... Does val = 0 assignment is due to false positive (or missing error check)?
> 

I can remove the explicit assignment to zero.

>> +	u8 bank, reg, cmd;
>> +
>> +	/* Read target status register */
>> +	reg = inb_p(ASFSLVSTA);
>> +
>> +	/* Check if no error bits are set in target status register */
>> +	if (reg & ASF_ERROR_STATUS) {
>> +		/* Set bank as full */
>> +		cmd = 0;
>> +		reg = reg | GENMASK(3, 2);
>> +		outb_p(reg, ASFDATABNKSEL);
>> +	} else {
>> +		/* Read data bank */
>> +		reg = inb_p(ASFDATABNKSEL);
>> +		bank = (reg & BIT(3)) ? 1 : 0;
>> +
>> +		/* Set read data bank */
>> +		if (bank) {
>> +			reg = reg | BIT(4);
>> +			reg = reg & ~BIT(3);
>> +		} else {
>> +			reg = reg & ~BIT(4);
>> +			reg = reg & ~BIT(2);
>> +		}
>> +
>> +		/* Read command register */
>> +		outb_p(reg, ASFDATABNKSEL);
>> +		cmd = inb_p(ASFINDEX);
>> +		len = inb_p(ASFDATARWPTR);
>> +		for (idx = 0; idx < len; idx++)
>> +			data[idx] = inb_p(ASFINDEX);
>> +
>> +		/* Clear data bank status */
>> +		if (bank) {
>> +			reg = reg | BIT(3);
>> +			outb_p(reg, ASFDATABNKSEL);
>> +		} else {
>> +			reg = reg | BIT(2);
>> +			outb_p(reg, ASFDATABNKSEL);
>> +		}
>> +	}
>> +
>> +	outb_p(0, ASFSETDATARDPTR);
>> +	if (cmd & BIT(0))
>> +		return;
>> +
>> +	i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
> 
> Can this fail / return an error code? (I haven't checked)

i2c_slave_event() returns an error code, but here it is done with the
workqueue callback context. Hence I skipped the error checking part.

> 
>> +	for (idx = 0; idx < len; idx++) {
>> +		val = data[idx];
>> +		i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
>> +	}
>> +	i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val);
>> +}
> 
> ...
> 
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (!irq)
> 
> Incorrect check.
> 
>> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing IRQ resources\n");
> 
> Shadower real error code.
> 
> ...
> 
>> +static void amd_asf_remove(struct platform_device *pdev)
>> +{
>> +	struct amd_asf_dev *dev = platform_get_drvdata(pdev);
>> +
>> +	cancel_delayed_work_sync(&dev->work_buf);
>> +}
> 
> Wouldn't devm-helpers.h APIs help with avoiding ->remove() creation?
> 

Re: [PATCH v5 6/8] i2c: amd-asf: Add routine to handle the ASF slave process

[Andy Shevchenko]

On Tue, Sep 17, 2024 at 11:51:27PM +0530, Shyam Sundar S K wrote:
> On 9/14/2024 00:47, Andy Shevchenko wrote:
> > On Fri, Sep 13, 2024 at 05:41:08PM +0530, Shyam Sundar S K wrote:

...

> >> +	i2c_slave_event(dev->target, I2C_SLAVE_WRITE_REQUESTED, &val);
> > 
> > Can this fail / return an error code? (I haven't checked)
> 
> i2c_slave_event() returns an error code, but here it is done with the
> workqueue callback context. Hence I skipped the error checking part.

This requires a comment why it's okay.

...

> >> +	for (idx = 0; idx < len; idx++) {
> >> +		val = data[idx];
> >> +		i2c_slave_event(dev->target, I2C_SLAVE_WRITE_RECEIVED, &val);
> >> +	}
> >> +	i2c_slave_event(dev->target, I2C_SLAVE_STOP, &val);

Same here.

> >> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Shyam Sundar S K]

To ensure successive interrupts upon packet reception, it is necessary to
clear the remote IRR bit by writing the interrupt number to the EOI
register. The base address for this operation is provided by the BIOS and
retrieved by the driver by traversing the ASF object's namespace.

Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/i2c/busses/i2c-amd-asf-plat.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/i2c/busses/i2c-amd-asf-plat.c b/drivers/i2c/busses/i2c-amd-asf-plat.c
index 1beef717ef40..77555416597f 100644
--- a/drivers/i2c/busses/i2c-amd-asf-plat.c
+++ b/drivers/i2c/busses/i2c-amd-asf-plat.c
@@ -45,6 +45,7 @@ static const char *amd_asf_port_name = " port 1";
 
 struct amd_asf_dev {
 	struct i2c_adapter adap;
+	void __iomem *eoi_base;
 	struct device *dev;
 	struct i2c_client *target;
 	struct delayed_work work_buf;
@@ -292,12 +293,14 @@ static irqreturn_t amd_asf_irq_handler(int irq, void *ptr)
 		amd_asf_update_bits(piix4_smba, ASF_SLV_INTR, SMBHSTSTS, true);
 	}
 
+	iowrite32(irq, dev->eoi_base);
 	return IRQ_HANDLED;
 }
 
 static int amd_asf_probe(struct platform_device *pdev)
 {
 	struct amd_asf_dev *asf_dev;
+	struct resource *eoi_addr;
 	int ret, irq;
 
 	asf_dev = devm_kzalloc(&pdev->dev, sizeof(*asf_dev), GFP_KERNEL);
@@ -312,6 +315,14 @@ static int amd_asf_probe(struct platform_device *pdev)
 	if (!asf_dev->port_addr)
 		return dev_err_probe(&pdev->dev, -EINVAL, "missing IO resources\n");
 
+	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!eoi_addr)
+		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
+
+	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
+	if (!asf_dev->eoi_base)
+		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");
+
 	INIT_DELAYED_WORK(&asf_dev->work_buf, amd_asf_process_target);
 
 	irq = platform_get_irq(pdev, 0);
-- 
2.25.1

Re: [PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:09PM +0530, Shyam Sundar S K wrote:
> To ensure successive interrupts upon packet reception, it is necessary to
> clear the remote IRR bit by writing the interrupt number to the EOI
> register. The base address for this operation is provided by the BIOS and
> retrieved by the driver by traversing the ASF object's namespace.

...

> +	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!eoi_addr)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
> +
> +	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
> +	if (!asf_dev->eoi_base)
> +		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");

Home grown copy of devm_platform_ioremap_resource().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Shyam Sundar S K]

On 9/14/2024 00:49, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:09PM +0530, Shyam Sundar S K wrote:
>> To ensure successive interrupts upon packet reception, it is necessary to
>> clear the remote IRR bit by writing the interrupt number to the EOI
>> register. The base address for this operation is provided by the BIOS and
>> retrieved by the driver by traversing the ASF object's namespace.
> 
> ...
> 
>> +	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!eoi_addr)
>> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
>> +
>> +	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
>> +	if (!asf_dev->eoi_base)
>> +		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");
> 
> Home grown copy of devm_platform_ioremap_resource().
> 

devm_platform_ioremap_resource() internally calls
devm_platform_get_and_ioremap_resource(), performing two main actions:

It uses platform_get_resource().
It then calls devm_ioremap_resource().

However, there's an issue.

devm_ioremap_resource() invokes devm_request_mem_region() followed by
__devm_ioremap(). In this driver, the resource obtained via ASL might
not actually belong to the ASF device address space. Instead, it could
be within other IP blocks of the ASIC, which are crucial for
generating subsequent interrupts (the main focus of this patch). As a
result, devm_request_mem_region() fails, preventing __devm_ioremap()
from being executed.

TL;DR, it’s more appropriate to call platform_get_resource() and
devm_ioremap() separately in this scenario.

Thanks,
Shyam

Re: [PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Andy Shevchenko]

On Wed, Sep 18, 2024 at 12:01:19AM +0530, Shyam Sundar S K wrote:
> On 9/14/2024 00:49, Andy Shevchenko wrote:
> > On Fri, Sep 13, 2024 at 05:41:09PM +0530, Shyam Sundar S K wrote:

...

> >> +	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	if (!eoi_addr)
> >> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
> >> +
> >> +	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
> >> +	if (!asf_dev->eoi_base)
> >> +		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");
> > 
> > Home grown copy of devm_platform_ioremap_resource().
> 
> devm_platform_ioremap_resource() internally calls
> devm_platform_get_and_ioremap_resource(), performing two main actions:
> 
> It uses platform_get_resource().
> It then calls devm_ioremap_resource().
> 
> However, there's an issue.
> 
> devm_ioremap_resource() invokes devm_request_mem_region() followed by
> __devm_ioremap(). In this driver, the resource obtained via ASL might
> not actually belong to the ASF device address space. Instead, it could
> be within other IP blocks of the ASIC, which are crucial for
> generating subsequent interrupts (the main focus of this patch). As a
> result, devm_request_mem_region() fails, preventing __devm_ioremap()
> from being executed.
> 
> TL;DR, it’s more appropriate to call platform_get_resource() and
> devm_ioremap() separately in this scenario.

Okay, at bare minimum this must be commented in the code (like the above
summary). Ideally it should be done differently as the resource regions
should not be shared (it's an exceptional case and usually shows bad design
of the driver). If you can't really split, regmap APIs help with that
(and they also may provide an adequate serialisation to IO).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Shyam Sundar S K]

On 9/18/2024 15:33, Andy Shevchenko wrote:
> On Wed, Sep 18, 2024 at 12:01:19AM +0530, Shyam Sundar S K wrote:
>> On 9/14/2024 00:49, Andy Shevchenko wrote:
>>> On Fri, Sep 13, 2024 at 05:41:09PM +0530, Shyam Sundar S K wrote:
> 
> ...
> 
>>>> +	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!eoi_addr)
>>>> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
>>>> +
>>>> +	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
>>>> +	if (!asf_dev->eoi_base)
>>>> +		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");
>>>
>>> Home grown copy of devm_platform_ioremap_resource().
>>
>> devm_platform_ioremap_resource() internally calls
>> devm_platform_get_and_ioremap_resource(), performing two main actions:
>>
>> It uses platform_get_resource().
>> It then calls devm_ioremap_resource().
>>
>> However, there's an issue.
>>
>> devm_ioremap_resource() invokes devm_request_mem_region() followed by
>> __devm_ioremap(). In this driver, the resource obtained via ASL might
>> not actually belong to the ASF device address space. Instead, it could
>> be within other IP blocks of the ASIC, which are crucial for
>> generating subsequent interrupts (the main focus of this patch). As a
>> result, devm_request_mem_region() fails, preventing __devm_ioremap()
>> from being executed.
>>
>> TL;DR, it’s more appropriate to call platform_get_resource() and
>> devm_ioremap() separately in this scenario.
> 
> Okay, at bare minimum this must be commented in the code (like the above
> summary). 

Okay, I will leave a comment.

> Ideally it should be done differently as the resource regions
> should not be shared (it's an exceptional case and usually shows bad design
> of the driver). If you can't really split, regmap APIs help with that
> (and they also may provide an adequate serialisation to IO).
> 

Unfortunately, this is the only way to get subsequent interrupts from
the ASF IP block (based on the AMD ASF databook).

Thanks,
Shyam

Re: [PATCH v5 7/8] i2c: amd-asf: Clear remote IRR bit to get successive interrupt

[Andy Shevchenko]

On Wed, Sep 18, 2024 at 03:58:11PM +0530, Shyam Sundar S K wrote:
> On 9/18/2024 15:33, Andy Shevchenko wrote:
> > On Wed, Sep 18, 2024 at 12:01:19AM +0530, Shyam Sundar S K wrote:
> >> On 9/14/2024 00:49, Andy Shevchenko wrote:
> >>> On Fri, Sep 13, 2024 at 05:41:09PM +0530, Shyam Sundar S K wrote:

...

> >>>> +	eoi_addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>> +	if (!eoi_addr)
> >>>> +		return dev_err_probe(&pdev->dev, -EINVAL, "missing MEM resources\n");
> >>>> +
> >>>> +	asf_dev->eoi_base = devm_ioremap(&pdev->dev, eoi_addr->start, resource_size(eoi_addr));
> >>>> +	if (!asf_dev->eoi_base)
> >>>> +		return dev_err_probe(&pdev->dev, -EBUSY, "failed mapping IO region\n");
> >>>
> >>> Home grown copy of devm_platform_ioremap_resource().
> >>
> >> devm_platform_ioremap_resource() internally calls
> >> devm_platform_get_and_ioremap_resource(), performing two main actions:
> >>
> >> It uses platform_get_resource().
> >> It then calls devm_ioremap_resource().
> >>
> >> However, there's an issue.
> >>
> >> devm_ioremap_resource() invokes devm_request_mem_region() followed by
> >> __devm_ioremap(). In this driver, the resource obtained via ASL might
> >> not actually belong to the ASF device address space. Instead, it could
> >> be within other IP blocks of the ASIC, which are crucial for
> >> generating subsequent interrupts (the main focus of this patch). As a
> >> result, devm_request_mem_region() fails, preventing __devm_ioremap()
> >> from being executed.
> >>
> >> TL;DR, it’s more appropriate to call platform_get_resource() and
> >> devm_ioremap() separately in this scenario.
> > 
> > Okay, at bare minimum this must be commented in the code (like the above
> > summary). 
> 
> Okay, I will leave a comment.
> 
> > Ideally it should be done differently as the resource regions
> > should not be shared (it's an exceptional case and usually shows bad design
> > of the driver). If you can't really split, regmap APIs help with that
> > (and they also may provide an adequate serialisation to IO).
> 
> Unfortunately, this is the only way to get subsequent interrupts from
> the ASF IP block (based on the AMD ASF databook).

How is it related to the pure software concept of the assigning (exclusively)
the resources? Again, if you need to share, switch over to use regmap APIs.
See how I2C DesignWare driver does. It's also used as the part of complex
hardware where the register windows may not be clearly split between drivers.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 8/8] MAINTAINERS: Add AMD ASF driver entry

[Shyam Sundar S K]

Update the MAINTAINERS file with AMD ASF driver details.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3f348b71b9c9..18b8975dd058 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1082,6 +1082,12 @@ L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/busses/i2c-amd-mp2*
 
+AMD ASF I2C DRIVER
+M:	Shyam Sundar S K <shyam-sundar.s-k@amd.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	drivers/i2c/busses/i2c-amd-asf-plat.c
+
 AMD PDS CORE DRIVER
 M:	Shannon Nelson <shannon.nelson@amd.com>
 M:	Brett Creeley <brett.creeley@amd.com>
-- 
2.25.1

Re: [PATCH v5 0/8] Introduce initial AMD ASF Controller driver support

[Andy Shevchenko]

On Fri, Sep 13, 2024 at 05:41:02PM +0530, Shyam Sundar S K wrote:
> The AMD ASF (Alert Standard Format) function block is essentially an SMBus
> controller with built-in ASF functionality. It features two pins SCL1 and
> SDA1 that facilitate communication with other SMBus devices. This dual
> capability allows the ASF controller to issue generic SMBus packets and
> communicate with the DASH controller using MCTP over ASF. Additionally,
> the ASF controller supports remote commands defined by the ASF
> specification, such as shutdown, reset, power-up, and power-down, without
> requiring any software interaction.
> 
> The concept is to enable a remote system to communicate with the target
> system over the network. The local network controller, such as an Ethernet
> MAC, receives remote packets and relays the commands to the FCH
> (Fusion Controller Hub) through the ASF. Examples of these commands
> include shutdown and reset. Since ASF uses the SMBus protocol, this
> controller can be configured as a secondary SMBus controller.
> 
> This series of updates focuses on extending the i2c-piix4 driver to
> support the ASF driver by exporting several functions from the i2c-piix4
> driver, allowing the AMD ASF driver to leverage existing functionalities.
> Additionally, this change incorporates core ASF functionality, including
> ACPI integration and the implementation of i2c_algorithm callbacks for ASF
> operations.

ACPI code is much better now, but can be even better.
See my individual comments.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 0/8] Introduce initial AMD ASF Controller driver support

[Shyam Sundar S K]

On 9/14/2024 00:50, Andy Shevchenko wrote:
> On Fri, Sep 13, 2024 at 05:41:02PM +0530, Shyam Sundar S K wrote:
>> The AMD ASF (Alert Standard Format) function block is essentially an SMBus
>> controller with built-in ASF functionality. It features two pins SCL1 and
>> SDA1 that facilitate communication with other SMBus devices. This dual
>> capability allows the ASF controller to issue generic SMBus packets and
>> communicate with the DASH controller using MCTP over ASF. Additionally,
>> the ASF controller supports remote commands defined by the ASF
>> specification, such as shutdown, reset, power-up, and power-down, without
>> requiring any software interaction.
>>
>> The concept is to enable a remote system to communicate with the target
>> system over the network. The local network controller, such as an Ethernet
>> MAC, receives remote packets and relays the commands to the FCH
>> (Fusion Controller Hub) through the ASF. Examples of these commands
>> include shutdown and reset. Since ASF uses the SMBus protocol, this
>> controller can be configured as a secondary SMBus controller.
>>
>> This series of updates focuses on extending the i2c-piix4 driver to
>> support the ASF driver by exporting several functions from the i2c-piix4
>> driver, allowing the AMD ASF driver to leverage existing functionalities.
>> Additionally, this change incorporates core ASF functionality, including
>> ACPI integration and the implementation of i2c_algorithm callbacks for ASF
>> operations.
> 
> ACPI code is much better now, but can be even better.
> See my individual comments.
> 

Thank you for the feedback and apologies for the delay. I have few
questions that needs a bit of clarity. Please see the individual patches.

Thanks,
Shyam