Re: [RFC v2 3/8] ci13xxx_udc: rename register layouts

[Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>]

On Thu, 12 Apr 2012 17:02:25 +0300, Felipe Balbi <balbi@xxxxxx> wrote:
> Hi,
> 
> On Thu, Apr 12, 2012 at 04:54:01PM +0300, Alexander Shishkin wrote:
> > On Wed, 11 Apr 2012 15:16:03 +0300, Felipe Balbi <balbi@xxxxxx> wrote:
> > > On Wed, Apr 11, 2012 at 03:14:12PM +0300, Alexander Shishkin wrote:
> > > > On Wed, 11 Apr 2012 14:37:35 +0300, Felipe Balbi <balbi@xxxxxx> wrote:
> > > > > On Wed, Apr 11, 2012 at 01:52:31PM +0300, Alexander Shishkin wrote:
> > > > > > On Wed, 11 Apr 2012 12:54:41 +0300, Felipe Balbi <balbi@xxxxxx> wrote:
> > > > > > > why do you need separate functions to read capability or operational
> > > > > > > registers ? They look the same. You could just define them a little
> > > > > > > differently:
> > > > > > > 
> > > > > > > static u32 hw_read(void *base, u32 offset, u32 mask)
> > > > > > > {
> > > > > > > 	return ioread32(base + offset) & mask;
> > > > > > > }
> > > > > > > 
> > > > > > > then, when calling it you can use:
> > > > > > > 
> > > > > > > hw_read(hw_bank.op, ADDRESS, mask);
> > > > > > > hw_read(hw_bank.cap, ADDRESS, mask);
> > > > > > 
> > > > > > That's what I've done in the "redo register access" patch. The reason I
> > > > > > didn't do this here is that I wanted to change one thing at a time. It
> > > > > > might make sense to squash these patches together, too.
> > > > > 
> > > > > that patch is different. You use some extra trickery to try and guess
> > > > > which base to use (operational or capability) whereas you could just
> > > > > pass that as an argument.
> > > > 
> > > > Yes, I really wanted to hide all that under the hood and let the hw_*
> > > > functions deal with the register banks, it looks better to me, but I
> > > > will change it to what you're suggesting, if you think it's better like
> > > > that.
> > > 
> > > to me it looks better than trying to guess which base to use. I prefer
> > > explicitly passing the base address as argument.
> > 
> > Actually, it's not so much *guessing*, we actually know which registers
> > are where. Another thing is, I still want to use lookup tables as in
> > "redo register access" to eliminate the ugly register definitions like
> > 
> > #define REGISTER1  (some_global_var.lpm ? 0x10 : 0x20)
> > 
> > and I couldn't come up with any better way for doing that.
> 
> struct my_struct {
> 	unsigned	lpm:1;
> };
> 
> ...
> 
> static u32 hw_read(struct my_struct *ptr, void __iomem *base,
> 	u32 register, u32 mask)
> {
> 	u32 reg = register;
> 
> 	if (ptr->lpm)
> 		reg += 0x10;
> 
> 	return readl(base + reg) & mask;
> }
> 
> or something similar. Another way could be:
> 
> struct my_struct {
> 	unsigned int	extra_offset;
> };
> 
> static u32 hw_read(struct my_struct *ptr, void __iomem *base,
> 	u32 register, u32 mask)
> {
> 	return readl(base + reg + ptr->extra_offset) & mask;
> }
> 
> ...
> 
> static int __devinit probe(struct platform_device *pdev)
> {
> 	...
> 
> 	if (lpm)
> 		ptr->extra_offset = 0x10;
> 	else
> 		ptr->extra_offset = 0x00;
> 
> 	...
> 
> 	return 0;
> }
> 
> which avoids extra branches on every read/write :-p

That would work if the extra_offset wan't randomly different for each
register. Unfortunately, what we have is:

#define OP_USBMODE          (hw_bank.lpm ? 0x0C8UL : 0x068UL)
#define OP_ENDPTSETUPSTAT   (hw_bank.lpm ? 0x0D8UL : 0x06CUL)
#define OP_ENDPTPRIME	    (hw_bank.lpm ? 0x0DCUL : 0x070UL)
#define OP_ENDPTFLUSH	    (hw_bank.lpm ? 0x0E0UL : 0x074UL)
#define OP_ENDPTSTAT	    (hw_bank.lpm ? 0x0E4UL : 0x078UL)
#define OP_ENDPTCOMPLETE    (hw_bank.lpm ? 0x0E8UL : 0x07CUL)
#define OP_ENDPTCTRL	    (hw_bank.lpm ? 0x0ECUL : 0x080UL)
#define OP_LAST		    (hw_bank.lpm ? 0x12CUL : 0x0C0UL)

and I suspect we will find even more bizzare differences in offsets if
we look at more different chipidea revisions. So, this is not an
option.

Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html