Re: [PATCH 07/26] clk: vt8500: parse pmc_base from clock driver

From: Tony Prisk <linux@prisktech.co.nz>
To: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Olof Johansson <olof@lixom.net>, Arnd Bergmann <arnd@arndb.de>,
	Mike Turquette <mturquette@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 07/26] clk: vt8500: parse pmc_base from clock driver
Date: Sat, 21 Sep 2013 06:23:13 +1200	[thread overview]
Message-ID: <523C9291.9060506@prisktech.co.nz> (raw)
In-Reply-To: <523BE9F8.1040708@gmail.com>

On 20/09/13 18:23, Sebastian Hesselbarth wrote:
> On 09/20/2013 06:51 AM, Tony Prisk wrote:
>> On 20/09/13 07:12, Sebastian Hesselbarth wrote:
>>> On 09/19/2013 09:02 PM, Tony Prisk wrote:
>>>> On 19/09/13 05:53, Sebastian Hesselbarth wrote:
>>>>> Currently, clock providers for vt8500 depend on machine_init 
>>>>> providing
>>>>> pmc_base address before calling of_clk_init. With upcoming arch-wide
>>>>> .time_init calling of_clk_init, we should make clock providers
>>>>> independent
>>>>> of mach code. This adds a pmc_base parsing helper to current clock
>>>>> provider
>>>>> that gets called if there is no pmc_base set, yet.
>>>>>
>>>>> Signed-off-by: Sebastian Hesselbarth 
>>>>> <sebastian.hesselbarth@gmail.com>
>>>>> ---
>>>>> Cc: Olof Johansson <olof@lixom.net>
>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>> Cc: Tony Prisk <linux@prisktech.co.nz>
>>>>> Cc: Mike Turquette <mturquette@linaro.org>
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> ---
>>>>>   drivers/clk/clk-vt8500.c |   21 +++++++++++++++++++++
>>>>>   1 file changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/clk-vt8500.c b/drivers/clk/clk-vt8500.c
>>>>> index 82306f5..a5ee01c 100644
>>>>> --- a/drivers/clk/clk-vt8500.c
>>>>> +++ b/drivers/clk/clk-vt8500.c
>>>>> @@ -15,11 +15,14 @@
>>>>>   #include <linux/io.h>
>>>>>   #include <linux/of.h>
>>>>> +#include <linux/of_address.h>
>>>>>   #include <linux/slab.h>
>>>>>   #include <linux/bitops.h>
>>>>>   #include <linux/clkdev.h>
>>>>>   #include <linux/clk-provider.h>
>>>>> +#define LEGACY_PMC_BASE        0xD8130000
>>>>> +
>>>>>   /* All clocks share the same lock as none can be changed
>>>>> concurrently */
>>>>>   static DEFINE_SPINLOCK(_lock);
>>>>> @@ -626,6 +629,21 @@ const struct clk_ops vtwm_pll_ops = {
>>>>>       .recalc_rate = vtwm_pll_recalc_rate,
>>>>>   };
>>>>> +static __init void vtwm_set_pmc_base(void)
>>>>> +{
>>>>> +    struct device_node *np =
>>>>> +        of_find_compatible_node(NULL, NULL, "via,vt8500-pmc");
>>>>> +
>>>>> +    if (np)
>>>>> +        pmc_base = of_iomap(np, 0);
>>>>> +    else
>>>>> +        pmc_base = ioremap(LEGACY_PMC_BASE, 0x1000);
>>>>> +    of_node_put(np);
>>>>> +
>>>>> +    if (!pmc_base)
>>>>> +        pr_err("%s:of_iomap(pmc) failed\n", __func__);
>>>>> +}
>>>>> +
>>>>>   static __init void vtwm_pll_clk_init(struct device_node *node, int
>>>>> pll_type)
>>>>>   {
>>>>>       u32 reg;
>>>>> @@ -636,6 +654,9 @@ static __init void vtwm_pll_clk_init(struct
>>>>> device_node *node, int pll_type)
>>>>>       struct clk_init_data init;
>>>>>       int rc;
>>>>> +    if (!pmc_base)
>>>>> +        vtwm_set_pmc_base();
>>>>> +
>>>>>       rc = of_property_read_u32(node, "reg", &reg);
>>>>>       if (WARN_ON(rc))
>>>>>           return;
>>>> What happens if the first clock registered is a 'device clock' rather
>>>> than a 'pll'?
>>>>
>>>> static __init void vtwm_device_clk_init(struct device_node *node)
>>>> {
>>>>      u32 en_reg, div_reg;
>>>>      struct clk *clk;
>>>>      struct clk_device *dev_clk;
>>>>      const char *clk_name = node->name;
>>>>      const char *parent_name;
>>>>      struct clk_init_data init;
>>>>      int rc;
>>>>      int clk_init_flags = 0;
>>>>
>>>>      dev_clk = kzalloc(sizeof(*dev_clk), GFP_KERNEL);
>>>>      if (WARN_ON(!dev_clk))
>>>>          return;
>>>>
>>>>      dev_clk->lock = &_lock;
>>>>
>>>>      rc = of_property_read_u32(node, "enable-reg", &en_reg);
>>>>      if (!rc) {
>>>>          dev_clk->en_reg = pmc_base + en_reg;
>>>> ...
>>>> }
>>>> CLK_OF_DECLARE(vt8500_device, "via,vt8500-device-clock",
>>>> vtwm_device_clk_init);
>>>>
>>>> If a device clock is initialized first, pmc_base will be null and
>>>> dev_clk->en_reg (+ other register offsets) will be incorrect.
>>>
>>> Tony,
>>>
>>> looks like I just missed to add the same check for !pmc_base to
>>> vtwm_device_clk_init. If you are ok with the general approach,
>>> I send v2 for this patch shortly.
>>>
>>> Optionally, you can also choose to take care of clk-vt8500 yourself,
>>> as mach-vt8500 has its own .init_time callback and will be unaffected
>>> by the arch-wide default callback. If so, I will drop vt8500 to not
>>> stall this series too much now.
>>>
>>> Sebastian
>> I have no issue with the concept - just pointing out the missing bit. If
>> you can fix that small issue for v2 then you can also add my:
>>
>> Acked-by: Tony Prisk <linux@prisktech.co.nz>
>
> Just to make sure, does that also count for the other vt8500 patches?
>
> Sebastian
Sorry, I should have been more specific.

For the whole series (vt8500-related):
Acked-by: Tony Prisk <linux@prisktech.co.nz>

Regards
Tony P