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@xxxxxxxxxx>
> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@xxxxxxxxxx/
> Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
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@xxxxxxxxxxx/
> * 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
>
