From d82c104a6a191158b9532aa7bb831d2b2ec77397 Mon Sep 17 00:00:00 2001 From: Jouni Ukkonen Date: Wed, 25 Sep 2024 12:49:15 +0300 Subject: [PATCH] arch/arm64/src/imx9/imx9_usbdev.c: Clean up cache operations, add DEBUGASSERTS Correct some of the cache operations: - EP0 request length was handled incorrectly - Received data cache invalidate was exceeding the received buffer - writedtd is also called with no data (EP0 ACK/NACK). Don't touch cache in that case. Fix trip wire handling to conform with the IMX93 reference manual Also add DEBUGASSERTS for future to check the validity of pointers and sizes Co-authored-by: Jukka Laitinen --- arch/arm64/src/imx9/imx9_usbdev.c | 87 ++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 30 deletions(-) diff --git a/arch/arm64/src/imx9/imx9_usbdev.c b/arch/arm64/src/imx9/imx9_usbdev.c index d34f6919c7fc8..aa7ad38b7ce18 100644 --- a/arch/arm64/src/imx9/imx9_usbdev.c +++ b/arch/arm64/src/imx9/imx9_usbdev.c @@ -60,7 +60,7 @@ #endif #define DCACHE_LINEMASK (ARMV8A_DCACHE_LINESIZE - 1) - +#define DCACHE_ALIGN_UP(a) (((a) + DCACHE_LINEMASK) & ~DCACHE_LINEMASK) #if !defined(CONFIG_ARM64_DCACHE_DISABLE) # define cache_aligned_alloc(s) kmm_memalign(ARMV8A_DCACHE_LINESIZE,(s)) # define CACHE_ALIGNED_DATA aligned_data(ARMV8A_DCACHE_LINESIZE) @@ -69,6 +69,11 @@ # define CACHE_ALIGNED_DATA #endif +/* Check that both start and length are cache-aligned */ + +#define IS_CACHE_ALIGNED(x,y) (((uintptr_t)(x) & DCACHE_LINEMASK) == 0 && \ + ((y) & DCACHE_LINEMASK) == 0) + /* Configuration ************************************************************/ #ifndef CONFIG_USBDEV_EP0_MAXSIZE @@ -218,6 +223,10 @@ const struct trace_msg_t g_usb_trace_strings_intdecode[] = }; #endif +/* Length of allocated EP0 buffer (DCACHE aligned) */ + +#define EP0_MAXBUFLEN 64 + /* Hardware interface *******************************************************/ /* This represents a Endpoint Transfer Descriptor dQH overlay (32 bytes) */ @@ -528,20 +537,20 @@ static int imx9_pullup(struct usbdev_s *dev, bool enable); */ #ifdef CONFIG_IMX9_USBDEV_USBC1 -static uint8_t g_usb0_ep0buf[64] CACHE_ALIGNED_DATA; +static uint8_t g_usb0_ep0buf[EP0_MAXBUFLEN] CACHE_ALIGNED_DATA; static struct imx9_dqh_s g_usb0_qh[IMX9_NPHYSENDPOINTS] aligned_data(2048); static struct imx9_dtd_s g_usb0_td[IMX9_NPHYSENDPOINTS]; #endif #ifdef CONFIG_IMX9_USBDEV_USBC2 -static uint8_t g_usb1_ep0buf[64] CACHE_ALIGNED_DATA; +static uint8_t g_usb1_ep0buf[EP0_MAXBUFLEN] CACHE_ALIGNED_DATA; static struct imx9_dqh_s g_usb1_qh[IMX9_NPHYSENDPOINTS] aligned_data(2048); static struct imx9_dtd_s g_usb1_td[IMX9_NPHYSENDPOINTS]; #endif static struct imx9_usb_s g_usbdev[] = { -#ifdef CONFIG_IMX9_USBDEV_USBC1 +#ifdef CONFIG_IMX9_USBDEV_USBC1 { .id = 0, .base = IMX9_USB_OTG1_BASE, @@ -692,10 +701,7 @@ static void imx9_putreg(struct imx9_usb_s *priv, off_t offset, uint32_t val) static inline void imx9_modifyreg(struct imx9_usb_s *priv, off_t offset, uint32_t clear, uint32_t set) { - uint32_t reg = imx9_getreg(priv, offset); - reg &= ~clear; - reg |= set; - imx9_putreg(priv, offset, reg); + modifyreg32(priv->base + offset, clear, set); } /**************************************************************************** @@ -764,6 +770,8 @@ static inline void imx9_writedtd(struct imx9_dtd_s *dtd, const uint8_t *data, uint32_t nbytes) { + DEBUGASSERT(IS_CACHE_ALIGNED(dtd, sizeof(*dtd))); + dtd->nextdesc = DTD_NEXTDESC_INVALID; dtd->config = DTD_CONFIG_LENGTH(nbytes) | DTD_CONFIG_IOC | DTD_CONFIG_ACTIVE; @@ -776,8 +784,18 @@ static inline void imx9_writedtd(struct imx9_dtd_s *dtd, up_clean_dcache((uintptr_t)dtd, (uintptr_t)dtd + sizeof(struct imx9_dtd_s)); - up_clean_dcache((uintptr_t)data, - (uintptr_t)data + nbytes); + + /* the pointer is NULL for EP0 NAK IN/OUT */ + + if (data != NULL) + { + DEBUGASSERT(IS_CACHE_ALIGNED(data, DCACHE_ALIGN_UP(nbytes))); + DEBUGASSERT(nbytes <= 0x5000); + + /* Data needs to be clean before TX, clean or invalid before RX */ + + up_clean_dcache((uintptr_t)data, (uintptr_t)data + nbytes); + } } /**************************************************************************** @@ -803,7 +821,7 @@ static void imx9_queuedtd(struct imx9_usb_s *priv, uint8_t epphy, dqh->overlay.nextdesc = (uint32_t)(uintptr_t)dtd; dqh->overlay.config &= ~(DTD_CONFIG_ACTIVE | DTD_CONFIG_HALTED); - up_flush_dcache((uintptr_t)dqh, + up_clean_dcache((uintptr_t)dqh, (uintptr_t)dqh + sizeof(struct imx9_dqh_s)); uint32_t bit = IMX9_ENDPTMASK(epphy); @@ -845,21 +863,20 @@ static void imx9_readsetup(struct imx9_usb_s *priv, uint8_t epphy, struct imx9_dqh_s *dqh = &priv->qh[epphy]; int i; - do - { - /* Set the trip wire */ + /* Set the trip wire */ - imx9_modifyreg(priv, IMX9_USBDEV_USBCMD_OFFSET, 0, USBDEV_USBCMD_SUTW); + imx9_modifyreg(priv, IMX9_USBDEV_USBCMD_OFFSET, 0, USBDEV_USBCMD_SUTW); + ARM64_DSB(); - up_invalidate_dcache((uintptr_t)dqh, - (uintptr_t)dqh + sizeof(struct imx9_dqh_s)); + DEBUGASSERT(IS_CACHE_ALIGNED(dqh, sizeof(struct imx9_dqh_s))); + up_invalidate_dcache((uintptr_t)dqh, + (uintptr_t)dqh + sizeof(struct imx9_dqh_s)); - /* Copy the request... */ + /* Copy the request... */ - for (i = 0; i < 8; i++) - { - ((uint8_t *) ctrl)[i] = ((uint8_t *) dqh->setup)[i]; - } + for (i = 0; i < 8; i++) + { + ((uint8_t *) ctrl)[i] = ((uint8_t *) dqh->setup)[i]; } while (!(imx9_getreg(priv, @@ -869,13 +886,11 @@ static void imx9_readsetup(struct imx9_usb_s *priv, uint8_t epphy, imx9_modifyreg(priv, IMX9_USBDEV_USBCMD_OFFSET, USBDEV_USBCMD_SUTW, 0); - up_clean_dcache((uintptr_t)dqh, - (uintptr_t)dqh + sizeof(struct imx9_dqh_s)); - /* Clear the Setup Interrupt */ imx9_putreg(priv, IMX9_USBDEV_ENDPTSETUPSTAT_OFFSET, IMX9_ENDPTMASK(IMX9_EP0_OUT)); + ARM64_DSB(); } /**************************************************************************** @@ -1120,8 +1135,9 @@ static void imx9_dispatchrequest(struct imx9_usb_s *priv, { /* Invalidate buffer data cache */ + DEBUGASSERT(IS_CACHE_ALIGNED(priv->ep0.buf, EP0_MAXBUFLEN)); up_invalidate_dcache((uintptr_t)priv->ep0.buf, - (uintptr_t)priv->ep0.buf + sizeof(priv->ep0.buf)); + (uintptr_t)priv->ep0.buf + EP0_MAXBUFLEN); /* Forward to the control request to the class driver implementation */ @@ -1162,7 +1178,7 @@ static void imx9_ep0configure(struct imx9_usb_s *priv) priv->qh[IMX9_EP0_IN].currdesc = DTD_NEXTDESC_INVALID; up_clean_dcache((uintptr_t)priv->qh, - (uintptr_t)priv->qh + (sizeof(struct imx9_dqh_s) * 2)); + (uintptr_t)priv->qh + (sizeof(struct imx9_dqh_s))); /* Enable EP0 */ @@ -1303,6 +1319,8 @@ static inline void imx9_ep0state(struct imx9_usb_s *priv, imx9_putreg(priv, IMX9_USBDEV_ENDPTNAKEN_OFFSET, 0); break; } + + ARM64_DSB(); } /**************************************************************************** @@ -1347,7 +1365,9 @@ static inline void imx9_ep0setup(struct imx9_usb_s *priv) index = GETUINT16(ctrl->index); len = GETUINT16(ctrl->len); - priv->ep0.buf_len = len; + /* Limit the ep0 length to maximum */ + + priv->ep0.buf_len = len <= EP0_MAXBUFLEN ? len : EP0_MAXBUFLEN; uinfo("type=%02x req=%02x value=%04x index=%04x len=%04x\n", ctrl->type, ctrl->req, value, index, len); @@ -1868,10 +1888,9 @@ bool imx9_epcomplete(struct imx9_usb_s *priv, uint8_t epphy) /* Make sure we have updated data after the DMA transfer. */ + DEBUGASSERT(IS_CACHE_ALIGNED(dtd, sizeof(struct imx9_dtd_s))); up_invalidate_dcache((uintptr_t)dtd, (uintptr_t)dtd + sizeof(struct imx9_dtd_s)); - up_invalidate_dcache((uintptr_t)dtd->buffer0, - (uintptr_t)dtd->buffer0 + dtd->xfer_len); int xfrd = dtd->xfer_len - (dtd->config >> 16); @@ -1885,6 +1904,14 @@ bool imx9_epcomplete(struct imx9_usb_s *priv, uint8_t epphy) */ usbtrace(TRACE_INTDECODE(IMX9_TRACEINTID_EPIN), complete); + + /* Invalidate rx buffer cache */ + + DEBUGASSERT(IS_CACHE_ALIGNED(privreq->req.buf, + DCACHE_ALIGN_UP(privreq->req.xfrd))); + up_invalidate_dcache((uintptr_t)privreq->req.buf, + (uintptr_t)privreq->req.buf + + DCACHE_ALIGN_UP(privreq->req.xfrd)); } else {