Hi Suman,
On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <[email protected]> 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.
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 <[email protected]> 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/
Hi Suman,
On Wed, Nov 12, 2014 at 9:50 PM, Suman Anna <[email protected]> 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.
On Wed, Nov 12, 2014 at 11:14 AM, Ohad Ben-Cohen <[email protected]> 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
Hi Bjorn,
On Thu, Nov 20, 2014 at 2:43 AM, Bjorn Andersson <[email protected]> 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.