lkml.org 
[lkml]   [2013]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 02/10] pinctrl: mvebu: dove pinctrl driver
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


\
 
 \ /
  Last update: 2013-06-19 08:21    [W:0.188 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site