Re: [08/11] ath10k_sdio: common read write

2017-12-08 Thread Gary Bisson 
Hi Alagu,

On Thu, Oct 05, 2017 at 11:03:12PM +0530, Alagu Sankar wrote:
> Hi Gary,
> 
> 
> On 2017-10-05 15:39, Gary Bisson wrote:
> > Hi Alagu,
> > 
> > On Sat, Sep 30, 2017 at 11:07:45PM +0530, silexcom...@gmail.com wrote:
> > > From: Alagu Sankar 
> > > 
> > > convert different read write functions in sdio hif to bring it under a
> > > single read-write path. This helps in having a common dma bounce
> > > buffer
> > > implementation. Also helps in address modification that is required
> > > specific to change in certain mbox addresses of sdio_write.
> > > 
> > > Signed-off-by: Alagu Sankar 
> > > ---
> > >  drivers/net/wireless/ath/ath10k/sdio.c | 131
> > > -
> > >  1 file changed, 64 insertions(+), 67 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/ath/ath10k/sdio.c
> > > b/drivers/net/wireless/ath/ath10k/sdio.c
> > > index 77d4fa4..bb6fa67 100644
> > > --- a/drivers/net/wireless/ath/ath10k/sdio.c
> > > +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> > > @@ -36,6 +36,11 @@
> > > 
> > >  #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
> > > 
> > > +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
> > > + u32 len, bool incr);
> > > +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const
> > > void *buf,
> > > +  u32 len, bool incr);
> > > +
> > 
> > As mentioned by Kalle, u32 needs to be size_t.
> Yes, the compiler I used is probably a step older and did not catch this.
> > 
> > >  /* inlined helper functions */
> > > 
> > >  /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
> > > @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
> > >   struct sdio_func *func = ar_sdio->func;
> > >   unsigned char byte, asyncintdelay = 2;
> > >   int ret;
> > > + u32 addr;
> > > 
> > >   ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");
> > > 
> > > @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
> > >CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
> > >CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);
> > > 
> > > - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
> > > -   
> > > CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> > > -   byte);
> > > + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> > > + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);
> > 
> > Not sure this part is needed.
> This is to overcome checkpatch warning. Too big a names for the function and
> macro
> not helping in there. Will have to move it as a separate patch.
> > 
> > >   if (ret) {
> > >   ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
> > >   goto out;
> > > @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)
> > > 
> > >  static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
> > >  {
> > > - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> > > - struct sdio_func *func = ar_sdio->func;
> > > + __le32 *buf;
> > >   int ret;
> > > 
> > > - sdio_claim_host(func);
> > > + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > 
> > > - sdio_writel(func, val, addr, &ret);
> > > + *buf = cpu_to_le32(val);
> > > +
> > > + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);
> > 
> > Shouldn't we use buf instead of val? buf seems pretty useless otherwise.
> Yes, thanks for pointing this out. will be corrected in v2.

Do you have any timeframe on the v2?

Regards,
Gary

Re: [08/11] ath10k_sdio: common read write

2017-10-05 Thread Alagu Sankar 

Hi Gary,


On 2017-10-05 15:39, Gary Bisson wrote:

Hi Alagu,

On Sat, Sep 30, 2017 at 11:07:45PM +0530, silexcom...@gmail.com wrote:

From: Alagu Sankar 

convert different read write functions in sdio hif to bring it under a
single read-write path. This helps in having a common dma bounce 
buffer

implementation. Also helps in address modification that is required
specific to change in certain mbox addresses of sdio_write.

Signed-off-by: Alagu Sankar 
---
 drivers/net/wireless/ath/ath10k/sdio.c | 131 
-

 1 file changed, 64 insertions(+), 67 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c

index 77d4fa4..bb6fa67 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -36,6 +36,11 @@

 #define ATH10K_SDIO_DMA_BUF_SIZE   (32 * 1024)

+static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
+   u32 len, bool incr);
+static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void 
*buf,

+u32 len, bool incr);
+


As mentioned by Kalle, u32 needs to be size_t.
Yes, the compiler I used is probably a step older and did not catch 
this.



 /* inlined helper functions */

 /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
@@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
struct sdio_func *func = ar_sdio->func;
unsigned char byte, asyncintdelay = 2;
int ret;
+   u32 addr;

ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");

@@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
 CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
 CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);

-   ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
- 
CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
- byte);
+   addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
+   ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);


Not sure this part is needed.
This is to overcome checkpatch warning. Too big a names for the function 
and macro

not helping in there. Will have to move it as a separate patch.



if (ret) {
ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
goto out;
@@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)

 static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
 {
-   struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
-   struct sdio_func *func = ar_sdio->func;
+   __le32 *buf;
int ret;

-   sdio_claim_host(func);
+   buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+   if (!buf)
+   return -ENOMEM;

-   sdio_writel(func, val, addr, &ret);
+   *buf = cpu_to_le32(val);
+
+   ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);


Shouldn't we use buf instead of val? buf seems pretty useless 
otherwise.

Yes, thanks for pointing this out. will be corrected in v2.


Regards,
Gary

___
ath10k mailing list
ath...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [08/11] ath10k_sdio: common read write

2017-10-05 Thread Gary Bisson 
Hi Alagu,

On Sat, Sep 30, 2017 at 11:07:45PM +0530, silexcom...@gmail.com wrote:
> From: Alagu Sankar 
> 
> convert different read write functions in sdio hif to bring it under a
> single read-write path. This helps in having a common dma bounce buffer
> implementation. Also helps in address modification that is required
> specific to change in certain mbox addresses of sdio_write.
> 
> Signed-off-by: Alagu Sankar 
> ---
>  drivers/net/wireless/ath/ath10k/sdio.c | 131 
> -
>  1 file changed, 64 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
> b/drivers/net/wireless/ath/ath10k/sdio.c
> index 77d4fa4..bb6fa67 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -36,6 +36,11 @@
>  
>  #define ATH10K_SDIO_DMA_BUF_SIZE (32 * 1024)
>  
> +static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
> + u32 len, bool incr);
> +static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
> +  u32 len, bool incr);
> +

As mentioned by Kalle, u32 needs to be size_t.

>  /* inlined helper functions */
>  
>  /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
> @@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
>   struct sdio_func *func = ar_sdio->func;
>   unsigned char byte, asyncintdelay = 2;
>   int ret;
> + u32 addr;
>  
>   ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");
>  
> @@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
>CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
>CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);
>  
> - ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
> -   
> CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> -   byte);
> + addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
> + ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);

Not sure this part is needed.

>   if (ret) {
>   ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
>   goto out;
> @@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)
>  
>  static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
>  {
> - struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> - struct sdio_func *func = ar_sdio->func;
> + __le32 *buf;
>   int ret;
>  
> - sdio_claim_host(func);
> + buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
>  
> - sdio_writel(func, val, addr, &ret);
> + *buf = cpu_to_le32(val);
> +
> + ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);

Shouldn't we use buf instead of val? buf seems pretty useless otherwise.

Regards,
Gary

Re: [PATCH 08/11] ath10k_sdio: common read write

2017-10-04 Thread Kalle Valo 
silexcom...@gmail.com writes:

> From: Alagu Sankar 
>
> convert different read write functions in sdio hif to bring it under a
> single read-write path. This helps in having a common dma bounce buffer
> implementation. Also helps in address modification that is required
> specific to change in certain mbox addresses of sdio_write.
>
> Signed-off-by: Alagu Sankar 

This didn't compile for me:

drivers/net/wireless/ath/ath10k/sdio.c:320:12: error: conflicting types for 
'ath10k_sdio_read'
drivers/net/wireless/ath/ath10k/sdio.c:39:12: note: previous declaration of 
'ath10k_sdio_read' was here
drivers/net/wireless/ath/ath10k/sdio.c:365:12: error: conflicting types for 
'ath10k_sdio_write'
drivers/net/wireless/ath/ath10k/sdio.c:41:12: note: previous declaration of 
'ath10k_sdio_write' was here
drivers/net/wireless/ath/ath10k/sdio.c:39:12: warning: 'ath10k_sdio_read' used 
but never defined
drivers/net/wireless/ath/ath10k/sdio.c:41:12: warning: 'ath10k_sdio_write' used 
but never defined

gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

I fixed it like below in the pending branch. But I'll review more
carefully later, I have quite a lot of patches pending right now.

--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -37,9 +37,9 @@
 #define ATH10K_SDIO_DMA_BUF_SIZE   (32 * 1024)
 
 static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
-   u32 len, bool incr);
+   size_t len, bool incr);
 static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
-u32 len, bool incr);
+size_t len, bool incr);
 
 /* inlined helper functions */

-- 
Kalle Valo

[PATCH 08/11] ath10k_sdio: common read write

2017-09-30 Thread silexcommon 
From: Alagu Sankar 

convert different read write functions in sdio hif to bring it under a
single read-write path. This helps in having a common dma bounce buffer
implementation. Also helps in address modification that is required
specific to change in certain mbox addresses of sdio_write.

Signed-off-by: Alagu Sankar 
---
 drivers/net/wireless/ath/ath10k/sdio.c | 131 -
 1 file changed, 64 insertions(+), 67 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c 
b/drivers/net/wireless/ath/ath10k/sdio.c
index 77d4fa4..bb6fa67 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -36,6 +36,11 @@
 
 #define ATH10K_SDIO_DMA_BUF_SIZE   (32 * 1024)
 
+static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
+   u32 len, bool incr);
+static int ath10k_sdio_write(struct ath10k *ar, u32 addr, const void *buf,
+u32 len, bool incr);
+
 /* inlined helper functions */
 
 /* Macro to check if DMA buffer is WORD-aligned and DMA-able.
@@ -152,6 +157,7 @@ static int ath10k_sdio_config(struct ath10k *ar)
struct sdio_func *func = ar_sdio->func;
unsigned char byte, asyncintdelay = 2;
int ret;
+   u32 addr;
 
ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio configuration\n");
 
@@ -180,9 +186,8 @@ static int ath10k_sdio_config(struct ath10k *ar)
 CCCR_SDIO_DRIVER_STRENGTH_ENABLE_C |
 CCCR_SDIO_DRIVER_STRENGTH_ENABLE_D);
 
-   ret = ath10k_sdio_func0_cmd52_wr_byte(func->card,
- 
CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
- byte);
+   addr = CCCR_SDIO_DRIVER_STRENGTH_ENABLE_ADDR,
+   ret = ath10k_sdio_func0_cmd52_wr_byte(func->card, addr, byte);
if (ret) {
ath10k_warn(ar, "failed to enable driver strength: %d\n", ret);
goto out;
@@ -233,13 +238,16 @@ static int ath10k_sdio_config(struct ath10k *ar)
 
 static int ath10k_sdio_write32(struct ath10k *ar, u32 addr, u32 val)
 {
-   struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
-   struct sdio_func *func = ar_sdio->func;
+   __le32 *buf;
int ret;
 
-   sdio_claim_host(func);
+   buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+   if (!buf)
+   return -ENOMEM;
 
-   sdio_writel(func, val, addr, &ret);
+   *buf = cpu_to_le32(val);
+
+   ret = ath10k_sdio_write(ar, addr, &val, sizeof(val), true);
if (ret) {
ath10k_warn(ar, "failed to write 0x%x to address 0x%x: %d\n",
val, addr, ret);
@@ -250,15 +258,13 @@ static int ath10k_sdio_write32(struct ath10k *ar, u32 
addr, u32 val)
   addr, val);
 
 out:
-   sdio_release_host(func);
+   kfree(buf);
 
return ret;
 }
 
 static int ath10k_sdio_writesb32(struct ath10k *ar, u32 addr, u32 val)
 {
-   struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
-   struct sdio_func *func = ar_sdio->func;
__le32 *buf;
int ret;
 
@@ -268,9 +274,7 @@ static int ath10k_sdio_writesb32(struct ath10k *ar, u32 
addr, u32 val)
 
*buf = cpu_to_le32(val);
 
-   sdio_claim_host(func);
-
-   ret = sdio_writesb(func, addr, buf, sizeof(*buf));
+   ret = ath10k_sdio_write(ar, addr, buf, sizeof(*buf), false);
if (ret) {
ath10k_warn(ar, "failed to write value 0x%x to fixed sb address 
0x%x: %d\n",
val, addr, ret);
@@ -281,8 +285,6 @@ static int ath10k_sdio_writesb32(struct ath10k *ar, u32 
addr, u32 val)
   addr, val);
 
 out:
-   sdio_release_host(func);
-
kfree(buf);
 
return ret;
@@ -290,28 +292,33 @@ static int ath10k_sdio_writesb32(struct ath10k *ar, u32 
addr, u32 val)
 
 static int ath10k_sdio_read32(struct ath10k *ar, u32 addr, u32 *val)
 {
-   struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
-   struct sdio_func *func = ar_sdio->func;
+   __le32 *buf;
int ret;
 
-   sdio_claim_host(func);
-   *val = sdio_readl(func, addr, &ret);
+   buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+   if (!buf)
+   return -ENOMEM;
+
+   ret = ath10k_sdio_read(ar, addr, buf, sizeof(*val), true);
if (ret) {
ath10k_warn(ar, "failed to read from address 0x%x: %d\n",
addr, ret);
goto out;
}
 
+   *val = le32_to_cpu(*buf);
+
ath10k_dbg(ar, ATH10K_DBG_SDIO, "sdio read32 addr 0x%x val 0x%x\n",
   addr, *val);
 
 out:
-   sdio_release_host(func);
+   kfree(buf);
 
return ret;
 }
 
-static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf, size_t len)
+static int ath10k_sdio_read(struct ath10k *ar, u32 addr, void *buf,
+   size_t len, bool incr)
 {
struct ath10k_sdio *ar_sdio = ath1