Both AP and CP are contained in Marvell PXA910 silicon. These two ARM
cores are sharing one pair of I2C pins.
In order to keep I2C transaction operated with atomic, hardware lock
(RIPC) is required. Because of this, bus lock in AP side can't afford
this requirement. Now hardware lock is appended.
Signed-off-by: Haojian Zhuang <[email protected]>
Cc: Ben Dooks <[email protected]>
---
drivers/i2c/i2c-core.c | 22 ++++++++++++++++++----
include/linux/i2c.h | 3 +++
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 045ba6e..412c7a5 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -448,8 +448,11 @@ void i2c_lock_adapter(struct i2c_adapter *adapter)
if (parent)
i2c_lock_adapter(parent);
- else
+ else {
rt_mutex_lock(&adapter->bus_lock);
+ if (adapter->hardware_lock)
+ adapter->hardware_lock();
+ }
}
EXPORT_SYMBOL_GPL(i2c_lock_adapter);
@@ -460,11 +463,19 @@ EXPORT_SYMBOL_GPL(i2c_lock_adapter);
static int i2c_trylock_adapter(struct i2c_adapter *adapter)
{
struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
+ int ret = 0;
if (parent)
return i2c_trylock_adapter(parent);
- else
- return rt_mutex_trylock(&adapter->bus_lock);
+ else {
+ ret = rt_mutex_trylock(&adapter->bus_lock);
+ if (ret && adapter->hardware_trylock) {
+ ret = adapter->hardware_trylock();
+ if (!ret)
+ i2c_unlock_adapter(adapter);
+ }
+ return ret;
+ }
}
/**
@@ -477,8 +488,11 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter)
if (parent)
i2c_unlock_adapter(parent);
- else
+ else {
+ if (adapter->hardware_unlock)
+ adapter->hardware_unlock();
rt_mutex_unlock(&adapter->bus_lock);
+ }
}
EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 06a8d9c..b283b4e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -361,6 +361,9 @@ struct i2c_adapter {
/* data fields that are valid for all devices */
struct rt_mutex bus_lock;
+ void (*hardware_lock)(void);
+ void (*hardware_unlock)(void);
+ int (*hardware_trylock)(void);
int timeout; /* in jiffies */
int retries;
--
1.7.1
Hi Haojian,
On Thu, 28 Apr 2011 12:02:36 +0800, Haojian Zhuang wrote:
> Both AP and CP are contained in Marvell PXA910 silicon. These two ARM
> cores are sharing one pair of I2C pins.
>
> In order to keep I2C transaction operated with atomic, hardware lock
> (RIPC) is required. Because of this, bus lock in AP side can't afford
> this requirement. Now hardware lock is appended.
I have no objection to the idea, but one question: when using the
hardware lock, isn't the software mutex redundant? I would expect that
you call the hardware_lock/unlock functions _instead_ of
rt_mutex_lock/unlock, rather than in addition to it. Or do you still
need the rt_mutex to prevent priority inversion?
>
> Signed-off-by: Haojian Zhuang <[email protected]>
> Cc: Ben Dooks <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 22 ++++++++++++++++++----
> include/linux/i2c.h | 3 +++
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 045ba6e..412c7a5 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -448,8 +448,11 @@ void i2c_lock_adapter(struct i2c_adapter *adapter)
>
> if (parent)
> i2c_lock_adapter(parent);
> - else
> + else {
> rt_mutex_lock(&adapter->bus_lock);
> + if (adapter->hardware_lock)
> + adapter->hardware_lock();
> + }
> }
> EXPORT_SYMBOL_GPL(i2c_lock_adapter);
>
> @@ -460,11 +463,19 @@ EXPORT_SYMBOL_GPL(i2c_lock_adapter);
> static int i2c_trylock_adapter(struct i2c_adapter *adapter)
> {
> struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
> + int ret = 0;
>
> if (parent)
> return i2c_trylock_adapter(parent);
> - else
> - return rt_mutex_trylock(&adapter->bus_lock);
> + else {
> + ret = rt_mutex_trylock(&adapter->bus_lock);
> + if (ret && adapter->hardware_trylock) {
> + ret = adapter->hardware_trylock();
> + if (!ret)
> + i2c_unlock_adapter(adapter);
> + }
> + return ret;
> + }
> }
>
> /**
> @@ -477,8 +488,11 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter)
>
> if (parent)
> i2c_unlock_adapter(parent);
> - else
> + else {
> + if (adapter->hardware_unlock)
> + adapter->hardware_unlock();
> rt_mutex_unlock(&adapter->bus_lock);
> + }
> }
> EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
>
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 06a8d9c..b283b4e 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -361,6 +361,9 @@ struct i2c_adapter {
>
> /* data fields that are valid for all devices */
> struct rt_mutex bus_lock;
> + void (*hardware_lock)(void);
> + void (*hardware_unlock)(void);
> + int (*hardware_trylock)(void);
>
> int timeout; /* in jiffies */
> int retries;
--
Jean Delvare
On Thu, Apr 28, 2011 at 4:22 PM, Jean Delvare <[email protected]> wrote:
> Hi Haojian,
>
> On Thu, 28 Apr 2011 12:02:36 +0800, Haojian Zhuang wrote:
>> Both AP and CP are contained in Marvell PXA910 silicon. These two ARM
>> cores are sharing one pair of I2C pins.
>>
>> In order to keep I2C transaction operated with atomic, hardware lock
>> (RIPC) is required. Because of this, bus lock in AP side can't afford
>> this requirement. Now hardware lock is appended.
>
> I have no objection to the idea, but one question: when using the
> hardware lock, isn't the software mutex redundant? I would expect that
> you call the hardware_lock/unlock functions _instead_ of
> rt_mutex_lock/unlock, rather than in addition to it. Or do you still
> need the rt_mutex to prevent priority inversion?
>
Jean,
It's actually not redundant. The hardware lock is used to protect
access to the same register regions between two processors (AP
and CP so called), while the software lock is used to protect
access from within the AP side.
>>
>> Signed-off-by: Haojian Zhuang <[email protected]>
>> Cc: Ben Dooks <[email protected]>
>> ---
>> drivers/i2c/i2c-core.c | 22 ++++++++++++++++++----
>> include/linux/i2c.h | 3 +++
>> 2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 045ba6e..412c7a5 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -448,8 +448,11 @@ void i2c_lock_adapter(struct i2c_adapter *adapter)
>>
>> if (parent)
>> i2c_lock_adapter(parent);
>> - else
>> + else {
>> rt_mutex_lock(&adapter->bus_lock);
>> + if (adapter->hardware_lock)
>> + adapter->hardware_lock();
>> + }
>> }
>> EXPORT_SYMBOL_GPL(i2c_lock_adapter);
>>
>> @@ -460,11 +463,19 @@ EXPORT_SYMBOL_GPL(i2c_lock_adapter);
>> static int i2c_trylock_adapter(struct i2c_adapter *adapter)
>> {
>> struct i2c_adapter *parent = i2c_parent_is_i2c_adapter(adapter);
>> + int ret = 0;
>>
>> if (parent)
>> return i2c_trylock_adapter(parent);
>> - else
>> - return rt_mutex_trylock(&adapter->bus_lock);
>> + else {
>> + ret = rt_mutex_trylock(&adapter->bus_lock);
>> + if (ret && adapter->hardware_trylock) {
>> + ret = adapter->hardware_trylock();
>> + if (!ret)
>> + i2c_unlock_adapter(adapter);
>> + }
>> + return ret;
>> + }
>> }
>>
>> /**
>> @@ -477,8 +488,11 @@ void i2c_unlock_adapter(struct i2c_adapter *adapter)
>>
>> if (parent)
>> i2c_unlock_adapter(parent);
>> - else
>> + else {
>> + if (adapter->hardware_unlock)
>> + adapter->hardware_unlock();
>> rt_mutex_unlock(&adapter->bus_lock);
>> + }
>> }
>> EXPORT_SYMBOL_GPL(i2c_unlock_adapter);
>>
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 06a8d9c..b283b4e 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -361,6 +361,9 @@ struct i2c_adapter {
>>
>> /* data fields that are valid for all devices */
>> struct rt_mutex bus_lock;
>> + void (*hardware_lock)(void);
>> + void (*hardware_unlock)(void);
>> + int (*hardware_trylock)(void);
>>
>> int timeout; /* in jiffies */
>> int retries;
>
>
> --
> Jean Delvare
>
Hi Eric,
On Thu, 28 Apr 2011 16:36:02 +0800, Eric Miao wrote:
> On Thu, Apr 28, 2011 at 4:22 PM, Jean Delvare <[email protected]> wrote:
> > Hi Haojian,
> >
> > On Thu, 28 Apr 2011 12:02:36 +0800, Haojian Zhuang wrote:
> >> Both AP and CP are contained in Marvell PXA910 silicon. These two ARM
> >> cores are sharing one pair of I2C pins.
> >>
> >> In order to keep I2C transaction operated with atomic, hardware lock
> >> (RIPC) is required. Because of this, bus lock in AP side can't afford
> >> this requirement. Now hardware lock is appended.
> >
> > I have no objection to the idea, but one question: when using the
> > hardware lock, isn't the software mutex redundant? I would expect that
> > you call the hardware_lock/unlock functions _instead_ of
> > rt_mutex_lock/unlock, rather than in addition to it. Or do you still
> > need the rt_mutex to prevent priority inversion?
> >
>
> Jean,
>
> It's actually not redundant. The hardware lock is used to protect
> access to the same register regions between two processors (AP
> and CP so called), while the software lock is used to protect
> access from within the AP side.
Are you suggesting that the hardware lock wouldn't mind being taken
twice by the AP side? If it is the case, then indeed the software mutex
is still needed to prevent it from happening.
That being said... I guess that avoiding a priority inversion is a good
enough reason to always take the rt_mutex, regardless of the hardware
lock implementation.
So, this patch is
Acked-by: Jean Delvare <[email protected]>
I guess it makes more sense for me to let Ben apply it, as the other
two patches in the series are for him too. This will avoid a dependency
between our trees.
--
Jean Delvare
On Thu, Apr 28, 2011 at 4:36 PM, Eric Miao <[email protected]> wrote:
> On Thu, Apr 28, 2011 at 4:22 PM, Jean Delvare <[email protected]> wrote:
>> Hi Haojian,
>>
>> On Thu, 28 Apr 2011 12:02:36 +0800, Haojian Zhuang wrote:
>>> Both AP and CP are contained in Marvell PXA910 silicon. These two ARM
>>> cores are sharing one pair of I2C pins.
>>>
>>> In order to keep I2C transaction operated with atomic, hardware lock
>>> (RIPC) is required. Because of this, bus lock in AP side can't afford
>>> this requirement. Now hardware lock is appended.
>>
>> I have no objection to the idea, but one question: when using the
>> hardware lock, isn't the software mutex redundant? I would expect that
>> you call the hardware_lock/unlock functions _instead_ of
>> rt_mutex_lock/unlock, rather than in addition to it. Or do you still
>> need the rt_mutex to prevent priority inversion?
>>
>
> Jean,
>
> It's actually not redundant. The hardware lock is used to protect
> access to the same register regions between two processors (AP
> and CP so called), while the software lock is used to protect
> access from within the AP side.
>
Jean,
It's not redundant. Reading RIPC register will try to get the lock. We're always
using __raw_readl() API to read register. I think the read operation
couldn't be
atomic and finished in one instruction cycle. If two processes in AP
side try to
get the RIPC lock with __raw_readl(), it may result dead lock. If we fetch the
RIPC lock behind software bus lock, it's safe.
If process on AP try to get the RIPC lock and compete with CP, it won't be an
issue. It should always be atomic.
On Thu, Apr 28, 2011 at 04:16:25PM +0200, Jean Delvare wrote:
> Are you suggesting that the hardware lock wouldn't mind being taken
> twice by the AP side? If it is the case, then indeed the software mutex
> is still needed to prevent it from happening.
>
> That being said... I guess that avoiding a priority inversion is a good
> enough reason to always take the rt_mutex, regardless of the hardware
> lock implementation.
>
> So, this patch is
>
> Acked-by: Jean Delvare <[email protected]>
>
> I guess it makes more sense for me to let Ben apply it, as the other
> two patches in the series are for him too. This will avoid a dependency
> between our trees.
Only change I'd suggest is passing adapter to the hardware_lock/unlock
methods. Having no arguments what so ever in generic code for this kind
of stuff looks rather strange and limiting.
On Thu, Apr 28, 2011 at 10:37 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Thu, Apr 28, 2011 at 04:16:25PM +0200, Jean Delvare wrote:
>> Are you suggesting that the hardware lock wouldn't mind being taken
>> twice by the AP side? If it is the case, then indeed the software mutex
>> is still needed to prevent it from happening.
>>
>> That being said... I guess that avoiding a priority inversion is a good
>> enough reason to always take the rt_mutex, regardless of the hardware
>> lock implementation.
>>
>> So, this patch is
>>
>> Acked-by: Jean Delvare <[email protected]>
>>
>> I guess it makes more sense for me to let Ben apply it, as the other
>> two patches in the series are for him too. This will avoid a dependency
>> between our trees.
>
> Only change I'd suggest is passing adapter to the hardware_lock/unlock
> methods. ?Having no arguments what so ever in generic code for this kind
> of stuff looks rather strange and limiting.
>
OK. I'll update it.