| Message ID | 1387366615-23182-4-git-send-email-sourav.poddar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; val |= set; writew(val , info->mmio_base + PWMSS_CLKCONFIG); mutex_unlock(&info->pwmss_lock);
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(-)