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

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] [day] [month] [year] [list]

Date:   Sun, 29 Oct 2017 15:59:03 +0100
From:   Kevin Hilman <khilman@...libre.com>
To:     Neil Armstrong <narmstrong@...libre.com>
Cc:     carlo@...one.org, linux-pm@...r.kernel.org,
        linux-amlogic@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH v2 1/2] soc: amlogic: add Meson GX VPU Domains driver

Hi Neil,

Neil Armstrong <narmstrong@...libre.com> 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@...libre.com>

[...]

> +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