| Message ID | 20191107111603.12317-4-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: enable PCI CLKREQ and ASPM | expand |
On Thu, Nov 7, 2019 at 7:16 PM <yhchuang@realtek.com> wrote: > > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > > By Realtek's design, there are two HW modules associated for CLKREQ, > one is responsible to follow the PCIE host settings, and another > is to actually working on it. But the module that is actually working > on it is default disabled, and driver should enable that module if > host and device have successfully sync'ed with each other. > > The module is default disabled because sometimes the host does not > support it, and if there is any incorrect settings (ex. CLKREQ# is > not Bi-Direction), device can be lost and disconnected to the host. > > So driver should first check after host and device are sync'ed, and > the host does support the function and set it in configuration > space, then driver can turn on the HW module to working on it. > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > --- > drivers/net/wireless/realtek/rtw88/pci.c | 83 ++++++++++++++++++++++++ > drivers/net/wireless/realtek/rtw88/pci.h | 5 ++ > 2 files changed, 88 insertions(+) > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c > index 6d1aa6f41e84..4fcef8a6fc42 100644 > --- a/drivers/net/wireless/realtek/rtw88/pci.c > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > @@ -1081,6 +1081,33 @@ static void rtw_dbi_write8(struct rtw_dev *rtwdev, u16 addr, u8 data) > WARN(flag, "failed to write to DBI register, addr=0x%04x\n", addr); > } > > +static int rtw_dbi_read8(struct rtw_dev *rtwdev, u16 addr, u8 *value) > +{ > + u16 read_addr = addr & BITS_DBI_ADDR_MASK; > + u8 flag; > + u8 cnt; > + > + rtw_write16(rtwdev, REG_DBI_FLAG_V1, read_addr); > + rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, BIT_DBI_RFLAG >> 16); > + > + for (cnt = 0; cnt < RTW_PCI_WR_RETRY_CNT; cnt++) { > + flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2); > + if (flag == 0) > + break; Why not just doing ' rtw_read8(rtwdev, read_addr);' and return here? Then you don't need to check the flag != 0 at the following. It would make the code cleaner and same expressive. > + > + udelay(10); > + } > + > + if (flag != 0) { > + WARN(1, "failed to read DBI register, addr=0x%04x\n", addr); > + return -EIO; > + } > + > + read_addr = REG_DBI_RDATA_V1 + (addr & 3); > + *value = rtw_read8(rtwdev, read_addr); > + return 0; > +} > + > -- > 2.17.1 >
> On Thu, Nov 7, 2019 at 7:16 PM <yhchuang@realtek.com> wrote: > > > > From: Yan-Hsuan Chuang <yhchuang@realtek.com> > > > > By Realtek's design, there are two HW modules associated for CLKREQ, > > one is responsible to follow the PCIE host settings, and another > > is to actually working on it. But the module that is actually working > > on it is default disabled, and driver should enable that module if > > host and device have successfully sync'ed with each other. > > > > The module is default disabled because sometimes the host does not > > support it, and if there is any incorrect settings (ex. CLKREQ# is > > not Bi-Direction), device can be lost and disconnected to the host. > > > > So driver should first check after host and device are sync'ed, and > > the host does support the function and set it in configuration > > space, then driver can turn on the HW module to working on it. > > > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > > --- > > drivers/net/wireless/realtek/rtw88/pci.c | 83 > ++++++++++++++++++++++++ > > drivers/net/wireless/realtek/rtw88/pci.h | 5 ++ > > 2 files changed, 88 insertions(+) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c > b/drivers/net/wireless/realtek/rtw88/pci.c > > index 6d1aa6f41e84..4fcef8a6fc42 100644 > > --- a/drivers/net/wireless/realtek/rtw88/pci.c > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c > > @@ -1081,6 +1081,33 @@ static void rtw_dbi_write8(struct rtw_dev > *rtwdev, u16 addr, u8 data) > > WARN(flag, "failed to write to DBI register, addr=0x%04x\n", > addr); > > } > > > > +static int rtw_dbi_read8(struct rtw_dev *rtwdev, u16 addr, u8 *value) > > +{ > > + u16 read_addr = addr & BITS_DBI_ADDR_MASK; > > + u8 flag; > > + u8 cnt; > > + > > + rtw_write16(rtwdev, REG_DBI_FLAG_V1, read_addr); > > + rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, BIT_DBI_RFLAG >> > 16); > > + > > + for (cnt = 0; cnt < RTW_PCI_WR_RETRY_CNT; cnt++) { > > + flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2); > > + if (flag == 0) > > + break; > Why not just doing ' rtw_read8(rtwdev, read_addr);' and return here? > Then you don't need to check the flag != 0 at the following. It would > make the code cleaner and same expressive. Maybe it would look cleaner, but you need to add statements when 'if (flag == 0)', then the indents will be deeper. Also you will need to return 0 at the end if 'flag == 0'. So you still think it's cleaner to write it that way? If so, I can try to send v2 for it. > > > + > > + udelay(10); > > + } > > + > > + if (flag != 0) { > > + WARN(1, "failed to read DBI register, addr=0x%04x\n", > addr); > > + return -EIO; > > + } > > + > > + read_addr = REG_DBI_RDATA_V1 + (addr & 3); > > + *value = rtw_read8(rtwdev, read_addr); > > + return 0; > > +} > > + > > -- > > 2.17.1 > > > Yan-Hsuan
Tony Chuang <yhchuang@realtek.com> 於 2019年11月13日 下午3:16 寫道： >>> On Thu, Nov 7, 2019 at 7:16 PM <yhchuang@realtek.com> wrote: >>> >>> From: Yan-Hsuan Chuang <yhchuang@realtek.com> >>> >>> By Realtek's design, there are two HW modules associated for CLKREQ, >>> one is responsible to follow the PCIE host settings, and another >>> is to actually working on it. But the module that is actually working >>> on it is default disabled, and driver should enable that module if >>> host and device have successfully sync'ed with each other. >>> >>> The module is default disabled because sometimes the host does not >>> support it, and if there is any incorrect settings (ex. CLKREQ# is >>> not Bi-Direction), device can be lost and disconnected to the host. >>> >>> So driver should first check after host and device are sync'ed, and >>> the host does support the function and set it in configuration >>> space, then driver can turn on the HW module to working on it. >>> >>> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> >>> --- >>> drivers/net/wireless/realtek/rtw88/pci.c | 83 >> ++++++++++++++++++++++++ >>> drivers/net/wireless/realtek/rtw88/pci.h | 5 ++ >>> 2 files changed, 88 insertions(+) >>> >>> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c >> b/drivers/net/wireless/realtek/rtw88/pci.c >>> index 6d1aa6f41e84..4fcef8a6fc42 100644 >>> --- a/drivers/net/wireless/realtek/rtw88/pci.c >>> +++ b/drivers/net/wireless/realtek/rtw88/pci.c >>> @@ -1081,6 +1081,33 @@ static void rtw_dbi_write8(struct rtw_dev >> *rtwdev, u16 addr, u8 data) >>> WARN(flag, "failed to write to DBI register, addr=0x%04x\n", >> addr); >>> } >>> >>> +static int rtw_dbi_read8(struct rtw_dev *rtwdev, u16 addr, u8 *value) >>> +{ >>> + u16 read_addr = addr & BITS_DBI_ADDR_MASK; >>> + u8 flag; >>> + u8 cnt; >>> + >>> + rtw_write16(rtwdev, REG_DBI_FLAG_V1, read_addr); >>> + rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, BIT_DBI_RFLAG >> >> 16); >>> + >>> + for (cnt = 0; cnt < RTW_PCI_WR_RETRY_CNT; cnt++) { >>> + flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2); >>> + if (flag == 0) >>> + break; >> Why not just doing ' rtw_read8(rtwdev, read_addr);' and return here? >> Then you don't need to check the flag != 0 at the following. It would >> make the code cleaner and same expressive. > > Maybe it would look cleaner, but you need to add statements when > 'if (flag == 0)', then the indents will be deeper. > Also you will need to return 0 at the end if 'flag == 0'. > Yup. And I believe it’s still under 80 characters so I think I’m cool with it. > So you still think it's cleaner to write it that way? If so, I can try to send > v2 for it. >> >>> + >>> + udelay(10); >>> + } >>> + >>> + if (flag != 0) { >>> + WARN(1, "failed to read DBI register, addr=0x%04x\n", >> addr); >>> + return -EIO; >>> + } >>> + >>> + read_addr = REG_DBI_RDATA_V1 + (addr & 3); >>> + *value = rtw_read8(rtwdev, read_addr); >>> + return 0; >>> +} >>> + >>> -- >>> 2.17.1 >>> >> > > Yan-Hsuan
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 6d1aa6f41e84..4fcef8a6fc42 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -1081,6 +1081,33 @@ static void rtw_dbi_write8(struct rtw_dev *rtwdev, u16 addr, u8 data) WARN(flag, "failed to write to DBI register, addr=0x%04x\n", addr); } +static int rtw_dbi_read8(struct rtw_dev *rtwdev, u16 addr, u8 *value) +{ + u16 read_addr = addr & BITS_DBI_ADDR_MASK; + u8 flag; + u8 cnt; + + rtw_write16(rtwdev, REG_DBI_FLAG_V1, read_addr); + rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, BIT_DBI_RFLAG >> 16); + + for (cnt = 0; cnt < RTW_PCI_WR_RETRY_CNT; cnt++) { + flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2); + if (flag == 0) + break; + + udelay(10); + } + + if (flag != 0) { + WARN(1, "failed to read DBI register, addr=0x%04x\n", addr); + return -EIO; + } + + read_addr = REG_DBI_RDATA_V1 + (addr & 3); + *value = rtw_read8(rtwdev, read_addr); + return 0; +} + static void rtw_mdio_write(struct rtw_dev *rtwdev, u8 addr, u16 data, bool g1) { u8 page; @@ -1107,6 +1134,60 @@ static void rtw_mdio_write(struct rtw_dev *rtwdev, u8 addr, u16 data, bool g1) WARN(wflag, "failed to write to MDIO register, addr=0x%02x\n", addr); } +static void rtw_pci_clkreq_set(struct rtw_dev *rtwdev, bool enable) +{ + u8 value; + int ret; + + ret = rtw_dbi_read8(rtwdev, RTK_PCIE_LINK_CFG, &value); + if (ret) { + rtw_err(rtwdev, "failed to read CLKREQ_L1, ret=%d", ret); + return; + } + + if (enable) + value |= BIT_CLKREQ_SW_EN; + else + value &= ~BIT_CLKREQ_SW_EN; + + rtw_dbi_write8(rtwdev, RTK_PCIE_LINK_CFG, value); +} + +static void rtw_pci_link_cfg(struct rtw_dev *rtwdev) +{ + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv; + struct pci_dev *pdev = rtwpci->pdev; + u16 link_ctrl; + int ret; + + /* Though there is standard PCIE configuration space to set the + * link control register, but by Realtek's design, driver should + * check if host supports CLKREQ/ASPM to enable the HW module. + * + * These functions are implemented by two HW modules associated, + * one is responsible to access PCIE configuration space to + * follow the host settings, and another is in charge of doing + * CLKREQ/ASPM mechanisms, it is default disabled. Because sometimes + * the host does not support it, and due to some reasons or wrong + * settings (ex. CLKREQ# not Bi-Direction), it could lead to device + * loss if HW misbehaves on the link. + * + * Hence it's designed that driver should first check the PCIE + * configuration space is sync'ed and enabled, then driver can turn + * on the other module that is actually working on the mechanism. + */ + ret = pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &link_ctrl); + if (ret) { + rtw_err(rtwdev, "failed to read PCI cap, ret=%d\n", ret); + return; + } + + if (link_ctrl & PCI_EXP_LNKCTL_CLKREQ_EN) + rtw_pci_clkreq_set(rtwdev, true); + + rtwpci->link_ctrl = link_ctrl; +} + static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev) { struct rtw_chip_info *chip = rtwdev->chip; @@ -1145,6 +1226,8 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev) else rtw_dbi_write8(rtwdev, offset, value); } + + rtw_pci_link_cfg(rtwdev); } static int rtw_pci_claim(struct rtw_dev *rtwdev, struct pci_dev *pdev) diff --git a/drivers/net/wireless/realtek/rtw88/pci.h b/drivers/net/wireless/realtek/rtw88/pci.h index 50aff49738d4..90efb73c607e 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.h +++ b/drivers/net/wireless/realtek/rtw88/pci.h @@ -20,6 +20,7 @@ #define BIT_RST_TRXDMA_INTF BIT(20) #define BIT_RX_TAG_EN BIT(15) #define REG_DBI_WDATA_V1 0x03E8 +#define REG_DBI_RDATA_V1 0x03EC #define REG_DBI_FLAG_V1 0x03F0 #define BIT_DBI_RFLAG BIT(17) #define BIT_DBI_WFLAG BIT(16) @@ -35,6 +36,9 @@ #define RTW_PCI_MDIO_PG_OFFS_G2 2 #define RTW_PCI_WR_RETRY_CNT 20 +#define RTK_PCIE_LINK_CFG 0x0719 +#define BIT_CLKREQ_SW_EN BIT(4) + #define BIT_PCI_BCNQ_FLAG BIT(4) #define RTK_PCI_TXBD_DESA_BCNQ 0x308 #define RTK_PCI_TXBD_DESA_H2CQ 0x1320 @@ -200,6 +204,7 @@ struct rtw_pci { u16 rx_tag; struct rtw_pci_tx_ring tx_rings[RTK_MAX_TX_QUEUE_NUM]; struct rtw_pci_rx_ring rx_rings[RTK_MAX_RX_QUEUE_NUM]; + u16 link_ctrl; void __iomem *mmap; };