diff mbox series

regulator: ti-abb: don't use devm_platform_ioremap_resource_byname for shared interrupt register

Message ID 20240122170442.729374-1-romain.naour@smile.fr
State New
Headers
Series regulator: ti-abb: don't use devm_platform_ioremap_resource_byname for shared interrupt register |

Commit Message

Romain Naour Jan. 22, 2024, 5:04 p.m. UTC 
  From: Romain Naour <romain.naour@skf.com>

We can't use devm_platform_ioremap_resource_byname() to remap the
interrupt register that can be shared between
regulator-abb-{ivahd,dspeve,gpu} drivers instance.

From arm/boot/dts/dra7.dtsi:

The abb_mpu is the only instance using its own interrupt register:
  (0x4ae06014) PRM_IRQSTATUS_MPU_2, ABB_MPU_DONE_ST (bit 7)

The other tree instance (abb_ivahd, abb_dspeve, abb_gpu) share
PRM_IRQSTATUS_MPU register (0x4ae06010) but uses different bit
ABB_IVA_DONE_ST (bit 30), ABB_DSPEVE_DONE_ST( bit 29) and
ABB_GPU_DONE_ST (but 28).

The commit b36c6b1887ff (regulator: ti-abb: Make use of the helper
function devm_ioremap related) overlooked the following comment
explaining why devm_ioremap() is used in this case:

/*
 * We may have shared interrupt register offsets which are
 * write-1-to-clear between domains ensuring exclusivity.
 */

Fixes:
  [    1.326660] ti_abb 4ae07e30.regulator-abb-dspeve: can't request region for resource [mem 0x4ae06010-0x4ae06013]
  [    1.326660] ti_abb: probe of 4ae07e30.regulator-abb-dspeve failed with error -16
  [    1.327239] ti_abb 4ae07de4.regulator-abb-gpu: can't request region for resource [mem 0x4ae06010-0x4ae06013]
  [    1.327270] ti_abb: probe of 4ae07de4.regulator-abb-gpu failed with error -16

This partially reverts commit b36c6b1887ffc6b58b556120bfbd511880515247.

Signed-off-by: Romain Naour <romain.naour@skf.com>
---
 drivers/regulator/ti-abb-regulator.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Mark Brown Jan. 22, 2024, 5:30 p.m. UTC | #1 
On Mon, Jan 22, 2024 at 06:04:42PM +0100, Romain Naour wrote:

> We can't use devm_platform_ioremap_resource_byname() to remap the
> interrupt register that can be shared between
> regulator-abb-{ivahd,dspeve,gpu} drivers instance.

...

> The commit b36c6b1887ff (regulator: ti-abb: Make use of the helper
> function devm_ioremap related) overlooked the following comment
> explaining why devm_ioremap() is used in this case:

> /*
>  * We may have shared interrupt register offsets which are
>  * write-1-to-clear between domains ensuring exclusivity.
>  */

I have to say that I wouldn't infer from that comment that there is any
reason why _byname() won't work - one would generally expect that a
get_resource_by_name() followed by an ioremap() of that resource would
be equivalent to the combined helper.  Based on the commit log here I
frankly have no idea what the issue is.  You should also add something
to the code which makes it clear what the issue is so the same
conversion isn't performed again, assuming that the fix isn't in the
helper.

> 
> Fixes:

You're missing the commit here.

> This partially reverts commit b36c6b1887ffc6b58b556120bfbd511880515247.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
Romain Naour Jan. 23, 2024, 9:48 a.m. UTC | #2 
Hello,

Le 22/01/2024 à 18:30, Mark Brown a écrit :
> On Mon, Jan 22, 2024 at 06:04:42PM +0100, Romain Naour wrote:
> 
>> We can't use devm_platform_ioremap_resource_byname() to remap the
>> interrupt register that can be shared between
>> regulator-abb-{ivahd,dspeve,gpu} drivers instance.
> 
> ...
> 
>> The commit b36c6b1887ff (regulator: ti-abb: Make use of the helper
>> function devm_ioremap related) overlooked the following comment
>> explaining why devm_ioremap() is used in this case:
> 
>> /*
>>  * We may have shared interrupt register offsets which are
>>  * write-1-to-clear between domains ensuring exclusivity.
>>  */
> 
> I have to say that I wouldn't infer from that comment that there is any
> reason why _byname() won't work - one would generally expect that a
> get_resource_by_name() followed by an ioremap() of that resource would
> be equivalent to the combined helper.  Based on the commit log here I
> frankly have no idea what the issue is.  You should also add something
> to the code which makes it clear what the issue is so the same
> conversion isn't performed again, assuming that the fix isn't in the
> helper.

I'm agree with you about the existing comment that is not really crystal clear.

The combined helper introduce a call to devm_request_mem_region() that create a
new busy resource region on PRM_IRQSTATUS_MPU register (0x4ae06010). The first
devm_request_mem_region() call succeed for regulator-abb-ivahd but fail for the
two other regulator-abb-dspeve and regulator-abb-gpu.

Here is the iomem content without this patch:
# cat /proc/iomem | grep -i 4ae06
4ae06010-4ae06013 : 4ae07e34.regulator-abb-ivahd int-address
4ae06014-4ae06017 : 4ae07ddc.regulator-abb-mpu int-address

regulator-abb-dspeve and regulator-abb-gpu are missing due to
devm_request_mem_region() failure (EBUSY)

I don't know how to fix this issue keeping
devm_platform_ioremap_resource_byname() when the same address is used several
time... suggestion welcome.

> 
>>
>> Fixes:
> 
> You're missing the commit here.
> 
>> This partially reverts commit b36c6b1887ffc6b58b556120bfbd511880515247.
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

I added such human description above in the commit log but forgot to update this
one, sorry.

Thank you for the review.

Best regards,
Romain
diff mbox series

Patch

diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
index f48214e2c3b4..21392b9261f4 100644
--- a/drivers/regulator/ti-abb-regulator.c
+++ b/drivers/regulator/ti-abb-regulator.c
@@ -726,9 +726,22 @@  static int ti_abb_probe(struct platform_device *pdev)
 			return PTR_ERR(abb->setup_reg);
 	}
 
-	abb->int_base = devm_platform_ioremap_resource_byname(pdev, "int-address");
-	if (IS_ERR(abb->int_base))
-		return PTR_ERR(abb->int_base);
+	pname = "int-address";
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
+	if (!res) {
+		dev_err(dev, "Missing '%s' IO resource\n", pname);
+		return -ENODEV;
+	}
+	/*
+	 * We may have shared interrupt register offsets which are
+	 * write-1-to-clear between domains ensuring exclusivity.
+	 */
+	abb->int_base = devm_ioremap(dev, res->start,
+					     resource_size(res));
+	if (!abb->int_base) {
+		dev_err(dev, "Unable to map '%s'\n", pname);
+		return -ENOMEM;
+	}
 
 	/* Map Optional resources */
 	pname = "efuse-address";