| Message ID | 20170809095910.5650-1-sr@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Aug 09, 2017 at 11:59:10AM +0200, Stefan Roese wrote: > This patch fixes some sparse warnings by using "void __iomem *" for > the register variables as this is required for the ioread32/iowrite32 > accessor functions. A patch subject line should describe the change not the tool that found it, so can you please revise the fixes. One possible way is to club similar type of fixes in a file. It is also helpful to show the sparse warns to help understand the fixes. > > Please note that the following patch needs to be applied to quiet some > incorrect sparse warnings when compiling this driver for ARM on 64bit > platforms (GENMASK issue): > > "arm: fix sparse flags for build on 64bit machines" > https://patchwork.kernel.org/patch/9864431/ This info is useless for applying but useful for review and test, this should not be kept part of the changelog and moved after S-o-b line.. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Vinod Koul <vinod.koul@intel.com> > --- > drivers/dma/altera-msgdma.c | 46 +++++++++++++++++++++++++-------------------- > 1 file changed, 26 insertions(+), 20 deletions(-) > > diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c > index 33b87b413793..c3cb92587001 100644 > --- a/drivers/dma/altera-msgdma.c > +++ b/drivers/dma/altera-msgdma.c > @@ -166,6 +166,10 @@ struct msgdma_response { > #define MSGDMA_RESP_EARLY_TERM BIT(8) > #define MSGDMA_RESP_ERR_MASK 0xff > > +#define respoffs(a) (offsetof(struct msgdma_response, a)) > +#define csroffs(a) (offsetof(struct msgdma_csr, a)) > +#define descoffs(a) (offsetof(struct msgdma_extended_desc, a)) > + > /** > * struct msgdma_sw_desc - implements a sw descriptor > * @async_tx: support for the async_tx api > @@ -204,13 +208,13 @@ struct msgdma_device { > int irq; > > /* mSGDMA controller */ > - struct msgdma_csr *csr; > + void __iomem *csr; > > /* mSGDMA descriptors */ > - struct msgdma_extended_desc *desc; > + void __iomem *desc; > > /* mSGDMA response */ > - struct msgdma_response *resp; > + void __iomem *resp; > }; > > #define to_mdev(chan) container_of(chan, struct msgdma_device, dmachan) > @@ -576,21 +580,21 @@ static void msgdma_reset(struct msgdma_device *mdev) > int ret; > > /* Reset mSGDMA */ > - iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status); > - iowrite32(MSGDMA_CSR_CTL_RESET, &mdev->csr->control); > + iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status)); > + iowrite32(MSGDMA_CSR_CTL_RESET, mdev->csr + csroffs(control)); why do you need to use offsetof here and other places? something doesnt seem right here...
Hi Vinod, On 21.08.2017 18:18, Vinod Koul wrote: > On Wed, Aug 09, 2017 at 11:59:10AM +0200, Stefan Roese wrote: >> This patch fixes some sparse warnings by using "void __iomem *" for >> the register variables as this is required for the ioread32/iowrite32 >> accessor functions. > > A patch subject line should describe the change not the tool that found it, > so can you please revise the fixes. One possible way is to club similar type > of fixes in a file. It is also helpful to show the sparse warns to help > understand the fixes. Okay. >> >> Please note that the following patch needs to be applied to quiet some >> incorrect sparse warnings when compiling this driver for ARM on 64bit >> platforms (GENMASK issue): >> >> "arm: fix sparse flags for build on 64bit machines" >> https://patchwork.kernel.org/patch/9864431/ > > This info is useless for applying but useful for review and test, this > should not be kept part of the changelog and moved after S-o-b line.. Okay. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Vinod Koul <vinod.koul@intel.com> >> --- >> drivers/dma/altera-msgdma.c | 46 +++++++++++++++++++++++++-------------------- >> 1 file changed, 26 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c >> index 33b87b413793..c3cb92587001 100644 >> --- a/drivers/dma/altera-msgdma.c >> +++ b/drivers/dma/altera-msgdma.c >> @@ -166,6 +166,10 @@ struct msgdma_response { >> #define MSGDMA_RESP_EARLY_TERM BIT(8) >> #define MSGDMA_RESP_ERR_MASK 0xff >> >> +#define respoffs(a) (offsetof(struct msgdma_response, a)) >> +#define csroffs(a) (offsetof(struct msgdma_csr, a)) >> +#define descoffs(a) (offsetof(struct msgdma_extended_desc, a)) >> + >> /** >> * struct msgdma_sw_desc - implements a sw descriptor >> * @async_tx: support for the async_tx api >> @@ -204,13 +208,13 @@ struct msgdma_device { >> int irq; >> >> /* mSGDMA controller */ >> - struct msgdma_csr *csr; >> + void __iomem *csr; >> >> /* mSGDMA descriptors */ >> - struct msgdma_extended_desc *desc; >> + void __iomem *desc; >> >> /* mSGDMA response */ >> - struct msgdma_response *resp; >> + void __iomem *resp; >> }; >> >> #define to_mdev(chan) container_of(chan, struct msgdma_device, dmachan) >> @@ -576,21 +580,21 @@ static void msgdma_reset(struct msgdma_device *mdev) >> int ret; >> >> /* Reset mSGDMA */ >> - iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status); >> - iowrite32(MSGDMA_CSR_CTL_RESET, &mdev->csr->control); >> + iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status)); >> + iowrite32(MSGDMA_CSR_CTL_RESET, mdev->csr + csroffs(control)); > > why do you need to use offsetof here and other places? > something doesnt seem right here... ioread32/iowrite32() operates on "void __iomem *" and not on pointer to struct members. So adding these offsetof constructs helps to use the correct function parameter types here. The other solutions would be to either add some casts (which I really mis-like) or to move away from using a struct for the IP core registers description to macros instead, like this: #define MSGDMA_CSR_STATUS 0x00 #define MSGDMA_CSR_CONTROL 0x04 ... If you prefer the macros, I can change it this way as well - just let me know. BTW: I've received the linux-next build failure warning, resulting from DMA_SG being removed. I plan to remove the DMA_SG support from the driver and send a new patch version with all changes included (sparse warnings fixed, W=1 warning removed and DMA_SG removed), if this is okay for you. When do you plan to pull the DMA_MEMCPY_SG patchset? For v4.14? If yes, would it be okay to already integrate support for this OP-type into the next patch version then? Or do you prefer that I add support for this in some follow-up patch instead? Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 22, 2017 at 09:29:02AM +0200, Stefan Roese wrote: > Hi Vinod, > > On 21.08.2017 18:18, Vinod Koul wrote: > >>@@ -166,6 +166,10 @@ struct msgdma_response { > >> #define MSGDMA_RESP_EARLY_TERM BIT(8) > >> #define MSGDMA_RESP_ERR_MASK 0xff > >>+#define respoffs(a) (offsetof(struct msgdma_response, a)) > >>+#define csroffs(a) (offsetof(struct msgdma_csr, a)) > >>+#define descoffs(a) (offsetof(struct msgdma_extended_desc, a)) > >>+ > >> /** > >> * struct msgdma_sw_desc - implements a sw descriptor > >> * @async_tx: support for the async_tx api > >>@@ -204,13 +208,13 @@ struct msgdma_device { > >> int irq; > >> /* mSGDMA controller */ > >>- struct msgdma_csr *csr; > >>+ void __iomem *csr; > >> /* mSGDMA descriptors */ > >>- struct msgdma_extended_desc *desc; > >>+ void __iomem *desc; > >> /* mSGDMA response */ > >>- struct msgdma_response *resp; > >>+ void __iomem *resp; > >> }; > >> #define to_mdev(chan) container_of(chan, struct msgdma_device, dmachan) > >>@@ -576,21 +580,21 @@ static void msgdma_reset(struct msgdma_device *mdev) > >> int ret; > >> /* Reset mSGDMA */ > >>- iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status); > >>- iowrite32(MSGDMA_CSR_CTL_RESET, &mdev->csr->control); > >>+ iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status)); > >>+ iowrite32(MSGDMA_CSR_CTL_RESET, mdev->csr + csroffs(control)); > > > >why do you need to use offsetof here and other places? > >something doesnt seem right here... > > ioread32/iowrite32() operates on "void __iomem *" and not on > pointer to struct members. So adding these offsetof constructs helps > to use the correct function parameter types here. The other solutions > would be to either add some casts (which I really mis-like) > or to move away from using a struct for the IP core registers > description to macros instead, like this: > > #define MSGDMA_CSR_STATUS 0x00 > #define MSGDMA_CSR_CONTROL 0x04 > ... > > If you prefer the macros, I can change it this way as well - just > let me know. I think moving away from structures is better and ore intutive. We can have a void__iomeme * as hw base address and you can use offsets as described.. > BTW: I've received the linux-next build failure warning, resulting > from DMA_SG being removed. I plan to remove the DMA_SG support from > the driver and send a new patch version with all changes included > (sparse warnings fixed, W=1 warning removed and DMA_SG removed), > if this is okay for you. I have already fixed that up (i think you were cced on my reply) with the patch to remove, do check if that was fine... > When do you plan to pull the DMA_MEMCPY_SG patchset? For v4.14? > If yes, would it be okay to already integrate support for this > OP-type into the next patch version then? Or do you prefer that > I add support for this in some follow-up patch instead? I think we might be bit late, MERGE window opens in a week ish...
Hi Vinod, On 25.08.2017 08:20, Vinod Koul wrote: > On Tue, Aug 22, 2017 at 09:29:02AM +0200, Stefan Roese wrote: >> Hi Vinod, >> >> On 21.08.2017 18:18, Vinod Koul wrote: >>>> @@ -166,6 +166,10 @@ struct msgdma_response { >>>> #define MSGDMA_RESP_EARLY_TERM BIT(8) >>>> #define MSGDMA_RESP_ERR_MASK 0xff >>>> +#define respoffs(a) (offsetof(struct msgdma_response, a)) >>>> +#define csroffs(a) (offsetof(struct msgdma_csr, a)) >>>> +#define descoffs(a) (offsetof(struct msgdma_extended_desc, a)) >>>> + >>>> /** >>>> * struct msgdma_sw_desc - implements a sw descriptor >>>> * @async_tx: support for the async_tx api >>>> @@ -204,13 +208,13 @@ struct msgdma_device { >>>> int irq; >>>> /* mSGDMA controller */ >>>> - struct msgdma_csr *csr; >>>> + void __iomem *csr; >>>> /* mSGDMA descriptors */ >>>> - struct msgdma_extended_desc *desc; >>>> + void __iomem *desc; >>>> /* mSGDMA response */ >>>> - struct msgdma_response *resp; >>>> + void __iomem *resp; >>>> }; >>>> #define to_mdev(chan) container_of(chan, struct msgdma_device, dmachan) >>>> @@ -576,21 +580,21 @@ static void msgdma_reset(struct msgdma_device *mdev) >>>> int ret; >>>> /* Reset mSGDMA */ >>>> - iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status); >>>> - iowrite32(MSGDMA_CSR_CTL_RESET, &mdev->csr->control); >>>> + iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status)); >>>> + iowrite32(MSGDMA_CSR_CTL_RESET, mdev->csr + csroffs(control)); >>> >>> why do you need to use offsetof here and other places? >>> something doesnt seem right here... >> >> ioread32/iowrite32() operates on "void __iomem *" and not on >> pointer to struct members. So adding these offsetof constructs helps >> to use the correct function parameter types here. The other solutions >> would be to either add some casts (which I really mis-like) >> or to move away from using a struct for the IP core registers >> description to macros instead, like this: >> >> #define MSGDMA_CSR_STATUS 0x00 >> #define MSGDMA_CSR_CONTROL 0x04 >> ... >> >> If you prefer the macros, I can change it this way as well - just >> let me know. > > I think moving away from structures is better and ore intutive. We can have > a void__iomeme * as hw base address and you can use offsets as described.. Okay, I'll remove the structs from the driver in a follow-up patch. >> BTW: I've received the linux-next build failure warning, resulting >> from DMA_SG being removed. I plan to remove the DMA_SG support from >> the driver and send a new patch version with all changes included >> (sparse warnings fixed, W=1 warning removed and DMA_SG removed), >> if this is okay for you. > > I have already fixed that up (i think you were cced on my reply) with the > patch to remove, do check if that was fine... Looks good. Thanks for working on this. >> When do you plan to pull the DMA_MEMCPY_SG patchset? For v4.14? >> If yes, would it be okay to already integrate support for this >> OP-type into the next patch version then? Or do you prefer that >> I add support for this in some follow-up patch instead? > > I think we might be bit late, MERGE window opens in a week ish... I see. I'll send a patch adding DMA_MEMCPY_SG support to this mSGDMA driver, once the DMA_MEMCPY_SG infrastructure is in place. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c index 33b87b413793..c3cb92587001 100644 --- a/drivers/dma/altera-msgdma.c +++ b/drivers/dma/altera-msgdma.c @@ -166,6 +166,10 @@ struct msgdma_response { #define MSGDMA_RESP_EARLY_TERM BIT(8) #define MSGDMA_RESP_ERR_MASK 0xff +#define respoffs(a) (offsetof(struct msgdma_response, a)) +#define csroffs(a) (offsetof(struct msgdma_csr, a)) +#define descoffs(a) (offsetof(struct msgdma_extended_desc, a)) + /** * struct msgdma_sw_desc - implements a sw descriptor * @async_tx: support for the async_tx api @@ -204,13 +208,13 @@ struct msgdma_device { int irq; /* mSGDMA controller */ - struct msgdma_csr *csr; + void __iomem *csr; /* mSGDMA descriptors */ - struct msgdma_extended_desc *desc; + void __iomem *desc; /* mSGDMA response */ - struct msgdma_response *resp; + void __iomem *resp; }; #define to_mdev(chan) container_of(chan, struct msgdma_device, dmachan) @@ -576,21 +580,21 @@ static void msgdma_reset(struct msgdma_device *mdev) int ret; /* Reset mSGDMA */ - iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status); - iowrite32(MSGDMA_CSR_CTL_RESET, &mdev->csr->control); + iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status)); + iowrite32(MSGDMA_CSR_CTL_RESET, mdev->csr + csroffs(control)); - ret = readl_poll_timeout(&mdev->csr->status, val, + ret = readl_poll_timeout(mdev->csr + csroffs(status), val, (val & MSGDMA_CSR_STAT_RESETTING) == 0, 1, 10000); if (ret) dev_err(mdev->dev, "DMA channel did not reset\n"); /* Clear all status bits */ - iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status); + iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status)); /* Enable the DMA controller including interrupts */ iowrite32(MSGDMA_CSR_CTL_STOP_ON_ERR | MSGDMA_CSR_CTL_STOP_ON_EARLY | - MSGDMA_CSR_CTL_GLOBAL_INTR, &mdev->csr->control); + MSGDMA_CSR_CTL_GLOBAL_INTR, mdev->csr + csroffs(control)); mdev->idle = true; }; @@ -598,13 +602,14 @@ static void msgdma_reset(struct msgdma_device *mdev) static void msgdma_copy_one(struct msgdma_device *mdev, struct msgdma_sw_desc *desc) { - struct msgdma_extended_desc *hw_desc = mdev->desc; + void __iomem *hw_desc = mdev->desc; /* * Check if the DESC FIFO it not full. If its full, we need to wait * for at least one entry to become free again */ - while (ioread32(&mdev->csr->status) & MSGDMA_CSR_STAT_DESC_BUF_FULL) + while (ioread32(mdev->csr + csroffs(status)) & + MSGDMA_CSR_STAT_DESC_BUF_FULL) mdelay(1); /* @@ -616,12 +621,13 @@ static void msgdma_copy_one(struct msgdma_device *mdev, * sure this control word is written last by single coding it and * adding some write-barriers here. */ - memcpy(hw_desc, &desc->hw_desc, sizeof(desc->hw_desc) - sizeof(u32)); + memcpy((void __force *)hw_desc, &desc->hw_desc, + sizeof(desc->hw_desc) - sizeof(u32)); /* Write control word last to flush this descriptor into the FIFO */ mdev->idle = false; wmb(); - iowrite32(desc->hw_desc.control, &hw_desc->control); + iowrite32(desc->hw_desc.control, hw_desc + descoffs(control)); wmb(); } @@ -788,7 +794,7 @@ static void msgdma_tasklet(unsigned long data) spin_lock(&mdev->lock); /* Read number of responses that are available */ - count = ioread32(&mdev->csr->resp_fill_level); + count = ioread32(mdev->csr + csroffs(resp_fill_level)); dev_dbg(mdev->dev, "%s (%d): response count=%d\n", __func__, __LINE__, count); @@ -799,8 +805,8 @@ static void msgdma_tasklet(unsigned long data) * have any real values, like transferred bytes or error * bits. So we need to just drop these values. */ - size = ioread32(&mdev->resp->bytes_transferred); - status = ioread32(&mdev->resp->status); + size = ioread32(mdev->resp + respoffs(bytes_transferred)); + status = ioread32(mdev->resp + respoffs(status)); msgdma_complete_descriptor(mdev); msgdma_chan_desc_cleanup(mdev); @@ -821,7 +827,7 @@ static irqreturn_t msgdma_irq_handler(int irq, void *data) struct msgdma_device *mdev = data; u32 status; - status = ioread32(&mdev->csr->status); + status = ioread32(mdev->csr + csroffs(status)); if ((status & MSGDMA_CSR_STAT_BUSY) == 0) { /* Start next transfer if the DMA controller is idle */ spin_lock(&mdev->lock); @@ -833,7 +839,7 @@ static irqreturn_t msgdma_irq_handler(int irq, void *data) tasklet_schedule(&mdev->irq_tasklet); /* Clear interrupt in mSGDMA controller */ - iowrite32(MSGDMA_CSR_STAT_IRQ, &mdev->csr->status); + iowrite32(MSGDMA_CSR_STAT_IRQ, mdev->csr + csroffs(status)); return IRQ_HANDLED; } @@ -901,17 +907,17 @@ static int msgdma_probe(struct platform_device *pdev) mdev->dev = &pdev->dev; /* Map CSR space */ - ret = request_and_map(pdev, "csr", &dma_res, (void **)&mdev->csr); + ret = request_and_map(pdev, "csr", &dma_res, &mdev->csr); if (ret) return ret; /* Map (extended) descriptor space */ - ret = request_and_map(pdev, "desc", &dma_res, (void **)&mdev->desc); + ret = request_and_map(pdev, "desc", &dma_res, &mdev->desc); if (ret) return ret; /* Map response space */ - ret = request_and_map(pdev, "resp", &dma_res, (void **)&mdev->resp); + ret = request_and_map(pdev, "resp", &dma_res, &mdev->resp); if (ret) return ret;
This patch fixes some sparse warnings by using "void __iomem *" for the register variables as this is required for the ioread32/iowrite32 accessor functions. Please note that the following patch needs to be applied to quiet some incorrect sparse warnings when compiling this driver for ARM on 64bit platforms (GENMASK issue): "arm: fix sparse flags for build on 64bit machines" https://patchwork.kernel.org/patch/9864431/ Signed-off-by: Stefan Roese <sr@denx.de> Cc: Vinod Koul <vinod.koul@intel.com> --- drivers/dma/altera-msgdma.c | 46 +++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)