Re: [PATCH] reset: hi6220: Add support for AO reset controller

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi John, Peter,

On Tue, 2019-10-01 at 18:21 +0000, John Stultz wrote:
> From: Peter Griffin <peter.griffin@linaro.org>
> 
> This is required to bring Mali450 gpu out of reset.

Do you know whether this is actually a module reset going to the GPU,
or if this is somehow part of the clock and power gating machinery?
There's also clock and isolation cell control going on, so this looks a
bit like it should be part of a power domain driver.
Do you have an (internal or external) regulator that could be disabled
while the GPU is inactive, like [1]?

[1] https://raw.githubusercontent.com/96boards/meta-96boards/master/recipes-kernel/linux/linux-96boards/0004-drivers-gpu-arm-utgard-add-basic-HiKey-platform-file.patch

> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Peter Griffin <peter.griffin@linaro.org>
> Cc: Enrico Weigelt <info@metux.net>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/reset/hisilicon/hi6220_reset.c | 51 +++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/hisilicon/hi6220_reset.c b/drivers/reset/hisilicon/hi6220_reset.c
> index 24e6d420b26b..d84674a2cead 100644
> --- a/drivers/reset/hisilicon/hi6220_reset.c
> +++ b/drivers/reset/hisilicon/hi6220_reset.c
> @@ -33,6 +33,7 @@
>  enum hi6220_reset_ctrl_type {
>  	PERIPHERAL,
>  	MEDIA,
> +	AO,
>  };
>  
>  struct hi6220_reset_data {
> @@ -92,6 +93,47 @@ static const struct reset_control_ops hi6220_media_reset_ops = {
>  	.deassert = hi6220_media_deassert,
>  };
>  
> +#define AO_SCTRL_SC_PW_CLKEN0     0x800
> +#define AO_SCTRL_SC_PW_CLKDIS0    0x804
> +
> +#define AO_SCTRL_SC_PW_RSTEN0     0x810
> +#define AO_SCTRL_SC_PW_RSTDIS0    0x814
> +
> +#define AO_SCTRL_SC_PW_ISOEN0     0x820
> +#define AO_SCTRL_SC_PW_ISODIS0    0x824
> +#define AO_MAX_INDEX              12
> +
> +static int hi6220_ao_assert(struct reset_controller_dev *rc_dev,
> +			    unsigned long idx)
> +{
> +	struct hi6220_reset_data *data = to_reset_data(rc_dev);
> +	struct regmap *regmap = data->regmap;
> +	int ret;
> +
> +	ret = regmap_write(regmap, AO_SCTRL_SC_PW_RSTEN0, BIT(idx));
> +	ret |= regmap_write(regmap, AO_SCTRL_SC_PW_ISOEN0, BIT(idx));
> +	ret |= regmap_write(regmap, AO_SCTRL_SC_PW_CLKDIS0, BIT(idx));

I would like the return values not to be ORed together, since they are
returned to the caller.

> +	return ret;
> +}
> +
> +static int hi6220_ao_deassert(struct reset_controller_dev *rc_dev,
> +			      unsigned long idx)
> +{
> +	struct hi6220_reset_data *data = to_reset_data(rc_dev);
> +	struct regmap *regmap = data->regmap;
> +	int ret;
> +
> +	ret = regmap_write(regmap, AO_SCTRL_SC_PW_RSTDIS0, BIT(idx));
> +	ret |= regmap_write(regmap, AO_SCTRL_SC_PW_ISODIS0, BIT(idx));
> +	ret |= regmap_write(regmap, AO_SCTRL_SC_PW_CLKEN0, BIT(idx));

Are you sure these are fire and forget, or might the order be important?

It might be necessary to disable isolation before enabling the clocks
and deasserting reset, to avoid glitches.

> +	return ret;
> +}
> +
> +static const struct reset_control_ops hi6220_ao_reset_ops = {
> +	.assert = hi6220_ao_assert,
> +	.deassert = hi6220_ao_deassert,
> +};
> +
>  static int hi6220_reset_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -117,9 +159,12 @@ static int hi6220_reset_probe(struct platform_device *pdev)
>  	if (type == MEDIA) {
>  		data->rc_dev.ops = &hi6220_media_reset_ops;
>  		data->rc_dev.nr_resets = MEDIA_MAX_INDEX;
> -	} else {
> +	} else if (type == PERIPHERAL) {
>  		data->rc_dev.ops = &hi6220_peripheral_reset_ops;
>  		data->rc_dev.nr_resets = PERIPH_MAX_INDEX;
> +	} else {
> +		data->rc_dev.ops = &hi6220_ao_reset_ops;
> +		data->rc_dev.nr_resets = AO_MAX_INDEX;
>  	}
>  
>  	return reset_controller_register(&data->rc_dev);
> @@ -134,6 +179,10 @@ static const struct of_device_id hi6220_reset_match[] = {
>  		.compatible = "hisilicon,hi6220-mediactrl",
>  		.data = (void *)MEDIA,
>  	},
> +	{
> +		.compatible = "hisilicon,hi6220-aoctrl",
> +		.data = (void *)AO,
> +	},
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, hi6220_reset_match);

regards
Philipp