Message ID | 20250110051409.4099727-1-quic_mdalam@quicinc.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling | expand |
On Fri, Jan 10, 2025 at 10:44:09AM +0530, Md Sadre Alam wrote: > This patch fixes a bug introduced in the previous commit where the > BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP > mode. Additionally, it addresses an issue where reading the BAM_REVISION > register hangs if num-ees is not zero. A check has been added to prevent > this. > > Cc: stable@vger.kernel.org > 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> > --- > drivers/dma/qcom/bam_dma.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c14557efd577..2b88b27f2f91 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -445,11 +445,15 @@ static void bam_reset(struct bam_device *bdev) > writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > > /* set descriptor threshold, start with 4 bytes */ > - if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START, > - BAM_NDP_REVISION_END)) > + if (!bdev->num_ees && in_range(bdev->bam_revision, BAM_NDP_REVISION_START, > + BAM_NDP_REVISION_END)) > writel_relaxed(DEFAULT_CNT_THRSHLD, > bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); > > + if (bdev->num_ees && !bdev->bam_revision) > + writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, > + BAM_DESC_CNT_TRSHLD)); > + > /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ > writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); > > @@ -1006,10 +1010,14 @@ static void bam_apply_new_config(struct bam_chan *bchan, > maxburst = bchan->slave.src_maxburst; > else > maxburst = bchan->slave.dst_maxburst; > - if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START, > - BAM_NDP_REVISION_END)) > + if (!bdev->num_ees && in_range(bdev->bam_revision, BAM_NDP_REVISION_START, > + BAM_NDP_REVISION_END)) > writel_relaxed(maxburst, > bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); > + > + if (bdev->num_ees && !bdev->bam_revision) > + writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, > + BAM_DESC_CNT_TRSHLD)); I guess you meant writel_relaxed(maxburst, ...) here? This patch is quite confusing. We shouldn't duplicate the register writes here just to have different handling for if (bdev->num_ees) and if (!bdev->num_ees). Also, num-ees is unrelated to the question if the BAM is BAM-NDP or BAM-Lite. Typically we specify qcom,num-ees in the device tree for a BAM if the BAM is either: - Controlled remotely (= powered on and initialized outside of Linux) This is the case for the SLIMbus BAM Georgi mentioned. - Powered remotely (= powered on outside of Linux, but must be initialized inside Linux) Reading BAM_REVISION in these cases will hang in bam_init(), because we cannot guarantee the BAM is already powered on when the bam_dma driver is being loaded in Linux. We need to delay reading the register until the BAM is up. Given that these writes happen only for the !bdev->controlled_remotely case, you could fix this more cleanly by reading the BAM revision inside bam_reset(). Thanks, Stephan
On 1/10/2025 3:45 PM, Stephan Gerhold wrote: > On Fri, Jan 10, 2025 at 10:44:09AM +0530, Md Sadre Alam wrote: >> This patch fixes a bug introduced in the previous commit where the >> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP >> mode. Additionally, it addresses an issue where reading the BAM_REVISION >> register hangs if num-ees is not zero. A check has been added to prevent >> this. >> >> Cc: stable@vger.kernel.org >> 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> >> --- >> drivers/dma/qcom/bam_dma.c | 23 ++++++++++++++++------- >> 1 file changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c >> index c14557efd577..2b88b27f2f91 100644 >> --- a/drivers/dma/qcom/bam_dma.c >> +++ b/drivers/dma/qcom/bam_dma.c >> @@ -445,11 +445,15 @@ static void bam_reset(struct bam_device *bdev) >> writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); >> >> /* set descriptor threshold, start with 4 bytes */ >> - if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START, >> - BAM_NDP_REVISION_END)) >> + if (!bdev->num_ees && in_range(bdev->bam_revision, BAM_NDP_REVISION_START, >> + BAM_NDP_REVISION_END)) >> writel_relaxed(DEFAULT_CNT_THRSHLD, >> bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); >> >> + if (bdev->num_ees && !bdev->bam_revision) >> + writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, >> + BAM_DESC_CNT_TRSHLD)); >> + >> /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ >> writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); >> >> @@ -1006,10 +1010,14 @@ static void bam_apply_new_config(struct bam_chan *bchan, >> maxburst = bchan->slave.src_maxburst; >> else >> maxburst = bchan->slave.dst_maxburst; >> - if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START, >> - BAM_NDP_REVISION_END)) >> + if (!bdev->num_ees && in_range(bdev->bam_revision, BAM_NDP_REVISION_START, >> + BAM_NDP_REVISION_END)) >> writel_relaxed(maxburst, >> bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); >> + >> + if (bdev->num_ees && !bdev->bam_revision) >> + writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, >> + BAM_DESC_CNT_TRSHLD)); > > I guess you meant writel_relaxed(maxburst, ...) here? > > This patch is quite confusing. We shouldn't duplicate the register > writes here just to have different handling for if (bdev->num_ees) and > if (!bdev->num_ees). > > Also, num-ees is unrelated to the question if the BAM is BAM-NDP or > BAM-Lite. Typically we specify qcom,num-ees in the device tree for a BAM > if the BAM is either: > > - Controlled remotely (= powered on and initialized outside of Linux) > This is the case for the SLIMbus BAM Georgi mentioned. > > - Powered remotely (= powered on outside of Linux, but must be > initialized inside Linux) > > Reading BAM_REVISION in these cases will hang in bam_init(), because we > cannot guarantee the BAM is already powered on when the bam_dma driver > is being loaded in Linux. We need to delay reading the register until > the BAM is up. > > Given that these writes happen only for the !bdev->controlled_remotely > case, you could fix this more cleanly by reading the BAM revision inside > bam_reset(). Thank you for review and suggestion. Will clean up in next revision. Thanks, Alam.
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index c14557efd577..2b88b27f2f91 100644 --- a/drivers/dma/qcom/bam_dma.c +++ b/drivers/dma/qcom/bam_dma.c @@ -445,11 +445,15 @@ static void bam_reset(struct bam_device *bdev) writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); /* set descriptor threshold, start with 4 bytes */ - if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START, - BAM_NDP_REVISION_END)) + if (!bdev->num_ees && in_range(bdev->bam_revision, BAM_NDP_REVISION_START, + BAM_NDP_REVISION_END)) writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); + if (bdev->num_ees && !bdev->bam_revision) + writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, + BAM_DESC_CNT_TRSHLD)); + /* Enable default set of h/w workarounds, ie all except BAM_FULL_PIPE */ writel_relaxed(BAM_CNFG_BITS_DEFAULT, bam_addr(bdev, 0, BAM_CNFG_BITS)); @@ -1006,10 +1010,14 @@ static void bam_apply_new_config(struct bam_chan *bchan, maxburst = bchan->slave.src_maxburst; else maxburst = bchan->slave.dst_maxburst; - if (in_range(bdev->bam_revision, BAM_NDP_REVISION_START, - BAM_NDP_REVISION_END)) + if (!bdev->num_ees && in_range(bdev->bam_revision, BAM_NDP_REVISION_START, + BAM_NDP_REVISION_END)) writel_relaxed(maxburst, bam_addr(bdev, 0, BAM_DESC_CNT_TRSHLD)); + + if (bdev->num_ees && !bdev->bam_revision) + writel_relaxed(DEFAULT_CNT_THRSHLD, bam_addr(bdev, 0, + BAM_DESC_CNT_TRSHLD)); } bchan->reconfigure = 0; @@ -1196,12 +1204,13 @@ static struct dma_chan *bam_dma_xlate(struct of_phandle_args *dma_spec, */ static int bam_init(struct bam_device *bdev) { - u32 val; + u32 val = 0; /* 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;
This patch fixes a bug introduced in the previous commit where the BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP mode. Additionally, it addresses an issue where reading the BAM_REVISION register hangs if num-ees is not zero. A check has been added to prevent this. Cc: stable@vger.kernel.org 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> --- drivers/dma/qcom/bam_dma.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)