mbox series

[v4,0/8] Introduce initial AMD ASF Controller driver support

Message ID 20240911115407.1090046-1-Shyam-sundar.S-k@amd.com
Headers show
Series Introduce initial AMD ASF Controller driver support | expand

Message

Shyam Sundar S K Sept. 11, 2024, 11:53 a.m. UTC 
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.

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 signature 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 | 398 ++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-piix4.c        |  54 ++--
 drivers/i2c/busses/i2c-piix4.h        |  45 +++
 6 files changed, 489 insertions(+), 34 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-amd-asf-plat.c
 create mode 100644 drivers/i2c/busses/i2c-piix4.h

Comments

Andi Shyti Sept. 12, 2024, 6:15 a.m. UTC | #1 
Hi Shyam,

what does it mean signature? Do you mean "parameter list"?

Andi

On Wed, Sep 11, 2024 at 05:24:00PM GMT, 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 signature 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
>
Shyam Sundar S K Sept. 12, 2024, 6:22 a.m. UTC | #2 
Hi Andi

On 9/12/2024 11:45, Andi Shyti wrote:
> Hi Shyam,
> 
> what does it mean signature? Do you mean "parameter list"?

Yes. I mean "parameter list". I can amend it in the next version.

Thanks,
Shyam

> 
> Andi
> 
> On Wed, Sep 11, 2024 at 05:24:00PM GMT, 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 signature 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
>>
Andi Shyti Sept. 12, 2024, 7:40 a.m. UTC | #3 
Hi Shyam,

On Wed, Sep 11, 2024 at 05:24:02PM GMT, 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.

...

> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 2c2a466e2f85..174cce254e96 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -153,8 +153,7 @@ 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)

I’m not entirely happy with this change, or the others above. If
someone runs a git bisect, they would be confused by not seeing
this change described in the commit log.

While it's true that the accepted line length is now 100
characters, the 80-character limit is still preferred (and
personally, I prefer 80, though that’s just my opinion).

This change doesn’t seem necessary, so please amend it along with
the others below in the next version.

Thanks,
Andi

>  {
>  	if (mmio_cfg->use_mmio) {
>  		void __iomem *addr;
> @@ -192,9 +191,9 @@ 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 +204,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 +514,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 +587,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 +741,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 +763,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.

...
Shyam Sundar S K Sept. 13, 2024, 6:24 a.m. UTC | #4 
Hi Andi,

On 9/12/2024 13:10, Andi Shyti wrote:
> Hi Shyam,
> 
> On Wed, Sep 11, 2024 at 05:24:02PM GMT, 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.
> 
> ...
> 
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 2c2a466e2f85..174cce254e96 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -153,8 +153,7 @@ 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)
> 
> I’m not entirely happy with this change, or the others above. If
> someone runs a git bisect, they would be confused by not seeing
> this change described in the commit log.

Can you elaborate a bit more on the expectation? The commit message
has the details on the each of the functions that has to be exported.

Can you please take a look at it again?

> 
> While it's true that the accepted line length is now 100
> characters, the 80-character limit is still preferred (and
> personally, I prefer 80, though that’s just my opinion).
> 
> This change doesn’t seem necessary, so please amend it along with
> the others below in the next version.

Ok. I will move to 80 char length.

Thanks,
Shyam

> 
> Thanks,
> Andi
> 
>>  {
>>  	if (mmio_cfg->use_mmio) {
>>  		void __iomem *addr;
>> @@ -192,9 +191,9 @@ 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 +204,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 +514,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 +587,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 +741,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 +763,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.
> 
> ...
Andi Shyti Sept. 13, 2024, 8:36 a.m. UTC | #5 
Hi Shyam,

On Fri, Sep 13, 2024 at 11:54:55AM GMT, Shyam Sundar S K wrote:
> On 9/12/2024 13:10, Andi Shyti wrote:
> > On Wed, Sep 11, 2024 at 05:24:02PM GMT, 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.
> > 
> > ...
> > 
> >> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> >> index 2c2a466e2f85..174cce254e96 100644
> >> --- a/drivers/i2c/busses/i2c-piix4.c
> >> +++ b/drivers/i2c/busses/i2c-piix4.c
> >> @@ -153,8 +153,7 @@ 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)
> > 
> > I’m not entirely happy with this change, or the others above. If
> > someone runs a git bisect, they would be confused by not seeing
> > this change described in the commit log.
> 
> Can you elaborate a bit more on the expectation? The commit message
> has the details on the each of the functions that has to be exported.
> 
> Can you please take a look at it again?

The change I am referring to is that style change I described
here below, that consists in putting in one line the functions.

You are describing the logical changes, but there is no mention
of the fact that you are moving the second parameter of the
function in the same line with the other.

> > While it's true that the accepted line length is now 100
> > characters, the 80-character limit is still preferred (and
> > personally, I prefer 80, though that’s just my opinion).
> > 
> > This change doesn’t seem necessary, so please amend it along with
> > the others below in the next version.
> 
> Ok. I will move to 80 char length.

Thanks,
Andi