The SDIO HCI implementation needs to know when the MAC is powered on.
This is needed because 32-bit register access has to be split into 4x
8-bit register access when the MAC is not fully powered on or while
powering off. When the MAC is powered on 32-bit register access can be
used to reduce the number of transfers but splitting into 4x 8-bit
register access still works in that case.
During the power on sequence is how RTW_FLAG_POWERON is only set when
the power on sequence has completed successfully. During power off
however RTW_FLAG_POWERON is set. This means that the upcoming SDIO HCI
implementation does not know that it has to use 4x 8-bit register
accessors. Clear the RTW_FLAG_POWERON flag early when powering off the
MAC so the whole power off sequence is processed with RTW_FLAG_POWERON
unset. This will make it possible to use the RTW_FLAG_POWERON flag in
the upcoming SDIO HCI implementation.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
Changes since v1:
- This replaces a previous patch called "rtw88: hci: Add an optional
power_switch() callback to rtw_hci_ops" which added a new callback
to the HCI ops.
drivers/net/wireless/realtek/rtw88/mac.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c
index f3a566cf979b..cfdfc8a2c836 100644
--- a/drivers/net/wireless/realtek/rtw88/mac.c
+++ b/drivers/net/wireless/realtek/rtw88/mac.c
@@ -273,6 +273,9 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on)
if (pwr_on == cur_pwr)
return -EALREADY;
+ if (!pwr_on)
+ clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
+
pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
if (ret)
@@ -280,8 +283,6 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on)
if (pwr_on)
set_bit(RTW_FLAG_POWERON, rtwdev->flags);
- else
- clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
return 0;
}
--
2.39.2
> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Saturday, March 11, 2023 4:29 AM
> To: [email protected]
> Cc: Yan-Hsuan Chuang <[email protected]>; Kalle Valo <[email protected]>; Ulf Hansson
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Chris Morgan <[email protected]>; Nitin Gupta <[email protected]>;
> Neo Jou <[email protected]>; Ping-Ke Shih <[email protected]>; Jernej Skrabec <[email protected]>;
> Martin Blumenstingl <[email protected]>
> Subject: [PATCH v2 RFC 1/9] wifi: rtw88: Clear RTW_FLAG_POWERON early in rtw_mac_power_switch()
>
> The SDIO HCI implementation needs to know when the MAC is powered on.
> This is needed because 32-bit register access has to be split into 4x
> 8-bit register access when the MAC is not fully powered on or while
> powering off. When the MAC is powered on 32-bit register access can be
> used to reduce the number of transfers but splitting into 4x 8-bit
> register access still works in that case.
>
> During the power on sequence is how RTW_FLAG_POWERON is only set when
> the power on sequence has completed successfully. During power off
> however RTW_FLAG_POWERON is set. This means that the upcoming SDIO HCI
> implementation does not know that it has to use 4x 8-bit register
> accessors. Clear the RTW_FLAG_POWERON flag early when powering off the
> MAC so the whole power off sequence is processed with RTW_FLAG_POWERON
> unset. This will make it possible to use the RTW_FLAG_POWERON flag in
> the upcoming SDIO HCI implementation.
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> Changes since v1:
> - This replaces a previous patch called "rtw88: hci: Add an optional
> power_switch() callback to rtw_hci_ops" which added a new callback
> to the HCI ops.
>
>
> drivers/net/wireless/realtek/rtw88/mac.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c
> index f3a566cf979b..cfdfc8a2c836 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac.c
> @@ -273,6 +273,9 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on)
> if (pwr_on == cur_pwr)
> return -EALREADY;
>
> + if (!pwr_on)
> + clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
> +
> pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
> ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
> if (ret)
This patch changes the behavior if rtw_pwr_seq_parser() returns error while
doing power-off, but I dig and think further about this case hardware stays in
abnormal state. I think it would be fine to see this state as POWER_OFF.
Do you agree this as well?
> @@ -280,8 +283,6 @@ static int rtw_mac_power_switch(struct rtw_dev *rtwdev, bool pwr_on)
>
> if (pwr_on)
> set_bit(RTW_FLAG_POWERON, rtwdev->flags);
> - else
> - clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
>
> return 0;
> }
> --
> 2.39.2
Hello Ping-Ke,
On Mon, Mar 13, 2023 at 3:29 AM Ping-Ke Shih <[email protected]> wrote:
[...]
> > + if (!pwr_on)
> > + clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
> > +
> > pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
> > ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
> > if (ret)
>
> This patch changes the behavior if rtw_pwr_seq_parser() returns error while
> doing power-off, but I dig and think further about this case hardware stays in
> abnormal state. I think it would be fine to see this state as POWER_OFF.
> Do you agree this as well?
I agree with you. Also I think I should have made it clearer in the
description of the patch that I'm potentially changing the behavior
(and that this is not an issue in my opinion).
If there's any problem during the power on/off sequence then we can't
be fully sure about the power state.
If you have any suggestions how to improve this then please let me know.
Best regards,
Martin
> -----Original Message-----
> From: Martin Blumenstingl <[email protected]>
> Sent: Tuesday, March 14, 2023 4:08 AM
> To: Ping-Ke Shih <[email protected]>
> Cc: [email protected]; Yan-Hsuan Chuang <[email protected]>; Kalle Valo
> <[email protected]>; Ulf Hansson <[email protected]>; [email protected];
> [email protected]; [email protected]; Chris Morgan <[email protected]>; Nitin Gupta
> <[email protected]>; Neo Jou <[email protected]>; Jernej Skrabec <[email protected]>
> Subject: Re: [PATCH v2 RFC 1/9] wifi: rtw88: Clear RTW_FLAG_POWERON early in rtw_mac_power_switch()
>
> Hello Ping-Ke,
>
> On Mon, Mar 13, 2023 at 3:29 AM Ping-Ke Shih <[email protected]> wrote:
> [...]
> > > + if (!pwr_on)
> > > + clear_bit(RTW_FLAG_POWERON, rtwdev->flags);
> > > +
> > > pwr_seq = pwr_on ? chip->pwr_on_seq : chip->pwr_off_seq;
> > > ret = rtw_pwr_seq_parser(rtwdev, pwr_seq);
> > > if (ret)
> >
> > This patch changes the behavior if rtw_pwr_seq_parser() returns error while
> > doing power-off, but I dig and think further about this case hardware stays in
> > abnormal state. I think it would be fine to see this state as POWER_OFF.
> > Do you agree this as well?
> I agree with you. Also I think I should have made it clearer in the
> description of the patch that I'm potentially changing the behavior
> (and that this is not an issue in my opinion).
> If there's any problem during the power on/off sequence then we can't
> be fully sure about the power state.
> If you have any suggestions how to improve this then please let me know.
>
No more suggestion for now. Just apply your thought.
Ping-Ke