On Thursday 08 May 2014 05:34 PM, Javier Martinez Canillas wrote:
> Hello Rajendra,
>
> On Thu, May 8, 2014 at 1:10 PM, Rajendra Nayak <rna...@ti.com> wrote:
>> On Thursday 08 May 2014 02:56 PM, Javier Martinez Canillas wrote:
>>> Hello Rajendra,
>>>
>>> On Thu, May 8, 2014 at 9:06 AM, Rajendra Nayak <rna...@ti.com> wrote:
>>>> On Wednesday 23 April 2014 11:41 AM, Rajendra Nayak wrote:
>>>>> Replace the clk_enable()s with a clk_prepare_enable() and
>>>>> the clk_disables()s with a clk_disable_unprepare()
>>>>>
>>>>> This never showed issues due to the OMAP platform code (hwmod)
>>>>> leaving these clocks in clk_prepare()ed state by default.
>>>>>
>>>>> Reported-by: Kishon Vijay Abraham I <kis...@ti.com>
>>>>> Signed-off-by: Rajendra Nayak <rna...@ti.com>
>>>>> Cc: linux-g...@vger.kernel.org
>>>>> Cc: Santosh Shilimkar <santosh.shilim...@ti.com>
>>>>> Cc: Kevin Hilman <khil...@deeprootsystems.com>
>>>>
>>>> Linus,
>>>>
>>>> Do you mind picking this fix up via the GPIO tree? Alternatively you could
>>>> Ack this if you are fine and we can take both Patch 1/2 and Patch 2/2 from
>>>> this
>>>> series via the OMAP tree.
>>>>
>>>> Patch 2/2 has a dependency on Patch 1/2 and they need to go in in that
>>>> order else
>>>> gpio would break. More discussions are here [1].
>>>> Let us know what you think. Thanks.
>>>>
>>>
>>> I wonder if that is really the case. Your Patch 2/2 removes the call
>>> to clk_prepare on _init_opt_clks() but it also replaces
>>> clk_{enable,disable} with clk_prepare_enable()/clk_disable_unprepare()
>>> on _enable_optional_clocks() and _disable_optional_clocks()
>>> respectively.
>>
>> Right, the difference being, by the time hwmod is done enabling/disabling
>> the opt clocks, without patch 2/2, the prepare count is 1, with patch 2/2
>> prepare count is 0.
>>
>
> Ok, got it now.
>
>>>
>>> And GPIO banks are reset by hwmod on init which as far as I know
>>> happen very early before the GPIO OMAP driver is even probed so by the
>>> time clk_enable() is called on the GPIO driver the clock will already
>>> be prepared by _enable_optional_clocks(). I tested linux-gpio/devel
>>
>> and unprepared by _disable_optional_clocks()?
>>
>
> I see that _disable_optional_clocks() is called as well so the clock
> is left unprepared as you said.
>
>>> branch + only your Patch 2/2 and the GPIOs were working correctly on a
>>> OMAP3 board.
>>
>> Did gpio_debounce() ever get called for any of the gpios?
>>
>
> I don't see gpio_debounce() to be called indeed.
>
> omap_gpio_runtime_resume() is executed and calls
> _gpio_dbck_enable(bank) but clk_enable(bank->dbck) is not called since
> bank->dbck_enable_mask is 0, that was my confusion since I thought
> that clk_enable() was called.
>
> Now I understand the dependency between the two patches.
>
>>>
>>> So I think that there isn't a strict dependency between these two
>>> patches or am I missing something?
>>>
>>> In fact now that I think about it I wonder what's the functional
>>> change of your Patch 2/2 since hwmod is still calling clk_prepare()
>>> before the driver. If the clocks should actually be controlled by the
>>
>> I don't understand why you say 'before the driver'. Hwmod needs to control
>> optional clocks for some devices in order to do a ocp reset. So it does
>> touch these optional clocks, but if you look at the code it subsequently
>> also disables (and unprepares with patch 2/2) these clocks before returning
>> the control to the driver.
>>
>
> Right, it was just me getting confused by the interaction between
> hwmod and the GPIO driver. Thanks a lot for the explanation and sorry
> for the noise.
No issues, thanks for the review and ack.
>
> Feel free to add my:
>
> Acked-by: Javier Martinez Canillas <jav...@dowhile0.org>
>
> Best regards,
> Javier
>
>>> drivers like you said then I think that we should remove
>>> _{enable,disable}_optional_clocks() completely and let the drivers do
>>> the clock prepare and enable like is made on your Patch 1/2 for the
>>> GPIO driver.
>>>
>>> What do you think about it?
>>>
>>> Best regards,
>>> Javier
>>>
>>>> regards,
>>>> Rajendra
>>>>
>>>> [1] http://www.mail-archive.com/linux-gpio@vger.kernel.org/msg02801.html
>>>>
>>>>> ---
>>>>> drivers/gpio/gpio-omap.c | 10 +++++-----
>>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>>> index 19b886c..78bc5a4 100644
>>>>> --- a/drivers/gpio/gpio-omap.c
>>>>> +++ b/drivers/gpio/gpio-omap.c
>>>>> @@ -180,7 +180,7 @@ static inline void _gpio_rmw(void __iomem *base, u32
>>>>> reg, u32 mask, bool set)
>>>>> static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>>>> {
>>>>> if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>>>>> - clk_enable(bank->dbck);
>>>>> + clk_prepare_enable(bank->dbck);
>>>>> bank->dbck_enabled = true;
>>>>>
>>>>> writel_relaxed(bank->dbck_enable_mask,
>>>>> @@ -198,7 +198,7 @@ static inline void _gpio_dbck_disable(struct
>>>>> gpio_bank *bank)
>>>>> */
>>>>> writel_relaxed(0, bank->base + bank->regs->debounce_en);
>>>>>
>>>>> - clk_disable(bank->dbck);
>>>>> + clk_disable_unprepare(bank->dbck);
>>>>> bank->dbck_enabled = false;
>>>>> }
>>>>> }
>>>>> @@ -231,7 +231,7 @@ static void _set_gpio_debounce(struct gpio_bank
>>>>> *bank, unsigned gpio,
>>>>>
>>>>> l = GPIO_BIT(bank, gpio);
>>>>>
>>>>> - clk_enable(bank->dbck);
>>>>> + clk_prepare_enable(bank->dbck);
>>>>> reg = bank->base + bank->regs->debounce;
>>>>> writel_relaxed(debounce, reg);
>>>>>
>>>>> @@ -245,7 +245,7 @@ static void _set_gpio_debounce(struct gpio_bank
>>>>> *bank, unsigned gpio,
>>>>> bank->dbck_enable_mask = val;
>>>>>
>>>>> writel_relaxed(val, reg);
>>>>> - clk_disable(bank->dbck);
>>>>> + clk_disable_unprepare(bank->dbck);
>>>>> /*
>>>>> * Enable debounce clock per module.
>>>>> * This call is mandatory because in omap_gpio_request() when
>>>>> @@ -290,7 +290,7 @@ static void _clear_gpio_debounce(struct gpio_bank
>>>>> *bank, unsigned gpio)
>>>>> bank->context.debounce = 0;
>>>>> writel_relaxed(bank->context.debounce, bank->base +
>>>>> bank->regs->debounce);
>>>>> - clk_disable(bank->dbck);
>>>>> + clk_disable_unprepare(bank->dbck);
>>>>> bank->dbck_enabled = false;
>>>>> }
>>>>> }
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
>>>> the body of a message to majord...@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
