| Message ID | 1506793068-27445-3-git-send-email-alagusankar@silex-india.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alagu, On Sat, Sep 30, 2017 at 10:37 AM, <silexcommon@gmail.com> wrote: > > From: Alagu Sankar <alagusankar@silex-india.com> > > 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 <alagusankar@silex-india.com> > --- > 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
Hi Steve, On 2017-10-02 04:17, Steve deRosier wrote: > Hi Alagu, > > > On Sat, Sep 30, 2017 at 10:37 AM, <silexcommon@gmail.com> wrote: >> >> From: Alagu Sankar <alagusankar@silex-india.com> >> >> 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 <alagusankar@silex-india.com> >> --- >> 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
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, <silexcommon@gmail.com> wrote: >>> >>> From: Alagu Sankar <alagusankar@silex-india.com> >>> >>> 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 <alagusankar@silex-india.com> >>> --- >>> 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: <description>" or perhaps just: "ath10k: <description>" > Best Regards, > Alagu Sankar > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k
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) {