Message ID | 20170119100659.11370-1-abailon@baylibre.com |
---|---|
Headers | show |
Series | usb: musb: cppi41: Add a way to manage DMA irq | expand |
On Thu, Jan 19, 2017 at 11:06:57AM +0100, Alexandre Bailon wrote: > Currently, the CPPI 4.1 driver is not completely generic and > only work on dsps. This is because of IRQ management. > Add a callback to dma_controller that could be invoked on DMA completion > to acknodlege the IRQ. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > drivers/usb/musb/musb_cppi41.c | 7 +++++-- > drivers/usb/musb/musb_dma.h | 4 ++++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c > index 1636385..f7d3d27 100644 > --- a/drivers/usb/musb/musb_cppi41.c > +++ b/drivers/usb/musb/musb_cppi41.c > @@ -217,6 +217,10 @@ static void cppi41_dma_callback(void *private_data) > int is_hs = 0; > bool empty; > > + controller = cppi41_channel->controller; > + if (controller->controller.dma_callback) > + controller->controller.dma_callback(&controller->controller); > + > spin_lock_irqsave(&musb->lock, flags); > > dmaengine_tx_status(cppi41_channel->dc, cppi41_channel->cookie, > @@ -249,8 +253,6 @@ static void cppi41_dma_callback(void *private_data) > * We spin on HS (no longer than than 25us and setup a timer on > * FS to check for the bit and complete the transfer. > */ > - controller = cppi41_channel->controller; > - > if (is_host_active(musb)) { > if (musb->port1_status & USB_PORT_STAT_HIGH_SPEED) > is_hs = 1; > @@ -695,6 +697,7 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base) > controller->controller.channel_program = cppi41_dma_channel_program; > controller->controller.channel_abort = cppi41_dma_channel_abort; > controller->controller.is_compatible = cppi41_is_compatible; > + controller->controller.musb = musb; > > ret = cppi41_dma_controller_start(controller); > if (ret) > diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h > index 46357e1..8bea0cd 100644 > --- a/drivers/usb/musb/musb_dma.h > +++ b/drivers/usb/musb/musb_dma.h > @@ -181,10 +181,13 @@ dma_channel_status(struct dma_channel *c) > * @channel_release: call this to release a DMA channel > * @channel_abort: call this to abort a pending DMA transaction, > * returning it to FREE (but allocated) state > + * @platform_dma_callback: invoked on DMA completion, useful to run platform > + * code such IRQ acknowledgment. > * > * Controllers manage dma channels. > */ > struct dma_controller { > + struct musb *musb; Since musb is added here, can we clean up the corresponding one in struct cppi41_dma_controller and struct cppi? Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 19, 2017 at 11:06:59AM +0100, Alexandre Bailon wrote: > Despite the CPPI 4.1 is a generic DMA, it is tied to USB. > On the dsps, CPPI 4.1 interrupt's registers are in USBSS (the MUSB glue). > Currently, to enable / disable and clear interrupts, the CPPI 4.1 driver > maps and accesses to USBSS's register, which making CPPI 4.1 driver not > really generic. > Move the interrupt management to dsps driver. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > --- > drivers/dma/cppi41.c | 28 ++++------------ > drivers/usb/musb/musb_dsps.c | 77 ++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 82 insertions(+), 23 deletions(-) This patch touches both dma and musb modules, I know it makes review easier, but how we get it merged? One maintainer ACK it and the other pick it up? Sorry for the dumb question, I am new as a maintainer... > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c > index d5ba43a..4999e7d 100644 > --- a/drivers/dma/cppi41.c > +++ b/drivers/dma/cppi41.c [...] > diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c > index 9f125e1..9dad3a6 100644 > --- a/drivers/usb/musb/musb_dsps.c > +++ b/drivers/usb/musb/musb_dsps.c > @@ -121,6 +121,7 @@ struct dsps_glue { > struct timer_list timer; /* otg_workaround timer */ > unsigned long last_timer; /* last timer data for each instance */ > bool sw_babble_enabled; > + void __iomem *usbss_base; > > struct dsps_context context; > struct debugfs_regset32 regset; > @@ -145,6 +146,13 @@ static const struct debugfs_reg32 dsps_musb_regs[] = { > { "mode", 0xe8 }, > }; > > +/* USBSS / USB AM335x */ > +#define USBSS_IRQ_STATUS 0x28 > +#define USBSS_IRQ_ENABLER 0x2c > +#define USBSS_IRQ_CLEARR 0x30 > + > +#define USBSS_IRQ_PD_COMP (1 << 2) Please fix the double white spaces bwteen '<' and '2' this time. > + > /** > * dsps_musb_enable - enable interrupts > */ > @@ -619,14 +627,72 @@ static void dsps_read_fifo32(struct musb_hw_ep *hw_ep, u16 len, u8 *dst) > } > } > > +#ifdef CONFIG_USB_TI_CPPI41_DMA > +static void dsps_dma_controller_callback(struct dma_controller *c) > +{ > + struct musb *musb = c->musb; > + struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent); > + void __iomem *usbss_base = glue->usbss_base; > + u32 status; > + > + status = musb_readl(usbss_base, USBSS_IRQ_STATUS); > + if (status & USBSS_IRQ_PD_COMP) > + musb_writel(usbss_base, USBSS_IRQ_STATUS, USBSS_IRQ_PD_COMP); > +} > + > +static struct dma_controller * > +dsps_dma_controller_create(struct musb *musb, void __iomem *base) > +{ > + struct dma_controller *controller; > + struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent); > + void __iomem *usbss_base = glue->usbss_base; > + > + controller = cppi41_dma_controller_create(musb, base); > + if (IS_ERR_OR_NULL(controller)) > + return controller; > + > + musb_writel(usbss_base, USBSS_IRQ_ENABLER, USBSS_IRQ_PD_COMP); > + controller->dma_callback = dsps_dma_controller_callback; > + > + return controller; > +} > + > +static void dsps_dma_controller_destroy(struct dma_controller *c) > +{ > + struct musb *musb = c->musb; > + struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent); > + void __iomem *usbss_base = glue->usbss_base; > + > + musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP); > + cppi41_dma_controller_destroy(c); > +} > + > +static void dsps_dma_controller_suspend(struct dsps_glue *glue) > +{ > + void __iomem *usbss_base = glue->usbss_base; > + > + musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP); > +} > + > +static void dsps_dma_controller_resume(struct dsps_glue *glue) > +{ > + void __iomem *usbss_base = glue->usbss_base; > + > + musb_writel(usbss_base, USBSS_IRQ_ENABLER, USBSS_IRQ_PD_COMP); > +} The two functions above need to be wrapped in CONFIG_PM_SLEEP. > +#else > +static void dsps_dma_controller_suspend(struct dsps_glue *glue) {} > +static void dsps_dma_controller_resume(struct dsps_glue *glue) {} > +#endif > + > static struct musb_platform_ops dsps_ops = { > .quirks = MUSB_DMA_CPPI41 | MUSB_INDEXED_EP, > .init = dsps_musb_init, > .exit = dsps_musb_exit, > > #ifdef CONFIG_USB_TI_CPPI41_DMA > - .dma_init = cppi41_dma_controller_create, > - .dma_exit = cppi41_dma_controller_destroy, > + .dma_init = dsps_dma_controller_create, > + .dma_exit = dsps_dma_controller_destroy, > #endif > .enable = dsps_musb_enable, > .disable = dsps_musb_disable, > @@ -792,6 +858,9 @@ static int dsps_probe(struct platform_device *pdev) > > glue->dev = &pdev->dev; > glue->wrp = wrp; > + glue->usbss_base = of_iomap(pdev->dev.parent->of_node, 0); > + if (!glue->usbss_base) use IS_ERR()? > + return -ENXIO; and return PTR_ERR()? Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2017 09:17 PM, Bin Liu wrote: > On Thu, Jan 19, 2017 at 11:06:59AM +0100, Alexandre Bailon wrote: >> Despite the CPPI 4.1 is a generic DMA, it is tied to USB. >> On the dsps, CPPI 4.1 interrupt's registers are in USBSS (the MUSB glue). >> Currently, to enable / disable and clear interrupts, the CPPI 4.1 driver >> maps and accesses to USBSS's register, which making CPPI 4.1 driver not >> really generic. >> Move the interrupt management to dsps driver. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> drivers/dma/cppi41.c | 28 ++++------------ >> drivers/usb/musb/musb_dsps.c | 77 ++++++++++++++++++++++++++++++++++++++++++-- >> 2 files changed, 82 insertions(+), 23 deletions(-) > > This patch touches both dma and musb modules, I know it makes review > easier, but how we get it merged? One maintainer ACK it and the other > pick it up? Sorry for the dumb question, I am new as a maintainer... > >> >> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c >> index d5ba43a..4999e7d 100644 >> --- a/drivers/dma/cppi41.c >> +++ b/drivers/dma/cppi41.c > > [...] > >> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c >> index 9f125e1..9dad3a6 100644 >> --- a/drivers/usb/musb/musb_dsps.c >> +++ b/drivers/usb/musb/musb_dsps.c >> @@ -121,6 +121,7 @@ struct dsps_glue { >> struct timer_list timer; /* otg_workaround timer */ >> unsigned long last_timer; /* last timer data for each instance */ >> bool sw_babble_enabled; >> + void __iomem *usbss_base; >> >> struct dsps_context context; >> struct debugfs_regset32 regset; >> @@ -145,6 +146,13 @@ static const struct debugfs_reg32 dsps_musb_regs[] = { >> { "mode", 0xe8 }, >> }; >> >> +/* USBSS / USB AM335x */ >> +#define USBSS_IRQ_STATUS 0x28 >> +#define USBSS_IRQ_ENABLER 0x2c >> +#define USBSS_IRQ_CLEARR 0x30 >> + >> +#define USBSS_IRQ_PD_COMP (1 << 2) > > Please fix the double white spaces bwteen '<' and '2' this time. > >> + >> /** >> * dsps_musb_enable - enable interrupts >> */ >> @@ -619,14 +627,72 @@ static void dsps_read_fifo32(struct musb_hw_ep *hw_ep, u16 len, u8 *dst) >> } >> } >> >> +#ifdef CONFIG_USB_TI_CPPI41_DMA >> +static void dsps_dma_controller_callback(struct dma_controller *c) >> +{ >> + struct musb *musb = c->musb; >> + struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent); >> + void __iomem *usbss_base = glue->usbss_base; >> + u32 status; >> + >> + status = musb_readl(usbss_base, USBSS_IRQ_STATUS); >> + if (status & USBSS_IRQ_PD_COMP) >> + musb_writel(usbss_base, USBSS_IRQ_STATUS, USBSS_IRQ_PD_COMP); >> +} >> + >> +static struct dma_controller * >> +dsps_dma_controller_create(struct musb *musb, void __iomem *base) >> +{ >> + struct dma_controller *controller; >> + struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent); >> + void __iomem *usbss_base = glue->usbss_base; >> + >> + controller = cppi41_dma_controller_create(musb, base); >> + if (IS_ERR_OR_NULL(controller)) >> + return controller; >> + >> + musb_writel(usbss_base, USBSS_IRQ_ENABLER, USBSS_IRQ_PD_COMP); >> + controller->dma_callback = dsps_dma_controller_callback; >> + >> + return controller; >> +} >> + >> +static void dsps_dma_controller_destroy(struct dma_controller *c) >> +{ >> + struct musb *musb = c->musb; >> + struct dsps_glue *glue = dev_get_drvdata(musb->controller->parent); >> + void __iomem *usbss_base = glue->usbss_base; >> + >> + musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP); >> + cppi41_dma_controller_destroy(c); >> +} >> + >> +static void dsps_dma_controller_suspend(struct dsps_glue *glue) >> +{ >> + void __iomem *usbss_base = glue->usbss_base; >> + >> + musb_writel(usbss_base, USBSS_IRQ_CLEARR, USBSS_IRQ_PD_COMP); >> +} >> + >> +static void dsps_dma_controller_resume(struct dsps_glue *glue) >> +{ >> + void __iomem *usbss_base = glue->usbss_base; >> + >> + musb_writel(usbss_base, USBSS_IRQ_ENABLER, USBSS_IRQ_PD_COMP); >> +} > > The two functions above need to be wrapped in CONFIG_PM_SLEEP. > >> +#else >> +static void dsps_dma_controller_suspend(struct dsps_glue *glue) {} >> +static void dsps_dma_controller_resume(struct dsps_glue *glue) {} >> +#endif >> + >> static struct musb_platform_ops dsps_ops = { >> .quirks = MUSB_DMA_CPPI41 | MUSB_INDEXED_EP, >> .init = dsps_musb_init, >> .exit = dsps_musb_exit, >> >> #ifdef CONFIG_USB_TI_CPPI41_DMA >> - .dma_init = cppi41_dma_controller_create, >> - .dma_exit = cppi41_dma_controller_destroy, >> + .dma_init = dsps_dma_controller_create, >> + .dma_exit = dsps_dma_controller_destroy, >> #endif >> .enable = dsps_musb_enable, >> .disable = dsps_musb_disable, >> @@ -792,6 +858,9 @@ static int dsps_probe(struct platform_device *pdev) >> >> glue->dev = &pdev->dev; >> glue->wrp = wrp; >> + glue->usbss_base = of_iomap(pdev->dev.parent->of_node, 0); >> + if (!glue->usbss_base) > > use IS_ERR()? I don't we can use it here. As far I know, of_iomap() only return NULL in the case of error. > >> + return -ENXIO; > > and return PTR_ERR()? Again, I don't think we can use it here. THough, may be -ENXIO is not the best error code to return. I used it because it was the used in DMA driver. > > Regards, > -Bin. > Regards, Alexandre -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2017 09:00 PM, Bin Liu wrote: > On Thu, Jan 19, 2017 at 11:06:57AM +0100, Alexandre Bailon wrote: >> Currently, the CPPI 4.1 driver is not completely generic and >> only work on dsps. This is because of IRQ management. >> Add a callback to dma_controller that could be invoked on DMA completion >> to acknodlege the IRQ. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> --- >> drivers/usb/musb/musb_cppi41.c | 7 +++++-- >> drivers/usb/musb/musb_dma.h | 4 ++++ >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c >> index 1636385..f7d3d27 100644 >> --- a/drivers/usb/musb/musb_cppi41.c >> +++ b/drivers/usb/musb/musb_cppi41.c >> @@ -217,6 +217,10 @@ static void cppi41_dma_callback(void *private_data) >> int is_hs = 0; >> bool empty; >> >> + controller = cppi41_channel->controller; >> + if (controller->controller.dma_callback) >> + controller->controller.dma_callback(&controller->controller); >> + >> spin_lock_irqsave(&musb->lock, flags); >> >> dmaengine_tx_status(cppi41_channel->dc, cppi41_channel->cookie, >> @@ -249,8 +253,6 @@ static void cppi41_dma_callback(void *private_data) >> * We spin on HS (no longer than than 25us and setup a timer on >> * FS to check for the bit and complete the transfer. >> */ >> - controller = cppi41_channel->controller; >> - >> if (is_host_active(musb)) { >> if (musb->port1_status & USB_PORT_STAT_HIGH_SPEED) >> is_hs = 1; >> @@ -695,6 +697,7 @@ cppi41_dma_controller_create(struct musb *musb, void __iomem *base) >> controller->controller.channel_program = cppi41_dma_channel_program; >> controller->controller.channel_abort = cppi41_dma_channel_abort; >> controller->controller.is_compatible = cppi41_is_compatible; >> + controller->controller.musb = musb; >> >> ret = cppi41_dma_controller_start(controller); >> if (ret) >> diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h >> index 46357e1..8bea0cd 100644 >> --- a/drivers/usb/musb/musb_dma.h >> +++ b/drivers/usb/musb/musb_dma.h >> @@ -181,10 +181,13 @@ dma_channel_status(struct dma_channel *c) >> * @channel_release: call this to release a DMA channel >> * @channel_abort: call this to abort a pending DMA transaction, >> * returning it to FREE (but allocated) state >> + * @platform_dma_callback: invoked on DMA completion, useful to run platform >> + * code such IRQ acknowledgment. >> * >> * Controllers manage dma channels. >> */ >> struct dma_controller { >> + struct musb *musb; > > Since musb is added here, can we clean up the corresponding one in > struct cppi41_dma_controller and struct cppi? Yes, and also for the one in tusb_omap_dma. > > Regards, > -Bin. > Regards, Alexandre -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 23, 2017 at 09:44:42AM -0800, Kevin Hilman wrote: > Bin Liu <b-liu@ti.com> writes: > > > On Thu, Jan 19, 2017 at 11:06:59AM +0100, Alexandre Bailon wrote: > >> Despite the CPPI 4.1 is a generic DMA, it is tied to USB. > >> On the dsps, CPPI 4.1 interrupt's registers are in USBSS (the MUSB glue). > >> Currently, to enable / disable and clear interrupts, the CPPI 4.1 driver > >> maps and accesses to USBSS's register, which making CPPI 4.1 driver not > >> really generic. > >> Move the interrupt management to dsps driver. > >> > >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > >> --- > >> drivers/dma/cppi41.c | 28 ++++------------ > >> drivers/usb/musb/musb_dsps.c | 77 ++++++++++++++++++++++++++++++++++++++++++-- > >> 2 files changed, 82 insertions(+), 23 deletions(-) > > > > This patch touches both dma and musb modules, I know it makes review > > easier, but how we get it merged? One maintainer ACK it and the other > > pick it up? Sorry for the dumb question, I am new as a maintainer... > > For patches where the change needs to go together, then one maintainer > can ack, and the other can merge. Alternately, if the patch can be done Ok, this is what I thought :) > in a way that the parts can go independently, that is sometimes > cleaner. I don't know this code well enough to know which is which. In this case it is better to keep all together, since it moves code from one place to another. I think it makes sense it goes to my tree, since the reset patches are for musb. Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html