| Message ID | 1410553499-55951-6-git-send-email-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Suman, On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote: > static int omap_hwspinlock_probe(struct platform_device *pdev) > { > - struct hwspinlock_pdata *pdata = pdev->dev.platform_data; > + struct device_node *node = pdev->dev.of_node; > struct hwspinlock_device *bank; > struct hwspinlock *hwlock; > struct resource *res; > void __iomem *io_base; > - int num_locks, i, ret; > + int num_locks, i, ret, base_id; > > - if (!pdata) > + if (!node) > return -ENODEV; > > + ret = of_hwspin_lock_get_base_id(node); > + if (ret < 0 && ret != -EINVAL) > + return -ENODEV; > + base_id = (ret > 0 ? ret : 0); Does this mean you allow nodes not to have the base_id property? How do we protect against multiple nodes not having a base_id property then? Implicitly assuming a base_id value (zero in this case) may not be always safe. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ohad, On 11/12/2014 01:14 PM, Ohad Ben-Cohen wrote: > Hi Suman, > > On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote: >> static int omap_hwspinlock_probe(struct platform_device *pdev) >> { >> - struct hwspinlock_pdata *pdata = pdev->dev.platform_data; >> + struct device_node *node = pdev->dev.of_node; >> struct hwspinlock_device *bank; >> struct hwspinlock *hwlock; >> struct resource *res; >> void __iomem *io_base; >> - int num_locks, i, ret; >> + int num_locks, i, ret, base_id; >> >> - if (!pdata) >> + if (!node) >> return -ENODEV; >> >> + ret = of_hwspin_lock_get_base_id(node); >> + if (ret < 0 && ret != -EINVAL) >> + return -ENODEV; >> + base_id = (ret > 0 ? ret : 0); > > Does this mean you allow nodes not to have the base_id property? How > do we protect against multiple nodes not having a base_id property > then? > > Implicitly assuming a base_id value (zero in this case) may not be always safe. None of the OMAPs have multiple IP instances, and as such the base-id is an optional property. I have made this change to make sure we atleast attempt to use the value if mentioned in DT and not hard-coding the value to begin with (going by the optional property semantics). If and when multiple instances get added and a secondary node doesn't add the property, the node will not be registered with the core due to an overlap failure. Here is the previous version [1] for reference. regards Suman [1] https://patchwork.kernel.org/patch/4096881/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suman, On Wed, Nov 12, 2014 at 9:50 PM, Suman Anna <s-anna@ti.com> wrote: > None of the OMAPs have multiple IP instances, and as such the base-id is > an optional property. I have made this change to make sure we atleast > attempt to use the value if mentioned in DT and not hard-coding the > value to begin with (going by the optional property semantics). If and > when multiple instances get added and a secondary node doesn't add the > property, the node will not be registered with the core due to an > overlap failure. Ok, that sounds good. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 12, 2014 at 11:14 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote: > Hi Suman, [..] > > Does this mean you allow nodes not to have the base_id property? How > do we protect against multiple nodes not having a base_id property > then? > > Implicitly assuming a base_id value (zero in this case) may not be always safe. > Hi Ohad, I still have a huge problem understanding the awesomeness with the "base_id". If you have a SoC with 2 hwlock blocks; say 8+8 locks, used for interaction with e.g. a modem and a video core respectively. Why would you in either remote system offset the locks with 8? Wouldn't e.g the modem use locks hwlock0:0-7 and video core use locks hwlock1:0-7? What systems use more than one hwlock block and do you know of any reasons why these hwlocks are globally numbered? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On Thu, Nov 20, 2014 at 2:43 AM, Bjorn Andersson <bjorn@kryo.se> wrote: > I still have a huge problem understanding the awesomeness with the > "base_id". If you have a SoC with 2 hwlock blocks; say 8+8 locks, used > for interaction with e.g. a modem and a video core respectively. > Why would you in either remote system offset the locks with 8? > Wouldn't e.g the modem use locks hwlock0:0-7 and video core use locks > hwlock1:0-7? Please see below > What systems use more than one hwlock block None that we know of. This concern was raised some time ago by Arnd and since it was easy to deal with we added the 'base_id' notion. > and do you know of any reasons why these hwlocks are globally numbered? Because on an heterogeneous asymmetric multiprocessing systems you sometimes have use cases where hwlocks are dynamically allocated and their id numbers need to be synchronized between user space applications, the Linux kernel, and entities running on remote processors (which are likely not running Linux). For this to work we have to have some system-wide global hwlocks naming that is predefined and known to all processors. A mere number seems like the simplest naming method. A dynamic hwlock request API could return "hwlock1:0" but a mere 8 seems easier to deal with. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/MAINTAINERS b/MAINTAINERS index cf24bb5..351885e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6531,7 +6531,6 @@ M: Ohad Ben-Cohen <ohad@wizery.com> L: linux-omap@vger.kernel.org S: Maintained F: drivers/hwspinlock/omap_hwspinlock.c -F: arch/arm/mach-omap2/hwspinlock.c OMAP MMC SUPPORT M: Jarkko Lavinen <jarkko.lavinen@nokia.com> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile index 69bbcba..088b6b0 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -286,9 +286,6 @@ obj-y += $(smc91x-m) $(smc91x-y) smsc911x-$(CONFIG_SMSC911X) := gpmc-smsc911x.o obj-y += $(smsc911x-m) $(smsc911x-y) -ifneq ($(CONFIG_HWSPINLOCK_OMAP),) -obj-y += hwspinlock.o -endif emac-$(CONFIG_TI_DAVINCI_EMAC) := am35xx-emac.o obj-y += $(emac-m) $(emac-y) diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c deleted file mode 100644 index ef175ac..0000000 --- a/arch/arm/mach-omap2/hwspinlock.c +++ /dev/null @@ -1,60 +0,0 @@ -/* - * OMAP hardware spinlock device initialization - * - * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com - * - * Contact: Simon Que <sque@ti.com> - * Hari Kanigeri <h-kanigeri2@ti.com> - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, but - * WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * General Public License for more details. - */ - -#include <linux/kernel.h> -#include <linux/init.h> -#include <linux/err.h> -#include <linux/hwspinlock.h> - -#include "soc.h" -#include "omap_hwmod.h" -#include "omap_device.h" - -static struct hwspinlock_pdata omap_hwspinlock_pdata __initdata = { - .base_id = 0, -}; - -static int __init hwspinlocks_init(void) -{ - int retval = 0; - struct omap_hwmod *oh; - struct platform_device *pdev; - const char *oh_name = "spinlock"; - const char *dev_name = "omap_hwspinlock"; - - /* - * Hwmod lookup will fail in case our platform doesn't support the - * hardware spinlock module, so it is safe to run this initcall - * on all omaps - */ - oh = omap_hwmod_lookup(oh_name); - if (oh == NULL) - return -EINVAL; - - pdev = omap_device_build(dev_name, 0, oh, &omap_hwspinlock_pdata, - sizeof(struct hwspinlock_pdata)); - if (IS_ERR(pdev)) { - pr_err("Can't build omap_device for %s:%s\n", dev_name, - oh_name); - retval = PTR_ERR(pdev); - } - - return retval; -} -/* early board code might need to reserve specific hwspinlock instances */ -omap_postcore_initcall(hwspinlocks_init); diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c index c1e2cd4..2e8c7c3 100644 --- a/drivers/hwspinlock/omap_hwspinlock.c +++ b/drivers/hwspinlock/omap_hwspinlock.c @@ -1,7 +1,7 @@ /* * OMAP hardware spinlock driver * - * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com + * Copyright (C) 2010-2014 Texas Instruments Incorporated - http://www.ti.com * * Contact: Simon Que <sque@ti.com> * Hari Kanigeri <h-kanigeri2@ti.com> @@ -27,6 +27,7 @@ #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/hwspinlock.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include "hwspinlock_internal.h" @@ -80,16 +81,21 @@ static const struct hwspinlock_ops omap_hwspinlock_ops = { static int omap_hwspinlock_probe(struct platform_device *pdev) { - struct hwspinlock_pdata *pdata = pdev->dev.platform_data; + struct device_node *node = pdev->dev.of_node; struct hwspinlock_device *bank; struct hwspinlock *hwlock; struct resource *res; void __iomem *io_base; - int num_locks, i, ret; + int num_locks, i, ret, base_id; - if (!pdata) + if (!node) return -ENODEV; + ret = of_hwspin_lock_get_base_id(node); + if (ret < 0 && ret != -EINVAL) + return -ENODEV; + base_id = (ret > 0 ? ret : 0); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) return -ENODEV; @@ -141,7 +147,7 @@ static int omap_hwspinlock_probe(struct platform_device *pdev) hwlock->priv = io_base + LOCK_BASE_OFFSET + sizeof(u32) * i; ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops, - pdata->base_id, num_locks); + base_id, num_locks); if (ret) goto reg_fail; @@ -174,12 +180,19 @@ static int omap_hwspinlock_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id omap_hwspinlock_of_match[] = { + { .compatible = "ti,omap4-hwspinlock", }, + { /* end */ }, +}; +MODULE_DEVICE_TABLE(of, omap_hwspinlock_of_match); + static struct platform_driver omap_hwspinlock_driver = { .probe = omap_hwspinlock_probe, .remove = omap_hwspinlock_remove, .driver = { .name = "omap_hwspinlock", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(omap_hwspinlock_of_match), }, };