| Message ID | 20191001182114.69699-1-john.stultz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reset: hi6220: Add support for AO reset controller | expand |
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
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
Hi Peter, On Wed, 2019-10-09 at 17:20 +0100, Peter Griffin wrote: [...] > > 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. Ok. > > 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. I agree here ... > 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]. ... but not here, necessarily. These register names make it look like this is the wrong place. On the other hand, if you don't actually have a way to disable power to the GPU domain, it is a bit hard for me to argue that it would be more correct to place this in a power domain driver. Certainly the power domain driver could (de)assert the reset in the right order though. I notice the actual GPU reset seems to be MEDIA_G3D, and is deasserted after whatever is controlled via AO_SCTRL_SC_PW_* in [1], so maybe this is indeed just the power domain control? > 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 The comment "Note: GPU DVFS is not implemented and a custom Device Tree entry is needed by these platform files. This is a first working version that needs to be improved." doesn't exactly make it clear whether it is "not implemented" in the SoC, not implemented on the board, or just not implemented in the driver in this patchset. So even if this board has the regulator fixed, could there be others where GPU power could be switched off? I wouldn't want to encode the the limitations of a particular board into the SoC DT bindings. > > > 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 Is this the only reset register in the "Always-On" (that would have been nice to include in the commit description for my education) controller? According to [3] there also seem to be rst4 and rst5 registers? [3] https://github.com/u-boot/u-boot/blob/master/arch/arm/include/asm/arch-hi6220/hi6220_regs_alwayson.h > > > + > > > +#define AO_SCTRL_SC_PW_ISOEN0 0x820 > > > +#define AO_SCTRL_SC_PW_ISODIS0 0x824 > > > +#define AO_MAX_INDEX 12 Where does this number come from? I can't find any information about the bits in the PW_* registers... > > > + > > > +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. I'm sorry, I missed that. > > > + 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. Ok, that makes sense. > > 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)); I'm just asking whether you have considered potential side effects, but without better knowledge about the SoC, following the order found in the vendor code, is probably the best we can do. In [1] the corresponding _STAT registers (+0x8) are checked after setting each bit, is there a reason you dropped these checks? regards Philipp
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)); + 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)); + 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);