| Message ID | 20240913121110.1611340-1-Shyam-sundar.S-k@amd.com |
|---|---|
| Headers | show |
| Series | Introduce initial AMD ASF Controller driver support | expand |
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
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().
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>
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
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 >
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
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.
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).
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
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
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.
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.
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