Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Alexandre Bailon]

On 08/15/2017 12:12 PM, Sergei Shtylyov wrote:
> Hello!
>
> On 8/13/2017 3:04 PM, Alexandre Bailon wrote:
>
>> The DA8xx and DSPS platforms don't use the same address for few
>> registers.
>> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
>> Configure the address of the register during the init and use them
>> instead
>> of constants.
>>
>> Reported-by: nsek...@ti.com
>> Signed-off-by: Alexandre Bailon 
>> ---
>>   drivers/usb/musb/musb_cppi41.c | 23 +++
>>   1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_cppi41.c
>> b/drivers/usb/musb/musb_cppi41.c
>> index ba255280a624..dbff0e0a4ff5 100644
>> --- a/drivers/usb/musb/musb_cppi41.c
>> +++ b/drivers/usb/musb/musb_cppi41.c
>> @@ -26,6 +26,9 @@
>> #define MUSB_DMA_NUM_CHANNELS 15
>>   +#define DA8XX_USB_AUTOREQ_REG0x14
>> +#define DA8XX_USB_TEARDOWN_REG0x1c
>
>Why these _REG suffixes suddenly?
I took these defines in da8xx.c.
Actually, I think I should make a third patch to remove them from
da8xx.c because they are not even used.
>
> [...]
>
>Other than that looks sane. Need my ACK?
I don't know if that is needed but I guess it would be appreciated.

Best Regards,
Alexandre
>
> WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Alexandre Bailon]

Hi Bin, Sekhar
On 09/07/2017 07:16 PM, Bin Liu wrote:
> On Mon, Sep 04, 2017 at 06:32:11PM +0530, Sekhar Nori wrote:
>> On Monday 14 August 2017 07:06 PM, Sekhar Nori wrote:
>>> On Monday 14 August 2017 07:01 PM, Sekhar Nori wrote:
 Hi,

 On Sunday 13 August 2017 05:34 PM, Alexandre Bailon wrote:
> The DA8xx and DSPS platforms don't use the same address for few registers.
> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
> Configure the address of the register during the init and use them instead
> of constants.
>
> Reported-by: nsek...@ti.com
>>> Reported-by: Sekhar Nori 
>> Tested-by: Sekhar Nori 
>>
>> Hi Bin,
>>
>> Do you have any additional comments on this series or are you waiting
>> for v2 to be posted?
> I don't have other comments, just am waiting for v2.
My apologize. I was in vacation. I will send the v2 soon.

Best Regards,
Alexandre
>
> Regards,
> -Bin.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Bin Liu]

On Mon, Sep 04, 2017 at 06:32:11PM +0530, Sekhar Nori wrote:
> On Monday 14 August 2017 07:06 PM, Sekhar Nori wrote:
> > On Monday 14 August 2017 07:01 PM, Sekhar Nori wrote:
> >> Hi,
> >>
> >> On Sunday 13 August 2017 05:34 PM, Alexandre Bailon wrote:
> >>> The DA8xx and DSPS platforms don't use the same address for few registers.
> >>> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
> >>> Configure the address of the register during the init and use them instead
> >>> of constants.
> >>>
> >>> Reported-by: nsek...@ti.com
> > 
> > Reported-by: Sekhar Nori 
> 
> Tested-by: Sekhar Nori 
> 
> Hi Bin,
> 
> Do you have any additional comments on this series or are you waiting
> for v2 to be posted?

I don't have other comments, just am waiting for v2.

Regards,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Sekhar Nori]

On Monday 14 August 2017 07:06 PM, Sekhar Nori wrote:
> On Monday 14 August 2017 07:01 PM, Sekhar Nori wrote:
>> Hi,
>>
>> On Sunday 13 August 2017 05:34 PM, Alexandre Bailon wrote:
>>> The DA8xx and DSPS platforms don't use the same address for few registers.
>>> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
>>> Configure the address of the register during the init and use them instead
>>> of constants.
>>>
>>> Reported-by: nsek...@ti.com
> 
> Reported-by: Sekhar Nori 

Tested-by: Sekhar Nori 

Hi Bin,

Do you have any additional comments on this series or are you waiting
for v2 to be posted?

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Sergei Shtylyov]

Hello!

On 8/13/2017 3:04 PM, Alexandre Bailon wrote:


The DA8xx and DSPS platforms don't use the same address for few registers.
On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
Configure the address of the register during the init and use them instead
of constants.

Reported-by: nsek...@ti.com
Signed-off-by: Alexandre Bailon 
---
  drivers/usb/musb/musb_cppi41.c | 23 +++
  1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index ba255280a624..dbff0e0a4ff5 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -26,6 +26,9 @@
  
  #define MUSB_DMA_NUM_CHANNELS 15
  
+#define DA8XX_USB_AUTOREQ_REG	0x14

+#define DA8XX_USB_TEARDOWN_REG 0x1c


   Why these _REG suffixes suddenly?

[...]

   Other than that looks sane. Need my ACK?

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Sekhar Nori]

On Monday 14 August 2017 07:01 PM, Sekhar Nori wrote:
> Hi,
> 
> On Sunday 13 August 2017 05:34 PM, Alexandre Bailon wrote:
>> The DA8xx and DSPS platforms don't use the same address for few registers.
>> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
>> Configure the address of the register during the init and use them instead
>> of constants.
>>
>> Reported-by: nsek...@ti.com

Reported-by: Sekhar Nori 

>> Signed-off-by: Alexandre Bailon 
>> ---
>>  drivers/usb/musb/musb_cppi41.c | 23 +++
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
>> index ba255280a624..dbff0e0a4ff5 100644
>> --- a/drivers/usb/musb/musb_cppi41.c
>> +++ b/drivers/usb/musb/musb_cppi41.c
>> @@ -26,6 +26,9 @@
>>  
>>  #define MUSB_DMA_NUM_CHANNELS 15
>>  
>> +#define DA8XX_USB_AUTOREQ_REG   0x14
>> +#define DA8XX_USB_TEARDOWN_REG  0x1c
> 
> Nice catch. I noticed that the USB_CTRL_TX_MODE and USB_CTRL_RX_MODE are
> incorrect for DA8xx too. Perhaps those should be fixed too.

Ah, just read 2/2 now, and seems like thats exactly what you are
handling there.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Sekhar Nori]

Hi,

On Sunday 13 August 2017 05:34 PM, Alexandre Bailon wrote:
> The DA8xx and DSPS platforms don't use the same address for few registers.
> On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
> Configure the address of the register during the init and use them instead
> of constants.
> 
> Reported-by: nsek...@ti.com
> Signed-off-by: Alexandre Bailon 
> ---
>  drivers/usb/musb/musb_cppi41.c | 23 +++
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
> index ba255280a624..dbff0e0a4ff5 100644
> --- a/drivers/usb/musb/musb_cppi41.c
> +++ b/drivers/usb/musb/musb_cppi41.c
> @@ -26,6 +26,9 @@
>  
>  #define MUSB_DMA_NUM_CHANNELS 15
>  
> +#define DA8XX_USB_AUTOREQ_REG0x14
> +#define DA8XX_USB_TEARDOWN_REG   0x1c

Nice catch. I noticed that the USB_CTRL_TX_MODE and USB_CTRL_RX_MODE are
incorrect for DA8xx too. Perhaps those should be fixed too.

Also, since the original code does not use the _REG suffix for register
offsets, perhaps the new defines should avoid those as well for consistency.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] usb: musb: musb_cppi41: Fix the address of teardown and autoreq registers

[Alexandre Bailon]

The DA8xx and DSPS platforms don't use the same address for few registers.
On Da8xx, this is causing some issues (e.g. teardown that doesn't work).
Configure the address of the register during the init and use them instead
of constants.

Reported-by: nsek...@ti.com
Signed-off-by: Alexandre Bailon 
---
 drivers/usb/musb/musb_cppi41.c | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index ba255280a624..dbff0e0a4ff5 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -26,6 +26,9 @@
 
 #define MUSB_DMA_NUM_CHANNELS 15
 
+#define DA8XX_USB_AUTOREQ_REG  0x14
+#define DA8XX_USB_TEARDOWN_REG 0x1c
+
 struct cppi41_dma_controller {
struct dma_controller controller;
struct cppi41_dma_channel rx_channel[MUSB_DMA_NUM_CHANNELS];
@@ -35,6 +38,9 @@ struct cppi41_dma_controller {
u32 rx_mode;
u32 tx_mode;
u32 auto_req;
+
+   u32 tdown_reg;
+   u32 autoreq_reg;
 };
 
 static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)
@@ -364,8 +370,8 @@ static void cppi41_set_autoreq_mode(struct 
cppi41_dma_channel *cppi41_channel,
if (new_mode == old_mode)
return;
controller->auto_req = new_mode;
-   musb_writel(controller->controller.musb->ctrl_base, USB_CTRL_AUTOREQ,
-   new_mode);
+   musb_writel(controller->controller.musb->ctrl_base,
+   controller->autoreq_reg, new_mode);
 }
 
 static bool cppi41_configure_channel(struct dma_channel *channel,
@@ -581,12 +587,13 @@ static int cppi41_dma_channel_abort(struct dma_channel 
*channel)
 
do {
if (is_tx)
-   musb_writel(musb->ctrl_base, USB_TDOWN, tdbit);
+   musb_writel(musb->ctrl_base, controller->tdown_reg,
+   tdbit);
ret = dmaengine_terminate_all(cppi41_channel->dc);
} while (ret == -EAGAIN);
 
if (is_tx) {
-   musb_writel(musb->ctrl_base, USB_TDOWN, tdbit);
+   musb_writel(musb->ctrl_base, controller->tdown_reg, tdbit);
 
csr = musb_readw(epio, MUSB_TXCSR);
if (csr & MUSB_TXCSR_TXPKTRDY) {
@@ -727,6 +734,14 @@ cppi41_dma_controller_create(struct musb *musb, void 
__iomem *base)
controller->controller.is_compatible = cppi41_is_compatible;
controller->controller.musb = musb;
 
+   if (musb->io.quirks & MUSB_DA8XX) {
+   controller->tdown_reg = DA8XX_USB_TEARDOWN_REG;
+   controller->autoreq_reg = DA8XX_USB_AUTOREQ_REG;
+   } else {
+   controller->tdown_reg = USB_TDOWN;
+   controller->autoreq_reg = USB_CTRL_AUTOREQ;
+   }
+
ret = cppi41_dma_controller_start(controller);
if (ret)
goto plat_get_fail;
-- 
2.13.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html