| Message ID | 20190828071941.32378-6-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: ti: add OMAP PRM driver (for reset) | expand |
On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote: > Hardware reset signals are tightly coupled with associated clocks, and > basically de-asserting a reset won't succeed properly if the clock is > not enabled, and vice-versa. Also, disabling a clock won't fully succeed > if the associated hardware resets are not asserted. Add status sync > functionality between these two for TI drivers so that the situations > can be handled properly without generating any timeouts. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/soc/ti/omap_prm.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c > index 38998ce19c71..e876bad8f8d5 100644 > --- a/drivers/soc/ti/omap_prm.c > +++ b/drivers/soc/ti/omap_prm.c > @@ -15,6 +15,8 @@ > #include <linux/platform_device.h> > #include <linux/reset-controller.h> > #include <linux/delay.h> > +#include <linux/clk.h> > +#include <linux/clk/ti.h> > > #include <linux/platform_data/ti-prm.h> > > @@ -42,7 +44,9 @@ struct omap_reset_data { > struct reset_controller_dev rcdev; > struct omap_prm *prm; > struct clockdomain *clkdm; > + struct clk *clk; > struct device *dev; > + u32 mask; > }; > > #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev) > @@ -102,6 +106,8 @@ static int omap_reset_assert(struct reset_controller_dev *rcdev, > v |= 1 << id; > writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl); > > + ti_clk_notify_resets(reset->clk, v == reset->mask); > + > return 0; > } > > @@ -163,9 +169,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev, > v &= ~(1 << id); > writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl); > > + ti_clk_notify_resets(reset->clk, v == reset->mask); > + > if (!has_rstst) > goto exit; > > + /* If associated clock is disabled, we can't poll completion status */ > + if (reset->clk) { > + struct clk_hw *hw = __clk_get_hw(reset->clk); > + > + if (!clk_hw_is_enabled(hw)) > + return ret; > + } > + > /* wait for the status to be set */ > while (1) { > v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); > @@ -199,8 +215,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, > struct omap_prm *prm) > { > struct omap_reset_data *reset; > + const struct omap_rst_map *map; > struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev); > char buf[32]; > + u32 v; > > /* > * Check if we have controllable resets. If either rstctrl is non-zero > @@ -215,6 +233,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, > !pdata->clkdm_allow_idle) > return -EINVAL; > > + map = prm->data->rstmap; > + if (!map) > + return -EINVAL; Can this actually happen? > + > reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); > if (!reset) > return -ENOMEM; > @@ -224,6 +246,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, > reset->rcdev.of_node = pdev->dev.of_node; > reset->rcdev.nr_resets = OMAP_MAX_RESETS; > reset->dev = &pdev->dev; > + reset->clk = of_clk_get(pdev->dev.of_node, 0); > + > + if (IS_ERR(reset->clk)) > + reset->clk = NULL; Maybe only ignore -ENOENT? > reset->prm = prm; > > @@ -234,6 +260,16 @@ static int omap_prm_reset_init(struct platform_device *pdev, > if (!reset->clkdm) > return -EINVAL; > > + while (map->rst >= 0) { > + reset->mask |= BIT(map->rst); > + map++; > + } With this, you could use reset->mask to simplify _is_valid_reset. regards Philipp
On 29/08/2019 16:25, Philipp Zabel wrote: > On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote: >> Hardware reset signals are tightly coupled with associated clocks, and >> basically de-asserting a reset won't succeed properly if the clock is >> not enabled, and vice-versa. Also, disabling a clock won't fully succeed >> if the associated hardware resets are not asserted. Add status sync >> functionality between these two for TI drivers so that the situations >> can be handled properly without generating any timeouts. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/soc/ti/omap_prm.c | 36 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c >> index 38998ce19c71..e876bad8f8d5 100644 >> --- a/drivers/soc/ti/omap_prm.c >> +++ b/drivers/soc/ti/omap_prm.c >> @@ -15,6 +15,8 @@ >> #include <linux/platform_device.h> >> #include <linux/reset-controller.h> >> #include <linux/delay.h> >> +#include <linux/clk.h> >> +#include <linux/clk/ti.h> >> >> #include <linux/platform_data/ti-prm.h> >> >> @@ -42,7 +44,9 @@ struct omap_reset_data { >> struct reset_controller_dev rcdev; >> struct omap_prm *prm; >> struct clockdomain *clkdm; >> + struct clk *clk; >> struct device *dev; >> + u32 mask; >> }; >> >> #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev) >> @@ -102,6 +106,8 @@ static int omap_reset_assert(struct reset_controller_dev *rcdev, >> v |= 1 << id; >> writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl); >> >> + ti_clk_notify_resets(reset->clk, v == reset->mask); >> + >> return 0; >> } >> >> @@ -163,9 +169,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev, >> v &= ~(1 << id); >> writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl); >> >> + ti_clk_notify_resets(reset->clk, v == reset->mask); >> + >> if (!has_rstst) >> goto exit; >> >> + /* If associated clock is disabled, we can't poll completion status */ >> + if (reset->clk) { >> + struct clk_hw *hw = __clk_get_hw(reset->clk); >> + >> + if (!clk_hw_is_enabled(hw)) >> + return ret; >> + } >> + >> /* wait for the status to be set */ >> while (1) { >> v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); >> @@ -199,8 +215,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, >> struct omap_prm *prm) >> { >> struct omap_reset_data *reset; >> + const struct omap_rst_map *map; >> struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev); >> char buf[32]; >> + u32 v; >> >> /* >> * Check if we have controllable resets. If either rstctrl is non-zero >> @@ -215,6 +233,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, >> !pdata->clkdm_allow_idle) >> return -EINVAL; >> >> + map = prm->data->rstmap; >> + if (!map) >> + return -EINVAL; > > Can this actually happen? It can, if user writes bad data to the omap_prm_data structs. Without this check it would make the probe crash which is bad. > >> + >> reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); >> if (!reset) >> return -ENOMEM; >> @@ -224,6 +246,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, >> reset->rcdev.of_node = pdev->dev.of_node; >> reset->rcdev.nr_resets = OMAP_MAX_RESETS; >> reset->dev = &pdev->dev; >> + reset->clk = of_clk_get(pdev->dev.of_node, 0); >> + >> + if (IS_ERR(reset->clk)) >> + reset->clk = NULL; > > Maybe only ignore -ENOENT? Yeah, I can modify this. > >> reset->prm = prm; >> >> @@ -234,6 +260,16 @@ static int omap_prm_reset_init(struct platform_device *pdev, >> if (!reset->clkdm) >> return -EINVAL; >> >> + while (map->rst >= 0) { >> + reset->mask |= BIT(map->rst); >> + map++; >> + } > > With this, you could use reset->mask to simplify _is_valid_reset. True, let me re-write it. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c index 38998ce19c71..e876bad8f8d5 100644 --- a/drivers/soc/ti/omap_prm.c +++ b/drivers/soc/ti/omap_prm.c @@ -15,6 +15,8 @@ #include <linux/platform_device.h> #include <linux/reset-controller.h> #include <linux/delay.h> +#include <linux/clk.h> +#include <linux/clk/ti.h> #include <linux/platform_data/ti-prm.h> @@ -42,7 +44,9 @@ struct omap_reset_data { struct reset_controller_dev rcdev; struct omap_prm *prm; struct clockdomain *clkdm; + struct clk *clk; struct device *dev; + u32 mask; }; #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev) @@ -102,6 +106,8 @@ static int omap_reset_assert(struct reset_controller_dev *rcdev, v |= 1 << id; writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl); + ti_clk_notify_resets(reset->clk, v == reset->mask); + return 0; } @@ -163,9 +169,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev, v &= ~(1 << id); writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl); + ti_clk_notify_resets(reset->clk, v == reset->mask); + if (!has_rstst) goto exit; + /* If associated clock is disabled, we can't poll completion status */ + if (reset->clk) { + struct clk_hw *hw = __clk_get_hw(reset->clk); + + if (!clk_hw_is_enabled(hw)) + return ret; + } + /* wait for the status to be set */ while (1) { v = readl_relaxed(reset->prm->base + reset->prm->data->rstst); @@ -199,8 +215,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, struct omap_prm *prm) { struct omap_reset_data *reset; + const struct omap_rst_map *map; struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev); char buf[32]; + u32 v; /* * Check if we have controllable resets. If either rstctrl is non-zero @@ -215,6 +233,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, !pdata->clkdm_allow_idle) return -EINVAL; + map = prm->data->rstmap; + if (!map) + return -EINVAL; + reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL); if (!reset) return -ENOMEM; @@ -224,6 +246,10 @@ static int omap_prm_reset_init(struct platform_device *pdev, reset->rcdev.of_node = pdev->dev.of_node; reset->rcdev.nr_resets = OMAP_MAX_RESETS; reset->dev = &pdev->dev; + reset->clk = of_clk_get(pdev->dev.of_node, 0); + + if (IS_ERR(reset->clk)) + reset->clk = NULL; reset->prm = prm; @@ -234,6 +260,16 @@ static int omap_prm_reset_init(struct platform_device *pdev, if (!reset->clkdm) return -EINVAL; + while (map->rst >= 0) { + reset->mask |= BIT(map->rst); + map++; + } + + if (reset->clk) { + v = readl_relaxed(reset->prm->base + reset->prm->data->rstctrl); + ti_clk_notify_resets(reset->clk, v == reset->mask); + } + return devm_reset_controller_register(&pdev->dev, &reset->rcdev); }
Hardware reset signals are tightly coupled with associated clocks, and basically de-asserting a reset won't succeed properly if the clock is not enabled, and vice-versa. Also, disabling a clock won't fully succeed if the associated hardware resets are not asserted. Add status sync functionality between these two for TI drivers so that the situations can be handled properly without generating any timeouts. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/soc/ti/omap_prm.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)