[PATCHv2,05/11] soc: ti: omap-prm: sync func clock status with resets
diff mbox series

Message ID 20190828071941.32378-6-t-kristo@ti.com
State New
Headers show
Series
  • soc: ti: add OMAP PRM driver (for reset)
Related show

Commit Message

Tero Kristo Aug. 28, 2019, 7:19 a.m. UTC
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(+)

Comments

Philipp Zabel Aug. 29, 2019, 1:25 p.m. UTC | #1
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
Tero Kristo Aug. 30, 2019, 7:03 a.m. UTC | #2
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

Patch
diff mbox series

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);
 }