Re: [RESEND PATCH v2 1/2] soc: amlogic: add Meson GX VPU Domains driver

[Kevin Hilman]

Hi Neil,

Neil Armstrong <narmstrong@xxxxxxxxxxxx> writes:

> The Video Processing Unit needs a specific Power Domain powering scheme
> this driver handles this as a PM Power Domain driver.
>
> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>

[...]

> +static inline
> +struct meson_gx_pwrc_vpu *genpd_to_pd(struct generic_pm_domain *d)
> +{
> +	return container_of(d, struct meson_gx_pwrc_vpu, genpd);
> +}
> +
> +static int meson_gx_pwrc_vpu_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct meson_gx_pwrc_vpu *pd = genpd_to_pd(genpd);
> +	int i;
> +
> +	regmap_update_bits(pd->regmap_ao, AO_RTI_GEN_PWR_SLEEP0,
> +			   GEN_PWR_VPU_HDMI_ISO, GEN_PWR_VPU_HDMI_ISO);
> +	udelay(20);

Some minor nits/questions.  I know the vendor code does it this way,
but...

> +	/* Power Down Memories */
> +	for (i = 0; i < 32; i += 2) {
> +		regmap_update_bits(pd->regmap_hhi, HHI_VPU_MEM_PD_REG0,
> +				   0x2 << i, 0x3 << i);
> +		udelay(5);
> +	}

several of these bits are marked "reserved" in the datasheet.  Have you tried
without writing to the reserved bits?

> +	for (i = 0; i < 32; i += 2) {
> +		regmap_update_bits(pd->regmap_hhi, HHI_VPU_MEM_PD_REG1,
> +				   0x2 << i, 0x3 << i);
> +		udelay(5);
> +	}

ditto

> +	for (i = 8; i < 16; i++) {
> +		regmap_update_bits(pd->regmap_hhi, HHI_MEM_PD_REG0,
> +				   BIT(i), BIT(i));
> +		udelay(5);
> +	}

Do these really have to be written one bit at a time?

I'm actually OK to merge things like this, since it also captures the
sequences used by the vendor kernel, but I'm just curious if you did any
of the other experiments.

If you can try those experiments, we can take them as follow-on patches.

Kevin