Hi,
On Wed, Jan 30, 2019 at 12:02:13PM +0800, yhchuang@xxxxxxxxxxx wrote:
> From: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx>
>
> fw and efuse files for Realtek 802.11ac wireless network chips
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx>
> ---
> drivers/net/wireless/realtek/rtw88/efuse.c | 150 +++++++
> drivers/net/wireless/realtek/rtw88/efuse.h | 53 +++
> drivers/net/wireless/realtek/rtw88/fw.c | 611 +++++++++++++++++++++++++++++
> drivers/net/wireless/realtek/rtw88/fw.h | 213 ++++++++++
> 4 files changed, 1027 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/efuse.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/efuse.h
> create mode 100644 drivers/net/wireless/realtek/rtw88/fw.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/fw.h
>
> diff --git a/drivers/net/wireless/realtek/rtw88/efuse.c b/drivers/net/wireless/realtek/rtw88/efuse.c
> new file mode 100644
> index 0000000..7c1b782
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/efuse.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2018 Realtek Corporation.
> + */
> +
> +#include "main.h"
> +#include "efuse.h"
> +#include "reg.h"
> +#include "debug.h"
> +
> +#define RTW_EFUSE_BANK_WIFI 0x0
> +
> +static void switch_efuse_bank(struct rtw_dev *rtwdev)
> +{
> + rtw_write32_mask(rtwdev, REG_LDO_EFUSE_CTRL, BIT_MASK_EFUSE_BANK_SEL,
> + RTW_EFUSE_BANK_WIFI);
> +}
> +
> +static int rtw_dump_logical_efuse_map(struct rtw_dev *rtwdev, u8 *phy_map,
> + u8 *log_map)
> +{
> + u32 physical_size = rtwdev->efuse.physical_size;
> + u32 protect_size = rtwdev->efuse.protect_size;
> + u32 logical_size = rtwdev->efuse.logical_size;
> + u32 phy_idx, log_idx;
> + u8 hdr1, hdr2;
> + u8 blk_idx;
> + u8 valid;
> + u8 word_en;
> + int i;
> +
> + phy_idx = 0;
> +
> + do {
See my comments below about termination, but I think you need some
bounds checks up front to ensure you're not running over the buffers.
You have some checks at the end of the embedded for-loop, but it's not
clear you will always run them.
> + hdr1 = *(phy_map + phy_idx);
> + if ((hdr1 & 0x1f) == 0xf) {
> + phy_idx++;
> + hdr2 = *(phy_map + phy_idx);
> + if (hdr2 == 0xff)
> + break;
> + blk_idx = ((hdr2 & 0xf0) >> 1) | ((hdr1 >> 5) & 0x07);
> + word_en = hdr2 & 0x0f;
> + } else {
> + blk_idx = (hdr1 & 0xf0) >> 4;
> + word_en = hdr1 & 0x0f;
> + }
> +
> + if (hdr1 == 0xff)
> + break;
> +
> + phy_idx++;
> + for (i = 0; i < 4; i++) {
> + valid = (~(word_en >> i)) & 0x1;
> + if (valid != 0x1)
> + continue;
> + log_idx = (blk_idx << 3) + (i << 1);
> + *(log_map + log_idx) = *(phy_map + phy_idx);
> + log_idx++;
> + phy_idx++;
> + *(log_map + log_idx) = *(phy_map + phy_idx);
> + phy_idx++;
> + if (phy_idx > physical_size - protect_size ||
> + log_idx > logical_size)
> + return -EINVAL;
> + }
> + } while (1);
This is a complicated and ugly loop. Can you make this easier to read?
Comments? Describe the layout in words or a diagram? Macros? At the
moment, I can't even guarantee that this while(1) loop is guaranteed to
terminate, let alone actually determine what exactly you're trying to
parse.
> +
> + return 0;
> +}
> +
> +static int rtw_dump_physical_efuse_map(struct rtw_dev *rtwdev, u8 *map)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + u32 size = rtwdev->efuse.physical_size;
> + u32 efuse_ctl;
> + u32 addr;
> + u32 cnt;
> +
> + switch_efuse_bank(rtwdev);
> +
> + /* disable 2.5V LDO */
> + chip->ops->cfg_ldo25(rtwdev, false);
> +
> + efuse_ctl = rtw_read32(rtwdev, REG_EFUSE_CTRL);
> +
> + for (addr = 0; addr < size; addr++) {
> + efuse_ctl &= ~(BIT_MASK_EF_DATA | BITS_EF_ADDR);
> + efuse_ctl |= (addr & BIT_MASK_EF_ADDR) << BIT_SHIFT_EF_ADDR;
> + rtw_write32(rtwdev, REG_EFUSE_CTRL, efuse_ctl & (~BIT_EF_FLAG));
> +
> + cnt = 1000000;
> + do {
> + udelay(1);
> + efuse_ctl = rtw_read32(rtwdev, REG_EFUSE_CTRL);
> + if (--cnt == 0)
> + return -EBUSY;
> + } while (!(efuse_ctl & BIT_EF_FLAG));
> +
> + *(map + addr) = (u8)(efuse_ctl & BIT_MASK_EF_DATA);
> + }
> +
> + return 0;
> +}
> +
> +int rtw_parse_efuse_map(struct rtw_dev *rtwdev)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + struct rtw_efuse *efuse = &rtwdev->efuse;
> + u32 phy_size = efuse->physical_size;
> + u32 log_size = efuse->logical_size;
> + u8 *phy_map = NULL;
> + u8 *log_map = NULL;
> + int ret = 0;
> +
> + phy_map = kmalloc(phy_size, GFP_KERNEL);
> + log_map = kmalloc(log_size, GFP_KERNEL);
> + if (!phy_map || !log_map) {
> + ret = -ENOMEM;
> + goto out_free;
> + }
> +
> + ret = rtw_dump_physical_efuse_map(rtwdev, phy_map);
> + if (ret) {
> + rtw_err(rtwdev, "failed to dump efuse physical map\n");
> + goto out_free;
> + }
> +
> + memset(log_map, 0xff, log_size);
> + ret = rtw_dump_logical_efuse_map(rtwdev, phy_map, log_map);
> + if (ret) {
> + rtw_err(rtwdev, "failed to dump efuse logical map\n");
> + goto out_free;
> + }
> +
> + print_hex_dump_bytes("efuse: ", DUMP_PREFIX_OFFSET, log_map, log_size);
Do you really want to dump this at every boot? It goes at KERN_DEBUG
level, so it may or may not be showing up by default, but still, this
doesn't feel like the right thing here.
> +
> + efuse->x3d7 = phy_map[0x3d7];
> + efuse->x3d8 = phy_map[0x3d8];
Fortunately I had KASAN enabled (you should try it!), because it noticed
that on 8822C, this is out of bounds. See how 8822c's phy_efuse_size is
only 512, and so you end up reading beyond the end of the boundary.
Why are you doing this anyway? You don't use the ->x3d{7,8} fields
anywhere.
On a related note, it still feels like you have too many magic nubers in
some places.
> +
> + ret = chip->ops->read_efuse(rtwdev, log_map);
> + if (ret) {
> + rtw_err(rtwdev, "failed to read efuse map\n");
> + goto out_free;
> + }
> +
> +out_free:
> + kfree(log_map);
> + kfree(phy_map);
> +
> + return ret;
> +}
> diff --git a/drivers/net/wireless/realtek/rtw88/efuse.h b/drivers/net/wireless/realtek/rtw88/efuse.h
> new file mode 100644
> index 0000000..3635d08
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/efuse.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2018 Realtek Corporation.
> + */
> +
> +#ifndef __RTW_EFUSE_H__
> +#define __RTW_EFUSE_H__
> +
> +#define EFUSE_HW_CAP_IGNORE 0
> +#define EFUSE_HW_CAP_PTCL_VHT 3
> +#define EFUSE_HW_CAP_SUPP_BW80 7
> +#define EFUSE_HW_CAP_SUPP_BW40 6
> +
> +struct efuse_hw_cap {
> + u8 rsvd_0;
> + u8 rsvd_1;
> + u8 rsvd_2;
> + u8 rsvd_3;
> +#ifdef __LITTLE_ENDIAN
> + u8 hci:4;
> + u8 rsvd_4:4;
> +#else
> + u8 rsvd_4:4;
> + u8 hci:4;
> +#endif
Ugh, do you *really* have too all this endian-aware bitfield layout?
IIUC, a lot of the layout behavior is completely implementation
specific. While you might get away with something like this, it doesn't
seem particularly wise to me.
Also, don't you need __packed on this struct? Otherwise, you're not even
really guaranteed your u8 fields to be aligned contiguously.
> + u8 rsvd_5;
> +#ifdef __LITTLE_ENDIAN
> + u8 bw:3;
> + u8 nss:2;
> + u8 ant_num:3;
> +#else
> + u8 ant_num:3;
> + u8 nss:2;
> + u8 bw:3;
> +#endif
> +#ifdef __LITTLE_ENDIAN
> + u8 rsvd_7_1:2;
> + u8 ptcl:2;
> + u8 rsvd_7_2:4;
> +#else
> + u8 rsvd_7_2:4;
> + u8 ptcl:2;
> + u8 rsvd_7_1:2;
> +#endif
> + u8 rsvd_8;
> + u8 rsvd_9;
> + u8 rsvd_10;
> + u8 rsvd_11;
> + u8 rsvd_12;
> +};
> +
> +int rtw_parse_efuse_map(struct rtw_dev *rtwdev);
> +
> +#endif
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> new file mode 100644
> index 0000000..194bb87
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -0,0 +1,611 @@
...
> +int rtw_fw_write_data_rsvd_page(struct rtw_dev *rtwdev, u16 pg_addr,
> + u8 *buf, u32 size)
> +{
> + u8 bckp[2];
> + u8 val;
> + u16 rsvd_pg_head;
> + int ret;
> +
> + lockdep_assert_held(&rtwdev->mutex);
> +
> + if (!size)
> + return -EINVAL;
> +
> + pg_addr &= BIT_MASK_BCN_HEAD_1_V1;
> + rtw_write16(rtwdev, REG_FIFOPAGE_CTRL_2, pg_addr | BIT_BCN_VALID_V1);
> +
> + val = rtw_read8(rtwdev, REG_CR + 1);
> + bckp[0] = val;
> + val |= BIT(0);
Magic number.
> + rtw_write8(rtwdev, REG_CR + 1, val);
> +
> + val = rtw_read8(rtwdev, REG_FWHW_TXQ_CTRL + 2);
> + bckp[1] = val;
> + val &= ~BIT(6);
Magic number.
Brian
> + rtw_write8(rtwdev, REG_FWHW_TXQ_CTRL + 2, val);
> +
> + ret = rtw_hci_write_data_rsvd_page(rtwdev, buf, size);
> + if (ret) {
> + rtw_err(rtwdev, "failed to write data to rsvd page\n");
> + goto restore;
> + }
> +
> + if (!check_hw_ready(rtwdev, REG_FIFOPAGE_CTRL_2, BIT_BCN_VALID_V1, 1)) {
> + rtw_err(rtwdev, "error beacon valid\n");
> + ret = -EBUSY;
> + }
> +
> +restore:
> + rsvd_pg_head = rtwdev->fifo.rsvd_boundary;
> + rtw_write16(rtwdev, REG_FIFOPAGE_CTRL_2,
> + rsvd_pg_head | BIT_BCN_VALID_V1);
> + rtw_write8(rtwdev, REG_FWHW_TXQ_CTRL + 2, bckp[1]);
> + rtw_write8(rtwdev, REG_CR + 1, bckp[0]);
> +
> + return ret;
> +}
> +
...
