[U-Boot] [PATCH] usb: Make USB_MUSB_PIO_ONLY conditional on USB_MUSB_{HOST, GADGET}

* [U-Boot] [PATCH] usb: Make USB_MUSB_PIO_ONLY conditional on USB_MUSB_{HOST, GADGET}
@ 2019-11-22 21:14 Samuel Dionne-Riel
  2019-11-27 19:56 ` [U-Boot] [PATCH v2] usb: Make USB_MUSB_PIO_ONLY selected by USB_MUSB_SUNXI Samuel Dionne-Riel
  0 siblings, 1 reply; 3+ messages in thread
From: Samuel Dionne-Riel @ 2019-11-22 21:14 UTC (permalink / raw)
  To: u-boot

This ensures the USB_MUSB_PIO_ONLY config is set to an apppropriate
default values from the changes enabling USB_MUSB_GADGET does.

Namely, USB_MUSB_PIO_ONLY default to =y on USB_MUSB_SUNXI being y.
Since USB_MUSB_SUNXI is behind the conditional, the option does not
exist when using make ..._defconfig, thus the default of
USB_MUSB_PIO_ONLY is kept "# ... is not set". This, _is_
counter-intuitively a set value, meaning that the default will not be
applied once USB_MUSB_SUNXI is set to y by toggling USB_MUSB_GADGET
via menuconfig.

Signed-off-by: Samuel Dionne-Riel <samuel@dionne-riel.com>

---
Some additional context about the bug.

Using `make ..._defconfig` for a sunxi board, then using `make
menuconfig` to change the option `CONFIG_USB_MUSB_GADGET=y` causes
builds to fail. When the same option is set to `=y` via the defconfig
file, the option work. Follows the failure:

```
  CC      drivers/usb/musb-new/musb_gadget.o
drivers/usb/musb-new/musb_gadget.c: In function 'map_dma_buffer':
drivers/usb/musb-new/musb_gadget.c:103:26: warning: implicit declaration of function 'dma_map_single'; did you mean 'malloc_simple'? [-Wimplicit-function-declaration]
   request->request.dma = dma_map_single(
                          ^~~~~~~~~~~~~~
                          malloc_simple
drivers/usb/musb-new/musb_gadget.c:108:8: error: 'DMA_TO_DEVICE' undeclared (first use in this function); did you mean 'USB_DT_DEVICE'?
      ? DMA_TO_DEVICE
        ^~~~~~~~~~~~~
        USB_DT_DEVICE
drivers/usb/musb-new/musb_gadget.c:108:8: note: each undeclared identifier is reported only once for each function it appears in
drivers/usb/musb-new/musb_gadget.c:109:8: error: 'DMA_FROM_DEVICE' undeclared (first use in this function); did you mean 'U_BOOT_DEVICE'?
      : DMA_FROM_DEVICE);
        ^~~~~~~~~~~~~~~
        U_BOOT_DEVICE
drivers/usb/musb-new/musb_gadget.c:112:3: warning: implicit declaration of function 'dma_sync_single_for_device' [-Wimplicit-function-declaration]
   dma_sync_single_for_device(musb->controller,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/usb/musb-new/musb_gadget.c: In function 'unmap_dma_buffer':
drivers/usb/musb-new/musb_gadget.c:135:3: warning: implicit declaration of function 'dma_unmap_single' [-Wimplicit-function-declaration]
   dma_unmap_single(musb->controller,
   ^~~~~~~~~~~~~~~~
drivers/usb/musb-new/musb_gadget.c:139:7: error: 'DMA_TO_DEVICE' undeclared (first use in this function); did you mean 'USB_DT_DEVICE'?
     ? DMA_TO_DEVICE
       ^~~~~~~~~~~~~
       USB_DT_DEVICE
drivers/usb/musb-new/musb_gadget.c:140:7: error: 'DMA_FROM_DEVICE' undeclared (first use in this function); did you mean 'U_BOOT_DEVICE'?
     : DMA_FROM_DEVICE);
       ^~~~~~~~~~~~~~~
       U_BOOT_DEVICE
drivers/usb/musb-new/musb_gadget.c:143:3: warning: implicit declaration of function 'dma_sync_single_for_cpu' [-Wimplicit-function-declaration]
   dma_sync_single_for_cpu(musb->controller,
   ^~~~~~~~~~~~~~~~~~~~~~~
make[1]: *** [scripts/Makefile.build:279: drivers/usb/musb-new/musb_gadget.o] Error 1
```

By diffing the resulting configuration, Othe main difference found is
`CONFIG_USB_MUSB_PIO_ONLY` is not set when failing, and set to `=y`
when working. This was verified to be the missing option.

As far as I understand it in Kconfig, "is not set" basically marks the
option as using a negative "unset" value, and does not mean that it
will use the default value. This difference in meaning makes it so
that when toggling the option with `make menuconfig`, the option has
already been set to "is not set", and it does not change to its
default value.

The actual bug may be that there is a regression and the code should
work when this value is unset, in this case I do not know how to
provide the proper fix.

This fix corrects the issue by gating the option behind
`if USB_MUSB_HOST || USB_MUSB_GADGET`. To me, it looks like this was
the intention, in the initial patch, but might have been overlooked.
I assume so since the option was defined after the `if`, rather than
before, like the other options not gated behind the option.


 drivers/usb/musb-new/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb-new/Kconfig b/drivers/usb/musb-new/Kconfig
index 79ad14ef66..95499f1b17 100644
--- a/drivers/usb/musb-new/Kconfig
+++ b/drivers/usb/musb-new/Kconfig
@@ -72,11 +72,11 @@ config USB_MUSB_DISABLE_BULK_COMBINE_SPLIT
 	  support it yet. Select this option to disable the feature until the
 	  driver adds the support.
 
-endif
-
 config USB_MUSB_PIO_ONLY
 	bool "Disable DMA (always use PIO)"
 	default y if USB_MUSB_AM35X || USB_MUSB_PIC32 || USB_MUSB_OMAP2PLUS || USB_MUSB_DSPS || USB_MUSB_SUNXI
 	help
 	  All data is copied between memory and FIFO by the CPU.
 	  DMA controllers are ignored.
+
+endif
-- 
2.23.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread