On Mon 11 Oct 07:17 PDT 2021, Stephan Gerhold wrote: > In some configurations, the BAM DMA controller is set up by a remote > processor and the local processor can simply start making use of it > without setting up the BAM. This is already supported using the > "qcom,controlled-remotely" property. > > However, for some reason another possible configuration is that the > remote processor is responsible for powering up the BAM, but we are > still responsible for initializing it (e.g. resetting it etc). > > This configuration is quite challenging to handle properly because > the power control is handled through separate channels > (e.g. device-specific SMSM interrupts / smem-states). Great care > must be taken to ensure the BAM registers are not accessed while > the BAM is powered off since this results in a bus stall. > > Attempt to support this configuration with minimal device-specific > code in the bam_dma driver by tracking the number of requested > channels. Consumers of DMA channels are responsible to only request > DMA channels when the BAM was powered on by the remote processor, > and to release them before the BAM is powered off. > > When the first channel is requested the BAM is initialized (reset) > and it is also put into reset when the last channel was released. > > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx> > --- > Changes since RFC: > - Drop qcom-specific terminology "power collapse", instead rename > "qcom,remote-power-collapse" -> "qcom,powered-remotely" > > NOTE: This is *not* a compile-time requirement for the BAM-DMUX driver > so this could also go through the dmaengine tree. > > See original RFC for a discussion of alternative approaches to handle > this configuration: > https://lore.kernel.org/netdev/20210719145317.79692-3-stephan@xxxxxxxxxxx/ > --- > drivers/dma/qcom/bam_dma.c | 88 ++++++++++++++++++++++++-------------- > 1 file changed, 56 insertions(+), 32 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c8a77b428b52..1b33a3ebbfec 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -388,6 +388,8 @@ struct bam_device { > /* execution environment ID, from DT */ > u32 ee; > bool controlled_remotely; > + bool powered_remotely; > + u32 active_channels; > > const struct reg_offset_data *layout; > > @@ -415,6 +417,44 @@ static inline void __iomem *bam_addr(struct bam_device *bdev, u32 pipe, > r.ee_mult * bdev->ee; > } > > +/** > + * bam_reset - reset and initialize BAM registers Please include a set of () after the function name. > + * @bdev: bam device > + */ > +static void bam_reset(struct bam_device *bdev) > +{ > + u32 val; > + > + /* s/w reset bam */ > + /* after reset all pipes are disabled and idle */ > + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > + val |= BAM_SW_RST; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > + val &= ~BAM_SW_RST; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); Seems odd to me that we assert and deassert the reset in back-to-back writes, without any delay etc. That said, this is unrelated to your patch as you just moved this hunk from below. > + > + /* make sure previous stores are visible before enabling BAM */ > + wmb(); > + > + /* enable bam */ > + val |= BAM_EN; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > + > + /* set descriptor threshhold, start with 4 bytes */ > + 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)); > + > + /* enable irqs for errors */ > + writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, > + bam_addr(bdev, 0, BAM_IRQ_EN)); > + > + /* unmask global bam interrupt */ > + writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); > +} > + > /** > * bam_reset_channel - Reset individual BAM DMA channel > * @bchan: bam channel > @@ -512,6 +552,9 @@ static int bam_alloc_chan(struct dma_chan *chan) > return -ENOMEM; > } > > + if (bdev->active_channels++ == 0 && bdev->powered_remotely) > + bam_reset(bdev); > + > return 0; > } > > @@ -565,6 +608,13 @@ static void bam_free_chan(struct dma_chan *chan) > /* disable irq */ > writel_relaxed(0, bam_addr(bdev, bchan->id, BAM_P_IRQ_EN)); > > + if (--bdev->active_channels == 0 && bdev->powered_remotely) { > + /* s/w reset bam */ > + val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > + val |= BAM_SW_RST; > + writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > + } > + > err: > pm_runtime_mark_last_busy(bdev->dev); > pm_runtime_put_autosuspend(bdev->dev); > @@ -1164,38 +1214,10 @@ static int bam_init(struct bam_device *bdev) > bdev->num_channels = val & BAM_NUM_PIPES_MASK; > } > > - if (bdev->controlled_remotely) > + if (bdev->controlled_remotely || bdev->powered_remotely) > return 0; I think the resulting code would be cleaner if you flipped it around as: /* Reset BAM now if fully controlled locally */ if (!bdev->controlled_remotely && !bdev->powered_remotely) bam_reset(bdev); return 0; Regards, Bjorn > > - /* s/w reset bam */ > - /* after reset all pipes are disabled and idle */ > - val = readl_relaxed(bam_addr(bdev, 0, BAM_CTRL)); > - val |= BAM_SW_RST; > - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > - val &= ~BAM_SW_RST; > - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > - > - /* make sure previous stores are visible before enabling BAM */ > - wmb(); > - > - /* enable bam */ > - val |= BAM_EN; > - writel_relaxed(val, bam_addr(bdev, 0, BAM_CTRL)); > - > - /* set descriptor threshhold, start with 4 bytes */ > - 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)); > - > - /* enable irqs for errors */ > - writel_relaxed(BAM_ERROR_EN | BAM_HRESP_ERR_EN, > - bam_addr(bdev, 0, BAM_IRQ_EN)); > - > - /* unmask global bam interrupt */ > - writel_relaxed(BAM_IRQ_MSK, bam_addr(bdev, 0, BAM_IRQ_SRCS_MSK_EE)); > - > + bam_reset(bdev); > return 0; > }
