Re: [PATCHv2 05/11] soc: ti: omap-prm: sync func clock status with resets

[Tero Kristo <t-kristo@xxxxxx>]

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@xxxxxx>
> > ---
> >   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