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

[Thierry Reding]

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.pod...@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

pgpdM3XKc1EgF.pgp
Description: PGP signature