On Thu, Aug 5, 2010 at 12:30 AM, Linus Walleij
We have the Acked-by from Viresh, Alessandro looks like he is
interested in trying it out, the driver is self contained, marked
experimental, and you have a track-record of fixing things up, so I'll
include it with the idea it will be easier to fixup in-tree. One
thing that should be on the todo list is to review the usage of memory
barriers. I don't think they are serving the purpose that you think
they are. For example in pl08x_irq() there is:
+ /*
+ * Clear only the terminal interrupts on channels we processed
+ */
+ writel(mask, pl08x->base + PL080_TC_CLEAR);
+ mb();
+
+ return mask ? IRQ_HANDLED : IRQ_NONE;
Which seems to imply you want to ensure the interrupt bits are cleared
before the handler returns. But the only way you can guarantee that
the write has actually taken effect is to read back the register.
Does the amba bus interface make this guarantee with a barrier? If so
then we need a comment about why we need a full barrier versus just a
wmb()?
+static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
+{
+ u32 val;
+
+ /* Set the HALT bit and wait for the FIFO to drain */
+ val = readl(ch->base + PL080_CH_CONFIG);
+ val |= PL080_CONFIG_HALT;
+ writel(val, ch->base + PL080_CH_CONFIG);
+
+ /* Wait for channel inactive */
+ while (pl08x_phy_channel_busy(ch))
+ ;
+ mb();
+}
Doesn't the while loop guarantee that the write has taken effect?
+static void pl08x_ensure_on(struct pl08x_driver_data *pl08x)
+{
+ u32 val;
+
+ val = readl(pl08x->base + PL080_CONFIG);
+ val &= ~(PL080_CONFIG_M2_BE | PL080_CONFIG_M1_BE | PL080_CONFIG_ENABLE);
+ /* We implictly clear bit 1 and that means little-endian mode */
+ val |= PL080_CONFIG_ENABLE;
+ mb();
+ writel(val, pl08x->base + PL080_CONFIG);
+ mb();
+}
This almost looks like you are using mb() as a ...
