| Message ID | 1518603031-416-1-git-send-email-faiz_abbas@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [U-Boot,RFC] usb: host: xhci-omap: Remove redundant board_usb_init and board_usb_cleanup functions | expand |
On 02/14/2018 11:10 AM, Faiz Abbas wrote: > board_usb_init()/_cleanup() should be in board files and don't have > a place in the xhci-omap driver. Weak versions for > board_usb_init()/_cleanup() already exist in common/usb.c > (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> Reviewed-by: Marek Vasut <marex@denx.de> I'd like some TBs from the people using those boards. > --- > Since 1a9a5f7 ("fix double weak board_usb_init functions") the > board_usb_init()/_cleanup() a prefix omap_xhci_* was added to the board > files and in omap-xhci.c because there was a duplicate implementation > in common/usb.c. However, this broke the gadget mode path which uses the > same board_usb_init/cleanup() in the board specific files. > > I think a better way would be to just remove the functions from xhci-omap > like in this patch. Any issues with this? > > board/ti/am43xx/board.c | 4 ++-- > board/ti/am57xx/board.c | 4 ++-- > board/ti/dra7xx/evm.c | 4 ++-- > drivers/usb/host/xhci-omap.c | 22 ---------------------- > 4 files changed, 6 insertions(+), 28 deletions(-) > > diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c > index 06082f1..6286af4 100644 > --- a/board/ti/am43xx/board.c > +++ b/board/ti/am43xx/board.c > @@ -744,7 +744,7 @@ int usb_gadget_handle_interrupts(int index) > #endif /* CONFIG_USB_DWC3 */ > > #if defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_XHCI_OMAP) > -int omap_xhci_board_usb_init(int index, enum usb_init_type init) > +int board_usb_init(int index, enum usb_init_type init) > { > enable_usb_clocks(index); > #ifdef CONFIG_USB_DWC3 > @@ -775,7 +775,7 @@ int omap_xhci_board_usb_init(int index, enum usb_init_type init) > return 0; > } > > -int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) > +int board_usb_cleanup(int index, enum usb_init_type init) > { > #ifdef CONFIG_USB_DWC3 > switch (index) { > diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c > index 1128784..c3f60f6 100644 > --- a/board/ti/am57xx/board.c > +++ b/board/ti/am57xx/board.c > @@ -867,7 +867,7 @@ int usb_gadget_handle_interrupts(int index) > #endif /* CONFIG_USB_DWC3 */ > > #if defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_XHCI_OMAP) > -int omap_xhci_board_usb_init(int index, enum usb_init_type init) > +int board_usb_init(int index, enum usb_init_type init) > { > enable_usb_clocks(index); > switch (index) { > @@ -901,7 +901,7 @@ int omap_xhci_board_usb_init(int index, enum usb_init_type init) > return 0; > } > > -int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) > +int board_usb_cleanup(int index, enum usb_init_type init) > { > #ifdef CONFIG_USB_DWC3 > switch (index) { > diff --git a/board/ti/dra7xx/evm.c b/board/ti/dra7xx/evm.c > index 6ecf971..519475e 100644 > --- a/board/ti/dra7xx/evm.c > +++ b/board/ti/dra7xx/evm.c > @@ -907,7 +907,7 @@ static struct ti_usb_phy_device usb_phy2_device = { > .index = 1, > }; > > -int omap_xhci_board_usb_init(int index, enum usb_init_type init) > +int board_usb_init(int index, enum usb_init_type init) > { > enable_usb_clocks(index); > switch (index) { > @@ -944,7 +944,7 @@ int omap_xhci_board_usb_init(int index, enum usb_init_type init) > return 0; > } > > -int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) > +int board_usb_cleanup(int index, enum usb_init_type init) > { > switch (index) { > case 0: > diff --git a/drivers/usb/host/xhci-omap.c b/drivers/usb/host/xhci-omap.c > index d6c5744..b814500 100644 > --- a/drivers/usb/host/xhci-omap.c > +++ b/drivers/usb/host/xhci-omap.c > @@ -27,28 +27,6 @@ DECLARE_GLOBAL_DATA_PTR; > > static struct omap_xhci omap; > > -__weak int omap_xhci_board_usb_init(int index, enum usb_init_type init) > -{ > - enable_usb_clocks(index); > - return 0; > -} > - > -int board_usb_init(int index, enum usb_init_type init) > -{ > - return omap_xhci_board_usb_init(index, init); > -} > - > -__weak int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) > -{ > - disable_usb_clocks(index); > - return 0; > -} > - > -int board_usb_cleanup(int index, enum usb_init_type init) > -{ > - return omap_xhci_board_usb_cleanup(index, init); > -} > - > static int omap_xhci_core_init(struct omap_xhci *omap) > { > int ret = 0; >
Hi, On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: > On 02/14/2018 11:10 AM, Faiz Abbas wrote: >> board_usb_init()/_cleanup() should be in board files and don't have >> a place in the xhci-omap driver. Weak versions for >> board_usb_init()/_cleanup() already exist in common/usb.c >> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > > Reviewed-by: Marek Vasut <marex@denx.de> > > I'd like some TBs from the people using those boards. I have tested this for dra7xx, am43xx and am57xx. Thanks, Faiz
On 02/14/2018 12:20 PM, Faiz Abbas wrote: > Hi, > > On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: >> On 02/14/2018 11:10 AM, Faiz Abbas wrote: >>> board_usb_init()/_cleanup() should be in board files and don't have >>> a place in the xhci-omap driver. Weak versions for >>> board_usb_init()/_cleanup() already exist in common/usb.c >>> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >>> >>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >> >> Reviewed-by: Marek Vasut <marex@denx.de> >> >> I'd like some TBs from the people using those boards. > > I have tested this for dra7xx, am43xx and am57xx. So why is it marked RFC ?
On Wed, Feb 14, 2018 at 6:10 PM, Faiz Abbas <faiz_abbas@ti.com> wrote: > board_usb_init()/_cleanup() should be in board files and don't have > a place in the xhci-omap driver. Weak versions for > board_usb_init()/_cleanup() already exist in common/usb.c > (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > --- > Since 1a9a5f7 ("fix double weak board_usb_init functions") the > board_usb_init()/_cleanup() a prefix omap_xhci_* was added to the board > files and in omap-xhci.c because there was a duplicate implementation > in common/usb.c. However, this broke the gadget mode path which uses the > same board_usb_init/cleanup() in the board specific files. > > I think a better way would be to just remove the functions from xhci-omap > like in this patch. Any issues with this? > > board/ti/am43xx/board.c | 4 ++-- > board/ti/am57xx/board.c | 4 ++-- > board/ti/dra7xx/evm.c | 4 ++-- > drivers/usb/host/xhci-omap.c | 22 ---------------------- > 4 files changed, 6 insertions(+), 28 deletions(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Hi, On Wednesday 14 February 2018 06:53 PM, Marek Vasut wrote: > On 02/14/2018 12:20 PM, Faiz Abbas wrote: >> Hi, >> >> On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: >>> On 02/14/2018 11:10 AM, Faiz Abbas wrote: >>>> board_usb_init()/_cleanup() should be in board files and don't have >>>> a place in the xhci-omap driver. Weak versions for >>>> board_usb_init()/_cleanup() already exist in common/usb.c >>>> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >>>> >>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>> >>> Reviewed-by: Marek Vasut <marex@denx.de> >>> >>> I'd like some TBs from the people using those boards. >> >> I have tested this for dra7xx, am43xx and am57xx. > > So why is it marked RFC ? > Because I was unclear why Uri Mashiach did not do this in 1a9a5f7 ("fix double weak board_usb_init functions"). Thanks, Faiz
On 02/14/2018 03:14 PM, Faiz Abbas wrote: > Hi, > > On Wednesday 14 February 2018 06:53 PM, Marek Vasut wrote: >> On 02/14/2018 12:20 PM, Faiz Abbas wrote: >>> Hi, >>> >>> On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: >>>> On 02/14/2018 11:10 AM, Faiz Abbas wrote: >>>>> board_usb_init()/_cleanup() should be in board files and don't have >>>>> a place in the xhci-omap driver. Weak versions for >>>>> board_usb_init()/_cleanup() already exist in common/usb.c >>>>> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >>>>> >>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>> >>>> Reviewed-by: Marek Vasut <marex@denx.de> >>>> >>>> I'd like some TBs from the people using those boards. >>> >>> I have tested this for dra7xx, am43xx and am57xx. >> >> So why is it marked RFC ? >> > > Because I was unclear why Uri Mashiach did not do this in 1a9a5f7 ("fix > double weak board_usb_init functions"). OK, submit it as a normal patch with my RB.
Hi, Sorry for the late response. On 02/14/2018 04:19 PM, Marek Vasut wrote: > On 02/14/2018 03:14 PM, Faiz Abbas wrote: >> Hi, >> >> On Wednesday 14 February 2018 06:53 PM, Marek Vasut wrote: >>> On 02/14/2018 12:20 PM, Faiz Abbas wrote: >>>> Hi, >>>> >>>> On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: >>>>> On 02/14/2018 11:10 AM, Faiz Abbas wrote: >>>>>> board_usb_init()/_cleanup() should be in board files and don't have >>>>>> a place in the xhci-omap driver. Weak versions for >>>>>> board_usb_init()/_cleanup() already exist in common/usb.c >>>>>> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >>>>>> >>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>>> >>>>> Reviewed-by: Marek Vasut <marex@denx.de> >>>>> >>>>> I'd like some TBs from the people using those boards. >>>> >>>> I have tested this for dra7xx, am43xx and am57xx. >>> >>> So why is it marked RFC ? >>> >> >> Because I was unclear why Uri Mashiach did not do this in 1a9a5f7 ("fix >> double weak board_usb_init functions"). > > OK, submit it as a normal patch with my RB. > As mentioned in 1a9a5f7, the functions omap_xhci_board_usb_init() and omap_xhci_board_usb_cleanup() are called for the boards with the CONFIG_USB_XHCI_OMAP definition. The weak implementation of the functions omap_xhci_board_usb_init() executed enable_usb_clocks(). The weak implementation of the function omap_xhci_board_usb_cleanup() executed the function disable_usb_clocks(). For the boards compulab/cl-som-am57x and compulab/cm_t43: * CONFIG_USB_XHCI_OMAP is defined * omap_xhci_board_usb_init is not implemented, relying on the weak implementation. * omap_xhci_board_usb_cleanup is not defined, relying on the weak implementation. The fix is missing the implementation of board_usb_init and board_usb_cleanup in the compulab/cl-som-am57x and compulab/cm_t43. The implementation should include the content of the deleted weak functions omap_xhci_board_usb_init() and omap_xhci_board_usb_cleanup().
Hi Uri, On Wednesday 14 February 2018 08:56 PM, Uri Mashiach wrote: > Hi, > Sorry for the late response. > > On 02/14/2018 04:19 PM, Marek Vasut wrote: >> On 02/14/2018 03:14 PM, Faiz Abbas wrote: >>> Hi, >>> >>> On Wednesday 14 February 2018 06:53 PM, Marek Vasut wrote: >>>> On 02/14/2018 12:20 PM, Faiz Abbas wrote: >>>>> Hi, >>>>> >>>>> On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: >>>>>> On 02/14/2018 11:10 AM, Faiz Abbas wrote: >>>>>>> board_usb_init()/_cleanup() should be in board files and don't have >>>>>>> a place in the xhci-omap driver. Weak versions for >>>>>>> board_usb_init()/_cleanup() already exist in common/usb.c >>>>>>> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >>>>>>> >>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>>>> >>>>>> Reviewed-by: Marek Vasut <marex@denx.de> >>>>>> >>>>>> I'd like some TBs from the people using those boards. >>>>> >>>>> I have tested this for dra7xx, am43xx and am57xx. >>>> >>>> So why is it marked RFC ? >>>> >>> >>> Because I was unclear why Uri Mashiach did not do this in 1a9a5f7 ("fix >>> double weak board_usb_init functions"). >> >> OK, submit it as a normal patch with my RB. >> > > As mentioned in 1a9a5f7, the functions omap_xhci_board_usb_init() and > omap_xhci_board_usb_cleanup() are called for the boards with the > CONFIG_USB_XHCI_OMAP definition. > > The weak implementation of the functions omap_xhci_board_usb_init() > executed enable_usb_clocks(). > The weak implementation of the function omap_xhci_board_usb_cleanup() > executed the function disable_usb_clocks(). > > For the boards compulab/cl-som-am57x and compulab/cm_t43: > * CONFIG_USB_XHCI_OMAP is defined > * omap_xhci_board_usb_init is not implemented, relying on the weak > implementation. > * omap_xhci_board_usb_cleanup is not defined, relying on the weak > implementation. > > The fix is missing the implementation of board_usb_init and > board_usb_cleanup in the compulab/cl-som-am57x and compulab/cm_t43. > The implementation should include the content of the deleted weak > functions omap_xhci_board_usb_init() and omap_xhci_board_usb_cleanup(). > Thanks for clarifying. Shouldn't that be implemented in the board files? I can add board_usb_init and board_usb_cleanup for those platforms in v2. Can you help me test this on those platforms? Thanks, Faiz
Hi Faiz, On 02/14/2018 05:47 PM, Faiz Abbas wrote: > Hi Uri, > > On Wednesday 14 February 2018 08:56 PM, Uri Mashiach wrote: >> Hi, >> Sorry for the late response. >> >> On 02/14/2018 04:19 PM, Marek Vasut wrote: >>> On 02/14/2018 03:14 PM, Faiz Abbas wrote: >>>> Hi, >>>> >>>> On Wednesday 14 February 2018 06:53 PM, Marek Vasut wrote: >>>>> On 02/14/2018 12:20 PM, Faiz Abbas wrote: >>>>>> Hi, >>>>>> >>>>>> On Wednesday 14 February 2018 03:46 PM, Marek Vasut wrote: >>>>>>> On 02/14/2018 11:10 AM, Faiz Abbas wrote: >>>>>>>> board_usb_init()/_cleanup() should be in board files and don't have >>>>>>>> a place in the xhci-omap driver. Weak versions for >>>>>>>> board_usb_init()/_cleanup() already exist in common/usb.c >>>>>>>> (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). >>>>>>>> >>>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>>>>> >>>>>>> Reviewed-by: Marek Vasut <marex@denx.de> >>>>>>> >>>>>>> I'd like some TBs from the people using those boards. >>>>>> >>>>>> I have tested this for dra7xx, am43xx and am57xx. >>>>> >>>>> So why is it marked RFC ? >>>>> >>>> >>>> Because I was unclear why Uri Mashiach did not do this in 1a9a5f7 ("fix >>>> double weak board_usb_init functions"). >>> >>> OK, submit it as a normal patch with my RB. >>> >> >> As mentioned in 1a9a5f7, the functions omap_xhci_board_usb_init() and >> omap_xhci_board_usb_cleanup() are called for the boards with the >> CONFIG_USB_XHCI_OMAP definition. >> >> The weak implementation of the functions omap_xhci_board_usb_init() >> executed enable_usb_clocks(). >> The weak implementation of the function omap_xhci_board_usb_cleanup() >> executed the function disable_usb_clocks(). >> >> For the boards compulab/cl-som-am57x and compulab/cm_t43: >> * CONFIG_USB_XHCI_OMAP is defined >> * omap_xhci_board_usb_init is not implemented, relying on the weak >> implementation. >> * omap_xhci_board_usb_cleanup is not defined, relying on the weak >> implementation. >> >> The fix is missing the implementation of board_usb_init and >> board_usb_cleanup in the compulab/cl-som-am57x and compulab/cm_t43. >> The implementation should include the content of the deleted weak >> functions omap_xhci_board_usb_init() and omap_xhci_board_usb_cleanup(). >> > > Thanks for clarifying. Shouldn't that be implemented in the board files? Yes > I can add board_usb_init and board_usb_cleanup for those platforms in > v2. Can you help me test this on those platforms? Yes > > Thanks, > Faiz >
diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c index 06082f1..6286af4 100644 --- a/board/ti/am43xx/board.c +++ b/board/ti/am43xx/board.c @@ -744,7 +744,7 @@ int usb_gadget_handle_interrupts(int index) #endif /* CONFIG_USB_DWC3 */ #if defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_XHCI_OMAP) -int omap_xhci_board_usb_init(int index, enum usb_init_type init) +int board_usb_init(int index, enum usb_init_type init) { enable_usb_clocks(index); #ifdef CONFIG_USB_DWC3 @@ -775,7 +775,7 @@ int omap_xhci_board_usb_init(int index, enum usb_init_type init) return 0; } -int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) +int board_usb_cleanup(int index, enum usb_init_type init) { #ifdef CONFIG_USB_DWC3 switch (index) { diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c index 1128784..c3f60f6 100644 --- a/board/ti/am57xx/board.c +++ b/board/ti/am57xx/board.c @@ -867,7 +867,7 @@ int usb_gadget_handle_interrupts(int index) #endif /* CONFIG_USB_DWC3 */ #if defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_XHCI_OMAP) -int omap_xhci_board_usb_init(int index, enum usb_init_type init) +int board_usb_init(int index, enum usb_init_type init) { enable_usb_clocks(index); switch (index) { @@ -901,7 +901,7 @@ int omap_xhci_board_usb_init(int index, enum usb_init_type init) return 0; } -int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) +int board_usb_cleanup(int index, enum usb_init_type init) { #ifdef CONFIG_USB_DWC3 switch (index) { diff --git a/board/ti/dra7xx/evm.c b/board/ti/dra7xx/evm.c index 6ecf971..519475e 100644 --- a/board/ti/dra7xx/evm.c +++ b/board/ti/dra7xx/evm.c @@ -907,7 +907,7 @@ static struct ti_usb_phy_device usb_phy2_device = { .index = 1, }; -int omap_xhci_board_usb_init(int index, enum usb_init_type init) +int board_usb_init(int index, enum usb_init_type init) { enable_usb_clocks(index); switch (index) { @@ -944,7 +944,7 @@ int omap_xhci_board_usb_init(int index, enum usb_init_type init) return 0; } -int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) +int board_usb_cleanup(int index, enum usb_init_type init) { switch (index) { case 0: diff --git a/drivers/usb/host/xhci-omap.c b/drivers/usb/host/xhci-omap.c index d6c5744..b814500 100644 --- a/drivers/usb/host/xhci-omap.c +++ b/drivers/usb/host/xhci-omap.c @@ -27,28 +27,6 @@ DECLARE_GLOBAL_DATA_PTR; static struct omap_xhci omap; -__weak int omap_xhci_board_usb_init(int index, enum usb_init_type init) -{ - enable_usb_clocks(index); - return 0; -} - -int board_usb_init(int index, enum usb_init_type init) -{ - return omap_xhci_board_usb_init(index, init); -} - -__weak int omap_xhci_board_usb_cleanup(int index, enum usb_init_type init) -{ - disable_usb_clocks(index); - return 0; -} - -int board_usb_cleanup(int index, enum usb_init_type init) -{ - return omap_xhci_board_usb_cleanup(index, init); -} - static int omap_xhci_core_init(struct omap_xhci *omap) { int ret = 0;
board_usb_init()/_cleanup() should be in board files and don't have a place in the xhci-omap driver. Weak versions for board_usb_init()/_cleanup() already exist in common/usb.c (for host mode) and drivers/usb/gadget/g_dnl.c (for gadget mode). Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> --- Since 1a9a5f7 ("fix double weak board_usb_init functions") the board_usb_init()/_cleanup() a prefix omap_xhci_* was added to the board files and in omap-xhci.c because there was a duplicate implementation in common/usb.c. However, this broke the gadget mode path which uses the same board_usb_init/cleanup() in the board specific files. I think a better way would be to just remove the functions from xhci-omap like in this patch. Any issues with this? board/ti/am43xx/board.c | 4 ++-- board/ti/am57xx/board.c | 4 ++-- board/ti/dra7xx/evm.c | 4 ++-- drivers/usb/host/xhci-omap.c | 22 ---------------------- 4 files changed, 6 insertions(+), 28 deletions(-)