[PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[Ye Xiang]

ISH firmware uses connected standby state bit (CONNECTED_STANDBY_STATE_BIT bit 1)
to notify current power state to sensors instead of suspend state bit (bit 0).
So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to firmware
to be compatible with the previous version.

Signed-off-by: xiangye <xiang.ye@intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/hbm.c | 6 +++---
 drivers/hid/intel-ish-hid/ishtp/hbm.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/hbm.c b/drivers/hid/intel-ish-hid/ishtp/hbm.c
index 30a91d068306..dbfae60f2621 100644
--- a/drivers/hid/intel-ish-hid/ishtp/hbm.c
+++ b/drivers/hid/intel-ish-hid/ishtp/hbm.c
@@ -914,7 +914,7 @@ static inline void fix_cl_hdr(struct ishtp_msg_hdr *hdr, size_t length,
 /*** Suspend and resume notification ***/
 
 static uint32_t current_state;
-static uint32_t supported_states = 0 | SUSPEND_STATE_BIT;
+static uint32_t supported_states = SUSPEND_STATE_BIT | CONNECTED_STANDBY_STATE_BIT;
 
 /**
  * ishtp_send_suspend() - Send suspend message to FW
@@ -933,7 +933,7 @@ void ishtp_send_suspend(struct ishtp_device *dev)
 	memset(&state_status_msg, 0, len);
 	state_status_msg.hdr.cmd = SYSTEM_STATE_STATUS;
 	state_status_msg.supported_states = supported_states;
-	current_state |= SUSPEND_STATE_BIT;
+	current_state |= (SUSPEND_STATE_BIT | CONNECTED_STANDBY_STATE_BIT);
 	dev->print_log(dev, "%s() sends SUSPEND notification\n", __func__);
 	state_status_msg.states_status = current_state;
 
@@ -959,7 +959,7 @@ void ishtp_send_resume(struct ishtp_device *dev)
 	memset(&state_status_msg, 0, len);
 	state_status_msg.hdr.cmd = SYSTEM_STATE_STATUS;
 	state_status_msg.supported_states = supported_states;
-	current_state &= ~SUSPEND_STATE_BIT;
+	current_state &= ~(CONNECTED_STANDBY_STATE_BIT | SUSPEND_STATE_BIT);
 	dev->print_log(dev, "%s() sends RESUME notification\n", __func__);
 	state_status_msg.states_status = current_state;
 
diff --git a/drivers/hid/intel-ish-hid/ishtp/hbm.h b/drivers/hid/intel-ish-hid/ishtp/hbm.h
index 7c445b203f2a..08f3f3ceb18c 100644
--- a/drivers/hid/intel-ish-hid/ishtp/hbm.h
+++ b/drivers/hid/intel-ish-hid/ishtp/hbm.h
@@ -235,6 +235,7 @@ struct dma_xfer_hbm {
 #define SYSTEM_STATE_QUERY_SUBSCRIBERS		0x3
 #define SYSTEM_STATE_STATE_CHANGE_REQ		0x4
 /*indicates suspend and resume states*/
+#define CONNECTED_STANDBY_STATE_BIT		(1<<0)
 #define SUSPEND_STATE_BIT			(1<<1)
 
 struct ish_system_states_header {
-- 
2.17.1

Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[Jiri Kosina]

On Wed, 3 Mar 2021, Ye Xiang wrote:

> ISH firmware uses connected standby state bit (CONNECTED_STANDBY_STATE_BIT bit 1)
> to notify current power state to sensors instead of suspend state bit (bit 0).
> So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to firmware
> to be compatible with the previous version.

Could you please make the changelog more verbose -- namely what 
user-visible issue this is fixing?

Thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[Srinivas Pandruvada]

On Mon, 2021-03-08 at 11:26 +0100, Jiri Kosina wrote:
> On Wed, 3 Mar 2021, Ye Xiang wrote:
> 
> > ISH firmware uses connected standby state bit
> > (CONNECTED_STANDBY_STATE_BIT bit 1)
> > to notify current power state to sensors instead of suspend state
> > bit (bit 0).
> > So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to
> > firmware
> > to be compatible with the previous version.
> 
> Could you please make the changelog more verbose -- namely what 
> user-visible issue this is fixing?
Xiang,

I think this change is for related to Elkhart Lake for support of
connected standby (keep listening for sensor events during Linux
suspend for some sensors). In this way some sensor can wake up the
system.

Thanks,
Srinivas





> 
> Thanks.
> 

Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[Ye, Xiang]

Hi Srinivas, Jiri

On Mon, Mar 08, 2021 at 08:00:41AM -0800, Srinivas Pandruvada wrote:
> On Mon, 2021-03-08 at 11:26 +0100, Jiri Kosina wrote:
> > On Wed, 3 Mar 2021, Ye Xiang wrote:
> > 
> > > ISH firmware uses connected standby state bit
> > > (CONNECTED_STANDBY_STATE_BIT bit 1)
> > > to notify current power state to sensors instead of suspend state
> > > bit (bit 0).
> > > So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to
> > > firmware
> > > to be compatible with the previous version.
> > 
> > Could you please make the changelog more verbose -- namely what 
> > user-visible issue this is fixing?
> Xiang,
> 
> I think this change is for related to Elkhart Lake for support of
> connected standby (keep listening for sensor events during Linux
> suspend for some sensors). In this way some sensor can wake up the
> system.
This change is for all ISH platform. Currently, ISH firmware use
both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to identify
system state. It is related to system wake up by ISH and it enable each
sensor in ISH to be notified the current system state, when system state
change.

Thanks
Ye Xiang
> 
> 
> 
> 
> 
> 
> 

Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[Srinivas Pandruvada]

On Tue, 2021-03-09 at 11:47 +0800, Ye, Xiang wrote:
> Hi Srinivas, Jiri
> 
> On Mon, Mar 08, 2021 at 08:00:41AM -0800, Srinivas Pandruvada wrote:
> > On Mon, 2021-03-08 at 11:26 +0100, Jiri Kosina wrote:
> > > On Wed, 3 Mar 2021, Ye Xiang wrote:
> > > 
> > > > ISH firmware uses connected standby state bit
> > > > (CONNECTED_STANDBY_STATE_BIT bit 1)
> > > > to notify current power state to sensors instead of suspend
> > > > state
> > > > bit (bit 0).
> > > > So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT
> > > > to
> > > > firmware
> > > > to be compatible with the previous version.
> > > 
> > > Could you please make the changelog more verbose -- namely what 
> > > user-visible issue this is fixing?
> > Xiang,
> > 
> > I think this change is for related to Elkhart Lake for support of
> > connected standby (keep listening for sensor events during Linux
> > suspend for some sensors). In this way some sensor can wake up the
> > system.
> This change is for all ISH platform. Currently, ISH firmware use
> both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to identify
> system state. It is related to system wake up by ISH and it enable
> each
> sensor in ISH to be notified the current system state, when system
> state
> change.
What will sensors do with this additional information?
I think the individual sensors in ISH can decide whether to power OFF
or ON based on this information to save power during system suspend to
idle.

Thanks,
Srinivas

> 
> Thanks
> Ye Xiang
> > 
> > 
> > 
> > 
> > 
> > 

Re: [PATCH] HID: intel_ish-hid: HBM: Use connected standby state bit during suspend/resume

[Ye, Xiang]

Hi Srinivas

Thanks for the review.

On Tue, Mar 09, 2021 at 08:08:36AM -0800, Srinivas Pandruvada wrote:
> On Tue, 2021-03-09 at 11:47 +0800, Ye, Xiang wrote:
> > Hi Srinivas, Jiri
> > 
> > On Mon, Mar 08, 2021 at 08:00:41AM -0800, Srinivas Pandruvada wrote:
> > > On Mon, 2021-03-08 at 11:26 +0100, Jiri Kosina wrote:
> > > > On Wed, 3 Mar 2021, Ye Xiang wrote:
> > > > 
> > > > > ISH firmware uses connected standby state bit
> > > > > (CONNECTED_STANDBY_STATE_BIT bit 1)
> > > > > to notify current power state to sensors instead of suspend
> > > > > state
> > > > > bit (bit 0).
> > > > > So send both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT
> > > > > to
> > > > > firmware
> > > > > to be compatible with the previous version.
> > > > 
> > > > Could you please make the changelog more verbose -- namely what 
> > > > user-visible issue this is fixing?
> > > Xiang,
> > > 
> > > I think this change is for related to Elkhart Lake for support of
> > > connected standby (keep listening for sensor events during Linux
> > > suspend for some sensors). In this way some sensor can wake up the
> > > system.
> > This change is for all ISH platform. Currently, ISH firmware use
> > both SUSPEND_STATE_BIT and CONNECTED_STANDBY_STATE_BIT to identify
> > system state. It is related to system wake up by ISH and it enable
> > each
> > sensor in ISH to be notified the current system state, when system
> > state
> > change.
> What will sensors do with this additional information?
> I think the individual sensors in ISH can decide whether to power OFF
> or ON based on this information to save power during system suspend to
> idle.
Currently, In ISH firmware, we are using CONNECTED_STANDBY_STATE_BIT
(To be compatible with Windows os which are using CONNECTED_STANDBY_STATE_BIT) to
notify sensor system power state. Then some sensor(such as hinge sensor)
can power On/Off itself according to current system power state.

Thanks
Ye Xiang