> -----Original Message-----
> From: Brian Norris [mailto:[email protected]]
> Sent: Friday, February 01, 2019 6:58 AM
> To: Tony Chuang
> Cc: [email protected]; [email protected];
> [email protected]; Pkshih; Andy Huang; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 06/13] rtw88: fw and efuse files
>
> Hi,
>
> On Wed, Jan 30, 2019 at 12:02:13PM +0800, [email protected] wrote:
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > fw and efuse files for Realtek 802.11ac wireless network chips
> >
> > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> > ---
> > 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.
>
Yes, I can make this easier to read.
> > +
> > + 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.
I will try it, looks like very useful.
>
> 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.
The three lines above can be removed. Should put the dump log in debugfs.
And x3d{7,8} indeed are not used in the code. I think it was written to
develop 8822BE and is no more required here.
>
> > +
> > + 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;
> > +};
I think I can turn them into macros, such as:
#define GET_HW_CAP le32_get_bits(...)
> > +
> > +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
>
Yan-Hsuan
