On 06/07/2017 10:03 AM, Dr. Philipp Tomsich wrote: > >> On 07 Jun 2017, at 09:53, Marek Vasut <ma...@denx.de> wrote: >> >> On 06/07/2017 09:49 AM, Dr. Philipp Tomsich wrote: >>>> On 07 Jun 2017, at 08:28, Marek Vasut <ma...@denx.de >>>> <mailto:ma...@denx.de>> wrote: >>>> >>>> On 06/06/2017 05:11 PM, Dr. Philipp Tomsich wrote: >>>>> >>>>>> On 06 Jun 2017, at 16:35, Marek Vasut <ma...@denx.de >>>>>> <mailto:ma...@denx.de>> wrote: >>>>>> >>>>>> On 06/06/2017 03:42 PM, Philipp Tomsich wrote: >>>>>>> For LP64 build w/ GCC 6.3, the handling of the ep->dma_buf variable >>>>>>> when (truncating and) writing into the controller's registers is >>>>>>> unsafe and triggers the following compiler warning (thus failing by >>>>>>> buildman tests): >>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c: In function 'setdma_rx': >>>>>>> ../drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c:116:9: warning: cast >>>>>>> from pointer to integer of different size [-Wpointer-to-int-cast] >>>>>>> writel((unsigned int) ep->dma_buf, ®->out_endp[ep_num].doepdma); >>>>>>> ^ >>>>>>> >>>>>>> This change fixes the warning and makes the code a bit more robust by >>>>>>> doing the following: >>>>>>> - we check whether ep->dma_buf can be safely truncated to 32 bits >>>>>>> (i.e. whether dma_buf is the same value when masked to 32 bits) and >>>>>>> emit an error message if that is not the case >>>>>>> - we cast to a uintptr_t and let the writel macro truncate the >>>>>>> uintptr_t (potentially a 64bit value) to 32bits >>>>>>> >>>>>>> Signed-off-by: Philipp Tomsich >>>>>>> <philipp.toms...@theobroma-systems.com >>>>>>> <mailto:philipp.toms...@theobroma-systems.com>> >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> Changes in v2: >>>>>>> - (new patch) fix warnings and add some robustness for the truncation >>>>>>> of ep->dma_buf in dwc2-otg for LP64 builds to fix a buildman failure >>>>>>> for u-boot-rockchip/master@2b19b2f >>>>>>> >>>>>>> drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c | 6 +++++- >>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c >>>>>>> b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c >>>>>>> index 0d6d2fb..f5c926c 100644 >>>>>>> --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c >>>>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c >>>>>>> @@ -113,7 +113,11 @@ static int setdma_rx(struct dwc2_ep *ep, >>>>>>> struct dwc2_request *req) >>>>>>> invalidate_dcache_range((unsigned long) ep->dma_buf, >>>>>>> (unsigned long) ep->dma_buf + ep->len); >>>>>>> >>>>>>> -writel((unsigned int) ep->dma_buf, ®->out_endp[ep_num].doepdma); >>>>>>> +/* ensure that ep->dma_buf fits into 32 bits */ >>>>>>> +if (((uintptr_t)ep->dma_buf & 0xffffffff) != (uintptr_t)ep->dma_buf) >>>>>>> +error("ep->dma_buf does not fit into a 32 bits"); >>>>>> >>>>>> You print an error, but still continue with the function ? That'll >>>>>> probably lead to a crash, you should rather abort. In fact, I suspect >>>>>> you can use bounce-buffer to assure the dma buffer is in 32bit space. >>>>> >>>>> This tries to stay close to the current behaviour (we have no >>>>> hardware to test this, >>>>> but I wanted to get it out of the way as it caused buildman-failures) >>>>> and to provide >>>>> diagnostics for whoever first encounters an issue here. >>>>> >>>>> Note that I would only expect a DMA error, if someone really hit this >>>>> issue ... >>>> >>>> So this patch is just some hypothetical fix ? What triggered creation of >>>> this patch ? >>> >>> Running buildman (w/ a GCC 6.3 configured as aarch-unknown-elf) against >>> unrelated >>> patch-series (for a Rockchip device that has a different USB controller) >>> fails due to the >>> warnings raised from this. Just casting those warnings away (without >>> having an >>> “assertion-like” diagnostic printed) doesn’t sound like the best plan >>> either, though. >>> >>> Details of the buildman-failure are in the above commit-message. >> >> Ah, hm. In that case, you should really implement the bounce buffer here >> I think. Letting the transfer continue will lead to really weird >> failures and/or memory corruption. > > Alright. I’ll put it to (the back of) my queue and resubmit. > Someone (with hardware) will need to test, once it’s submitted.
That's fine, thanks. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
