Fwd: [PATCH 5/9] magician: enable flash VPP GPIO and build in MTD, physmap-flash and JFFS2

[pHilipp Zabel]

Hi, I'd like to forward you a question about this patch that I'd like
to get in via Russell King's queue.

The way the MTD subsystem / physmap driver handle set_vpp, can the
magician_set_vpp function below race with itself? Or would it be
better to use a counting approach like omap_set_vpp in the omap-nor
driver?

The full patch is here:
http://lists.arm.linux.org.uk/lurker/message/20080317.201751.2fa54cff.en.html

thanks
Philipp

---------- Forwarded message ----------
From: pHilipp Zabel <philipp.zabel at gmail.com>
Date: Fri, Mar 21, 2008 at 12:48 PM
Subject: Re: [PATCH 5/9] magician: enable flash VPP GPIO and build in
MTD, physmap-flash and JFFS2
To: Russell King - ARM Linux <linux at arm.linux.org.uk>
Cc: linux-arm-kernel <linux-arm-kernel at lists.arm.linux.org.uk>

On Thu, Mar 20, 2008 at 10:01 PM, Russell King - ARM Linux
 <linux at arm.linux.org.uk> wrote:
 > On Mon, Mar 17, 2008 at 09:17:51PM +0100, Philipp Zabel wrote:
 >  > This enables rootfs on StrataFlash if the bootloader supplies the
 >  > partition list.
 >
 > > diff --git a/arch/arm/mach-pxa/magician.c b/arch/arm/mach-pxa/magician.c
 >  > index 3a3252e..531be3d 100644
 >  > --- a/arch/arm/mach-pxa/magician.c
 >  > +++ b/arch/arm/mach-pxa/magician.c
 >  > @@ -223,6 +223,15 @@ static struct pxaohci_platform_data
magician_ohci_info = {
 >  >   * StrataFlash
 >  >   */
 >  >
 >  > +static void magician_set_vpp(struct map_info *map, int vpp)
 >  > +{
 >  > +     static int old_vpp;
 >  > +     if (vpp == old_vpp)
 >  > +             return;
 >  > +     gpio_set_value(EGPIO_MAGICIAN_FLASH_VPP, vpp);
 >  > +     old_vpp = vpp;
 >  > +}
 >  > +
 >
 >  Have you checked that this can't race with itself?

 No, I have just seen that set_vpp is called very often in quick
 succession with the same vpp value and wanted to avoid overhead,
 assuming that the mtd subsystem cares about locking.
 I have just seen that omap-nor does something similar but with a
 counter. Maybe this would be better?

 static void omap_set_vpp(struct map_info *map, int enable)
 {
        static int      count;

        if (enable) {
                if (count++ == 0)
                        OMAP_EMIFS_CONFIG_REG |= OMAP_EMIFS_CONFIG_WP;
        } else {
                if (count && (--count == 0))
                        OMAP_EMIFS_CONFIG_REG &= ~OMAP_EMIFS_CONFIG_WP;
        }
 }

 There's also a comment in chips/cfi_cmdset_0001.c:
 /* We should really make set_vpp() count, rather than doing this */

 regards
 Philipp