Re: [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call.

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]

On Wed, Dec 18, 2013 at 05:06:55PM +0530, Sourav Poddar wrote:
> Writing to ecap register on second insmod crashes with an external
> abort. This happens becuase the STOP_CLK bit remains set(from rmmod) 
> during the second insmod thereby not allowing the clocks to get enabled.
> 
> So, we disable STOP clock bit while doing a clock enable.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>  drivers/pwm/pwm-tipwmss.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tipwmss.c b/drivers/pwm/pwm-tipwmss.c
> index 3b119bc..4749866 100644
> --- a/drivers/pwm/pwm-tipwmss.c
> +++ b/drivers/pwm/pwm-tipwmss.c
> @@ -40,6 +40,8 @@ u16 pwmss_submodule_state_change(struct device *dev, int set)
>  
>  	mutex_lock(&info->pwmss_lock);
>  	val = readw(info->mmio_base + PWMSS_CLKCONFIG);
> +	if (set == PWMSS_ECAPCLK_EN)
> +		val &= ~PWMSS_ECAPCLK_STOP_REQ;

Should this be done for set == PWMSS_EPWMCLK_EN as well? Also how does
this behave if somebody goes and passes:

	PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ

as the "set" parameter.

I think that perhaps the pwmss_submodule_state_change() API should be
rethought. Instead of taking a value that's directly written into the
register, perhaps it should abstract away what this does.

From my understanding this is used to enable (or disable) the clock for
a specific submodule (ECAP or EHRPWM). Perhaps an interface like the
following would be more intuitive:

	bool pwmss_module_enable(struct device *dev, enum pwmss_module module)
	{
		struct pwmss_info *info = dev_get_drvdata(dev);
		u16 value, mask, state, ack;

		switch (module) {
		case PWMSS_MODULE_ECAP:
			mask = PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ;
			state = PWMSS_ECAPCLK_EN;
			ack = PWMSS_ECAPCLK_EN_ACK;
			break;

		case PWMSS_MODULE_EPWM:
			mask = PWMSS_EPWMCLK_EN | PWMSS_EPWMCLK_STOP_REQ;
			state = PWMSS_EPWMCLK_EN;
			ack = PWMSS_ECAPCLK_EN_ACK;
			break;

		default:
			return false;
		}

		mutex_lock(&info->pwmss_lock);
		value = readw(info->mmio + PWMSS_CLKCONFIG);
		value &= ~mask;
		value |= state;
		writew(value, info->mmio + PWMSS_CLKCONFIG);
		mutex_unlock(&info->pwmss_lock);

		value = readw(info->mmio + PWMSS_CLKSTATUS);
		return (value & ack) != 0;
	}

	void pwmss_module_disable(struct device *dev, enum pwmss_module module)
	{
		struct pwmss_info *info = dev_get_drvdata(dev);
		u16 value, mask, state;

		switch (module) {
		case PWMSS_MODULE_ECAP:
			mask = PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ;
			state = PWMSS_ECAPCLK_STOP_REQ;
			break;

		case PWMSS_MODULE_EPWM:
			mask = PWMSS_EPWMCLK_EN | PWMSS_EPWMCLK_STOP_REQ;
			state = PWMSS_EPWMCLK_STOP_REQ;
			break;

		default:
			return false;
		}

		mutex_lock(&info->pwmss_lock);
		value = readw(info->mmio + PWMSS_CLKCONFIG);
		value &= ~mask;
		value |= state;
		writew(value, info->mmio + PWMSS_CLKCONFIG);
		mutex_unlock(&info->pwmss_lock);
	}

One possible other interesting alternative would be to export this
functionality via the common clock framework, since you're essentially
exposing clk_enable() and clk_disable(). That's probably overkill,
though.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]