Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 9 Oct 2019 17:20:20 +0100
From:   Peter Griffin <peter.griffin@linaro.org>
To:     Philipp Zabel <p.zabel@pengutronix.de>
Cc:     John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Enrico Weigelt <info@metux.net>
Subject: Re: [PATCH] reset: hi6220: Add support for AO reset controller
Message-ID: <20191009162020.GC29241@griffinp-mac>
References: <20191001182114.69699-1-john.stultz@linaro.org>
 <1570542167.18914.5.camel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1570542167.18914.5.camel@pengutronix.de>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Philipp,

Many thanks for your review.

(sending again as I dropped the cc list)

On Tue, 08 Oct 2019, Philipp Zabel wrote:

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

I don't know for sure, there is no documentation for these registers
apart from the code in [1] and [2], and that is really just the register name.

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

I choose to add it here as this driver was already managing the
MEDIA_SCTRL_SC_MEDIA_RSTDIS_ADDR register, so it seemed correct for it to also
manage the AO_SCTRL_SC_PW_RSTDIS0_ADDR register.

The write to AO_SCTRL_SC_PW_ISODIS0_ADDR and
AO_SCTRL_SC_PW_CLKEN0_ADDR I guess aren't so clear cut but I wanted to maintain the
same ordering from [1].

I have no info about the power domains on the SoC, so not sure it will be
possible to make it 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

This patch is based off [1] the regulator mentioned there G3D_PD_VDD it turns
out has never been available and always fails to do anything.

So AFAIK there is no external regulator available to disable. This code has since been
removed from Johns tree [2].

[2] https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey-5.3&id=64ec445a8271a2ced841484492ed9bf2e1ef4313

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

Yes I think you mentioned that before. I did send a v2 previously that
addressed that https://lkml.org/lkml/2019/4/19/253. That's my fault for
not following this series up sooner.

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

The order maybe important, I've not tried a different ordering to
what is here. I kept it the same as [1] which was working with the blob driver
and continues to work with driver.

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

You mean a register sequence like this?

regmap_write(regmap, AO_SCTRL_SC_PW_ISODIS0, BIT(idx));
regmap_write(regmap, AO_SCTRL_SC_PW_RSTDIS0, BIT(idx));
regmap_write(AO_SCTRL_SC_PW_CLKEN0, BIT(idx));

regards,

Peter.

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