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@xxxxxxxxxxxx>
>> ---
>> 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 dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
