Date:	Wed, 23 Apr 2014 09:27:36 +0200
From:	Michal Simek <monstr@...str.eu>
To:	Pankaj Dubey <pankaj.dubey@...sung.com>,
	Lee Jones <lee.jones@...aro.org>
CC:	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org, Samuel Ortiz <sameo@...ux.intel.com>,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v3] mfd: syscon: Support early initialization

On 04/10/2014 08:17 AM, Michal Simek wrote:
> On 04/10/2014 08:20 AM, Pankaj Dubey wrote:
>> Hi Michal,
>>
>> On 04/09/2014 12:00 AM, Michal Simek wrote:
>>> Some platforms need to get system controller
>>> ready as soon as possible.
>>> The patch provides early_syscon_initialization
>>> which create early mapping for all syscon compatible
>>> devices in early_syscon_probe.
>>> Regmap is get via syscon_early_regmap_lookup_by_phandle()
>>>
>>> Regular device probes attach device to regmap
>>> via regmap_attach_dev().
>>>
>>> For early syscon initialization is necessary to extend
>>> struct syscon and provide remove function
>>> which unmap all early init structures.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@...inx.com>
>>> ---
>>>
>>> Changes in v3:
>>> - Keep backward compatibility for platform drivers and test it
>>> - Use only one probe method which is early_syscon_probe
>>>    suggested by Lee Jones. Do not force anybody to call early_syscon_init
>>> - Add kernel-doc description
>>>
>>> Changes in v2:
>>> - Fix bad logic in early_syscon_probe
>>> - Fix compilation failure for x86_64 reported by zero day testing system
>>> - Regmap change available here
>>>    git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/nodev
>>>
>>>   drivers/mfd/syscon.c       | 159 ++++++++++++++++++++++++++++++++++++++++-----
>>>   include/linux/mfd/syscon.h |  11 ++++
>>>   2 files changed, 153 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
>>> index 71841f9..8e2ff88 100644
>>> --- a/drivers/mfd/syscon.c
>>> +++ b/drivers/mfd/syscon.c
>>> @@ -20,12 +20,15 @@
>>>   #include <linux/of_platform.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>>   #include <linux/mfd/syscon.h>
>>>
>>>   static struct platform_driver syscon_driver;
>>>
>>>   struct syscon {
>>> +    void __iomem *base;
>>>       struct regmap *regmap;
>>> +    struct resource res;
>>>   };
>>>
>>>   static int syscon_match_node(struct device *dev, void *data)
>>> @@ -95,6 +98,30 @@ struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
>>>   }
>>>   EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
>>>
>>> +/**
>>> + * syscon_early_regmap_lookup_by_phandle - Early phandle lookup function
>>> + * @np:        device_node pointer
>>> + * @property:    property name which handle system controller phandle
>>> + * Return:    regmap pointer, an error pointer otherwise
>>> + */
>>> +struct regmap *syscon_early_regmap_lookup_by_phandle(struct device_node *np,
>>> +                             const char *property)
>>> +{
>>> +    struct device_node *syscon_np;
>>> +    struct syscon *syscon;
>>> +
>>> +    syscon_np = of_parse_phandle(np, property, 0);
>>> +    if (!syscon_np)
>>> +        return ERR_PTR(-ENODEV);
>>> +
>>> +    syscon = syscon_np->data;
>>> +
>>> +    of_node_put(syscon_np);
>>> +
>>> +    return syscon->regmap;
>>> +}
>>> +EXPORT_SYMBOL_GPL(syscon_early_regmap_lookup_by_phandle);
>>> +
>>>   struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
>>>                       const char *property)
>>>   {
>>> @@ -123,36 +150,118 @@ static struct regmap_config syscon_regmap_config = {
>>>       .reg_stride = 4,
>>>   };
>>>
>>> -static int syscon_probe(struct platform_device *pdev)
>>> +/**
>>> + * early_syscon_probe - Early system controller probe method
>>> + * @np:        device_node pointer
>>> + * @syscon_p:    syscon pointer
>>> + * @res:    device IO resource
>>> + * Return:    0 if successful, a negative error code otherwise
>>> + */
>>> +static int early_syscon_probe(struct device_node *np, struct syscon **syscon_p,
>>> +                  struct resource *res)
>>>   {
>>> -    struct device *dev = &pdev->dev;
>>>       struct syscon *syscon;
>>> -    struct resource *res;
>>> -    void __iomem *base;
>>> +    int ret;
>>> +
>>> +    if (np && np->data) {
>>> +        pr_debug("Early syscon was called\n");
>>> +        *syscon_p = (struct syscon *)&np->data;
>>> +        return 0;
>>> +    }
>>>
>>> -    syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
>>> +    syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
>>>       if (!syscon)
>>>           return -ENOMEM;
>>>
>>> -    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> -    if (!res)
>>> -        return -ENOENT;
>>> +    *syscon_p = (struct syscon *)&syscon;
>>>
>>> -    base = devm_ioremap(dev, res->start, resource_size(res));
>>> -    if (!base)
>>> -        return -ENOMEM;
>>> +    if (!res && np) {
>>> +        if (of_address_to_resource(np, 0, &syscon->res)) {
>>> +            ret = -EINVAL;
>>> +            goto alloc;
>>> +        }
>>> +
>>> +        np->data = syscon;
>>> +        of_node_put(np);
>>> +    } else {
>>> +        syscon->res = *res;
>>> +    }
>>>
>>> -    syscon_regmap_config.max_register = res->end - res->start - 3;
>>> -    syscon->regmap = devm_regmap_init_mmio(dev, base,
>>> -                    &syscon_regmap_config);
>>> +    syscon->base = ioremap(syscon->res.start, resource_size(&syscon->res));
>>> +    if (!syscon->base) {
>>> +        pr_err("%s: Unable to map I/O memory\n", __func__);
>>> +        ret = PTR_ERR(syscon->base);
>>> +        goto alloc;
>>> +    }
>>> +
>>> +    syscon_regmap_config.max_register = syscon->res.end -
>>> +                        syscon->res.start - 3;
>>> +    syscon->regmap = regmap_init_mmio(NULL, syscon->base,
>>> +                      &syscon_regmap_config);
>>>       if (IS_ERR(syscon->regmap)) {
>>> -        dev_err(dev, "regmap init failed\n");
>>> -        return PTR_ERR(syscon->regmap);
>>> +        pr_err("regmap init failed\n");
>>> +        ret = PTR_ERR(syscon->regmap);
>>> +        goto iomap;
>>>       }
>>> +    if (np)
>>> +        pr_info("syscon: %s regmap %pR registered\n", np->name,
>>> +            &syscon->res);
>>> +
>>> +    return 0;
>>> +
>>> +iomap:
>>> +    iounmap(syscon->base);
>>> +alloc:
>>> +    kfree(syscon);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +/**
>>> + * early_syscon_init - Early system controller initialization
>>> + */
>>> +void __init early_syscon_init(void)
>>> +{
>>> +    struct device_node *np;
>>> +    struct syscon *syscon = NULL;
>>> +
>>> +    for_each_matching_node_and_match(np, of_syscon_match, NULL) {
>>> +        if (early_syscon_probe(np, &syscon, NULL))
>>> +            BUG();
>>> +    }
>>> +}
>>> +
>>> +/**
>>> + * syscon_probe - System controller probe method
>>> + * @pdev:    Platform device
>>> + * Return:    0 if successful, a negative error code otherwise
>>> + */
>>> +static int syscon_probe(struct platform_device *pdev)
>>> +{
>>> +    struct syscon *syscon, *syscon_p;
>>> +    struct resource *res = NULL;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct device_node *np = pdev->dev.of_node;
>>> +    int ret;
>>> +
>>> +    if (!np) {
>>> +        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +        if (!res)
>>> +            return -ENOENT;
>>> +    }
>>> +    ret = early_syscon_probe(np, &syscon_p, res);
>>> +    if (ret) {
>>> +        dev_err(dev, "Syscon probe failed\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    syscon = *(struct syscon **)syscon_p;
>>> +
>>> +    regmap_attach_dev(dev, syscon->regmap, &syscon_regmap_config);
>>>
>>>       platform_set_drvdata(pdev, syscon);
>>>
>>> -    dev_info(dev, "regmap %pR registered\n", res);
>>> +    dev_info(dev, "regmap attach device to %pR\n", &syscon->res);
>>>
>>>       return 0;
>>>   }
>>> @@ -162,6 +271,21 @@ static const struct platform_device_id syscon_ids[] = {
>>>       { }
>>>   };
>>>
>>> +/**
>>> + * syscon_remove - System controller cleanup function
>>> + * @pdev:    Platform device
>>> + * Return:    0 always
>>> + */
>>> +static int syscon_remove(struct platform_device *pdev)
>>> +{
>>> +    struct syscon *syscon = platform_get_drvdata(pdev);
>>> +
>>> +    iounmap(syscon->base);
>>> +    kfree(syscon);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static struct platform_driver syscon_driver = {
>>>       .driver = {
>>>           .name = "syscon",
>>> @@ -169,6 +293,7 @@ static struct platform_driver syscon_driver = {
>>>           .of_match_table = of_syscon_match,
>>>       },
>>>       .probe        = syscon_probe,
>>> +    .remove        = syscon_remove,
>>>       .id_table    = syscon_ids,
>>>   };
>>>
>>> diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
>>> index 8789fa3..465c092 100644
>>> --- a/include/linux/mfd/syscon.h
>>> +++ b/include/linux/mfd/syscon.h
>>> @@ -24,6 +24,10 @@ extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
>>>   extern struct regmap *syscon_regmap_lookup_by_phandle(
>>>                       struct device_node *np,
>>>                       const char *property);
>>> +extern struct regmap *syscon_early_regmap_lookup_by_phandle(
>>> +                    struct device_node *np,
>>> +                    const char *property);
>>> +extern void early_syscon_init(void);
>>>   #else
>>>   static inline struct regmap *syscon_node_to_regmap(struct device_node *np)
>>>   {
>>> @@ -46,6 +50,13 @@ static inline struct regmap *syscon_regmap_lookup_by_phandle(
>>>   {
>>>       return ERR_PTR(-ENOSYS);
>>>   }
>>> +
>>> +static struct regmap *syscon_early_regmap_lookup_by_phandle(
>>> +                    struct device_node *np,
>>> +                    const char *property)
>>> +{
>>> +    return ERR_PTR(-ENOSYS);
>>> +}
>>>   #endif
>>>
>>>   #endif /* __LINUX_MFD_SYSCON_H__ */
>>> -- 
>>> 1.8.2.3
>>>
>> Thanks for CC'ing me.
> 
> no problem.
> 
>>
>> I have tested this patch along with Exynos PMU related changes posted here
>> https://lkml.org/lkml/2014/4/2/53 and modified it for using Syscon, and it's working for me.
> 
> great.
> 
> 
>> I have observed one issue during this testing:
>>
>> Even though we are saying early initialization I could not use "early_syscon_init" from machine's
>> "map_io" or "init_early". I observed that "early_syscon_init" failed to called "early_syscon_probe",
>> because it can not get any matching node, when I debug further I found that as "early_syscon_init"
>> is calling "for_each_matching_node_and_match" and it can't work before unflatten'ing device tree,
>> which happens in "setup_arch" a bit late, after "map_io" and "init_early" calls of machine.
>>
>> So if I have to use "early_syscon_init" I MUST call it after device tree is unflattened. It worked for me
>> when I called it from "init_machine" (from exynos_dt_machine_init), But if someone needs it at very
>> early stage then it might not work. So how about using "of_scan_flat_dt" in "early_syscon_init"?, so that
>> we can use this functionality at very early stage if required.
> 
> Yes you are right. I have discussed this with Arnd and for Zynq there is no need to call it so early
> that's why using this function is fine. Arnd wasn't sure if there is any need to call it before unflattening
> that's why I didn't try to solve this case.
> 
> Can you send me that patch? I will test it on Zynq and if it is working, let's include it to v4.

Pankaj: Any update on this? I think that your patch can be applied on the top of this one.

Lee: Can you please look at these changes?  It is around for a while and I would like to close it.

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Download attachment "signature.asc" of type "application/pgp-signature" (264 bytes)

Powered by blists - more mailing lists