Re: [PATCH v4 02/10] pinctrl: mvebu: dove pinctrl driver

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

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