diff mbox series

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

Message ID 20240913121110.1611340-8-Shyam-sundar.S-k@amd.com
State Superseded
Headers show
Series Introduce initial AMD ASF Controller driver support | expand

Commit Message

Shyam Sundar S K Sept. 13, 2024, 12:11 p.m. UTC 
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(+)

Comments

Andy Shevchenko Sept. 13, 2024, 7:19 p.m. UTC | #1 
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().
Shyam Sundar S K Sept. 17, 2024, 6:31 p.m. UTC | #2 
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
Andy Shevchenko Sept. 18, 2024, 10:03 a.m. UTC | #3 
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).
Shyam Sundar S K Sept. 18, 2024, 10:28 a.m. UTC | #4 
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
Andy Shevchenko Sept. 18, 2024, 2:05 p.m. UTC | #5 
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.
diff mbox series

Patch

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);