[PATCH v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling

[PATCH v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling

[Md Sadre Alam]

This patch resolves a bug from the previous commit where the
BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP
mode. The issue was reading the BAM_REVISION register hanging if num-ees
was not zero, which occurs when the SoCs power on BAM remotely. So the
BAM_REVISION register read has been moved to inside if condition.

Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
Reported-by: Georgi Djakov <djakov@kernel.org>
Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---

Change in [v3]

* Revised commit details

Change in [v2]

* Removed unnecessary if checks.
* Relocated the BAM_REVISION register read within the if condition.

Change in [v1]

* https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/
* Posted initial fixup for BAM revision register read handling
 drivers/dma/qcom/bam_dma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
index c14557efd577..d227b4f5b6b9 100644
--- a/drivers/dma/qcom/bam_dma.c
+++ b/drivers/dma/qcom/bam_dma.c
@@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev)
 	u32 val;
 
 	/* read revision and configuration information */
-	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
-	if (!bdev->num_ees)
+	if (!bdev->num_ees) {
+		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
 		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
-
-	bdev->bam_revision = val & REVISION_MASK;
+		bdev->bam_revision = val & REVISION_MASK;
+	}
 
 	/* check that configured EE is within range */
 	if (bdev->ee >= bdev->num_ees)
-- 
2.34.1

Re: [PATCH v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling

[Dmitry Baryshkov]

On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote:
> This patch resolves a bug from the previous commit where the

Please check Documentation/process/submitting-patches.rst, 'This
patch...'

> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP
> mode. The issue was reading the BAM_REVISION register hanging if num-ees

First start with the issue description, then proceed to the changes
description.

> was not zero, which occurs when the SoCs power on BAM remotely. So the
> BAM_REVISION register read has been moved to inside if condition.

Imperative language, please. While we are at it, please also fix commit
subject.

> 
> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> Reported-by: Georgi Djakov <djakov@kernel.org>
> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> ---
> 
> Change in [v3]
> 
> * Revised commit details
> 
> Change in [v2]
> 
> * Removed unnecessary if checks.
> * Relocated the BAM_REVISION register read within the if condition.
> 
> Change in [v1]
> 
> * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/
> * Posted initial fixup for BAM revision register read handling
>  drivers/dma/qcom/bam_dma.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577..d227b4f5b6b9 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev)
>  	u32 val;
>  
>  	/* read revision and configuration information */

Please extend the comment, mentioning why the register is read only in
!num_ees case. BTW: how do we get revision if num_ees != 0?

> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> -	if (!bdev->num_ees)
> +	if (!bdev->num_ees) {
> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>  		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
> -
> -	bdev->bam_revision = val & REVISION_MASK;
> +		bdev->bam_revision = val & REVISION_MASK;
> +	}
>  
>  	/* check that configured EE is within range */
>  	if (bdev->ee >= bdev->num_ees)
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling

[Md Sadre Alam]

On 1/21/2025 4:24 PM, Dmitry Baryshkov wrote:
> On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote:
>> This patch resolves a bug from the previous commit where the
> 
> Please check Documentation/process/submitting-patches.rst, 'This
> patch...'
Ok
> 
>> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP
>> mode. The issue was reading the BAM_REVISION register hanging if num-ees
> 
> First start with the issue description, then proceed to the changes
> description.
Ok
> 
>> was not zero, which occurs when the SoCs power on BAM remotely. So the
>> BAM_REVISION register read has been moved to inside if condition.
> 
> Imperative language, please. While we are at it, please also fix commit
> subject.
Sure , will fix this in next revision.
> 
>>
>> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
>> Reported-by: Georgi Djakov <djakov@kernel.org>
>> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v3]
>>
>> * Revised commit details
>>
>> Change in [v2]
>>
>> * Removed unnecessary if checks.
>> * Relocated the BAM_REVISION register read within the if condition.
>>
>> Change in [v1]
>>
>> * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/
>> * Posted initial fixup for BAM revision register read handling
>>   drivers/dma/qcom/bam_dma.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index c14557efd577..d227b4f5b6b9 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev)
>>   	u32 val;
>>   
>>   	/* read revision and configuration information */
> 
> Please extend the comment, mentioning why the register is read only in
> !num_ees case. BTW: how do we get revision if num_ees != 0?
This revision register we need only to differentiate b/w BAM-Lite and
BAM-NDP. if num_ees != 0 then no need read this revision register.
The SOCs which is having BAM-NDP and BAM-Lite having num_ees = 0.
> 
>> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>> -	if (!bdev->num_ees)
>> +	if (!bdev->num_ees) {
>> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>>   		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
>> -
>> -	bdev->bam_revision = val & REVISION_MASK;
>> +		bdev->bam_revision = val & REVISION_MASK;
>> +	}
>>   
>>   	/* check that configured EE is within range */
>>   	if (bdev->ee >= bdev->num_ees)
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling

[Stephan Gerhold]

On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote:
> This patch resolves a bug from the previous commit where the
> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP
> mode. The issue was reading the BAM_REVISION register hanging if num-ees
> was not zero, which occurs when the SoCs power on BAM remotely. So the
> BAM_REVISION register read has been moved to inside if condition.
> 
> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
> Reported-by: Georgi Djakov <djakov@kernel.org>
> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

I'm afraid there are still two open problems here:

 1. In your original commit, you added the if (in_range(...)) checks to
    make the BAM_DESC_CNT_TRSHLD register write conditional. With this
    patch we only read the bam_revision for the !bdev->num_ees case.
    This means that even if we have e.g. a remotely powered BAM-NDP,
    we don't initialize BAM_DESC_CNT_TRSHLD anymore.

 2. Aside from BAM-NDP and BAM-Lite there is also plain "BAM". You
    mentioned we should only skip the register write for BAM-Lite, but
    the plain "BAM" isn't handled anywhere yet.

I would recommend inverting the in_range(...) checks to check for if
(!in_range(BAM-LITE) rather than if (in_range(BAM-NDP)). This should
also work for the plain "BAM" type. It will also avoid regressions if we
don't read the bam_revision in the !bdev->num_ees case. (Although
ideally you would lazily initialize the bam_revision to cover all the
configurations.)

Thanks,
Stephan

> ---
> 
> Change in [v3]
> 
> * Revised commit details
> 
> Change in [v2]
> 
> * Removed unnecessary if checks.
> * Relocated the BAM_REVISION register read within the if condition.
> 
> Change in [v1]
> 
> * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/
> * Posted initial fixup for BAM revision register read handling
>  drivers/dma/qcom/bam_dma.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index c14557efd577..d227b4f5b6b9 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev)
>  	u32 val;
>  
>  	/* read revision and configuration information */
> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
> -	if (!bdev->num_ees)
> +	if (!bdev->num_ees) {
> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>  		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
> -
> -	bdev->bam_revision = val & REVISION_MASK;
> +		bdev->bam_revision = val & REVISION_MASK;
> +	}
>  
>  	/* check that configured EE is within range */
>  	if (bdev->ee >= bdev->num_ees)
> -- 
> 2.34.1
> 

Re: [PATCH v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling

[Md Sadre Alam]

On 1/21/2025 10:39 PM, Stephan Gerhold wrote:
> On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote:
>> This patch resolves a bug from the previous commit where the
>> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP
>> mode. The issue was reading the BAM_REVISION register hanging if num-ees
>> was not zero, which occurs when the SoCs power on BAM remotely. So the
>> BAM_REVISION register read has been moved to inside if condition.
>>
>> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register")
>> Reported-by: Georgi Djakov <djakov@kernel.org>
>> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> 
> I'm afraid there are still two open problems here:
> 
>   1. In your original commit, you added the if (in_range(...)) checks to
>      make the BAM_DESC_CNT_TRSHLD register write conditional. With this
>      patch we only read the bam_revision for the !bdev->num_ees case.
>      This means that even if we have e.g. a remotely powered BAM-NDP,
>      we don't initialize BAM_DESC_CNT_TRSHLD anymore.
> 
>   2. Aside from BAM-NDP and BAM-Lite there is also plain "BAM". You
>      mentioned we should only skip the register write for BAM-Lite, but
>      the plain "BAM" isn't handled anywhere yet.
> 
> I would recommend inverting the in_range(...) checks to check for if
> (!in_range(BAM-LITE) rather than if (in_range(BAM-NDP)). This should
> also work for the plain "BAM" type. It will also avoid regressions if we
> don't read the bam_revision in the !bdev->num_ees case. (Although
> ideally you would lazily initialize the bam_revision to cover all the
> configurations.)
Thanks for explanation and suggestion. Will address all the above points
in next revision.
> 
> Thanks,
> Stephan
> 
>> ---
>>
>> Change in [v3]
>>
>> * Revised commit details
>>
>> Change in [v2]
>>
>> * Removed unnecessary if checks.
>> * Relocated the BAM_REVISION register read within the if condition.
>>
>> Change in [v1]
>>
>> * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/
>> * Posted initial fixup for BAM revision register read handling
>>   drivers/dma/qcom/bam_dma.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index c14557efd577..d227b4f5b6b9 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev)
>>   	u32 val;
>>   
>>   	/* read revision and configuration information */
>> -	val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>> -	if (!bdev->num_ees)
>> +	if (!bdev->num_ees) {
>> +		val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION));
>>   		bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK;
>> -
>> -	bdev->bam_revision = val & REVISION_MASK;
>> +		bdev->bam_revision = val & REVISION_MASK;
>> +	}
>>   
>>   	/* check that configured EE is within range */
>>   	if (bdev->ee >= bdev->num_ees)
>> -- 
>> 2.34.1
>>