[U-Boot] [PATCH 2/2] include:configs: Add usb device-tree fixup for all fsl platforms

[Marek Vasut <marex@denx.de>]

On Monday, February 08, 2016 at 12:07:29 PM, Ramneek Mehresh wrote:
> > -----Original Message-----
> > From: Ramneek Mehresh
> > Sent: Thursday, January 28, 2016 5:45 PM
> > To: 'Marek Vasut' <marex@denx.de>
> > Cc: Ramneek Mehresh <ramneek.mehresh@freescale.com>; u-
> > boot at lists.denx.de; Simon Glass <sjg@chromium.org>
> > Subject: RE: [PATCH 2/2] include:configs: Add usb device-tree fixup for
> > all fsl platforms
> > 
> > > -----Original Message-----
> > > From: Marek Vasut [mailto:marex at denx.de]
> > > Sent: Thursday, January 28, 2016 5:04 PM
> > > To: Ramneek Mehresh <ramneek.mehresh@nxp.com>
> > > Cc: Ramneek Mehresh <ramneek.mehresh@freescale.com>; u-
> > > boot at lists.denx.de; Simon Glass <sjg@chromium.org>
> > > Subject: Re: [PATCH 2/2] include:configs: Add usb device-tree fixup
> > > for all fsl platforms
> > > 
> > > On Thursday, January 28, 2016 at 12:05:29 PM, Ramneek Mehresh wrote:
> > > > > -----Original Message-----
> > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > Sent: Wednesday, January 27, 2016 5:13 PM
> > > > > To: Ramneek Mehresh <ramneek.mehresh@nxp.com>
> > > > > Cc: Ramneek Mehresh <ramneek.mehresh@freescale.com>; u-
> > > > > boot at lists.denx.de; Simon Glass <sjg@chromium.org>
> > > > > Subject: Re: [PATCH 2/2] include:configs: Add usb device-tree
> > > > > fixup for all fsl platforms
> > > > > 
> > > > > On Wednesday, January 27, 2016 at 12:33:04 PM, Ramneek Mehresh
> > > 
> > > wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > > > Sent: Wednesday, January 27, 2016 1:57 PM
> > > > > > > To: Ramneek Mehresh <ramneek.mehresh@nxp.com>
> > > > > > > Cc: Ramneek Mehresh <ramneek.mehresh@freescale.com>; u-
> > > > > > > boot at lists.denx.de; Simon Glass <sjg@chromium.org>
> > > > > > > Subject: Re: [PATCH 2/2] include:configs: Add usb device-tree
> > > > > > > fixup for all fsl platforms
> > > > > > > 
> > > > > > > On Wednesday, January 27, 2016 at 09:26:08 AM, Ramneek
> > 
> > Mehresh
> > 
> > > > > wrote:
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > > > > > Sent: Wednesday, January 27, 2016 1:05 PM
> > > > > > > > > To: Ramneek Mehresh <ramneek.mehresh@nxp.com>
> > > > > > > > > Cc: Ramneek Mehresh <ramneek.mehresh@freescale.com>; u-
> > > > > > > > > boot at lists.denx.de; Simon Glass <sjg@chromium.org>
> > > > > > > > > Subject: Re: [PATCH 2/2] include:configs: Add usb
> > > > > > > > > device-tree fixup for all fsl platforms
> > > > > > > > > 
> > > > > > > > > On Wednesday, January 27, 2016 at 05:30:51 AM, Ramneek
> > > > > > > > > Mehresh
> > > > > > > 
> > > > > > > wrote:
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > > > > > > > Sent: Wednesday, January 27, 2016 9:57 AM
> > > > > > > > > > > To: Ramneek Mehresh <ramneek.mehresh@nxp.com>
> > > > > > > > > > > Cc: Ramneek Mehresh
> > 
> > <ramneek.mehresh@freescale.com>;
> > 
> > > u-
> > > 
> > > > > > > > > > > boot at lists.denx.de; Simon Glass <sjg@chromium.org>
> > > > > > > > > > > Subject: Re: [PATCH 2/2] include:configs: Add usb
> > > > > > > > > > > device-tree fixup for all fsl platforms
> > > > > > > > > > > 
> > > > > > > > > > > On Wednesday, January 27, 2016 at 05:14:00 AM, Ramneek
> > > > > 
> > > > > Mehresh
> > > > > 
> > > > > > > > > wrote:
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Marek Vasut [mailto:marex at denx.de]
> > > > > > > > > > > > > Sent: Tuesday, January 26, 2016 4:58 PM
> > > > > > > > > > > > > To: Ramneek Mehresh
> > > 
> > > <ramneek.mehresh@freescale.com>
> > > 
> > > > > > > > > > > > > Cc: u-boot at lists.denx.de
> > > > > > > > > > > > > Subject: Re: [PATCH 2/2] include:configs: Add usb
> > > > > > > > > > > > > device-tree fixup for all fsl platforms
> > > > > > > > > > > > > 
> > > > > > > > > > > > > On Tuesday, January 26, 2016 at 12:36:58 PM,
> > > > > > > > > > > > > Ramneek
> > > > > 
> > > > > Mehresh
> > > > > 
> > > > > > > > > wrote:
> > > > > > > > > > > > > > Add usb device-tree fixup for all relevant fsl
> > > > > > > > > > > > > > ppc and arm platforms
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Ramneek Mehresh
> > > > > > > > > > > 
> > > > > > > > > > > <ramneek.mehresh@freescale.com>
> > > > > > > > > > > 
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >  board/freescale/b4860qds/b4860qds.c         | 2
> > > > > > > > > > > > > >  +- board/freescale/bsc9131rdb/bsc9131rdb.c    
> > > > > > > > > > > > > >  | 2 ++ board/freescale/bsc9132qds/bsc9132qds.c 
> > > > > > > > > > > > > >     | 2 ++
> > > > > > > > > > > > > >  board/freescale/corenet_ds/corenet_ds.c     | 4
> > > > > > > > > > > > > >  ++++ board/freescale/ls2080aqds/ls2080aqds.c   
> > > > > > > > > > > > > >   | 4 ++++
> > > > > > > > > > > > > >  board/freescale/ls2080ardb/ls2080ardb.c     | 4
> > > > > > > > > > > > > >  ++++ board/freescale/mpc8308rdb/mpc8308rdb.c   
> > > > > > > > > > > > > >   | 4
> > 
> > ++++
> > 
> > > > > > > > > > > > > >  board/freescale/mpc8315erdb/mpc8315erdb.c   | 2
> > > > > > > > > > > > > >  ++ board/freescale/mpc837xemds/mpc837xemds.c  
> > > > > > > > > > > > > >  | 2
> > 
> > ++
> > 
> > > > > > > > > > > > > >  board/freescale/mpc837xerdb/mpc837xerdb.c   | 2
> > > > > > > > > > > > > >  ++ board/freescale/mpc8536ds/mpc8536ds.c      
> > > > > > > > > > > > > >  | 2 +- board/freescale/p1010rdb/p1010rdb.c     
> > > > > > > > > > > > > >     | 2 +- board/freescale/p1022ds/p1022ds.c    
> > > > > > > > > > > > > >        | 2 +-
> > > > > > > > > > > > > >  board/freescale/p1023rdb/p1023rdb.c         | 2
> > > > > > > > > > > > > >  +- board/freescale/p1_p2_rdb_pc/p1_p2_rdb_pc.c
> > > > > > > > > > > > > >  | 2 +- board/freescale/p1_twr/p1_twr.c         
> > > > > > > > > > > > > >     | 3 +++ board/freescale/p2041rdb/p2041rdb.c 
> > > > > > > > > > > > > >         | 2 +-
> > > > > > > > > > > > > >  board/freescale/t102xqds/t102xqds.c         | 2
> > > > > > > > > > > > > >  +- board/freescale/t102xrdb/t102xrdb.c        
> > > > > > > > > > > > > >  | 3 +++ board/freescale/t1040qds/t1040qds.c    
> > > > > > > > > > > > > >      | 2 +- board/freescale/t104xrdb/t104xrdb.c 
> > > > > > > > > > > > > >         | 2 +-
> > > > > > > > > > > > > >  board/freescale/t208xqds/t208xqds.c         | 3
> > > > > > > > > > > > > >  +++ board/freescale/t208xrdb/t208xrdb.c        
> > > > > > > > > > > > > >  | 3 +++ board/freescale/t4qds/t4240emu.c       
> > > > > > > > > > > > > >      | 3 +++ board/freescale/t4qds/t4240qds.c   
> > > > > > > > > > > > > >          | 3 +++
> > > > > > > > > > > > > >  board/freescale/t4rdb/t4240rdb.c            | 3
> > > > > > > > > > > > > >  +++ include/configs/B4860QDS.h                 
> > > > > > > > > > > > > >  | 1 + include/configs/BSC9131RDB.h             
> > > > > > > > > > > > > >    | 1 + include/configs/BSC9132QDS.h           
> > > > > > > > > > > > > >      | 3 ++- include/configs/MPC8308RDB.h       
> > > > > > > > > > > > > >          | 3 +++ include/configs/MPC8315ERDB.h  
> > > > > > > > > > > > > >              | 1 + include/configs/MPC837XEMDS.h
> > > > > > > > > > > > > >                | 3 ++-
> > > > > > > > > > > > > >  include/configs/MPC837XERDB.h               | 1
> > > > > > > > > > > > > >  + include/configs/MPC8536DS.h                 |
> > > > > > > > > > > > > >  1 + include/configs/P1010RDB.h                 
> > > > > > > > > > > > > >  | 1 + include/configs/P1022DS.h                
> > > > > > > > > > > > > >    | 1 + include/configs/P1023RDB.h             
> > > > > > > > > > > > > >      | 1 + include/configs/P2041RDB.h           
> > > > > > > > > > > > > >        | 1 + include/configs/T102xQDS.h         
> > > > > > > > > > > > > >          | 1 + include/configs/T102xRDB.h       
> > > > > > > > > > > > > >            | 1 + include/configs/T1040QDS.h     
> > > > > > > > > > > > > >              | 1 + include/configs/T104xRDB.h   
> > > > > > > > > > > > > >                | 1 + include/configs/T208xQDS.h 
> > > > > > > > > > > > > >                  | 1 +
> > > > > > > > > > > > > >  include/configs/T208xRDB.h                  | 1
> > > > > > > > > > > > > >  + include/configs/T4240QDS.h                  |
> > > > > > > > > > > > > >  1 + include/configs/corenet_ds.h               
> > > > > > > > > > > > > >  | 1 + include/configs/ls2080aqds.h             
> > > > > > > > > > > > > >    | 1 + include/configs/ls2080ardb.h           
> > > > > > > > > > > > > >      | 1 + include/configs/p1_p2_rdb_pc.h       
> > > > > > > > > > > > > >        | 1 + include/configs/p1_twr.h           
> > > > > > > > > > > > > >          | 1 + 50 files changed, 85
> > > > > > > > > > > > > >  insertions(+), 12
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Each such new macro must be documented. What is
> > > > > > > > > > > > > the point
> > > > > 
> > > > > of
> > > > > 
> > > > > > > > > > > > > this bulk rename anyway?
> > > > > > > > > > > > 
> > > > > > > > > > > > Yes, I'll document the new MACRO defined for usb
> > > > > > > > > > > > device-tree
> > > > > > > 
> > > > > > > fixup.
> > > > > > > 
> > > > > > > > > > > > However, this is not bulk rename. I just modified
> > > > > > > > > > > > all these files to include usb device-tree fixup for
> > > > > > > > > > > > all fsl ppc platforms. Most of these platforms were
> > > > > > > > > > > > already using this mechanism (some via different
> > > > > > > > > > > > ways),
> > > > > > > > > > > 
> > > > > > > > > > > but
> > > > > > > > > > > 
> > > > > > > > > > > > now its consistent across them.
> > > > > > > > > > > 
> > > > > > > > > > > Wouldn't it make more sense to make this generic then
> > > > > > > > > > > instead of patching zillion files ?
> > > > > > > > > > 
> > > > > > > > > > The patch set is to make this support generic, but I do
> > > > > > > > > > need to make these platforms use this generic support in
> > > > > > > > > > consistent way via single macro inclusion in their
> > > > > > > > > > configs...
> > > > > > > > > 
> > > > > > > > > You can also just modify the function itself instead of
> > > > > > > > > million board files. Adding ifdef-endif combo all around
> > > > > > > > > the place is just not gonna work, sorry.
> > > > > > > > > 
> > > > > > > > > Also, the macro should probably contain _OF_ instead of
> > > > > > > > > DEVTREE ...
> > > > > > > > 
> > > > > > > > Ok, in this case, I'll use the already defined macros used
> > > > > > > > by these platforms. However, this approach will not make
> > > > > > > > sure that all these platforms are using this feature in a
> > > > > > > > consistent/similar
> > > 
> > > way.
> > > 
> > > > > > > Why not ?
> > > > > > 
> > > > > > All the platforms files are calling fdt_fixup_dr_usb() under
> > > > > > following
> > > > > > conditions: 1. no macro usage, direct call 2. Using
> > > > > > CONFIG_HAS_FSL_DR_USB macro (for socs having DR controller) 2.
> > > > > > Using CONFIG_HAS_FSL_MPH_USB (for socs having MPH controller)
> > 
> > 3.
> > 
> > > > > > Using CONFIG_HAS_FSL_XHCI_USB (for socs having XHCI controller)
> > > > > > 
> > > > > > This is why I wanted a single macro (CONFIG_USB_DEVTREE_FIXUP)
> > > > > > for invocation of fdt_fixup_dr_usb() from all platforms. One
> > > > > > macro --> one feature
> > > > > 
> > > > > That'd be fine, but you're adding ifdefs all around the place and
> > > > > that is not fine. You can solve this without ifdefs too, for
> > > > > example by having a __weak empty version of the function where
> > 
> > applicable.
> > 
> > > > Ok, so you're suggesting me to have multiple definitions of this
> > > > function...the function Itself being  __weak...one for ehci and
> > > > another for xhci. In this case, we'll be having problem with boards
> > > > of an soc having both ehci and xhci controller...then how do we
> > > > handle that situation?
> > > 
> > > I only see one implementation of fdt_fixup_dr_usb(blob, bd); being
> > > invoked throughout this patch. Where are these "multiple"
> > 
> > implementations ?
> > The only implementation right now was in drivers/usb/host/ehci-fsl.c
> > which I moved to boards/freescale/common/usb.c This was done so as not
> > to have another definition of this function in
> > drivers/usb/host/xhci-fsl.c.
> 
> Hi Marek...a kind reminder for the above discussion...please let me know if
> you're ok with the above patch or shall I resend the patch with #defines
> removed. Please suggest.

The later, patch which does not add billion #defines .