[U-Boot] [PATCH] usb: musb: add support for Blackfin MUSB

[Remy Bohmer]

Hello Mike,

> ping ... should still apply (or at least git-am should be able to handle it)

Well, this comment at least gave me the impression that you would
update your patch...
> i didnt add that, but adding packed would probably make sense
> -mike

Remy

2009/6/21 Mike Frysinger <vapier at gentoo.org>:
> On Sunday 21 June 2009 15:54:13 Remy Bohmer wrote:
>> Hello Mike,
>>
>> I got a remark about this patch:
>>
>> 2009/6/16 Mike Frysinger <vapier at gentoo.org>:
>> > From: Bryan Wu <bryan.wu at analog.com>
>> >
>> > Signed-off-by: Bryan Wu <bryan.wu at analog.com>
>> > Signed-off-by: Mike Frysinger <vapier at gentoo.org>
>> > ---
>> >  drivers/usb/musb/Makefile                   |    1 +
>> >  drivers/usb/musb/blackfin_usb.c             |  152 +++++++++++++++
>> >  drivers/usb/musb/blackfin_usb.h             |   27 +++
>> >  drivers/usb/musb/musb_core.h                |   73 +++++++
>> >  include/asm-blackfin/mach-common/bits/usb.h |  280
>> > +++++++++++++++++++++++++++ include/usb.h                               |
>> >    3 +-
>> >  6 files changed, 535 insertions(+), 1 deletions(-)
>> >
>> > --- a/drivers/usb/musb/musb_core.h
>> > +++ b/drivers/usb/musb/musb_core.h
>> > @@ -42,6 +42,68 @@
>> >
>> >  /* Mentor USB core register overlay structure */
>> >  struct musb_regs {
>> > +#if defined(CONFIG_USB_BLACKFIN)
>> > +       /* Every register is 32bit aligned, but only 16bits in size */
>> > +# define ureg(name) u16 name; u16 __pad_##name;
>> > +       /* common registers */
>> > +       ureg(faddr)
>> > +       ureg(power)
>> > +       ureg(intrtx)
>> > +       ureg(intrrx)
>> > +       ureg(intrtxe)
>> > +       ureg(intrrxe)
>> > +       ureg(intrusb)
>> > +       ureg(intrusbe)
>> > +       ureg(frame)
>> > +       ureg(index)
>> > +       ureg(testmode)
>> > +       ureg(globintr)
>> > +       ureg(global_ctl)
>> > +       u32     reserved0[3];
>> > +       /* indexed registers */
>> > +       ureg(txmaxp)
>> > +       ureg(txcsr)
>> > +       ureg(rxmaxp)
>> > +       ureg(rxcsr)
>> > +       ureg(rxcount)
>> > +       ureg(txtype)
>> > +       ureg(txinterval)
>> > +       ureg(rxtype)
>> > +       ureg(rxinterval)
>> > +       u32     reserved1;
>> > +       ureg(txcount)
>> > +       u32     reserved2[5];
>> > +       /* fifo */
>> > +       u16     fifox[32];
>> > +       /* OTG, dynamic FIFO, version & vendor registers */
>> > +       u32     reserved3[16];
>> > +       ureg(devctl)
>> > +       ureg(vbus_irq)
>> > +       ureg(vbus_mask)
>> > +       u32 reserved4[15];
>> > +       ureg(linkinfo)
>> > +       ureg(vplen)
>> > +       ureg(hseof1)
>> > +       ureg(fseof1)
>> > +       ureg(lseof1)
>> > +       u32 reserved5[41];
>> > +       /* target address registers */
>> > +       struct musb_tar_regs {
>> > +               ureg(txmaxp)
>> > +               ureg(txcsr)
>> > +               ureg(rxmaxp)
>> > +               ureg(rxcsr)
>> > +               ureg(rxcount)
>> > +               ureg(txtype)
>> > +               ureg(txinternal)
>> > +               ureg(rxtype)
>> > +               ureg(rxinternal)
>> > +               u32     reserved6;
>> > +               ureg(txcount)
>> > +               u32 reserved7[5];
>> > +       } tar[8];
>> > +# undef ureg
>> > +#else
>> >        /* common registers */
>> >        u8      faddr;
>> >        u8      power;
>> > @@ -97,6 +159,7 @@ struct musb_regs {
>> >                u8      rxhubaddr;
>> >                u8      rxhubport;
>> >        } tar[16];
>> > +#endif
>>
>> Can both branches of this ifdef be merged somehow, such that the ifdef
>> is much smaller?
>>
>> Both branches seem to contain the same elements, only type differs (
>> which could maybe be handled with ureg() constructions also)
>
> the layout isnt exactly the same, the sizes arent uniform enough for ureg(),
> and the registers available dont match either.  we'd still have ifdefs, but
> interleaved, and i think that'd be much more likely to lead to breakage for
> architectures invovled.
>
>> >  } __attribute__((aligned(32)));
>>
>> Shouldn't this be '__attribute__((packed))' too?
>
> i didnt add that, but adding packed would probably make sense
> -mike
>