netdev - Re: [PATCH net-next v2 2/4] dmaengine: qcom: bam

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YWR9DSp0ry/wprLn@gerhold.net>
Date:   Mon, 11 Oct 2021 20:06:12 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Loic Poulain <loic.poulain@...aro.org>,
        Sergey Ryazanov <ryazanov.s.a@...il.com>,
        Johannes Berg <johannes@...solutions.net>,
        Andy Gross <agross@...nel.org>, Vinod Koul <vkoul@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Aleksander Morgado <aleksander@...ksander.es>,
        netdev@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        dmaengine@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        Jeffrey Hugo <jeffrey.l.hugo@...il.com>
Subject: Re: [PATCH net-next v2 2/4] dmaengine: qcom: bam_dma: Add "powered
 remotely" mode

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@...hold.net>
> > ---
> > 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@gerhold.net/
> > ---
> >  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