lkml.org 
[lkml]   [2022]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 06/10] rtw88: Add common USB chip support
    Date
    On Wed, 2022-05-18 at 10:23 +0200, Sascha Hauer wrote:
    > Add the common bits and pieces to add USB support to the RTW88 driver.
    > This is based on https://github.com/ulli-kroll/rtw88-usb.git which
    > itself is first written by Neo Jou.
    >
    > Signed-off-by: neo_jou <neo_jou@realtek.com>
    > Signed-off-by: Hans Ulli Kroll <linux@ulli-kroll.de>
    > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
    > ---
    > drivers/net/wireless/realtek/rtw88/Kconfig | 3 +
    > drivers/net/wireless/realtek/rtw88/Makefile | 2 +
    > drivers/net/wireless/realtek/rtw88/mac.c | 3 +
    > drivers/net/wireless/realtek/rtw88/main.c | 5 +
    > drivers/net/wireless/realtek/rtw88/main.h | 4 +
    > drivers/net/wireless/realtek/rtw88/reg.h | 1 +
    > drivers/net/wireless/realtek/rtw88/tx.h | 31 +
    > drivers/net/wireless/realtek/rtw88/usb.c | 1051 +++++++++++++++++++
    > drivers/net/wireless/realtek/rtw88/usb.h | 109 ++
    > 9 files changed, 1209 insertions(+)
    > create mode 100644 drivers/net/wireless/realtek/rtw88/usb.c
    > create mode 100644 drivers/net/wireless/realtek/rtw88/usb.h
    >

    [...]

    > diff --git a/drivers/net/wireless/realtek/rtw88/reg.h b/drivers/net/wireless/realtek/rtw88/reg.h
    > index 84ba9ec489c37..a928899030863 100644
    > --- a/drivers/net/wireless/realtek/rtw88/reg.h
    > +++ b/drivers/net/wireless/realtek/rtw88/reg.h
    > @@ -184,6 +184,7 @@
    > #define BIT_TXDMA_VIQ_MAP(x) \
    ^^^^^^^ replace 8 spaces by one tab

    > (((x) & BIT_MASK_TXDMA_VIQ_MAP) << BIT_SHIFT_TXDMA_VIQ_MAP)
    > #define REG_TXDMA_PQ_MAP 0x010C
    > +#define BIT_RXDMA_ARBBW_EN BIT(0)
    > #define BIT_SHIFT_TXDMA_BEQ_MAP 8
    > #define BIT_MASK_TXDMA_BEQ_MAP 0x3
    > #define BIT_TXDMA_BEQ_MAP(x) \
    ^^^^^ use tab

    > diff --git a/drivers/net/wireless/realtek/rtw88/tx.h b/drivers/net/wireless/realtek/rtw88/tx.h
    > index 56371eff9f7ff..c02d7a15895c6 100644
    > --- a/drivers/net/wireless/realtek/rtw88/tx.h
    > +++ b/drivers/net/wireless/realtek/rtw88/tx.h
    > @@ -67,6 +67,14 @@
    > le32p_replace_bits((__le32 *)(txdesc) + 0x03, value, BIT(15))
    > #define SET_TX_DESC_BT_NULL(txdesc, value) \
    > le32p_replace_bits((__le32 *)(txdesc) + 0x02, value, BIT(23))
    > +#define SET_TX_DESC_TXDESC_CHECKSUM(txdesc, value) \
    > + le32p_replace_bits((__le32 *)(txdesc) + 0x07, value, GENMASK(15, 0))
    > +#define SET_TX_DESC_DMA_TXAGG_NUM(txdesc, value) \
    > + le32p_replace_bits((__le32 *)(txdesc) + 0x07, value, GENMASK(31, 24))
    > +#define GET_TX_DESC_PKT_OFFSET(txdesc) \
    > + le32_get_bits(*((__le32 *)(txdesc) + 0x01), GENMASK(28, 24))
    > +#define GET_TX_DESC_QSEL(txdesc) \
    ^^^^ use tab

    I think you can run ./script/checkpatch.pl to find out these coding style issues.

    > + le32_get_bits(*((__le32 *)(txdesc) + 0x01), GENMASK(12, 8))
    >
    > enum rtw_tx_desc_queue_select {
    > TX_DESC_QSEL_TID0 = 0,
    > @@ -119,4 +127,27 @@ rtw_tx_write_data_h2c_get(struct rtw_dev *rtwdev,
    > struct rtw_tx_pkt_info *pkt_info,
    > u8 *buf, u32 size);
    >
    >

    [...]

    > diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
    > new file mode 100644
    > index 0000000000000..7641ea6f6ad1a
    > --- /dev/null
    > +++ b/drivers/net/wireless/realtek/rtw88/usb.c
    > @@ -0,0 +1,1051 @@
    > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
    > +/* Copyright(c) 2018-2019 Realtek Corporation
    > + */
    > +
    > +#include <linux/module.h>
    > +#include <linux/usb.h>
    > +#include <linux/mutex.h>
    > +#include "main.h"
    > +#include "debug.h"
    > +#include "reg.h"
    > +#include "tx.h"
    > +#include "rx.h"
    > +#include "fw.h"
    > +#include "ps.h"
    > +#include "usb.h"
    > +
    > +#define RTW_USB_MAX_RXQ_LEN 128
    > +
    > +struct rtw_usb_txcb {
    > + struct rtw_dev *rtwdev;
    > + struct sk_buff_head tx_ack_queue;
    > +};
    > +
    > +static void rtw_usb_fill_tx_checksum(struct rtw_usb *rtwusb,
    > + struct sk_buff *skb, int agg_num)
    > +{
    > + struct rtw_dev *rtwdev = rtwusb->rtwdev;
    > + struct rtw_tx_pkt_info pkt_info;
    > +
    > + SET_TX_DESC_DMA_TXAGG_NUM(skb->data, agg_num);
    > + pkt_info.pkt_offset = GET_TX_DESC_PKT_OFFSET(skb->data);
    > + rtw_tx_fill_txdesc_checksum(rtwdev, &pkt_info, skb->data);
    > +}
    > +
    > +static void usbctrl_async_callback(struct urb *urb)
    > +{
    > + /* free dr */
    > + kfree(urb->setup_packet);
    > + /* free databuf */
    > + kfree(urb->transfer_buffer);
    > +}
    > +
    > +static int usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
    > + u16 value, u16 index, void *pdata,
    > + u16 len)
    > +{
    > + int rc;

    Normally, we use 'ret' as return code instead.

    > + unsigned int pipe;
    > + u8 reqtype;
    > + struct usb_ctrlrequest *dr;
    > + struct urb *urb;
    > + const u16 databuf_maxlen = RTW_USB_VENQT_MAX_BUF_SIZE;
    > + u8 *databuf;

    declare in reverse X'mas tree:

    const u16 databuf_maxlen = RTW_USB_VENQT_MAX_BUF_SIZE;
    struct usb_ctrlrequest *dr;
    unsigned int pipe;
    struct urb *urb;
    u8 *databuf;
    int ret;

    > +
    > + if (WARN_ON_ONCE(len > databuf_maxlen))
    > + len = databuf_maxlen;
    > +
    > + pipe = usb_sndctrlpipe(udev, 0); /* write_out */
    > + reqtype = RTW_USB_CMD_WRITE;
    > +
    > + dr = kzalloc(sizeof(*dr), GFP_ATOMIC);
    > + if (!dr)
    > + return -ENOMEM;
    > +
    > + databuf = kmemdup(pdata, len, GFP_ATOMIC);
    > + if (!databuf) {
    > + kfree(dr);
    > + return -ENOMEM;
    > + }
    > +
    > + urb = usb_alloc_urb(0, GFP_ATOMIC);
    > + if (!urb) {
    > + kfree(databuf);
    > + kfree(dr);
    > + return -ENOMEM;
    > + }
    > +
    > + dr->bRequestType = reqtype;
    > + dr->bRequest = request;
    > + dr->wValue = cpu_to_le16(value);
    > + dr->wIndex = cpu_to_le16(index);
    > + dr->wLength = cpu_to_le16(len);
    > +
    > + usb_fill_control_urb(urb, udev, pipe,
    > + (unsigned char *)dr, databuf, len,
    > + usbctrl_async_callback, NULL);
    > + rc = usb_submit_urb(urb, GFP_ATOMIC);
    > + if (rc < 0) {
    > + kfree(databuf);
    > + kfree(dr);
    > + }
    > +
    > + usb_free_urb(urb);
    > +
    > + return rc;
    > +}
    > +
    > +static u32 rtw_usb_read_sync(struct rtw_dev *rtwdev, u32 addr, u16 len)
    > +{
    > + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
    > + struct usb_device *udev = rtwusb->udev;
    > + __le32 *data;
    > + unsigned long flags;
    > + int ret;
    > + static int count;
    > +
    > + spin_lock_irqsave(&rtwusb->usb_lock, flags);
    > +
    > + if (++rtwusb->usb_data_index >= RTW_USB_MAX_RX_COUNT)
    > + rtwusb->usb_data_index = 0;
    > + data = &rtwusb->usb_data[rtwusb->usb_data_index];
    > +
    > + spin_unlock_irqrestore(&rtwusb->usb_lock, flags);
    > +
    > + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
    > + RTW_USB_CMD_REQ, RTW_USB_CMD_READ, addr,
    ^^^^ align open parenthesis (codging style)

    checkpatch.pl can help this.


    > + RTW_USB_VENQT_CMD_IDX, data, len, 1000);
    > + if (ret < 0 && ret != -ENODEV && count++ < 4)
    > + rtw_err(rtwdev, "reg 0x%x, usbctrl_vendorreq failed with %d\n",
    > + addr, ret);
    > +
    > + return le32_to_cpu(*data);
    > +}
    > +

    [...]

    > +
    > +static void rtw_usb_rx_refill_work(struct work_struct *work)
    > +{
    > + struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_refill_work);
    > + struct rtw_dev *rtwdev = rtwusb->rtwdev;
    > + struct rx_usb_ctrl_block *rxcb;
    > + unsigned long flags;
    > + int error;
    > +
    > + do {
    > + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
    > +
    > + rxcb = list_first_entry_or_null(&rtwusb->rx_data_free,
    > + struct rx_usb_ctrl_block, list);
    > +
    > + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
    > + if (!rxcb)
    > + return;
    > +
    > + rxcb->rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_KERNEL);
    > + if (!rxcb->rx_skb) {
    > + rtw_err(rtwdev, "could not allocate rx skbuff\n");
    > + return;
    > + }
    > +
    > + usb_fill_bulk_urb(rxcb->rx_urb, rtwusb->udev,
    > + usb_rcvbulkpipe(rtwusb->udev, rtwusb->pipe_in),
    > + rxcb->rx_skb->data, RTW_USB_MAX_RECVBUF_SZ,
    > + rtw_usb_read_port_complete, rxcb);
    > +
    > + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
    > + list_move(&rxcb->list, &rtwusb->rx_data_used);
    > + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
    > +
    > + error = usb_submit_urb(rxcb->rx_urb, GFP_KERNEL);
    > + if (error) {
    > + kfree_skb(rxcb->rx_skb);
    > + if (error != -ENODEV)
    > + rtw_err(rtwdev, "Err sending rx data urb %d\n",
    > + error);
    > + rtw_usb_rx_data_put(rtwusb, rxcb);
    > +
    > + return;
    > + }
    > + } while (true);

    Can we have a limit of 'for(;<limit;)' insetad of 'while (true)'?
    > +}
    > +
    > +static void rtw_usb_cancel_rx_bufs(struct rtw_usb *rtwusb)
    > +{
    > + struct rx_usb_ctrl_block *rxcb;
    > + unsigned long flags;
    > +
    > + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
    > +
    > + while (true) {
    > + rxcb = list_first_entry_or_null(&rtwusb->rx_data_used,
    > + struct rx_usb_ctrl_block, list);
    > +
    > + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
    > +
    > + if (!rxcb)
    > + break;
    > +
    > + usb_kill_urb(rxcb->rx_urb);
    > +
    > + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
    > + list_move(&rxcb->list, &rtwusb->rx_data_free);
    > + }
    > +}

    The spin_lock pairs are not intuitive.
    Can we change this chunk to

    while (true) {
    spin_lock();
    rxcb = list_first_entry_or_null();
    spin_unlock()
    if (!rxcb)
    return;
    usb_free_urb();
    spin_lock();
    list_del();
    spin_unlock();
    }
    The drawback is lock/unlock twice in single loop.

    rtw_usb_free_rx_bufs() has similar coding.


    > +
    > +static void rtw_usb_free_rx_bufs(struct rtw_usb *rtwusb)
    > +{
    > + struct rx_usb_ctrl_block *rxcb;
    > + unsigned long flags;
    > +
    > + rtw_usb_cancel_rx_bufs(rtwusb);
    > +
    > + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
    > +
    > + while (true) {
    > + rxcb = list_first_entry_or_null(&rtwusb->rx_data_free, struct rx_usb_ctrl_block,
    > list);
    > +
    > + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
    > +
    > + if (!rxcb)
    > + break;
    > +
    > + usb_free_urb(rxcb->rx_urb);
    > +
    > + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
    > + list_del(&rxcb->list);
    > + }


    > diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
    > new file mode 100644
    > index 0000000000000..4d714372f265c
    > --- /dev/null
    > +++ b/drivers/net/wireless/realtek/rtw88/usb.h
    > @@ -0,0 +1,109 @@
    > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
    > +/* Copyright(c) 2018-2019 Realtek Corporation
    > + */
    > +
    > +#ifndef __RTW_USB_H_
    > +#define __RTW_USB_H_
    > +
    > +#define FW_8192C_START_ADDRESS 0x1000
    > +#define FW_8192C_END_ADDRESS 0x5FFF
    ^^^^^^^^^^ use tab

    > +
    > +#define RTW_USB_MAX_RX_COUNT 100
    > +#define RTW_USB_VENQT_MAX_BUF_SIZE 254
    > +#define MAX_USBCTRL_VENDORREQ_TIMES 10
    > +
    > +#define RTW_USB_CMD_READ 0xc0
    > +#define RTW_USB_CMD_WRITE 0x40
    > +#define RTW_USB_CMD_REQ 0x05
    > +
    > +#define RTW_USB_VENQT_CMD_IDX 0x00
    ^^^^^ use space
    > +
    > +#define RTW_USB_SUPER_SPEED_BULK_SIZE 1024
    > +#define RTW_USB_HIGH_SPEED_BULK_SIZE 512
    > +#define RTW_USB_FULL_SPEED_BULK_SIZE 64
    > +

    --
    Ping-Ke


    \
     
     \ /
      Last update: 2022-05-20 09:40    [W:8.182 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site