Re: [PATCH v2 2/5] pwm: imx27: move constant PWMCR register values into probe

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4666 bytes --]

On Fri, Sep 25, 2020 at 05:53:27PM +0200, Marco Felsch wrote:
> The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> So it should be safe to move this bit settings into probe() and change
> only the necessary bits during apply(). Therefore I added the
> pwm_imx27_update_bits() helper.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - drop software reset from the logic
> - fix setting STOPEN, DOZEN, WAITEN and DBGEN bits in case the pwm is
>   already enabled
> 
>  drivers/pwm/pwm-imx27.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7edac4ac6395..b761764b8375 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -96,6 +96,16 @@ struct pwm_imx27_chip {
>  
>  #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
>  
> +static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(reg);
> +	tmp &= ~mask;
> +	tmp |= val & mask;
> +	return writel(tmp, reg);
> +}
> +
>  static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
>  {
>  	int ret;
> @@ -221,7 +231,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long c;
>  	unsigned long long clkrate;
>  	int ret;
> -	u32 cr;
> +	u32 cr, mask;
>  
>  	ret = pwm_imx27_clk_prepare_enable(imx);
>  	if (ret)
> @@ -267,10 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	imx->duty_cycle = duty_cycles;
>  
> -	cr = MX3_PWMCR_PRESCALER_SET(prescale) |
> -	     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -	     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> -	     MX3_PWMCR_DBGEN;
> +	cr = MX3_PWMCR_PRESCALER_SET(prescale);
>  
>  	if (state->polarity == PWM_POLARITY_INVERSED)
>  		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> @@ -279,7 +286,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->enabled)
>  		cr |= MX3_PWMCR_EN;
>  
> -	writel(cr, imx->mmio_base + MX3_PWMCR);
> +	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
> +
> +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
>  
>  	if (imx->enabled != state->enabled) {
>  		if (state->enabled) {
> @@ -314,7 +323,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  {
>  	struct pwm_imx27_chip *imx;
>  	int ret;
> -	u32 pwmcr;
> +	u32 pwmcr, mask;
>  
>  	imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
>  	if (imx == NULL)
> @@ -361,10 +370,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* keep clks on if pwm is running */
> +	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +	       MX3_PWMCR_DBGEN;
> +	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +		MX3_PWMCR_DBGEN;
> +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> +
> +	/* keep clks on and clk settings unchanged if pwm is running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> -	if (!(pwmcr & MX3_PWMCR_EN))
> +	if (!(pwmcr & MX3_PWMCR_EN)) {
> +		mask = MX3_PWMCR_CLKSRC;
> +		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
>  		pwm_imx27_clk_disable_unprepare(imx);
> +	}

You're doing more register accesses here than necessary, that is 3 reads
and two writes while one read and (maybe) one write would be enough.
(Though this doesn't work if you want to use the pwm_imx27_update_bits
helper.)

You'd need to do something like:

	val = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;

	pwmcr = readl(imx->mmio_base + MX3_PWMCR);

	if (pwmcr & MX3_PWMCR_EN) {
		imx->enabled = true;
	} else {
		val |= FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
		mask |= MX3_PWMCR_CLKSRC;
	}

	pwmcr_new = (pwmcr & ~mask) | val;

	if (pwmcr_new != val)
		writel(imx->mmio_base + MX3_PWMCR, pwmcr_new);

	if (!imx->enabled)
		pwm_imx27_clk_disable_unprepare(imx);

Hmm, not sure this is pretty enough to actually recommend this. I let
you decide.

>  	return pwmchip_add(&imx->chip);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]