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

[Philipp Zabel <p.zabel@pengutronix.de>]

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