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.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
