On Thu, Sep 13, 2012 at 05:41:44PM +0200, Sebastian Hesselbarth wrote:
> +#define DOVE_GLOBAL_CONFIG_1 (DOVE_SB_REGS_VIRT_BASE | 0xe802C)
> +#define DOVE_TWSI_ENABLE_OPTION1 BIT(7)
> +#define DOVE_GLOBAL_CONFIG_2 (DOVE_SB_REGS_VIRT_BASE | 0xe8030)
> +#define DOVE_TWSI_ENABLE_OPTION2 BIT(20)
> +#define DOVE_TWSI_ENABLE_OPTION3 BIT(21)
> +#define DOVE_TWSI_OPTION3_GPIO BIT(22)
...
> +static int dove_twsi_ctrl_set(struct mvebu_mpp_ctrl *ctrl,
> + unsigned long config)
> +{
> + unsigned long gcfg1 = readl(DOVE_GLOBAL_CONFIG_1);
> + unsigned long gcfg2 = readl(DOVE_GLOBAL_CONFIG_2);
> +
> + gcfg1 &= ~DOVE_TWSI_ENABLE_OPTION1;
> + gcfg2 &= ~(DOVE_TWSI_ENABLE_OPTION2 | DOVE_TWSI_ENABLE_OPTION2);
> +
> + switch (config) {
> + case 1:
> + gcfg1 |= DOVE_TWSI_ENABLE_OPTION1;
> + break;
> + case 2:
> + gcfg2 |= DOVE_TWSI_ENABLE_OPTION2;
> + break;
> + case 3:
> + gcfg2 |= DOVE_TWSI_ENABLE_OPTION3;
> + break;
> + }
> +
> + writel(gcfg1, DOVE_GLOBAL_CONFIG_1);
> + writel(gcfg2, DOVE_GLOBAL_CONFIG_2);
> +
> + return 0;
> +}
So, I've just been thinking about the LCD clocking on the Armada 510,
and found that there's dividers for the internal LCD clocks in the
global config 1/2 registers. So I grepped the kernel source for
references to these, expecting to find something in drivers/clk, but
found the above.
Now, the reason that I'm replying to this is that we're again falling
into the same trap that we did with SA1100 devices. Back in those
days, there wasn't so much of a problem because the kernel was basically
single threaded when it came to code like the above on such platforms.
There was no kernel preemption.
However, todays kernel is sometimes SMP, commonly with kernel preemption
enabled, maybe even RT. This makes things like the above sequence a
problem where a multifunction register is read, modified and then
written back.
Consider two threads doing this, and a preemption event happening in the
middle of this sequence to another thread also doing a read-modify-write
of the same register. Which one wins depends on the preemption sequence,
but ultimately one loses out.
Any access to such registers needs careful thought, and protection in some
manner.
Maybe what we need is something like this:
static DEFINE_SPINLOCK(io_lock);
static void modifyl(u32 new, u32 mask, void __iomem *reg)
{
unsigned long flags;
u32 val;
spin_lock_irqsave(&io_lock, flags);
val = readl(reg) & ~mask;
val |= new | mask;
writel(val, reg);
spin_unlock_irqrestore(&io_lock, flags);
}
in order to provide arbitrated access to these kinds of multifunction
registers in a safe, platform agnostic way.
Comments?
On 06/18/13 13:36, Russell King - ARM Linux wrote:
> On Thu, Sep 13, 2012 at 05:41:44PM +0200, Sebastian Hesselbarth wrote:
>> +#define DOVE_GLOBAL_CONFIG_1 (DOVE_SB_REGS_VIRT_BASE | 0xe802C)
>> +#define DOVE_TWSI_ENABLE_OPTION1 BIT(7)
>> +#define DOVE_GLOBAL_CONFIG_2 (DOVE_SB_REGS_VIRT_BASE | 0xe8030)
>> +#define DOVE_TWSI_ENABLE_OPTION2 BIT(20)
>> +#define DOVE_TWSI_ENABLE_OPTION3 BIT(21)
>> +#define DOVE_TWSI_OPTION3_GPIO BIT(22)
> ...
Russell,
the above absolute addresses already made me think of cleaning up dove
pinctrl a while ago. I also had in mind that below function exclusively
request ownership of global config registers.
>> +static int dove_twsi_ctrl_set(struct mvebu_mpp_ctrl *ctrl,
>> + unsigned long config)
>> +{
>> + unsigned long gcfg1 = readl(DOVE_GLOBAL_CONFIG_1);
>> + unsigned long gcfg2 = readl(DOVE_GLOBAL_CONFIG_2);
>> +
>> + gcfg1 &= ~DOVE_TWSI_ENABLE_OPTION1;
>> + gcfg2 &= ~(DOVE_TWSI_ENABLE_OPTION2 | DOVE_TWSI_ENABLE_OPTION2);
>> +
>> + switch (config) {
>> + case 1:
>> + gcfg1 |= DOVE_TWSI_ENABLE_OPTION1;
>> + break;
>> + case 2:
>> + gcfg2 |= DOVE_TWSI_ENABLE_OPTION2;
>> + break;
>> + case 3:
>> + gcfg2 |= DOVE_TWSI_ENABLE_OPTION3;
>> + break;
>> + }
>> +
>> + writel(gcfg1, DOVE_GLOBAL_CONFIG_1);
>> + writel(gcfg2, DOVE_GLOBAL_CONFIG_2);
>> +
>> + return 0;
>> +}
>
> So, I've just been thinking about the LCD clocking on the Armada 510,
> and found that there's dividers for the internal LCD clocks in the
> global config 1/2 registers. So I grepped the kernel source for
> references to these, expecting to find something in drivers/clk, but
> found the above.
We have no peripheral clock handling for Dove, yet. Just core clocks and
clock gates are implemented. And I guess they are DT only anyway.
> However, todays kernel is sometimes SMP, commonly with kernel preemption
> enabled, maybe even RT. This makes things like the above sequence a
> problem where a multifunction register is read, modified and then
> written back.
>
> Consider two threads doing this, and a preemption event happening in the
> middle of this sequence to another thread also doing a read-modify-write
> of the same register. Which one wins depends on the preemption sequence,
> but ultimately one loses out.
Yeah, sure. We have the same issue with watchdog driver messing with
timer registers. There I exported a function to _clrset TIMER_CTRL
register safely. Just went into irqchip (tip for-next).
> Any access to such registers needs careful thought, and protection in some
> manner.
>
> Maybe what we need is something like this:
>
> static DEFINE_SPINLOCK(io_lock);
> static void modifyl(u32 new, u32 mask, void __iomem *reg)
> {
> unsigned long flags;
> u32 val;
>
> spin_lock_irqsave(&io_lock, flags);
> val = readl(reg) & ~mask;
> val |= new | mask;
> writel(val, reg);
> spin_unlock_irqrestore(&io_lock, flags);
> }
>
> in order to provide arbitrated access to these kinds of multifunction
> registers in a safe, platform agnostic way.
I am fine with a generic modify function with a single lock. Most cases
should be fine with a single lock even for non-related register
accesses, e.g. watchdog will access TIMER_CTRL only once to enable
itself. If you think you need a special lock because you have a lot of
writes to shared registers, you can still have your own modify lock.
Sebastian
On Tue, Jun 18, 2013 at 1:36 PM, Russell King - ARM Linux
<[email protected]> wrote:
> Maybe what we need is something like this:
>
> static DEFINE_SPINLOCK(io_lock);
> static void modifyl(u32 new, u32 mask, void __iomem *reg)
> {
> unsigned long flags;
> u32 val;
>
> spin_lock_irqsave(&io_lock, flags);
> val = readl(reg) & ~mask;
> val |= new | mask;
> writel(val, reg);
> spin_unlock_irqrestore(&io_lock, flags);
> }
>
> in order to provide arbitrated access to these kinds of multifunction
> registers in a safe, platform agnostic way.
Nowadays I would do the above with regmap_update_bits().
Mutual exclusion for read-modify-write of individual bits in a
register is one of those cases where doing a regmap over
a memory-mapped register range makes a lot of sense.
(drivers/mfd/syscon.c being a nice example)
Yours,
Linus Walleij
On Tue, Jun 18, 2013 at 05:02:49PM +0200, Linus Walleij wrote:
> Nowadays I would do the above with regmap_update_bits().
>
> Mutual exclusion for read-modify-write of individual bits in a
> register is one of those cases where doing a regmap over
> a memory-mapped register range makes a lot of sense.
> (drivers/mfd/syscon.c being a nice example)
So, for that solution we need to have some kind of global regmap per
register or somesuch. Then you run into regmap needing a struct
device - well, with a shared register, which struct device do you
use, or do you have to invent one?
That sounds more heavy-weight than is really necessary.
On Tue, Jun 18, 2013 at 5:11 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Tue, Jun 18, 2013 at 05:02:49PM +0200, Linus Walleij wrote:
>> Nowadays I would do the above with regmap_update_bits().
>>
>> Mutual exclusion for read-modify-write of individual bits in a
>> register is one of those cases where doing a regmap over
>> a memory-mapped register range makes a lot of sense.
>> (drivers/mfd/syscon.c being a nice example)
>
> So, for that solution we need to have some kind of global regmap per
> register or somesuch. Then you run into regmap needing a struct
> device - well, with a shared register, which struct device do you
> use, or do you have to invent one?
Usually, like for syscon, you create an MFD hub which
carry the regmap.
> That sounds more heavy-weight than is really necessary.
May be so, especially if there is just one register to protect.
The usefulness of MM-regmap increase with the set of registers
needing to be protected.
Yours,
Linus Walleij
On Tue, Jun 18, 2013 at 04:11:16PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 18, 2013 at 05:02:49PM +0200, Linus Walleij wrote:
> > Nowadays I would do the above with regmap_update_bits().
> > Mutual exclusion for read-modify-write of individual bits in a
> > register is one of those cases where doing a regmap over
> > a memory-mapped register range makes a lot of sense.
> > (drivers/mfd/syscon.c being a nice example)
> So, for that solution we need to have some kind of global regmap per
> register or somesuch. Then you run into regmap needing a struct
> device - well, with a shared register, which struct device do you
> use, or do you have to invent one?
> That sounds more heavy-weight than is really necessary.
Yes, regmap is far too heavyweight for a lot of these things - it
shouldn't really go in performance critical at the CPU level paths, it's
designed around things where I/O costs are considerable and worth
avoiding.
I do think that this is a very good idea - with the I2C/SPI drivers I
was reviewing prior to regmap there were always just far too many simple
bugs with read/modify/write cycles so factoring out the code really
helped with review.