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:3.059 / U:2.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site