| Message ID | 20200417074653.15591-29-yhchuang@realtek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtw88: add support for 802.11n RTL8723DE devices | expand |
On 2020-04-17 15:46:41 [+0800], yhchuang@realtek.com wrote: > From: Ping-Ke Shih <pkshih@realtek.com> > > Without this patch, wifi card can't initialize properly due to BT in USB > suspend state. So, we disable BT USB suspend (wakeup) in shutdown callback > that is the moment before rebooting. To save BT USB power, we can't do this > in 'remove' callback. So you can't initialize the USB part because it is in suspend and the only way to avoid it to disable it on the PCI side. That means you don't see it enumerated on the USB bus at all? > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> Sebastian
> On 2020-04-17 15:46:41 [+0800], yhchuang@realtek.com wrote: > > From: Ping-Ke Shih <pkshih@realtek.com> > > > > Without this patch, wifi card can't initialize properly due to BT in USB > > suspend state. So, we disable BT USB suspend (wakeup) in shutdown callback > > that is the moment before rebooting. To save BT USB power, we can't do this > > in 'remove' callback. > > So you can't initialize the USB part because it is in suspend and the > only way to avoid it to disable it on the PCI side. That means you don't > see it enumerated on the USB bus at all? Yes, if we don't disable it on PCI side, then the USB part cannot be probed on USB bus. > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com> > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com> > > Sebastian > Yen-Hsuan
On 2020-05-06 02:35:21 [+0000], Tony Chuang wrote: > > On 2020-04-17 15:46:41 [+0800], yhchuang@realtek.com wrote: > > > From: Ping-Ke Shih <pkshih@realtek.com> > > > > > > Without this patch, wifi card can't initialize properly due to BT in USB > > > suspend state. So, we disable BT USB suspend (wakeup) in shutdown callback > > > that is the moment before rebooting. To save BT USB power, we can't do this > > > in 'remove' callback. > > > > So you can't initialize the USB part because it is in suspend and the > > only way to avoid it to disable it on the PCI side. That means you don't > > see it enumerated on the USB bus at all? > > Yes, if we don't disable it on PCI side, then the USB part cannot be > probed on USB bus. We talk here about USB's runtime-suspend / autosuspend? If so, are you aware of commit 7ecacafc24063 ("Bluetooth: btusb: Disable runtime suspend on Realtek devices") or is this an attempt to get rid of this change in favour of this one (so that the device can enter suspend-mode)? Sebastian
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes > On 2020-05-06 02:35:21 [+0000], Tony Chuang wrote: > > > On 2020-04-17 15:46:41 [+0800], yhchuang@realtek.com wrote: > > > > From: Ping-Ke Shih <pkshih@realtek.com> > > > > > > > > Without this patch, wifi card can't initialize properly due to BT in USB > > > > suspend state. So, we disable BT USB suspend (wakeup) in shutdown > callback > > > > that is the moment before rebooting. To save BT USB power, we can't do > this > > > > in 'remove' callback. > > > > > > So you can't initialize the USB part because it is in suspend and the > > > only way to avoid it to disable it on the PCI side. That means you don't > > > see it enumerated on the USB bus at all? > > > > Yes, if we don't disable it on PCI side, then the USB part cannot be > > probed on USB bus. > > We talk here about USB's runtime-suspend / autosuspend? If so, are you > aware of commit > 7ecacafc24063 ("Bluetooth: btusb: Disable runtime suspend on Realtek > devices") > > or is this an attempt to get rid of this change in favour of this one > (so that the device can enter suspend-mode)? > Ping-Ke, can you please help to check on this ? Looks like Kai-Heng is doing the much same thing here. But it's still worth to do it in wifi side I think, because it's difficult to make sure the synchronization of BT and Wifi patch. Yen-Hsuan
On 2020-05-07 04:26:24 [+0000], Tony Chuang wrote: > > Ping-Ke, can you please help to check on this ? > Looks like Kai-Heng is doing the much same thing here. > > But it's still worth to do it in wifi side I think, because it's difficult to > make sure the synchronization of BT and Wifi patch. Yes. It sounds reasonable to remove the patch in BT so the device is not always avoiding the suspend mode. I don't remember if I asked this: Shouldn't the USB reset get the device out of suspend? I thought this is part of the USB test. Could this be fixed in BT's firmware? > Yen-Hsuan Sebastian
Hi Tony, > -----Original Message----- > From: Tony Chuang > Sent: Thursday, May 07, 2020 12:26 PM > To: Sebastian Andrzej Siewior > Cc: kvalo@codeaurora.org; Pkshih; linux-wireless@vger.kernel.org; briannorris@chromium.org; Kevin > Yang > Subject: RE: [PATCH 28/40] rtw88: 8723d: Add shutdown callback to disable BT USB suspend > > Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes > > On 2020-05-06 02:35:21 [+0000], Tony Chuang wrote: > > > > On 2020-04-17 15:46:41 [+0800], yhchuang@realtek.com wrote: > > > > > From: Ping-Ke Shih <pkshih@realtek.com> > > > > > > > > > > Without this patch, wifi card can't initialize properly due to BT in USB > > > > > suspend state. So, we disable BT USB suspend (wakeup) in shutdown > > callback > > > > > that is the moment before rebooting. To save BT USB power, we can't do > > this > > > > > in 'remove' callback. > > > > > > > > So you can't initialize the USB part because it is in suspend and the > > > > only way to avoid it to disable it on the PCI side. That means you don't > > > > see it enumerated on the USB bus at all? > > > > > > Yes, if we don't disable it on PCI side, then the USB part cannot be > > > probed on USB bus. > > > > We talk here about USB's runtime-suspend / autosuspend? If so, are you > > aware of commit > > 7ecacafc24063 ("Bluetooth: btusb: Disable runtime suspend on Realtek > > devices") > > > > or is this an attempt to get rid of this change in favour of this one > > (so that the device can enter suspend-mode)? > > > > Ping-Ke, can you please help to check on this ? > Looks like Kai-Heng is doing the much same thing here. > The Kai-Heng's patch turns off suspend entirely, so I believe if the patch is existing, this patch doesn't affect the result. However, the patch seems like a temporal fix, so this patch is needed. > But it's still worth to do it in wifi side I think, because it's difficult to > make sure the synchronization of BT and Wifi patch. > Agree. Thank you PK
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h index 987573ddeefc..9421397ee444 100644 --- a/drivers/net/wireless/realtek/rtw88/main.h +++ b/drivers/net/wireless/realtek/rtw88/main.h @@ -793,6 +793,7 @@ struct rtw_regulatory { struct rtw_chip_ops { int (*mac_init)(struct rtw_dev *rtwdev); + void (*shutdown)(struct rtw_dev *rtwdev); int (*read_efuse)(struct rtw_dev *rtwdev, u8 *map); void (*phy_set_param)(struct rtw_dev *rtwdev); void (*set_channel)(struct rtw_dev *rtwdev, u8 channel, diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c index 8a8d746d3349..9f5edb8ea7a9 100644 --- a/drivers/net/wireless/realtek/rtw88/pci.c +++ b/drivers/net/wireless/realtek/rtw88/pci.c @@ -1577,6 +1577,22 @@ static void rtw_pci_remove(struct pci_dev *pdev) ieee80211_free_hw(hw); } +static void rtw_pci_shutdown(struct pci_dev *pdev) +{ + struct ieee80211_hw *hw = pci_get_drvdata(pdev); + struct rtw_dev *rtwdev; + struct rtw_chip_info *chip; + + if (!hw) + return; + + rtwdev = hw->priv; + chip = rtwdev->chip; + + if (chip->ops->shutdown) + chip->ops->shutdown(rtwdev); +} + static const struct pci_device_id rtw_pci_id_table[] = { #ifdef CONFIG_RTW88_8822BE { RTK_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0xB822, rtw8822b_hw_spec) }, @@ -1597,6 +1613,7 @@ static struct pci_driver rtw_pci_driver = { .probe = rtw_pci_probe, .remove = rtw_pci_remove, .driver.pm = RTW_PM_OPS, + .shutdown = rtw_pci_shutdown, }; module_pci_driver(rtw_pci_driver); diff --git a/drivers/net/wireless/realtek/rtw88/reg.h b/drivers/net/wireless/realtek/rtw88/reg.h index d57de1a6cdcc..5a3e9cc7c400 100644 --- a/drivers/net/wireless/realtek/rtw88/reg.h +++ b/drivers/net/wireless/realtek/rtw88/reg.h @@ -83,6 +83,7 @@ #define BIT_DBG_GNT_WL_BT BIT(27) #define BIT_LTE_MUX_CTRL_PATH BIT(26) #define REG_HCI_OPT_CTRL 0x0074 +#define BIT_USB_SUS_DIS BIT(8) #define REG_AFE_CTRL_4 0x0078 #define BIT_CK320M_AFE_EN BIT(4) diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723d.c b/drivers/net/wireless/realtek/rtw88/rtw8723d.c index 4fb361d91a0b..b58915e5e1de 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8723d.c +++ b/drivers/net/wireless/realtek/rtw88/rtw8723d.c @@ -580,6 +580,11 @@ static int rtw8723d_mac_init(struct rtw_dev *rtwdev) return 0; } +static void rtw8723d_shutdown(struct rtw_dev *rtwdev) +{ + rtw_write16_set(rtwdev, REG_HCI_OPT_CTRL, BIT_USB_SUS_DIS); +} + static void rtw8723d_cfg_ldo25(struct rtw_dev *rtwdev, bool enable) { u8 ldo_pwr; @@ -1834,6 +1839,7 @@ static struct rtw_chip_ops rtw8723d_ops = { .query_rx_desc = rtw8723d_query_rx_desc, .set_channel = rtw8723d_set_channel, .mac_init = rtw8723d_mac_init, + .shutdown = rtw8723d_shutdown, .read_rf = rtw_phy_read_rf_sipi, .write_rf = rtw_phy_write_rf_reg_sipi, .set_tx_power_index = rtw8723d_set_tx_power_index,