Re: [PATCH v4 1/5] dmaengine: imx-sdma: add clock ratio 1:1 check

From: Lucas Stach <l.stach@pengutronix.de>
To: "Angus Ainslie (Purism)" <angus@akkea.ca>
Cc: angus.ainslie@puri.sm, Vinod Koul <vkoul@kernel.org>,
	dmaengine@vger.kernel.org, NXP Linux Team <linux-imx@nxp.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Daniel Baluta <daniel.baluta@gmail.com>
Subject: Re: [PATCH v4 1/5] dmaengine: imx-sdma: add clock ratio 1:1 check
Date: Fri, 25 Jan 2019 10:42:46 +0100	[thread overview]
Message-ID: <1548409366.28802.39.camel@pengutronix.de> (raw)
In-Reply-To: <20190125025528.15645-2-angus@akkea.ca>

Am Donnerstag, den 24.01.2019, 19:55 -0700 schrieb Angus Ainslie (Purism):
> On i.mx8 mscale B0 chip, AHB/SDMA clock ratio 2:1 can't be supportted,
> since SDMA clock ratio has to be increased to 250Mhz, AHB can't reach
> to 500Mhz, so use 1:1 instead.
> 
> > Based on NXP commit MLK-16841-1 by Robin Gong <yibin.gong@nxp.com>
> 
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> ---
>  drivers/dma/imx-sdma.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 0b3a67ff8e82..5e5ef0b5a973 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -440,6 +440,8 @@ struct sdma_engine {
> > >  	unsigned int			irq;
> > >  	dma_addr_t			bd0_phys;
> > >  	struct sdma_buffer_descriptor	*bd0;
> > +	/* clock ratio for AHB:SDMA core. 1:1 is 1, 2:1 is 0*/
> > > +	bool				clk_ratio;
>  };
>  
>  static int sdma_config_write(struct dma_chan *chan,
> @@ -662,8 +664,14 @@ static int sdma_run_channel0(struct sdma_engine *sdma)
> >  		dev_err(sdma->dev, "Timeout waiting for CH0 ready\n");
>  
> >  	/* Set bits of CONFIG register with dynamic context switching */
> > -	if (readl(sdma->regs + SDMA_H_CONFIG) == 0)
> > -		writel_relaxed(SDMA_H_CONFIG_CSM, sdma->regs + SDMA_H_CONFIG);
> +	if ((readl(sdma->regs + SDMA_H_CONFIG) & ~SDMA_H_CONFIG_ACR) == 0) {

The intention of this code was probably to set the CSM bit if they
weren't set before, so masking out individual bits from the register
and risking to skip this when one of the other bits was set doesn't
seem right.

I guess the whole block can be simplified to:

reg = readl(sdma->regs + SDMA_H_CONFIG);
if ((reg & SDMA_H_CONFIG_CSM) != SDMA_H_CONFIG_CSM)
	reg |= SDMA_H_CONFIG_CSM;
writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);

Regards,
Lucas

> +		reg = SDMA_H_CONFIG_CSM;
> +
> > +		if (sdma->clk_ratio)
> > +			reg |= SDMA_H_CONFIG_ACR;
> +
> > +		writel_relaxed(reg, sdma->regs + SDMA_H_CONFIG);
> > +	}
>  
> >  	return ret;
>  }
> @@ -1840,6 +1848,9 @@ static int sdma_init(struct sdma_engine *sdma)
> >  	if (ret)
> >  		goto disable_clk_ipg;
>  
> > +	if (clk_get_rate(sdma->clk_ahb) == clk_get_rate(sdma->clk_ipg))
> > +		sdma->clk_ratio = 1;
> +
> >  	/* Be sure SDMA has not started yet */
> >  	writel_relaxed(0, sdma->regs + SDMA_H_C0PTR);
>  
> @@ -1880,8 +1891,10 @@ static int sdma_init(struct sdma_engine *sdma)
> >  	writel_relaxed(0x4050, sdma->regs + SDMA_CHN0ADDR);
>  
> >  	/* Set bits of CONFIG register but with static context switching */
> > -	/* FIXME: Check whether to set ACR bit depending on clock ratios */
> > -	writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
> > +	if (sdma->clk_ratio)
> > +		writel_relaxed(SDMA_H_CONFIG_ACR, sdma->regs + SDMA_H_CONFIG);
> > +	else
> > +		writel_relaxed(0, sdma->regs + SDMA_H_CONFIG);
>  
> >  	writel_relaxed(ccb_phys, sdma->regs + SDMA_H_C0PTR);
>  