| Message ID | 20230313144028.3156825-1-neeraj.sanjaykale@nxp.com |
|---|---|
| Headers | show |
| Series | Add support for NXP bluetooth chipsets | expand |
On Mon, Mar 13, 2023 at 08:10:26PM +0530, Neeraj Sanjay Kale wrote: > Adds serdev_device_break_ctl() and an implementation for ttyport. > This function simply calls the break_ctl in tty layer, which can > assert a break signal over UART-TX line, if the tty and the > underlying platform and UART peripheral supports this operation. > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> > --- > v3: Add details to the commit message. Replace ENOTSUPP with > EOPNOTSUPP. (Greg KH, Leon Romanovsky) > v9: Replace all instances of ENOTSUPP with EOPNOTSUPP. > (Simon Horman) I'm all for this change. But perhaps it should be a separate clean-up patch, that precedes the feature-patch which adds the new method. I think that would make things a bit clearer. ... > diff --git a/include/linux/serdev.h b/include/linux/serdev.h > index 66f624fc618c..c065ef1c82f1 100644 > --- a/include/linux/serdev.h > +++ b/include/linux/serdev.h ... > @@ -202,6 +203,7 @@ int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_ > void serdev_device_wait_until_sent(struct serdev_device *, long); > int serdev_device_get_tiocm(struct serdev_device *); > int serdev_device_set_tiocm(struct serdev_device *, int, int); > +int serdev_device_break_ctl(struct serdev_device *serdev, int break_state); > void serdev_device_write_wakeup(struct serdev_device *); > int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, long); > void serdev_device_write_flush(struct serdev_device *); > @@ -255,6 +257,10 @@ static inline int serdev_device_set_tiocm(struct serdev_device *serdev, int set, > { > return -ENOTSUPP; It seems that you might have missed at least this one. > } > +static inline int serdev_device_break_ctl(struct serdev_device *serdev, int break_state) > +{ > + return -EOPNOTSUPP; > +} > static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf, > size_t count, unsigned long timeout) > { > -- > 2.34.1 >
On Mon, Mar 13, 2023 at 08:10:28PM +0530, Neeraj Sanjay Kale wrote: > This adds a driver based on serdev driver for the NXP BT serial protocol > based on running H:4, which can enable the built-in Bluetooth device > inside an NXP BT chip. > > This driver has Power Save feature that will put the chip into sleep state > whenever there is no activity for 2000ms, and will be woken up when any > activity is to be initiated over UART. > > This driver enables the power save feature by default by sending the vendor > specific commands to the chip during setup. > > During setup, the driver checks if a FW is already running on the chip > by waiting for the bootloader signature, and downloads device specific FW > file into the chip over UART if bootloader signature is received.. > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Hi, some minor feedback from my side. ... > +/* for legacy chipsets with V1 bootloader */ > +static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct btnxpuart_data *nxp_data = nxpdev->nxp_data; > + struct v1_data_req *req; > + u32 requested_len; > + > + if (!process_boot_signature(nxpdev)) > + goto ret; > + > + req = (struct v1_data_req *)skb_pull_data(skb, sizeof(struct v1_data_req)); > + if (!req) > + goto ret; > + > + if ((req->len ^ req->len_comp) != 0xffff) { > + bt_dev_dbg(hdev, "ERR: Send NAK"); > + nxp_send_ack(NXP_NAK_V1, hdev); > + goto ret; > + } > + nxp_send_ack(NXP_ACK_V1, hdev); > + > + if (nxp_data->fw_dnld_use_high_baudrate) { > + if (!nxpdev->timeout_changed) { > + nxpdev->timeout_changed = nxp_fw_change_timeout(hdev, req->len); > + goto ret; > + } > + if (!nxpdev->baudrate_changed) { > + nxpdev->baudrate_changed = nxp_fw_change_baudrate(hdev, req->len); > + if (nxpdev->baudrate_changed) { > + serdev_device_set_baudrate(nxpdev->serdev, > + HCI_NXP_SEC_BAUDRATE); > + serdev_device_set_flow_control(nxpdev->serdev, 1); > + nxpdev->current_baudrate = HCI_NXP_SEC_BAUDRATE; > + } > + goto ret; > + } > + } > + > + if (nxp_request_firmware(hdev, nxp_data->fw_name)) > + goto ret; > + > + requested_len = req->len; > + if (requested_len == 0) { > + bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes", nxpdev->fw->size); > + clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state); > + wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q); > + goto ret; > + } > + if (requested_len & 0x01) { > + /* The CRC did not match at the other end. > + * Simply send the same bytes again. > + */ > + requested_len = nxpdev->fw_v1_sent_bytes; > + bt_dev_dbg(hdev, "CRC error. Resend %d bytes of FW.", requested_len); > + } else { > + nxpdev->fw_dnld_v1_offset += nxpdev->fw_v1_sent_bytes; > + > + /* The FW bin file is made up of many blocks of > + * 16 byte header and payload data chunks. If the > + * FW has requested a header, read the payload length > + * info from the header, before sending the header. > + * In the next iteration, the FW should request the > + * payload data chunk, which should be equal to the > + * payload length read from header. If there is a > + * mismatch, clearly the driver and FW are out of sync, > + * and we need to re-send the previous header again. > + */ > + if (requested_len == nxpdev->fw_v1_expected_len) { > + if (requested_len == HDR_LEN) > + nxpdev->fw_v1_expected_len = nxp_get_data_len(nxpdev->fw->data + > + nxpdev->fw_dnld_v1_offset); > + else > + nxpdev->fw_v1_expected_len = HDR_LEN; > + } else if (requested_len == HDR_LEN) { > + /* FW download out of sync. Send previous chunk again */ > + nxpdev->fw_dnld_v1_offset -= nxpdev->fw_v1_sent_bytes; > + nxpdev->fw_v1_expected_len = HDR_LEN; > + } > + } > + > + if (nxpdev->fw_dnld_v1_offset + requested_len <= nxpdev->fw->size) > + serdev_device_write_buf(nxpdev->serdev, > + nxpdev->fw->data + nxpdev->fw_dnld_v1_offset, > + requested_len); > + nxpdev->fw_v1_sent_bytes = requested_len; > + > +ret: nit: I think it would be more intuitive to call this label free_skb. Likewise elsewhere. > + kfree_skb(skb); > + return 0; > +} ... > +static int nxp_set_ind_reset(struct hci_dev *hdev, void *data) > +{ > + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct sk_buff *skb; > + u8 *status; > + u8 pcmd = 0; > + int err; > + > + skb = nxp_drv_send_cmd(hdev, HCI_NXP_IND_RESET, 1, &pcmd); > + if (IS_ERR(skb)) > + return PTR_ERR(skb); > + > + status = skb_pull_data(skb, 1); > + if (status) { > + if (*status == 0) { > + set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state); > + err = nxp_download_firmware(hdev); > + if (err < 0) > + return err; Does this leak skb? > + serdev_device_set_baudrate(nxpdev->serdev, nxpdev->fw_init_baudrate); > + nxpdev->current_baudrate = nxpdev->fw_init_baudrate; > + if (nxpdev->current_baudrate != HCI_NXP_SEC_BAUDRATE) { > + nxpdev->new_baudrate = HCI_NXP_SEC_BAUDRATE; > + nxp_set_baudrate_cmd(hdev, NULL); > + } > + hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL); > + hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL); > + } > + } > + kfree_skb(skb); > + > + return 0; > +} ... > +static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); > + struct ps_data *psdata = &nxpdev->psdata; > + struct hci_command_hdr *hdr; > + struct psmode_cmd_payload ps_parm; > + struct wakeup_cmd_payload wakeup_parm; > + __le32 baudrate_parm; > + > + /* if vendor commands are received from user space (e.g. hcitool), update > + * driver flags accordingly and ask driver to re-send the command to FW. > + * In case the payload for any command does not match expected payload > + * length, let the firmware and user space program handle it, or throw > + * an error. > + */ > + if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT && !psdata->driver_sent_cmd) { > + hdr = (struct hci_command_hdr *)skb->data; > + if (hdr->plen != (skb->len - HCI_COMMAND_HDR_SIZE)) > + goto send_skb; > + > + switch (__le16_to_cpu(hdr->opcode)) { > + case HCI_NXP_AUTO_SLEEP_MODE: > + if (hdr->plen == sizeof(ps_parm)) { > + memcpy(&ps_parm, skb->data + HCI_COMMAND_HDR_SIZE, hdr->plen); > + if (ps_parm.ps_cmd == BT_PS_ENABLE) > + psdata->target_ps_mode = PS_MODE_ENABLE; > + else if (ps_parm.ps_cmd == BT_PS_DISABLE) > + psdata->target_ps_mode = PS_MODE_DISABLE; > + psdata->c2h_ps_interval = __le16_to_cpu(ps_parm.c2h_ps_interval); > + hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL); > + goto free_skb; > + } > + break; > + case HCI_NXP_WAKEUP_METHOD: > + if (hdr->plen == sizeof(wakeup_parm)) { > + memcpy(&wakeup_parm, skb->data + HCI_COMMAND_HDR_SIZE, hdr->plen); > + psdata->c2h_wakeupmode = wakeup_parm.c2h_wakeupmode; > + psdata->c2h_wakeup_gpio = wakeup_parm.c2h_wakeup_gpio; > + psdata->h2c_wakeup_gpio = wakeup_parm.h2c_wakeup_gpio; > + switch (wakeup_parm.h2c_wakeupmode) { > + case BT_CTRL_WAKEUP_METHOD_DSR: > + psdata->h2c_wakeupmode = WAKEUP_METHOD_DTR; > + break; > + case BT_CTRL_WAKEUP_METHOD_BREAK: > + default: > + psdata->h2c_wakeupmode = WAKEUP_METHOD_BREAK; > + break; > + } > + hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL); > + goto free_skb; > + } > + break; > + case HCI_NXP_SET_OPER_SPEED: > + if (hdr->plen == sizeof(baudrate_parm)) { > + memcpy(&baudrate_parm, skb->data + HCI_COMMAND_HDR_SIZE, hdr->plen); > + nxpdev->new_baudrate = __le32_to_cpu(baudrate_parm); > + hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd, NULL, NULL); > + goto free_skb; > + } > + break; > + case HCI_NXP_IND_RESET: > + if (hdr->plen == 1) { > + hci_cmd_sync_queue(hdev, nxp_set_ind_reset, NULL, NULL); > + goto free_skb; > + } > + break; > + default: > + break; > + } > + } ... > +send_skb: > + /* Prepend skb with frame type */ > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > + skb_queue_tail(&nxpdev->txq, skb); > + > + btnxpuart_tx_wakeup(nxpdev); > +ret: > + return 0; > + > +free_skb: > + kfree_skb(skb); > + goto ret; nit: I think it would be nicer to simply return 0 here. And remove the ret label entirely. > +} ...
Hi Simon Thank you for reviewing the patch. I have a comment below: > > > +send_skb: > > + /* Prepend skb with frame type */ > > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > + skb_queue_tail(&nxpdev->txq, skb); > > + > > + btnxpuart_tx_wakeup(nxpdev); > > +ret: > > + return 0; > > + > > +free_skb: > > + kfree_skb(skb); > > + goto ret; > > nit: I think it would be nicer to simply return 0 here. > And remove the ret label entirely. > > > +} > We need to return from this function without clearing the skbs, unless "goto free_skb" is called. If I remove the ret label and return after kfree_skb() it causes a kernel crash. Keeping this change as it is. Please let me know if you have any further review comments on the v11 patch. Thanks, Neeraj
On Tue, Mar 14, 2023 at 03:40:34PM +0000, Neeraj sanjay kale wrote: > Hi Simon > > Thank you for reviewing the patch. I have a comment below: > > > > > > +send_skb: > > > + /* Prepend skb with frame type */ > > > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); > > > + skb_queue_tail(&nxpdev->txq, skb); > > > + > > > + btnxpuart_tx_wakeup(nxpdev); > > > +ret: > > > + return 0; > > > + > > > +free_skb: > > > + kfree_skb(skb); > > > + goto ret; > > > > nit: I think it would be nicer to simply return 0 here. > > And remove the ret label entirely. > > > > > +} > > > We need to return from this function without clearing the skbs, unless "goto free_skb" is called. > If I remove the ret label and return after kfree_skb() it causes a kernel crash. > Keeping this change as it is. > > Please let me know if you have any further review comments on the v11 patch. I'll look over v11. But for the record, I meant something like this: send_skb: /* Prepend skb with frame type */ memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1); skb_queue_tail(&nxpdev->txq, skb); btnxpuart_tx_wakeup(nxpdev); return 0; free_skb: kfree_skb(skb); return 0; } > We need to return from this function without clearing the skbs, unless "goto free_skb" is called. > If I remove the ret label and return after kfree_skb() it causes a kernel crash. > Keeping this change as it is. > > Please let me know if you have any further review comments on the v11 patch.