On Mon, Oct 11, 2021 at 10:39:20AM -0700, Bjorn Andersson wrote: > 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. > Thanks, will fix this in v3. > > + * @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. > It seems to work fine though. :) I'm pretty sure I checked that this actually resets at some point, but perhaps I was lucky there. But yeah, seems better to adjust this in some future patch instead of here. > > + > > + /* 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; > Hmm, you are probably right, I can flip it in v3. Thanks! Stephan
