Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

[Erik Stromdahl]

Hi Alagu,

On 2017-10-02 09:02, Alagu Sankar wrote:

Hi Steve,

On 2017-10-02 04:17, Steve deRosier wrote:

Hi Alagu,


On Sat, Sep 30, 2017 at 10:37 AM,  wrote:


From: Alagu Sankar 

The QCA9377-3 WB396 sdio reference card does not get initialized
due to the conflict in uart gpio pins. This fix is not required
for other QCA9377 sdio cards.

Signed-off-by: Alagu Sankar 
---
 drivers/net/wireless/ath/ath10k/core.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index b4f66cd..86247c8 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
    return ret;
    }

-   if (!uart_print)
+   if (!uart_print) {
+   /* Hack: override dbg TX pin to avoid side effects of default
+    * GPIO_6 in QCA9377 WB396 reference card
+    */
+   if (ar->hif.bus == ATH10K_BUS_SDIO)
+   ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
+  ar->hw_params.uart_pin);


If it is indeed a "hack", then I don't think the maintainer should
accept this upstream. If you want it upstream you need a clean enough
implementation that doesn't need to be labeled a "hack".


It is a hack as per the qcacld reference driver.


Your commit message states that this is only needed for a very
specific card and not for other QCA9377 sdio cards. Yet, you're doing
this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
quirk and it's limited to a particular implementation of the device.
My suggestion: if it can be automatically determined, then do so
explicitly. If not, then it needs to be a DT setting or a module
parameter or something like that so the platform maker can decide to
do it. Having it affect all users of a SDIO QCA9377 when it doesn't
apply doesn't seem like a good idea to me.


- Steve


Got it. The qcacld reference driver had it for all the QCA9377 sdio cards.
But we found it to be a problem only for the WB396 reference card. Will
have this checked again and release a v2 patch accordingly.


While you are at it, you might as well change the commit comments to:

"ath10k: sdio: "

or perhaps just:

"ath10k: "


Best Regards,
Alagu Sankar

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

Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

[Alagu Sankar]

Hi Steve,

On 2017-10-02 04:17, Steve deRosier wrote:

Hi Alagu,


On Sat, Sep 30, 2017 at 10:37 AM,  wrote:


From: Alagu Sankar 

The QCA9377-3 WB396 sdio reference card does not get initialized
due to the conflict in uart gpio pins. This fix is not required
for other QCA9377 sdio cards.

Signed-off-by: Alagu Sankar 
---
 drivers/net/wireless/ath/ath10k/core.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

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

index b4f66cd..86247c8 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
return ret;
}

-   if (!uart_print)
+   if (!uart_print) {
+   /* Hack: override dbg TX pin to avoid side effects of 
default

+* GPIO_6 in QCA9377 WB396 reference card
+*/
+   if (ar->hif.bus == ATH10K_BUS_SDIO)
+   ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
+  ar->hw_params.uart_pin);


If it is indeed a "hack", then I don't think the maintainer should
accept this upstream. If you want it upstream you need a clean enough
implementation that doesn't need to be labeled a "hack".


It is a hack as per the qcacld reference driver.


Your commit message states that this is only needed for a very
specific card and not for other QCA9377 sdio cards. Yet, you're doing
this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
quirk and it's limited to a particular implementation of the device.
My suggestion: if it can be automatically determined, then do so
explicitly. If not, then it needs to be a DT setting or a module
parameter or something like that so the platform maker can decide to
do it. Having it affect all users of a SDIO QCA9377 when it doesn't
apply doesn't seem like a good idea to me.


- Steve


Got it. The qcacld reference driver had it for all the QCA9377 sdio 
cards.

But we found it to be a problem only for the WB396 reference card. Will
have this checked again and release a v2 patch accordingly.

Best Regards,
Alagu Sankar

Re: [PATCH 02/11] ath10k_sdio: wb396 reference card fix

[Steve deRosier]

Hi Alagu,


On Sat, Sep 30, 2017 at 10:37 AM,  wrote:
>
> From: Alagu Sankar 
>
> The QCA9377-3 WB396 sdio reference card does not get initialized
> due to the conflict in uart gpio pins. This fix is not required
> for other QCA9377 sdio cards.
>
> Signed-off-by: Alagu Sankar 
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index b4f66cd..86247c8 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
> return ret;
> }
>
> -   if (!uart_print)
> +   if (!uart_print) {
> +   /* Hack: override dbg TX pin to avoid side effects of default
> +* GPIO_6 in QCA9377 WB396 reference card
> +*/
> +   if (ar->hif.bus == ATH10K_BUS_SDIO)
> +   ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
> +  ar->hw_params.uart_pin);

If it is indeed a "hack", then I don't think the maintainer should
accept this upstream. If you want it upstream you need a clean enough
implementation that doesn't need to be labeled a "hack".

Your commit message states that this is only needed for a very
specific card and not for other QCA9377 sdio cards. Yet, you're doing
this for all ATH10K_BUS_SDIO devices. Not good. I think that it's a
quirk and it's limited to a particular implementation of the device.
My suggestion: if it can be automatically determined, then do so
explicitly. If not, then it needs to be a DT setting or a module
parameter or something like that so the platform maker can decide to
do it. Having it affect all users of a SDIO QCA9377 when it doesn't
apply doesn't seem like a good idea to me.


- Steve

[PATCH 02/11] ath10k_sdio: wb396 reference card fix

[silexcommon]

From: Alagu Sankar 

The QCA9377-3 WB396 sdio reference card does not get initialized
due to the conflict in uart gpio pins. This fix is not required
for other QCA9377 sdio cards.

Signed-off-by: Alagu Sankar 
---
 drivers/net/wireless/ath/ath10k/core.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index b4f66cd..86247c8 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1708,8 +1708,15 @@ static int ath10k_init_uart(struct ath10k *ar)
return ret;
}
 
-   if (!uart_print)
+   if (!uart_print) {
+   /* Hack: override dbg TX pin to avoid side effects of default
+* GPIO_6 in QCA9377 WB396 reference card
+*/
+   if (ar->hif.bus == ATH10K_BUS_SDIO)
+   ath10k_bmi_write32(ar, hi_dbg_uart_txpin,
+  ar->hw_params.uart_pin);
return 0;
+   }
 
ret = ath10k_bmi_write32(ar, hi_dbg_uart_txpin, ar->hw_params.uart_pin);
if (ret) {
-- 
1.9.1