mbox series

[PATCHv2,00/11] soc: ti: add OMAP PRM driver (for reset)

Message ID 20190828071941.32378-1-t-kristo@ti.com
Headers show
Series soc: ti: add OMAP PRM driver (for reset) | expand

Message

Tero Kristo Aug. 28, 2019, 7:19 a.m. UTC 
Hi,

V2 of the series mostly has comments fixed from Suman.
- Added a link between reset + clock drivers to sync up the state between
  these; this is to avoid facing any timeout issues on either end due to
  sequencing of events (Patch #5.) This has been implemented via TI only
  private driver APIs, as at least I am not aware of anybody else needing
  similar mechanism and it is pretty SoC architecture specific.
- Dropped any powerdomain related data for now as it is not used for
  anything yet.
- Added checks against illegal reset IDs.
- Added checks for pdata validity during probe.
- Reset data is added for am4/omap5 SoCs.
- Some other minor tweaks.

This series depends on the clock driver changes [1] due to patch #5,
otherwise there will be build breakage.

Also, just as a background note, this driver has been implemented
under drivers/soc/ti due to the fact that I did not figure out any
better home for it. In its current form it would be suitable to
reside under drivers/reset, but there is a plan to extend this to
support powerdomain handling also (PRM stands for Power and Reset
Management.)

-Tero

[1] https://marc.info/?l=linux-clk&m=156697558331203&w=2


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Philipp Zabel Aug. 29, 2019, 1:14 p.m. UTC | #1 
Hi Tero,

On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:
> Poll for reset completion status during de-assertion of reset, otherwise

> the IP in question might be accessed before it has left reset properly.

> 

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++

>  1 file changed, 20 insertions(+)

> 

> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c

> index fd5c431f8736..afeb70761b27 100644

> --- a/drivers/soc/ti/omap_prm.c

> +++ b/drivers/soc/ti/omap_prm.c

> @@ -127,6 +127,7 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,

>  	u32 v;

>  	int st_bit;

>  	bool has_rstst;

> +	int timeout = 0;

>  

>  	if (!_is_valid_reset(reset, id))

>  		return -EINVAL;

> @@ -153,6 +154,25 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,

>  	v &= ~(1 << id);

>  	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);

>  

> +	if (!has_rstst)

> +		return 0;

> +

> +	/* wait for the status to be set */

> +	while (1) {

> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);

> +		v &= 1 << st_bit;

> +		if (v)

> +			break;

> +		timeout++;

> +		if (timeout > OMAP_RESET_MAX_WAIT) {

> +			pr_err("%s: timedout waiting for %s:%lu\n", __func__,

> +			       dev_name(rcdev->dev), id);

> +			return -EBUSY;

> +		}

> +

> +		udelay(1);

> +	}


This looks like you could use

	readl_relaxed_poll_timeout(_atomic)

regards
Philipp
Philipp Zabel Aug. 29, 2019, 1:25 p.m. UTC | #2 
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, 9:07 a.m. UTC | #3 
On 29/08/2019 16:14, Philipp Zabel wrote:
> Hi Tero,

> 

> On Wed, 2019-08-28 at 10:19 +0300, Tero Kristo wrote:

>> Poll for reset completion status during de-assertion of reset, otherwise

>> the IP in question might be accessed before it has left reset properly.

>>

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++

>>   1 file changed, 20 insertions(+)

>>

>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c

>> index fd5c431f8736..afeb70761b27 100644

>> --- a/drivers/soc/ti/omap_prm.c

>> +++ b/drivers/soc/ti/omap_prm.c

>> @@ -127,6 +127,7 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,

>>   	u32 v;

>>   	int st_bit;

>>   	bool has_rstst;

>> +	int timeout = 0;

>>   

>>   	if (!_is_valid_reset(reset, id))

>>   		return -EINVAL;

>> @@ -153,6 +154,25 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,

>>   	v &= ~(1 << id);

>>   	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);

>>   

>> +	if (!has_rstst)

>> +		return 0;

>> +

>> +	/* wait for the status to be set */

>> +	while (1) {

>> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);

>> +		v &= 1 << st_bit;

>> +		if (v)

>> +			break;

>> +		timeout++;

>> +		if (timeout > OMAP_RESET_MAX_WAIT) {

>> +			pr_err("%s: timedout waiting for %s:%lu\n", __func__,

>> +			       dev_name(rcdev->dev), id);

>> +			return -EBUSY;

>> +		}

>> +

>> +		udelay(1);

>> +	}

> 

> This looks like you could use

> 

> 	readl_relaxed_poll_timeout(_atomic)


Yeah true, let me change that.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Aug. 30, 2019, 9:18 a.m. UTC | #4 
Hi Rob,

Quick question below based on some discussion on the implementation 
details of the PRM support.

On 28/08/2019 10:19, Tero Kristo wrote:
> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each

> of these will act as a power domain controller and potentially as a reset

> provider.

> 

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>   .../devicetree/bindings/arm/omap/prm-inst.txt | 31 +++++++++++++++++++

>   1 file changed, 31 insertions(+)

>   create mode 100644 Documentation/devicetree/bindings/arm/omap/prm-inst.txt

> 

> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt

> new file mode 100644

> index 000000000000..7c7527c37734

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt

> @@ -0,0 +1,31 @@

> +OMAP PRM instance bindings

> +

> +Power and Reset Manager is an IP block on OMAP family of devices which

> +handle the power domains and their current state, and provide reset

> +handling for the domains and/or separate IP blocks under the power domain

> +hierarchy.

> +

> +Required properties:

> +- compatible:	Must be one of:

> +		"ti,am3-prm-inst"

> +		"ti,am4-prm-inst"

> +		"ti,omap4-prm-inst"

> +		"ti,omap5-prm-inst"

> +		"ti,dra7-prm-inst"


Each of these bindings describes multiple PRM instances, like 
ti,dra7-prm-inst maps eventually into 21 different PRM instances. I end 
up matching these in the kernel based on base address to map additional 
details, like support for reset handling, supported reset bits, etc.

What is your take on this, should I just provide individual compatible 
strings for these all, the total amount of them would end up being like 
70+, or keep them like this? I find it rather cumbersome if I would have 
to deal with 70+ different compatibles... Also, I would need to keep 
updating the bindings doc once I add new instances on the supported list.

-Tero

> +- reg:		Contains PRM instance register address range

> +		(base address and length)

> +

> +Optional properties:

> +- #reset-cells:	Should be 1 if the PRM instance in question supports resets.

> +- clocks:	Associated clocks for the reset signals if any. Certain reset

> +		signals can't be toggled properly without functional clock

> +		being active for them.

> +

> +Example:

> +

> +prm_dsp2: prm@1b00 {

> +	compatible = "ti,dra7-prm-inst";

> +	reg = <0x1b00 0x40>;

> +	#reset-cells = <1>;

> +	clocks = <&dsp2_clkctrl DRA7_DSP2_MMU0_DSP2_CLKCTRL 0>;

> +};

> 


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki