| Message ID | 20220831104741.v6.2.I75baff799a363bbb960376539e3a0f737377c3f1@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clk/qcom: Support gdsc collapse polling using 'reset' interface | expand |
Hi Akhil, On Wed, Aug 31, 2022 at 10:48:23AM +0530, Akhil P Oommen wrote: > Allow soc specific clk drivers to specify a custom reset operation. We > will use this in an upcoming patch to allow gpucc driver to specify a > differet reset operation for cx_gdsc. > > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > > (no changes since v3) > > Changes in v3: > - Use pointer to const for "struct qcom_reset_ops" in qcom_reset_map (Krzysztof) > > Changes in v2: > - Return error when a particular custom reset op is not implemented. (Dmitry) > > drivers/clk/qcom/reset.c | 27 +++++++++++++++++++++++++++ > drivers/clk/qcom/reset.h | 8 ++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c > index 819d194..b7ae4a3 100644 > --- a/drivers/clk/qcom/reset.c > +++ b/drivers/clk/qcom/reset.c > @@ -13,6 +13,21 @@ > > static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id) > { > + struct qcom_reset_controller *rst; > + const struct qcom_reset_map *map; > + > + rst = to_qcom_reset_controller(rcdev); > + map = &rst->reset_map[id]; > + > + if (map->ops && map->ops->reset) > + return map->ops->reset(map->priv); > + /* > + * If custom ops is implemented but just not this callback, return > + * error > + */ > + else if (map->ops) > + return -EOPNOTSUPP; > + It doesn't seem necessary to stack reset_ops -> qcom_reset_ops for what you need here. Just add an optional const struct reset_ops * to qcom_cc_desc and feed that into qcom_cc_really_probe to replace &qcom_reset_ops. [...] > +struct qcom_reset_ops { > + int (*reset)(void *priv); > + int (*assert)(void *priv); > + int (*deassert)(void *priv); Why add assert and deassert ops? There doesn't seem to be any user. > +}; > + > struct qcom_reset_map { > unsigned int reg; > u8 bit; > + const struct qcom_reset_ops *ops; > + void *priv; Adding per-reset ops + priv counters seems excessive to me. Are you expecting different reset controls in the same reset controller to have completely different ops at some point? If so, I would wonder whether it wouldn't be better to split the reset controller in that case. Either way, for the GDSC collapse workaround this does not seem to be required at all. regards Philipp
On 9/1/2022 3:47 PM, Philipp Zabel wrote: > Hi Akhil, > > On Wed, Aug 31, 2022 at 10:48:23AM +0530, Akhil P Oommen wrote: >> Allow soc specific clk drivers to specify a custom reset operation. We >> will use this in an upcoming patch to allow gpucc driver to specify a >> differet reset operation for cx_gdsc. >> >> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> >> (no changes since v3) >> >> Changes in v3: >> - Use pointer to const for "struct qcom_reset_ops" in qcom_reset_map (Krzysztof) >> >> Changes in v2: >> - Return error when a particular custom reset op is not implemented. (Dmitry) >> >> drivers/clk/qcom/reset.c | 27 +++++++++++++++++++++++++++ >> drivers/clk/qcom/reset.h | 8 ++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c >> index 819d194..b7ae4a3 100644 >> --- a/drivers/clk/qcom/reset.c >> +++ b/drivers/clk/qcom/reset.c >> @@ -13,6 +13,21 @@ >> >> static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id) >> { >> + struct qcom_reset_controller *rst; >> + const struct qcom_reset_map *map; >> + >> + rst = to_qcom_reset_controller(rcdev); >> + map = &rst->reset_map[id]; >> + >> + if (map->ops && map->ops->reset) >> + return map->ops->reset(map->priv); >> + /* >> + * If custom ops is implemented but just not this callback, return >> + * error >> + */ >> + else if (map->ops) >> + return -EOPNOTSUPP; >> + > It doesn't seem necessary to stack reset_ops -> qcom_reset_ops for what > you need here. > Just add an optional const struct reset_ops * to qcom_cc_desc and feed > that into qcom_cc_really_probe to replace &qcom_reset_ops. > > [...] >> +struct qcom_reset_ops { >> + int (*reset)(void *priv); >> + int (*assert)(void *priv); >> + int (*deassert)(void *priv); > Why add assert and deassert ops? There doesn't seem to be any user. I had a more minimal implementation in v1. But this makes it more complete and make it less prone to trip up ourselves in future. > >> +}; >> + >> struct qcom_reset_map { >> unsigned int reg; >> u8 bit; >> + const struct qcom_reset_ops *ops; >> + void *priv; > Adding per-reset ops + priv counters seems excessive to me. > > Are you expecting different reset controls in the same reset controller > to have completely different ops at some point? If so, I would wonder > whether it wouldn't be better to split the reset controller in that > case. Either way, for the GDSC collapse workaround this does not seem > to be required at all. Yes, like I responded in patch 4, we need different ops for the same reset controller in some gpucc implementations. For eg: https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/clk/qcom/gpucc-sdm660.c -Akhil > > regards > Philipp
diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c index 819d194..b7ae4a3 100644 --- a/drivers/clk/qcom/reset.c +++ b/drivers/clk/qcom/reset.c @@ -13,6 +13,21 @@ static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id) { + struct qcom_reset_controller *rst; + const struct qcom_reset_map *map; + + rst = to_qcom_reset_controller(rcdev); + map = &rst->reset_map[id]; + + if (map->ops && map->ops->reset) + return map->ops->reset(map->priv); + /* + * If custom ops is implemented but just not this callback, return + * error + */ + else if (map->ops) + return -EOPNOTSUPP; + rcdev->ops->assert(rcdev, id); udelay(1); rcdev->ops->deassert(rcdev, id); @@ -28,6 +43,12 @@ qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id) rst = to_qcom_reset_controller(rcdev); map = &rst->reset_map[id]; + + if (map->ops && map->ops->assert) + return map->ops->assert(map->priv); + else if (map->ops) + return -EOPNOTSUPP; + mask = BIT(map->bit); return regmap_update_bits(rst->regmap, map->reg, mask, mask); @@ -42,6 +63,12 @@ qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id) rst = to_qcom_reset_controller(rcdev); map = &rst->reset_map[id]; + + if (map->ops && map->ops->deassert) + return map->ops->deassert(map->priv); + else if (map->ops) + return -EOPNOTSUPP; + mask = BIT(map->bit); return regmap_update_bits(rst->regmap, map->reg, mask, 0); diff --git a/drivers/clk/qcom/reset.h b/drivers/clk/qcom/reset.h index 2a08b5e..f3147eb 100644 --- a/drivers/clk/qcom/reset.h +++ b/drivers/clk/qcom/reset.h @@ -8,9 +8,17 @@ #include <linux/reset-controller.h> +struct qcom_reset_ops { + int (*reset)(void *priv); + int (*assert)(void *priv); + int (*deassert)(void *priv); +}; + struct qcom_reset_map { unsigned int reg; u8 bit; + const struct qcom_reset_ops *ops; + void *priv; }; struct regmap;