From mboxrd@z Thu Jan  1 00:00:00 1970
From: marek.vasut@gmail.com (Marek Vasut)
Date: Wed, 3 Nov 2010 17:48:32 +0100
Subject: [PATCH 02/17] ARM: pxa: Access SMEMC via virtual addresses
In-Reply-To: <20101103164023.GB11751@n2100.arm.linux.org.uk>
References: <1288741914-21328-1-git-send-email-marek.vasut@gmail.com>
	<201011031630.03658.marek.vasut@gmail.com>
	<20101103164023.GB11751@n2100.arm.linux.org.uk>
Message-ID: <201011031748.32291.marek.vasut@gmail.com>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

On Wednesday 03 November 2010 17:40:23 Russell King - ARM Linux wrote:
> On Wed, Nov 03, 2010 at 04:30:03PM +0100, Marek Vasut wrote:
> > > >        /* Fix timings for dm9000s (CS1/CS2)*/
> > > > 
> > > > -       MSC0 = (MSC0 & 0xffff) | (dm9000_msc << 16);
> > > > -       MSC1 = (MSC1 & 0xffff0000) | dm9000_msc;
> > > > +       __raw_writel((MSC0 & 0xffff) | (dm9000_msc << 16), MSC0);
> > > > +       __raw_writel((MSC1 & 0xffff0000) | dm9000_msc, MSC1);
> > > 
> > > This isn't correct.
> > 
> > I don't see the difference (well, besides that this should be adjusted by
> > bootloader).
> 
> Eric is quite right - the above is not a correct conversion - it's a
> functional change.
> 
> 	MSC0 = (MSC0 & 0xffff) | (dm9000_msc << 16);
> 
> Reads the MSC0 register, modifies its value, and writes it back.
> 
> 	__raw_writel((MSC0 & 0xffff) | (dm9000_msc << 16), MSC0);
> 
> Uses the MSC0 register address, modifies that address value, and then
> writes it to the MSC0 register.

AAH!

Stupid and blind me !
> 
> > > > @@ -205,19 +218,18 @@ pxa2xx_pcmcia_frequency_change(struct
> > > > soc_pcmcia_socket *skt, static void pxa2xx_configure_sockets(struct
> > > > device *dev)
> > > > 
> > > >  {
> > > >  
> > > >        struct pcmcia_low_level *ops = dev->platform_data;
> > > > 
> > > > -
> > > > 
> > > >        /*
> > > >        
> > > >         * We have at least one socket, so set MECR:CIT
> > > >         * (Card Is There)
> > > >         */
> > > > 
> > > > -       MECR |= MECR_CIT;
> > > > +       uint32_t mecr = MECR_CIT;
> > > > 
> > > >        /* Set MECR:NOS (Number Of Sockets) */
> > > >        if ((ops->first + ops->nr) > 1 ||
> > > >        
> > > >            machine_is_viper() || machine_is_arcom_zeus())
> > > > 
> > > > -               MECR |= MECR_NOS;
> > > > -       else
> > > > -               MECR &= ~MECR_NOS;
> > > > +               mecr |= MECR_NOS;
> > > > +
> > > > +       __raw_writel(mecr, MECR);
> > > 
> > > This looks to be a bit inconsistent with the original code?
> 
> No comment for this - and I agree with Eric.  This again is not just a
> conversion from having MECR accessing the register, to using __raw_readl
> and __raw_writel.  It always forces MECR_CIT to be set - which may not
> be what was there originally.
> 
> It's another functional change.

You're right, sorry.