From: Balakrishna Godavarthi <bgodavar@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
Marcel Holtmann <marcel@holtmann.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
linux-arm-msm@vger.kernel.org,
linux-bluetooth-owner@vger.kernel.org, hemantg@codeaurora.org,
Harish Bandi <c-hbandi@codeaurora.org>,
Stephen Boyd <swboyd@chromium.org>
Subject: Re: [PATCH 2/4] Bluetooth: hci_qca: Don't vote for specific voltage
Date: Wed, 30 Oct 2019 11:48:11 +0530 [thread overview]
Message-ID: <2ab1d48ef09d779c702cf0342c5094ee@codeaurora.org> (raw)
In-Reply-To: <20191022171505.GK20212@google.com>
Hi Matthias,
On 2019-10-22 22:45, Matthias Kaehlcke wrote:
> On Tue, Oct 22, 2019 at 11:35:43AM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias, Bjorn andresson,
>>
>> On 2019-10-21 12:07, Harish Bandi wrote:
>> > + Bala
>> >
>> > On 2019-10-18 23:52, Matthias Kaehlcke wrote:
>> > > On Thu, Oct 17, 2019 at 10:24:02PM -0700, Bjorn Andersson wrote:
>> > > > Devices with specific voltage requirements should not request voltage
>> > > > from the driver, but instead rely on the system configuration to
>> > > > define
>> > > > appropriate voltages for each rail.
>> > > >
>> > > > This ensures that PMIC and board variations are accounted for,
>> > > > something
>> > > > that the 0.1V range in the hci_qca driver currently tries to address.
>> > > > But on the Lenovo Yoga C630 (with wcn3990) vddch0 is 3.1V, which
>> > > > means
>> > > > the driver will fail to set the voltage.
>> > > >
>> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> > > > ---
>> > > > drivers/bluetooth/hci_qca.c | 26 ++++++++------------------
>> > > > 1 file changed, 8 insertions(+), 18 deletions(-)
>> > > >
>> > > > diff --git a/drivers/bluetooth/hci_qca.c
>> > > > b/drivers/bluetooth/hci_qca.c
>> > > > index c07c529b0d81..54aafcc69d06 100644
>> > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > @@ -130,8 +130,6 @@ enum qca_speed_type {
>> > > > */
>> > > > struct qca_vreg {
>> > > > const char *name;
>> > > > - unsigned int min_uV;
>> > > > - unsigned int max_uV;
>> > > > unsigned int load_uA;
>> > > > };
>> > > >
>> > > > @@ -1332,10 +1330,10 @@ static const struct hci_uart_proto
>> > > > qca_proto = {
>> > > > static const struct qca_vreg_data qca_soc_data_wcn3990 = {
>> > > > .soc_type = QCA_WCN3990,
>> > > > .vregs = (struct qca_vreg []) {
>> > > > - { "vddio", 1800000, 1900000, 15000 },
>> > > > - { "vddxo", 1800000, 1900000, 80000 },
>> > > > - { "vddrf", 1300000, 1350000, 300000 },
>> > > > - { "vddch0", 3300000, 3400000, 450000 },
>> > > > + { "vddio", 15000 },
>> > > > + { "vddxo", 80000 },
>> > > > + { "vddrf", 300000 },
>> > > > + { "vddch0", 450000 },
>> > > > },
>> > > > .num_vregs = 4,
>> > > > };
>> > > > @@ -1343,10 +1341,10 @@ static const struct qca_vreg_data
>> > > > qca_soc_data_wcn3990 = {
>> > > > static const struct qca_vreg_data qca_soc_data_wcn3998 = {
>> > > > .soc_type = QCA_WCN3998,
>> > > > .vregs = (struct qca_vreg []) {
>> > > > - { "vddio", 1800000, 1900000, 10000 },
>> > > > - { "vddxo", 1800000, 1900000, 80000 },
>> > > > - { "vddrf", 1300000, 1352000, 300000 },
>> > > > - { "vddch0", 3300000, 3300000, 450000 },
>> > > > + { "vddio", 10000 },
>> > > > + { "vddxo", 80000 },
>> > > > + { "vddrf", 300000 },
>> > > > + { "vddch0", 450000 },
>> > > > },
>> > > > .num_vregs = 4,
>> > > > };
>> > > > @@ -1386,13 +1384,6 @@ static int qca_power_off(struct hci_dev *hdev)
>> > > > static int qca_enable_regulator(struct qca_vreg vregs,
>> > > > struct regulator *regulator)
>> > > > {
>> > > > - int ret;
>> > > > -
>> > > > - ret = regulator_set_voltage(regulator, vregs.min_uV,
>> > > > - vregs.max_uV);
>> > > > - if (ret)
>> > > > - return ret;
>> > > > -
>> > > > return regulator_enable(regulator);
>> > > >
>> > > > }
>> > > > @@ -1401,7 +1392,6 @@ static void qca_disable_regulator(struct
>> > > > qca_vreg vregs,
>> > > > struct regulator *regulator)
>> > > > {
>> > > > regulator_disable(regulator);
>> > > > - regulator_set_voltage(regulator, 0, vregs.max_uV);
>> > > >
>> > > > }
>> > >
>> > > This was brought up multiple times during the initial review, but
>> > > wasn't addressed.
>> > >
>> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>
>>
>> yes true PMIC dts regulator should do this.
>> But we have some real time issues observed.
>>
>> Issue 1:
>>
>> In PMIC dts node, ASAIK we have three levels of voltages.
>>
>> 1. Default voltage.
>> 2. Minimum voltage. (mandatory entry)
>> 3. Maximum voltage. (mandatory entry)
>>
>> Let us assume that the if PMIC regulator dts node supports defaults
>> voltage
>> to 3.2 Volts and Min as 3.1 V and max as 3.3V
>> So default operating voltage is 3.1V when we turn on BT (but
>> according to
>> spec SoC requires min of 3.3V to operate,
>> Might have some functionality failures on end to end testing
>
> The PMIC regulator shouldn't be configured with the entire range of
> voltages
> it can generate, but with a range of voltages that is suitable for all
> its
> consumers.
>
> In other words if BT requires a minimum voltage of 3.3V the minimum
> voltage
> of the regulator should be at least 3.3V.
>
>> Issue 2:
>>
>> WCN3990 RF is shared with WiFi, so it also try to turn on the
>> regulators.
>> Wifi driver uses the same approach of restricting to min and max
>> voltages in
>> driver.
>> Let us assume we turned ON BT and CH0 is set to 3.1v (as in your
>> case), Wifi
>> is tuned on now, as its request the CH0 to operate at 3.3 Volts,
>> regulator
>> will fail to set this voltage as BT is operating
>> at CH0 3.1v (assuming max voltage is 3.2v)
>> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/drivers/net/wireless/ath/ath10k/snoc.c#n39
>
> see above
>
>> Issue 3:
>>
>> By mistake PMIC has low min or default voltage and high max voltages,
>> that
>> is harm for WNC3990.
>>
>> I would suggest to restrict the min and max voltages in driver,
>> instead of
>> relaying on PMIC to do this.
>> BTW pmic will do this and doing it in our driver is safer.
>
> What if another device switches the regulator on before BT?
>
> Again, what you describe is a misconfiguration of the regulator and
> should
> be fixed at its root, instead of implementing unreliable 'safeguards'
> in each
> and every driver using regulators.
Thanks for detail analysis :)
--
Regards
Balakrishna.
next prev parent reply other threads:[~2019-10-30 6:18 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-18 5:24 [PATCH 0/4] Bluetooth: hci_qca: Regulator usage cleanup Bjorn Andersson
2019-10-18 5:24 ` [PATCH 1/4] Bluetooth: hci_qca: Update regulator_set_load() usage Bjorn Andersson
2019-10-18 11:18 ` Harish Bandi
2019-10-18 5:24 ` [PATCH 2/4] Bluetooth: hci_qca: Don't vote for specific voltage Bjorn Andersson
2019-10-18 11:18 ` Harish Bandi
2019-10-18 18:22 ` Matthias Kaehlcke
2019-10-21 6:37 ` Harish Bandi
2019-10-22 6:05 ` Balakrishna Godavarthi
2019-10-22 17:15 ` Matthias Kaehlcke
2019-10-30 6:18 ` Balakrishna Godavarthi [this message]
2019-10-18 5:24 ` [PATCH 3/4] Bluetooth: hci_qca: Use regulator bulk enable/disable Bjorn Andersson
2019-10-18 11:18 ` Harish Bandi
2019-10-18 5:24 ` [PATCH 4/4] Bluetooth: hci_qca: Split qca_power_setup() Bjorn Andersson
2019-10-18 11:19 ` Harish Bandi
2019-10-18 7:59 ` [PATCH 0/4] Bluetooth: hci_qca: Regulator usage cleanup Marcel Holtmann
2019-10-18 14:52 ` Jeffrey Hugo
2019-10-18 11:17 ` Harish Bandi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ab1d48ef09d779c702cf0342c5094ee@codeaurora.org \
--to=bgodavar@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=c-hbandi@codeaurora.org \
--cc=hemantg@codeaurora.org \
--cc=jeffrey.l.hugo@gmail.com \
--cc=johan.hedberg@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-bluetooth-owner@vger.kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcel@holtmann.org \
--cc=mka@chromium.org \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).