From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Date: Thu, 21 Jun 2018 16:50:28 +0530 From: Balakrishna Godavarthi To: Matthias Kaehlcke Cc: marcel@holtmann.org, johan.hedberg@gmail.com, thierry.escande@linaro.org, linux-bluetooth@vger.kernel.org, rtatiya@codeaurora.org, hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org Subject: Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function. In-Reply-To: <20180620233306.GK169030@google.com> References: <20180616062718.29844-1-bgodavar@codeaurora.org> <20180616062718.29844-5-bgodavar@codeaurora.org> <20180618211942.GT88063@google.com> <20180619200338.GD169030@google.com> <20180620233306.GK169030@google.com> Message-ID: List-ID: Hi Matthias, On 2018-06-21 05:03, Matthias Kaehlcke wrote: > Hi Balakrishna, > > On Wed, Jun 20, 2018 at 04:23:25PM +0530, Balakrishna Godavarthi wrote: >> Hi Matthias, >> >> On 2018-06-20 01:33, Matthias Kaehlcke wrote: >> > On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote: >> > > Hi Matthias, >> > > >> > > Please find my comments inline. >> > > >> > > On 2018-06-19 02:49, Matthias Kaehlcke wrote: >> > > > On Sat, Jun 16, 2018 at 11:57:14AM +0530, Balakrishna Godavarthi wrote: >> > > > >> > > > > @@ -337,19 +337,20 @@ int qca_uart_setup(struct hci_dev *hdev, >> > > > > uint8_t baudrate) >> > > > > >> > > > > config.user_baud_rate = baudrate; >> > > > > >> > > > > - /* Get QCA version information */ >> > > > > - err = qca_read_soc_version(hdev, &rome_ver); >> > > > > - if (err < 0 || rome_ver == 0) { >> > > > > - bt_dev_err(hdev, "QCA Failed to get version %d", err); >> > > > > - return err; >> > > > > + if (!soc_ver) { >> > > > > + /* Get QCA version information */ >> > > > > + err = qca_read_soc_version(hdev, &soc_ver); >> > > > > + if (err < 0 || soc_ver == 0) { >> > > > > + bt_dev_err(hdev, "QCA Failed to get version %d", err); >> > > > > + return err; >> > > > > + } >> > > > > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > > > > } >> > > > >> > > > Why is this needed? A caller that passes in soc_ver = 0 could just >> > > > call qca_read_soc_version() themselves. >> > > [Bala]: hci_qca support two chip one ROME (already up-streamed) and >> > > other >> > > wcn3990. >> > > there setup sequence differs. >> > > >> > > wcn3900: >> > > 1. set baudrate to 115200 >> > > 2. read soc version >> > > 3. set baudarte to 3.2Mbps >> > > 4. download firmware. (calls qca_uart_setup) >> > > >> > > ROME: >> > > 1. set baudrate to 3.0 mbps >> > > 2. calls qca_uart_setup to read soc version and download >> > > firmware. >> > > >> > > so to make use of existing function qca_setup_uart(). passing >> > > soc_ver >> > > as argument. >> > > if soc_ver is zero, then read soc version (i.e. for ROME chip). >> > > >> > > Pls let me know, if you have any queries. >> > >> > I don't really understand this argumentation. Let's look at the code >> > at the end of this series, where both Rome and wcn3900 are supported: >> > >> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059 >> > >> > The version could be read (for Rome only) after setting the operating >> > speed: >> >> [Bala]: yes we can read SoC version after setting operating baud rate. >> an >> advice from SoC vendor that, read SoC version before setting operating >> speed. >> one advantage from reading SoC version before setting >> operating >> baudrate,it will helps us to understand whether the soc is responding >> to any >> commands in default baud rate >> before setting operating speed. > > Is the recommendation for Rome or wcn3990? I was referring to Rome and > the current code sets the operating speed and then reads the SoC > version (inside qca_uart_setup()) > [Bala]: my statement is wrt to wcn3990. yes, ROME will set operating speed and read version. >> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111 >> > >> > I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in >> > qca_setup() is kinda ugly, but I think it's still better than the >> > conditional call of qca_read_soc_version() in qca_uart_setup(). >> >> [Bala]: we require an if-else block for soc type.As wcn3900 support >> hardware flow control where as ROME is not supporting hardware flow >> control. > > Since you mention if-else and flow control I'm not sure if you > understood correctly what I suggested. My suggestion is to add > something like this: > > if (qcadev->btsoc_type == QCA_ROME) { > ret = qca_read_soc_version(hdev, &soc_ver); > if (ret < 0 || soc_ver == 0) { > // Note mka@: we might want to skip this log, > // qca_read_soc_version() already logs in all error paths > bt_dev_err(hdev, "Failed to get version %d", ret); > return ret; > } > } > > right here: > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111 > > which is functionally equivalent to the current code. > > You could even do the error check in the common code (just checking > for 'soc_ver == 0', assuming that qca_read_soc_version() doesn't set > it to something different in case of error). > > Anyway, I won't insist if you prefer it as is and Marcel is ok with it. [Bala]: thanks for clarifying my doubt, but if remove the qca_read_soc_version() from qca_uart_setup() and handle it qca_setup(). in that case, we will have common blocks of code, when we integrate wcn3990 into existing qca_setup() static int qca_setup(struct hci_uart *hu) { ... qcadev = serdev_device_get_drvdata(hu->serdev); /* Patch downloading has to be done without IBS mode */ clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); /* Setup initial baudrate */ speed = qca_get_speed(hu, QCA_INIT_SPEED); if (speed) qca_set_speed(hu, speed, QCA_INIT_SPEED); if (qcadev->btsoc_type == QCA_WCN3990) { bt_dev_dbg(hdev, "setting up wcn3990"); hci_uart_set_flow_control(hu, true); ret = qca_send_vendor_cmd(hdev, QCA_WCN3990_POWERON_PULSE); if (ret) { bt_dev_err(hdev, "failed to send power on command"); return ret; } serdev_device_close(hu->serdev); ret = serdev_device_open(hu->serdev); if (ret) { bt_dev_err(hdev, "failed to open port"); return ret; } msleep(100); speed = qca_get_speed(hu, QCA_INIT_SPEED); if (speed) qca_set_speed(hu, speed, QCA_INIT_SPEED); hci_uart_set_flow_control(hu, false); } else { bt_dev_info(hdev, "ROME setup"); /* Setup user speed if needed */ speed = qca_get_speed(hu, QCA_OPER_SPEED); if (speed) { ret = qca_set_speed(hu, speed, QCA_OPER_SPEED); if (ret) return ret; qca_baudrate = qca_get_baudrate_value(speed); } } ret = qca_read_soc_version(hdev, &soc_ver); if (ret < 0 || soc_ver == 0) { bt_dev_err(hdev, "Failed to get version %d", ret); return ret; } bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver); if (qcadev->btsoc_type == QCA_WCN3990) { hci_uart_set_flow_control(hu, true); /* Setup user speed if needed */ speed = qca_get_speed(hu, QCA_OPER_SPEED); if (speed) { ret = qca_set_speed(hu, speed, QCA_OPER_SPEED); if (ret) return ret; qca_baudrate = qca_get_baudrate_value(speed); } /* Setup patch / NVM configurations */ ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver); if (!ret) { set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); qca_debugfs_init(hdev); } else if (ret == -ENOENT) { /* No patch/nvm-config found, run with original fw/config */ ret = 0; } else if (ret == -EAGAIN) { /* ... } -- Regards Balakrishna.