| Message ID | 1411360807-7750-1-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 22, 2014 at 10:10:07AM +0530, Pankaj Dubey wrote: > Currently a syscon entity can be only registered directly through a > platform device that binds to a dedicated syscon driver. However in > certain use cases it is desirable to make a device used with another > driver a syscon interface provider. > > For example, certain SoCs (e.g. Exynos) contain system controller > blocks which perform various functions such as power domain control, > CPU power management, low power mode control, but in addition contain > certain IP integration glue, such as various signal masks, > coprocessor power control, etc. In such case, there is a need to have > a dedicated driver for such system controller but also share registers > with other drivers. The latter is where the syscon interface is helpful. > > In case of DT based platforms, this patch decouples syscon object from > syscon platform driver, and allows to create syscon objects first time > when it is required by calling of syscon_regmap_lookup_by APIs and keep > a list of such syscon objects along with syscon provider device_nodes > and regmap handles. > > For non-DT based platforms, this patch keeps syscon platform driver > structure where is can be probed and such non-DT based drivers can use > syscon_regmap_lookup_by_pdev API and get access to regmap handles. > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based, > we can completly remove platform driver of syscon, and keep only helper > functions to get regmap handles. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> > Tested-by: Vivek Gautam <gautam.vivek@samsung.com> > Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > Patch v4 and related discussions can be found here [1]. > > Changes since v4: > - Addressed Tomasz Figa's comments for v4. > - Added error handing in of_syscon_register function. > - Using devm_regmap_init_mmio instead of regmap_init_mmio. > > Changes since v3: > - Addressed Arnd's comment for v2. > - Updated of_syscon_register for adding dev pointer in regmap_init_mmio. > - For early users created dummy platform device. > > Changes since v2: > - Added back platform device support from syscon, with one change that > syscon will not be probed for DT based platform. > - Added back syscon_regmap_lookup_by_pdevname API so that non-DT base > users of syscon will not be broken. > - Removed unwanted change in syscon.h. > - Modified Signed-off-by list, added Suggested-by of Tomasz Figa and > Arnd Bergmann. > - Added Tested-by of Vivek Gautam for testing on Exynos platform. > > Changes since v1: > - Removed of_syscon_unregister function. > - Modified of_syscon_register function and it will be used by syscon.c > to create syscon objects whenever required. > - Removed platform device support from syscon. > - Removed syscon_regmap_lookup_by_pdevname API support. > - As there are significant changes w.r.t patchset v1, I am taking over > author for this patchset from Tomasz Figa. > > [1]: https://lkml.org/lkml/2014/9/19/267 > > drivers/mfd/syscon.c | 107 +++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 85 insertions(+), 22 deletions(-) > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index ca15878..0b17f50 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -15,6 +15,7 @@ > #include <linux/err.h> > #include <linux/io.h> > #include <linux/module.h> > +#include <linux/list.h> > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_platform.h> > @@ -22,31 +23,105 @@ > #include <linux/platform_device.h> > #include <linux/regmap.h> > #include <linux/mfd/syscon.h> > +#include <linux/slab.h> > > static struct platform_driver syscon_driver; > > +static DEFINE_SPINLOCK(syscon_list_slock); > +static LIST_HEAD(syscon_list); > + > struct syscon { > + struct device_node *np; > struct regmap *regmap; > + struct list_head list; > +}; > + > +static struct regmap_config syscon_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > }; > > -static int syscon_match_node(struct device *dev, void *data) > +static struct syscon *of_syscon_register(struct device_node *np) > { > - struct device_node *dn = data; > + struct platform_device *pdev = NULL; > + struct syscon *syscon; > + struct regmap *regmap; > + void __iomem *base; > + int ret; > + > + if (!of_device_is_compatible(np, "syscon")) > + return ERR_PTR(-EINVAL); > + > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > + if (!syscon) > + return ERR_PTR(-ENOMEM); > + > + base = of_iomap(np, 0); > + if (!base) { > + ret = -ENOMEM; > + goto err_map; > + } > + > + /* if device is already populated and available then use it */ > + pdev = of_find_device_by_node(np); > + if (!pdev) { > + /* for early users create dummy syscon device and use it */ > + pdev = platform_device_alloc("dummy-syscon", -1); Can we create specific devices for each syscon device? That looks more reasonable to me. Then we can dump registers in regmap debugfs more clearly, just like other devices. And i think it's better to follow device tree way to generate devices from node. If DT core find a device is already created, it can ignore that device to avoid creating duplicated device during of_platform_populate. Regards Dong Aisheng > + if (!pdev) { > + ret = -ENOMEM; > + goto err_pdev; > + } > + pdev->dev.of_node = of_node_get(np); > + } > + > + regmap = devm_regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(&pdev->dev, "regmap init failed\n"); > + ret = PTR_ERR(regmap); > + goto err_regmap; > + } > + > + syscon->regmap = regmap; > + syscon->np = np; > > - return (dev->of_node == dn) ? 1 : 0; > + spin_lock(&syscon_list_slock); > + list_add_tail(&syscon->list, &syscon_list); > + spin_unlock(&syscon_list_slock); > + > + return syscon; > + > +err_regmap: > + if (!strcmp(pdev->name, "dummy-syscon")) { > + of_node_put(np); > + platform_device_put(pdev); > + } > +err_pdev: > + iounmap(base); > +err_map: > + kfree(syscon); > + return ERR_PTR(ret); > } > > struct regmap *syscon_node_to_regmap(struct device_node *np) > { > - struct syscon *syscon; > - struct device *dev; > + struct syscon *entry, *syscon = NULL; > > - dev = driver_find_device(&syscon_driver.driver, NULL, np, > - syscon_match_node); > - if (!dev) > - return ERR_PTR(-EPROBE_DEFER); > + spin_lock(&syscon_list_slock); > > - syscon = dev_get_drvdata(dev); > + list_for_each_entry(entry, &syscon_list, list) > + if (entry->np == np) { > + syscon = entry; > + break; > + } > + > + spin_unlock(&syscon_list_slock); > + > + if (!syscon) > + syscon = of_syscon_register(np); > + > + if (IS_ERR(syscon)) > + return ERR_CAST(syscon); > > return syscon->regmap; > } > @@ -110,17 +185,6 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, > } > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle); > > -static const struct of_device_id of_syscon_match[] = { > - { .compatible = "syscon", }, > - { }, > -}; > - > -static struct regmap_config syscon_regmap_config = { > - .reg_bits = 32, > - .val_bits = 32, > - .reg_stride = 4, > -}; > - > static int syscon_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -167,7 +231,6 @@ static struct platform_driver syscon_driver = { > .driver = { > .name = "syscon", > .owner = THIS_MODULE, > - .of_match_table = of_syscon_match, > }, > .probe = syscon_probe, > .id_table = syscon_ids, > -- > 1.7.9.5 >
Hi, [...] > +static struct regmap_config syscon_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > }; > > -static int syscon_match_node(struct device *dev, void *data) > +static struct syscon *of_syscon_register(struct device_node *np) > { > - struct device_node *dn = data; > + struct platform_device *pdev = NULL; struct platform_device *pdev; ? Thanks, BRs Xiubo
Hi Dong, On Monday, September 22, 2014, Dong Aisheng wrote, > On Mon, Sep 22, 2014 at 10:10:07AM +0530, Pankaj Dubey wrote: [snip] > > -static int syscon_match_node(struct device *dev, void *data) > > +static struct syscon *of_syscon_register(struct device_node *np) > > { > > - struct device_node *dn = data; > > + struct platform_device *pdev = NULL; > > + struct syscon *syscon; > > + struct regmap *regmap; > > + void __iomem *base; > > + int ret; > > + > > + if (!of_device_is_compatible(np, "syscon")) > > + return ERR_PTR(-EINVAL); > > + > > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > > + if (!syscon) > > + return ERR_PTR(-ENOMEM); > > + > > + base = of_iomap(np, 0); > > + if (!base) { > > + ret = -ENOMEM; > > + goto err_map; > > + } > > + > > + /* if device is already populated and available then use it */ > > + pdev = of_find_device_by_node(np); > > + if (!pdev) { > > + /* for early users create dummy syscon device and use it */ > > + pdev = platform_device_alloc("dummy-syscon", -1); > > Can we create specific devices for each syscon device? > That looks more reasonable to me. > Then we can dump registers in regmap debugfs more clearly, just like other devices. > And i think it's better to follow device tree way to generate devices from node. > If DT core find a device is already created, it can ignore that device to avoid creating > duplicated device during of_platform_populate. > If you are referring to use "of_platform_device_create", then I am afraid that it can't be used in very early stage. For example, current patch I tested by calling syscon_lookup_by_phandle from "init_irq" hook of machine_desc, and it worked well, but If I use "of_platform_device_create" instead of dummy device, it fails to create pdev and this I verified on Exynos platform. The reason I selected "init_irq" is because this comes just before smp init and where once we tried to get regmap handle, but could not do so. Also if it was just a matter of creating platform_device at early stage then early initialization of syscon could have been solved till now. So I would suggest if we really want this patch to cover early initialization of syscon (which is not it's main purpose) current patch is sufficient. @Arnd, since I have addressed crash issue as reported by Dong Aisheng, I would like to take your ack, but since there are some more code changes other than what you suggested I request you to check this and if you are ok, then I would like to your ack so that I can request to maintainer for taking this patch. Thanks, Pankaj Dubey > Regards > Dong Aisheng > > > + if (!pdev) { > > + ret = -ENOMEM; > > + goto err_pdev; > > + } > > + pdev->dev.of_node = of_node_get(np); > > + } > > + > > + regmap = devm_regmap_init_mmio(&pdev->dev, base, > &syscon_regmap_config); > > + if (IS_ERR(regmap)) { > > + dev_err(&pdev->dev, "regmap init failed\n"); > > + ret = PTR_ERR(regmap); > > + goto err_regmap; > > + } > > + > > + syscon->regmap = regmap; > > + syscon->np = np; > > > > - return (dev->of_node == dn) ? 1 : 0; > > + spin_lock(&syscon_list_slock); > > + list_add_tail(&syscon->list, &syscon_list); > > + spin_unlock(&syscon_list_slock); > > + > > + return syscon; > > + > > +err_regmap: > > + if (!strcmp(pdev->name, "dummy-syscon")) { > > + of_node_put(np); > > + platform_device_put(pdev); > > + } > > +err_pdev: > > + iounmap(base); > > +err_map: > > + kfree(syscon); > > + return ERR_PTR(ret); > > } > > > > struct regmap *syscon_node_to_regmap(struct device_node *np) { > > - struct syscon *syscon; > > - struct device *dev; > > + struct syscon *entry, *syscon = NULL; > > > > - dev = driver_find_device(&syscon_driver.driver, NULL, np, > > - syscon_match_node); > > - if (!dev) > > - return ERR_PTR(-EPROBE_DEFER); > > + spin_lock(&syscon_list_slock); > > > > - syscon = dev_get_drvdata(dev); > > + list_for_each_entry(entry, &syscon_list, list) > > + if (entry->np == np) { > > + syscon = entry; > > + break; > > + } > > + > > + spin_unlock(&syscon_list_slock); > > + > > + if (!syscon) > > + syscon = of_syscon_register(np); > > + > > + if (IS_ERR(syscon)) > > + return ERR_CAST(syscon); > > > > return syscon->regmap; > > } > > @@ -110,17 +185,6 @@ struct regmap > > *syscon_regmap_lookup_by_phandle(struct device_node *np, } > > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle); > > > > -static const struct of_device_id of_syscon_match[] = { > > - { .compatible = "syscon", }, > > - { }, > > -}; > > - > > -static struct regmap_config syscon_regmap_config = { > > - .reg_bits = 32, > > - .val_bits = 32, > > - .reg_stride = 4, > > -}; > > - > > static int syscon_probe(struct platform_device *pdev) { > > struct device *dev = &pdev->dev; > > @@ -167,7 +231,6 @@ static struct platform_driver syscon_driver = { > > .driver = { > > .name = "syscon", > > .owner = THIS_MODULE, > > - .of_match_table = of_syscon_match, > > }, > > .probe = syscon_probe, > > .id_table = syscon_ids, > > -- > > 1.7.9.5 > >
On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > Currently a syscon entity can be only registered directly through a > platform device that binds to a dedicated syscon driver. However in > certain use cases it is desirable to make a device used with another > driver a syscon interface provider. > > For example, certain SoCs (e.g. Exynos) contain system controller > blocks which perform various functions such as power domain control, > CPU power management, low power mode control, but in addition contain > certain IP integration glue, such as various signal masks, > coprocessor power control, etc. In such case, there is a need to have > a dedicated driver for such system controller but also share registers > with other drivers. The latter is where the syscon interface is helpful. > > In case of DT based platforms, this patch decouples syscon object from > syscon platform driver, and allows to create syscon objects first time > when it is required by calling of syscon_regmap_lookup_by APIs and keep > a list of such syscon objects along with syscon provider device_nodes > and regmap handles. > > For non-DT based platforms, this patch keeps syscon platform driver > structure where is can be probed and such non-DT based drivers can use > syscon_regmap_lookup_by_pdev API and get access to regmap handles. > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based, > we can completly remove platform driver of syscon, and keep only helper > functions to get regmap handles. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> > Tested-by: Vivek Gautam <gautam.vivek@samsung.com> > Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> Hi Pankaj, I wrote a clk driver using syscon and your patch. clk driver uses CLK_OF_DECLARE, btw. It works but I get a '(null): Failed to create debugfs directory' message in the boot log. Tested-by: Joachim Eastwood <manabian@gmail.com> regards, Joachim Eastwood
Hi Pankaj, Joachim, Am Dienstag, 23. September 2014, 20:12:50 schrieb Joachim Eastwood: > On 22 September 2014 06:40, Pankaj Dubey <pankaj.dubey@samsung.com> wrote: > > Currently a syscon entity can be only registered directly through a > > platform device that binds to a dedicated syscon driver. However in > > certain use cases it is desirable to make a device used with another > > driver a syscon interface provider. > > > > For example, certain SoCs (e.g. Exynos) contain system controller > > blocks which perform various functions such as power domain control, > > CPU power management, low power mode control, but in addition contain > > certain IP integration glue, such as various signal masks, > > coprocessor power control, etc. In such case, there is a need to have > > a dedicated driver for such system controller but also share registers > > with other drivers. The latter is where the syscon interface is helpful. > > > > In case of DT based platforms, this patch decouples syscon object from > > syscon platform driver, and allows to create syscon objects first time > > when it is required by calling of syscon_regmap_lookup_by APIs and keep > > a list of such syscon objects along with syscon provider device_nodes > > and regmap handles. > > > > For non-DT based platforms, this patch keeps syscon platform driver > > structure where is can be probed and such non-DT based drivers can use > > syscon_regmap_lookup_by_pdev API and get access to regmap handles. > > Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based, > > we can completly remove platform driver of syscon, and keep only helper > > functions to get regmap handles. > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> > > Tested-by: Vivek Gautam <gautam.vivek@samsung.com> > > Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > > I wrote a clk driver using syscon and your patch. clk driver uses > CLK_OF_DECLARE, btw. > > It works but I get a '(null): Failed to create debugfs directory' > message in the boot log. > > Tested-by: Joachim Eastwood <manabian@gmail.com> on Rockchip platforms this syscon support also helps quite a bit, as the pll lock-status is sitting in an external syscon register, so setting target pll-rates through assigned-clocks is not easily doable without it. Therefore I'm very much looking forward to this. Similar to Joachim I get an error about debugfs from regmap, which seems to be caused by name = dev_name(map->dev); returning NULL in regmap_debugfs_init in regmap-debugfs.c for such an "early" syscon. [...] __set_clk_rates: setting cpll from 384000000 to 891000000 rockchip_rk3066_pll_set_rate: trying to get grf rockchip_rk3066_pll_set_rate: changing pll_cpll from 384000000 to 891000000 with a parent rate of 24000000 __set_clk_rates: cpll is now 891000000 Architected cp15 timer(s) running at 24.00MHz (virt). [...] regulator-dummy: no parameters NET: Registered protocol family 16 DMA: preallocated 256 KiB pool for atomic coherent allocations Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.17.0-rc1+ #1184 task: ee069b40 ti: ee06a000 task.ti: ee06a000 PC is at strlen+0xc/0x20 LR is at __create_file+0x6c/0x1d0 pc : [<c01a02f8>] lr : [<c01725f0>] psr: 60000153 sp : ee06bea8 ip : 0000000e fp : 00000000 r10: ee06a000 r9 : 00000000 r8 : 00000000 r7 : c07ecb20 r6 : edc02cc0 r5 : 000041ed r4 : 00000000 r3 : 00000001 r2 : 00000000 r1 : a0000153 r0 : 00000000 Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel Control: 10c5387d Table: 0000406a DAC: 00000015 Process swapper/0 (pid: 1, stack limit = 0xee06a240) Stack: (0xee06bea8 to 0xee06c000) bea0: 00000000 000041ed ee002400 c07efc60 00000000 c05a9e50 bec0: c058c510 c01727c8 00000000 edc02cc0 ee0024b0 c021d410 ee002400 00000000 bee0: ee02ff40 c0793494 c07ab080 c021d6b4 00000000 ee11bbc0 c0782c58 c058c518 bf00: 00000000 c0008970 ee0f7f00 c00f9f54 ee0f7f00 ee0f7c80 ee0f7c00 c03fddc4 bf20: c07cda04 00000000 c054c8a4 c00fa0e0 c0575514 ef7fccc5 00000000 c0034fd0 bf40: 00000000 00000000 c054c8a4 c054bcd4 000000bd 00000002 c0786394 00000002 bf60: c059e840 c07ab080 c05a9e50 000000bd 00000000 00000000 00000000 c0575c94 bf80: 00000002 00000002 c0575514 ee06a000 00000000 c03f38f8 00000000 00000000 bfa0: 00000000 c03f3900 00000000 c000e7f8 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 ffffffff ffffffff [<c01a02f8>] (strlen) from [<c01725f0>] (__create_file+0x6c/0x1d0) [<c01725f0>] (__create_file) from [<c01727c8>] (debugfs_create_dir+0x18/0x1c) [<c01727c8>] (debugfs_create_dir) from [<c021d410>] (regmap_debugfs_init+0xd0/0x254) [<c021d410>] (regmap_debugfs_init) from [<c021d6b4>] (regmap_debugfs_initcall+0x5c/0xbc) [<c021d6b4>] (regmap_debugfs_initcall) from [<c058c518>] (regmap_initcall+0x8/0x10) [<c058c518>] (regmap_initcall) from [<c0008970>] (do_one_initcall+0x110/0x1bc) [<c0008970>] (do_one_initcall) from [<c0575c94>] (kernel_init_freeable+0xfc/0x1c8) [<c0575c94>] (kernel_init_freeable) from [<c03f3900>] (kernel_init+0x8/0xe4) [<c03f3900>] (kernel_init) from [<c000e7f8>] (ret_from_fork+0x14/0x3c) Code: c040e4cc e1a03000 e1a02003 e2833001 (e5d21000) ---[ end trace fa51dc7fc31bd989 ]---
On Tuesday 23 September 2014 15:59:43 Pankaj Dubey wrote: > > > -static int syscon_match_node(struct device *dev, void *data) > > > +static struct syscon *of_syscon_register(struct device_node *np) > > > { > > > - struct device_node *dn = data; > > > + struct platform_device *pdev = NULL; > > > + struct syscon *syscon; > > > + struct regmap *regmap; > > > + void __iomem *base; > > > + int ret; > > > + > > > + if (!of_device_is_compatible(np, "syscon")) > > > + return ERR_PTR(-EINVAL); > > > + > > > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > > > + if (!syscon) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + base = of_iomap(np, 0); > > > + if (!base) { > > > + ret = -ENOMEM; > > > + goto err_map; > > > + } > > > + > > > + /* if device is already populated and available then use it */ > > > + pdev = of_find_device_by_node(np); > > > + if (!pdev) { > > > + /* for early users create dummy syscon device and use it */ > > > + pdev = platform_device_alloc("dummy-syscon", -1); > > > > Can we create specific devices for each syscon device? > > That looks more reasonable to me. > > Then we can dump registers in regmap debugfs more clearly, just like other > devices. > > And i think it's better to follow device tree way to generate devices from > node. > > If DT core find a device is already created, it can ignore that device to > avoid creating > > duplicated device during of_platform_populate. > > > > If you are referring to use "of_platform_device_create", then I am afraid > that it can't be used > in very early stage. For example, current patch I tested by calling > syscon_lookup_by_phandle from > "init_irq" hook of machine_desc, and it worked well, but If I use > "of_platform_device_create" > instead of dummy device, it fails to create pdev and this I verified on > Exynos platform. The reason > I selected "init_irq" is because this comes just before smp init and where > once we tried to get regmap handle, > but could not do so. I don't remember noticing the of_find_device_by_node or platform_device_alloc in earlier versions of this patch, but that could just be me failing to read it right. I think this should get removed: it would be much better to ensure that the regmap code can work without a device pointer, which I thought it did. We should probably also skip the creation of the debugfs directory in that case. > Also if it was just a matter of creating platform_device at early stage then > early initialization of > syscon could have been solved till now. > > So I would suggest if we really want this patch to cover early > initialization of syscon > (which is not it's main purpose) current patch is sufficient. > > @Arnd, since I have addressed crash issue as reported by Dong Aisheng, I > would like to take your ack, > but since there are some more code changes other than what you suggested I > request you to check this > and if you are ok, then I would like to your ack so that I can request to > maintainer for taking this patch. I think we first need to resolve the question of the platform device. Arnd
On Thursday, September 25, 2014 6:12 PM, Arnd Bergmann wrote, > Subject: Re: [PATCH v5] mfd: syscon: Decouple syscon interface from platform > devices > > On Tuesday 23 September 2014 15:59:43 Pankaj Dubey wrote: > > > > -static int syscon_match_node(struct device *dev, void *data) > > > > +static struct syscon *of_syscon_register(struct device_node *np) > > > > { > > > > - struct device_node *dn = data; > > > > + struct platform_device *pdev = NULL; > > > > + struct syscon *syscon; > > > > + struct regmap *regmap; > > > > + void __iomem *base; > > > > + int ret; > > > > + > > > > + if (!of_device_is_compatible(np, "syscon")) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); > > > > + if (!syscon) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + base = of_iomap(np, 0); > > > > + if (!base) { > > > > + ret = -ENOMEM; > > > > + goto err_map; > > > > + } > > > > + > > > > + /* if device is already populated and available then use it */ > > > > + pdev = of_find_device_by_node(np); > > > > + if (!pdev) { > > > > + /* for early users create dummy syscon device and use it */ > > > > + pdev = platform_device_alloc("dummy-syscon", -1); > > > > > > Can we create specific devices for each syscon device? > > > That looks more reasonable to me. > > > Then we can dump registers in regmap debugfs more clearly, just like > > > other > > devices. > > > And i think it's better to follow device tree way to generate > > > devices from > > node. > > > If DT core find a device is already created, it can ignore that > > > device to > > avoid creating > > > duplicated device during of_platform_populate. > > > > > > > If you are referring to use "of_platform_device_create", then I am > > afraid that it can't be used in very early stage. For example, current > > patch I tested by calling syscon_lookup_by_phandle from "init_irq" > > hook of machine_desc, and it worked well, but If I use > > "of_platform_device_create" > > instead of dummy device, it fails to create pdev and this I verified > > on Exynos platform. The reason I selected "init_irq" is because this > > comes just before smp init and where once we tried to get regmap > > handle, but could not do so. > > I don't remember noticing the of_find_device_by_node or platform_device_alloc in > earlier versions of this patch, but that could just be me failing to read it right. > Yes, till v2 of this patch set we do not used this and while calling regmap_init_mmio, I used NULL pointer while creating regmap handle. But as you may be remember Dong Aisheng reported crash on latest Linus tree after applying this patch, and it caused because of some recent changes in regmap which has already landed in main tree, which makes passing of dev pointer compulsory while calling regmap_init_mmio. Please check following commits in current tree 1) d647c19 regmap: add DT endianness binding support. 2) ba1b53f regmap: Fix DT endianess parsing logic. 3) 45e1a27 regmap: of_regmap_get_endian() cleanup and following patch [1] for changing syscon binding from Xiubo Li [1] [PATCH] mfd: syscon: binding: Add syscon endianness support https://lkml.org/lkml/2014/9/18/67 So we discussed [2] and decided to use either actual device if it's populated and available for use or create either actual device or dummy device for using it during regmap_init_mmio calls. [2]: https://lkml.org/lkml/2014/9/18/35 So what I understood from these changes from Xiubo Li, they wanted to pass endianness of regmap handle to be passed from DT. So I am not sure if we can really make regmap code work without a device pointer. > I think this should get removed: it would be much better to ensure that the regmap > code can work without a device pointer, which I thought it did. We should probably > also skip the creation of the debugfs directory in that case. > > > Also if it was just a matter of creating platform_device at early > > stage then early initialization of syscon could have been solved till > > now. As far as debugfs of regmap is concerned it can be solved as I replied to Heiko in this thread [3] if we are using device pointer while creating regmap handle. If we are not using device pointer while creating regmap handle it won't cause any problem and there won't be any debugfs entry. [3]: https://lkml.org/lkml/2014/9/26/20 > > > > So I would suggest if we really want this patch to cover early > > initialization of syscon (which is not it's main purpose) current > > patch is sufficient. > > > > @Arnd, since I have addressed crash issue as reported by Dong Aisheng, > > I would like to take your ack, but since there are some more code > > changes other than what you suggested I request you to check this and > > if you are ok, then I would like to your ack so that I can request to > > maintainer for taking this patch. > > I think we first need to resolve the question of the platform device. > OK. Thanks, Pankaj Dubey > Arnd
On Friday 26 September 2014 10:58:33 Pankaj Dubey wrote: > On Thursday, September 25, 2014 6:12 PM, Arnd Bergmann wrote, > > I don't remember noticing the of_find_device_by_node or > > platform_device_alloc in > > earlier versions of this patch, but that could just be me failing to read > > it right. > > > Yes, till v2 of this patch set we do not used this and while calling > regmap_init_mmio, I used > NULL pointer while creating regmap handle. But as you may be remember Dong > Aisheng reported crash > on latest Linus tree after applying this patch, and it caused because of > some recent changes in > regmap which has already landed in main tree, which makes passing of dev > pointer compulsory > while calling regmap_init_mmio. Please check following commits in current > tree > > 1) d647c19 regmap: add DT endianness binding support. > 2) ba1b53f regmap: Fix DT endianess parsing logic. > 3) 45e1a27 regmap: of_regmap_get_endian() cleanup > > and following patch [1] for changing syscon binding from Xiubo Li > > [1] [PATCH] mfd: syscon: binding: Add syscon endianness support > https://lkml.org/lkml/2014/9/18/67 > > So we discussed [2] and decided to use either actual device if it's > populated and available for use or > create either actual device or dummy device for using it during > regmap_init_mmio calls. > > [2]: https://lkml.org/lkml/2014/9/18/35 > > So what I understood from these changes from Xiubo Li, they wanted to pass > endianness of regmap > handle to be passed from DT. > > So I am not sure if we can really make regmap code work without a device > pointer. Why can't we just have both a dev pointer and an explicit of_node pointer that can be used in the absence of the device? I think it's dangerous to have a platform device that is allocated but not registered as you do in your driver at the moment. We can't register it for early callers either, and if we found a way to do so, we'd end up with multiple platform devices that contain the same of_node, which would then cause other problems, such as binding both devices to the same driver. > > I think this should get removed: it would be much better to ensure that > the regmap > > code can work without a device pointer, which I thought it did. We should > probably > > also skip the creation of the debugfs directory in that case. > > > > > Also if it was just a matter of creating platform_device at early > > > stage then early initialization of syscon could have been solved till > > > now. > > As far as debugfs of regmap is concerned it can be solved as I replied to > Heiko in this thread [3] > if we are using device pointer while creating regmap handle. > If we are not using device pointer while creating regmap handle it won't > cause any problem and > there won't be any debugfs entry. > > [3]: https://lkml.org/lkml/2014/9/26/20 Right, this works, although my earlier thoughts were that we would just never have a device for a syscon any more. We could have a different representation for syscon regmaps in debugfs though, one that is separate from the devices and that is based on the just the of_node pointer. syscon is special enough that I think the regmap infrastructure should be aware of what is a syscon regmap as opposed to one from a device. Arnd
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index ca15878..0b17f50 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -15,6 +15,7 @@ #include <linux/err.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/list.h> #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_platform.h> @@ -22,31 +23,105 @@ #include <linux/platform_device.h> #include <linux/regmap.h> #include <linux/mfd/syscon.h> +#include <linux/slab.h> static struct platform_driver syscon_driver; +static DEFINE_SPINLOCK(syscon_list_slock); +static LIST_HEAD(syscon_list); + struct syscon { + struct device_node *np; struct regmap *regmap; + struct list_head list; +}; + +static struct regmap_config syscon_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, }; -static int syscon_match_node(struct device *dev, void *data) +static struct syscon *of_syscon_register(struct device_node *np) { - struct device_node *dn = data; + struct platform_device *pdev = NULL; + struct syscon *syscon; + struct regmap *regmap; + void __iomem *base; + int ret; + + if (!of_device_is_compatible(np, "syscon")) + return ERR_PTR(-EINVAL); + + syscon = kzalloc(sizeof(*syscon), GFP_KERNEL); + if (!syscon) + return ERR_PTR(-ENOMEM); + + base = of_iomap(np, 0); + if (!base) { + ret = -ENOMEM; + goto err_map; + } + + /* if device is already populated and available then use it */ + pdev = of_find_device_by_node(np); + if (!pdev) { + /* for early users create dummy syscon device and use it */ + pdev = platform_device_alloc("dummy-syscon", -1); + if (!pdev) { + ret = -ENOMEM; + goto err_pdev; + } + pdev->dev.of_node = of_node_get(np); + } + + regmap = devm_regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config); + if (IS_ERR(regmap)) { + dev_err(&pdev->dev, "regmap init failed\n"); + ret = PTR_ERR(regmap); + goto err_regmap; + } + + syscon->regmap = regmap; + syscon->np = np; - return (dev->of_node == dn) ? 1 : 0; + spin_lock(&syscon_list_slock); + list_add_tail(&syscon->list, &syscon_list); + spin_unlock(&syscon_list_slock); + + return syscon; + +err_regmap: + if (!strcmp(pdev->name, "dummy-syscon")) { + of_node_put(np); + platform_device_put(pdev); + } +err_pdev: + iounmap(base); +err_map: + kfree(syscon); + return ERR_PTR(ret); } struct regmap *syscon_node_to_regmap(struct device_node *np) { - struct syscon *syscon; - struct device *dev; + struct syscon *entry, *syscon = NULL; - dev = driver_find_device(&syscon_driver.driver, NULL, np, - syscon_match_node); - if (!dev) - return ERR_PTR(-EPROBE_DEFER); + spin_lock(&syscon_list_slock); - syscon = dev_get_drvdata(dev); + list_for_each_entry(entry, &syscon_list, list) + if (entry->np == np) { + syscon = entry; + break; + } + + spin_unlock(&syscon_list_slock); + + if (!syscon) + syscon = of_syscon_register(np); + + if (IS_ERR(syscon)) + return ERR_CAST(syscon); return syscon->regmap; } @@ -110,17 +185,6 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, } EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle); -static const struct of_device_id of_syscon_match[] = { - { .compatible = "syscon", }, - { }, -}; - -static struct regmap_config syscon_regmap_config = { - .reg_bits = 32, - .val_bits = 32, - .reg_stride = 4, -}; - static int syscon_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -167,7 +231,6 @@ static struct platform_driver syscon_driver = { .driver = { .name = "syscon", .owner = THIS_MODULE, - .of_match_table = of_syscon_match, }, .probe = syscon_probe, .id_table = syscon_ids,