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