diff mbox series

[v2,1/5] drm: bridge: dw_hdmi: cec: Add cec suspend/resume function

Message ID 45739041a743cd435415ca53264678e57a653147.1649412256.git.Sandor.yu@nxp.com (mailing list archive)
State New, archived
Headers show
Series DRM: Bridge: DW_HDMI: Add new features and bug fix | expand

Commit Message

Sandor Yu April 8, 2022, 10:32 a.m. UTC
CEC interrupt status/mask and logical address registers
will be reset when device enter suspend.
It will cause cec fail to work after device resume.
Add CEC suspend/resume functions, reinitialize logical address registers
and restore interrupt status/mask registers after resume.

Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Neil Armstrong April 8, 2022, 1:41 p.m. UTC | #1
On 08/04/2022 12:32, Sandor Yu wrote:
> CEC interrupt status/mask and logical address registers
> will be reset when device enter suspend.
> It will cause cec fail to work after device resume.
> Add CEC suspend/resume functions, reinitialize logical address registers
> and restore interrupt status/mask registers after resume.
> 
> Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 37 +++++++++++++++++++
>   1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> index c8f44bcb298a..ab176401b727 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> @@ -62,6 +62,10 @@ struct dw_hdmi_cec {
>   	bool rx_done;
>   	struct cec_notifier *notify;
>   	int irq;
> +
> +	u8 regs_polarity;
> +	u8 regs_mask;
> +	u8 regs_mute_stat0;
>   };
>   
>   static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset)
> @@ -306,11 +310,44 @@ static int dw_hdmi_cec_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int __maybe_unused dw_hdmi_cec_resume(struct device *dev)
> +{
> +	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
> +
> +	/* Restore logical address */
> +	dw_hdmi_write(cec, cec->addresses & 255, HDMI_CEC_ADDR_L);
> +	dw_hdmi_write(cec, cec->addresses >> 8, HDMI_CEC_ADDR_H);
> +
> +	/* Restore interrupt status/mask register */
> +	dw_hdmi_write(cec, cec->regs_polarity, HDMI_CEC_POLARITY);
> +	dw_hdmi_write(cec, cec->regs_mask, HDMI_CEC_MASK);
> +	dw_hdmi_write(cec, cec->regs_mute_stat0, HDMI_IH_MUTE_CEC_STAT0);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused dw_hdmi_cec_suspend(struct device *dev)
> +{
> +	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
> +
> +	/* store interrupt status/mask register */
> +	 cec->regs_polarity = dw_hdmi_read(cec, HDMI_CEC_POLARITY);
> +	 cec->regs_mask = dw_hdmi_read(cec, HDMI_CEC_MASK);
> +	 cec->regs_mute_stat0 = dw_hdmi_read(cec, HDMI_IH_MUTE_CEC_STAT0);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops dw_hdmi_cec_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(dw_hdmi_cec_suspend, dw_hdmi_cec_resume)
> +};
> +
>   static struct platform_driver dw_hdmi_cec_driver = {
>   	.probe	= dw_hdmi_cec_probe,
>   	.remove	= dw_hdmi_cec_remove,
>   	.driver = {
>   		.name = "dw-hdmi-cec",
> +		.pm = &dw_hdmi_cec_pm,
>   	},
>   };
>   module_platform_driver(dw_hdmi_cec_driver);

As Hans said on v1, why don't you call dw_hdmi_cec_enable(cec->adap, false) in suspend and dw_hdmi_cec_enable(cec->adap, true) in resume ?

With this, CEC engine is not disabled on suspend.

Do you plan to use the engine from the suspend code ?

Neil
Jernej Škrabec April 10, 2022, 10:13 a.m. UTC | #2
Dne petek, 08. april 2022 ob 15:41:36 CEST je Neil Armstrong napisal(a):
> On 08/04/2022 12:32, Sandor Yu wrote:
> > CEC interrupt status/mask and logical address registers
> > will be reset when device enter suspend.
> > It will cause cec fail to work after device resume.
> > Add CEC suspend/resume functions, reinitialize logical address registers
> > and restore interrupt status/mask registers after resume.
> > 
> > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > ---
> > 
> >   drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 37 +++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c index
> > c8f44bcb298a..ab176401b727 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> > @@ -62,6 +62,10 @@ struct dw_hdmi_cec {
> > 
> >   	bool rx_done;
> >   	struct cec_notifier *notify;
> >   	int irq;
> > 
> > +
> > +	u8 regs_polarity;
> > +	u8 regs_mask;
> > +	u8 regs_mute_stat0;
> > 
> >   };
> >   
> >   static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset)
> > 
> > @@ -306,11 +310,44 @@ static int dw_hdmi_cec_remove(struct platform_device
> > *pdev)> 
> >   	return 0;
> >   
> >   }
> > 
> > +static int __maybe_unused dw_hdmi_cec_resume(struct device *dev)
> > +{
> > +	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
> > +
> > +	/* Restore logical address */
> > +	dw_hdmi_write(cec, cec->addresses & 255, HDMI_CEC_ADDR_L);
> > +	dw_hdmi_write(cec, cec->addresses >> 8, HDMI_CEC_ADDR_H);
> > +
> > +	/* Restore interrupt status/mask register */
> > +	dw_hdmi_write(cec, cec->regs_polarity, HDMI_CEC_POLARITY);
> > +	dw_hdmi_write(cec, cec->regs_mask, HDMI_CEC_MASK);
> > +	dw_hdmi_write(cec, cec->regs_mute_stat0, HDMI_IH_MUTE_CEC_STAT0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused dw_hdmi_cec_suspend(struct device *dev)
> > +{
> > +	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
> > +
> > +	/* store interrupt status/mask register */
> > +	 cec->regs_polarity = dw_hdmi_read(cec, HDMI_CEC_POLARITY);
> > +	 cec->regs_mask = dw_hdmi_read(cec, HDMI_CEC_MASK);
> > +	 cec->regs_mute_stat0 = dw_hdmi_read(cec, HDMI_IH_MUTE_CEC_STAT0);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops dw_hdmi_cec_pm = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(dw_hdmi_cec_suspend, dw_hdmi_cec_resume)
> > +};
> > +
> > 
> >   static struct platform_driver dw_hdmi_cec_driver = {
> >   
> >   	.probe	= dw_hdmi_cec_probe,
> >   	.remove	= dw_hdmi_cec_remove,
> >   	.driver = {
> >   	
> >   		.name = "dw-hdmi-cec",
> > 
> > +		.pm = &dw_hdmi_cec_pm,
> > 
> >   	},
> >   
> >   };
> >   module_platform_driver(dw_hdmi_cec_driver);
> 
> As Hans said on v1, why don't you call dw_hdmi_cec_enable(cec->adap, false)
> in suspend and dw_hdmi_cec_enable(cec->adap, true) in resume ?
> 
> With this, CEC engine is not disabled on suspend.

This should not be done, at least not unconditionally. CEC wakeup 
functionality is used by Crust firmware for Allwinner boards. In fact, DW HDMI 
CEC controller was designed with suspend/resume functionality in mind. If 
properly set, it can autonomously scan for preset CEC messages and generate 
interrupt when found.

Actually, I'm not fan of this patch, since it looks like workaround for power 
management firmware not restoring previous state. Or is this HW issue? In any 
case, Allwinner SoCs with DW-HDMI CEC don't need restoring any register, so 
it's certainly not a general issue.

> 
> Do you plan to use the engine from the suspend code ?

As mentioned before, it's already done for Allwinner, so CEC should remain 
enabled.

Best regards,
Jernej

> 
> Neil
Neil Armstrong April 11, 2022, 7:42 a.m. UTC | #3
On 10/04/2022 12:13, Jernej Škrabec wrote:
> Dne petek, 08. april 2022 ob 15:41:36 CEST je Neil Armstrong napisal(a):
>> On 08/04/2022 12:32, Sandor Yu wrote:
>>> CEC interrupt status/mask and logical address registers
>>> will be reset when device enter suspend.
>>> It will cause cec fail to work after device resume.
>>> Add CEC suspend/resume functions, reinitialize logical address registers
>>> and restore interrupt status/mask registers after resume.
>>>
>>> Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
>>> ---
>>>
>>>    drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 37 +++++++++++++++++++
>>>    1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c index
>>> c8f44bcb298a..ab176401b727 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
>>> @@ -62,6 +62,10 @@ struct dw_hdmi_cec {
>>>
>>>    	bool rx_done;
>>>    	struct cec_notifier *notify;
>>>    	int irq;
>>>
>>> +
>>> +	u8 regs_polarity;
>>> +	u8 regs_mask;
>>> +	u8 regs_mute_stat0;
>>>
>>>    };
>>>    
>>>    static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset)
>>>
>>> @@ -306,11 +310,44 @@ static int dw_hdmi_cec_remove(struct platform_device
>>> *pdev)>
>>>    	return 0;
>>>    
>>>    }
>>>
>>> +static int __maybe_unused dw_hdmi_cec_resume(struct device *dev)
>>> +{
>>> +	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
>>> +
>>> +	/* Restore logical address */
>>> +	dw_hdmi_write(cec, cec->addresses & 255, HDMI_CEC_ADDR_L);
>>> +	dw_hdmi_write(cec, cec->addresses >> 8, HDMI_CEC_ADDR_H);
>>> +
>>> +	/* Restore interrupt status/mask register */
>>> +	dw_hdmi_write(cec, cec->regs_polarity, HDMI_CEC_POLARITY);
>>> +	dw_hdmi_write(cec, cec->regs_mask, HDMI_CEC_MASK);
>>> +	dw_hdmi_write(cec, cec->regs_mute_stat0, HDMI_IH_MUTE_CEC_STAT0);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int __maybe_unused dw_hdmi_cec_suspend(struct device *dev)
>>> +{
>>> +	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
>>> +
>>> +	/* store interrupt status/mask register */
>>> +	 cec->regs_polarity = dw_hdmi_read(cec, HDMI_CEC_POLARITY);
>>> +	 cec->regs_mask = dw_hdmi_read(cec, HDMI_CEC_MASK);
>>> +	 cec->regs_mute_stat0 = dw_hdmi_read(cec, HDMI_IH_MUTE_CEC_STAT0);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops dw_hdmi_cec_pm = {
>>> +	SET_SYSTEM_SLEEP_PM_OPS(dw_hdmi_cec_suspend, dw_hdmi_cec_resume)
>>> +};
>>> +
>>>
>>>    static struct platform_driver dw_hdmi_cec_driver = {
>>>    
>>>    	.probe	= dw_hdmi_cec_probe,
>>>    	.remove	= dw_hdmi_cec_remove,
>>>    	.driver = {
>>>    	
>>>    		.name = "dw-hdmi-cec",
>>>
>>> +		.pm = &dw_hdmi_cec_pm,
>>>
>>>    	},
>>>    
>>>    };
>>>    module_platform_driver(dw_hdmi_cec_driver);
>>
>> As Hans said on v1, why don't you call dw_hdmi_cec_enable(cec->adap, false)
>> in suspend and dw_hdmi_cec_enable(cec->adap, true) in resume ?
>>
>> With this, CEC engine is not disabled on suspend.
> 
> This should not be done, at least not unconditionally. CEC wakeup
> functionality is used by Crust firmware for Allwinner boards. In fact, DW HDMI
> CEC controller was designed with suspend/resume functionality in mind. If
> properly set, it can autonomously scan for preset CEC messages and generate
> interrupt when found.
> 
> Actually, I'm not fan of this patch, since it looks like workaround for power
> management firmware not restoring previous state. Or is this HW issue? In any
> case, Allwinner SoCs with DW-HDMI CEC don't need restoring any register, so
> it's certainly not a general issue.

In this case I think it should explicit in the hdmi_plat_data to either let
CEC enabled on suspend or then disable CEC on suspend.

> 
>>
>> Do you plan to use the engine from the suspend code ?
> 
> As mentioned before, it's already done for Allwinner, so CEC should remain
> enabled

Thanks for confirming that

> 
> Best regards,
> Jernej
> 
>>
>> Neil
> 
> 
> 
> 

Neil
Sandor Yu April 11, 2022, 7:47 a.m. UTC | #4
> -----Original Message-----
> From: Jernej Škrabec <jernej.skrabec@gmail.com>
> Sent: 2022年4月10日 18:13
> To: Sandor Yu <sandor.yu@nxp.com>; dri-devel@lists.freedesktop.org;
> linux-kernel@vger.kernel.org; andrzej.hajda@intel.com;
> robert.foss@linaro.org; Laurent.pinchart@ideasonboard.com;
> jonas@kwiboo.se; hverkuil-cisco@xs4all.nl; Neil Armstrong
> <narmstrong@baylibre.com>
> Cc: S.J. Wang <shengjiu.wang@nxp.com>; cai.huoqing@linux.dev;
> maxime@cerno.tech; harry.wentland@amd.com
> Subject: [EXT] Re: [PATCH v2 1/5] drm: bridge: dw_hdmi: cec: Add cec
> suspend/resume function
> 
> Caution: EXT Email
> 
> Dne petek, 08. april 2022 ob 15:41:36 CEST je Neil Armstrong napisal(a):
> > On 08/04/2022 12:32, Sandor Yu wrote:
> > > CEC interrupt status/mask and logical address registers will be
> > > reset when device enter suspend.
> > > It will cause cec fail to work after device resume.
> > > Add CEC suspend/resume functions, reinitialize logical address
> > > registers and restore interrupt status/mask registers after resume.
> > >
> > > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > > ---
> > >
> > >   drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 37
> +++++++++++++++++++
> > >   1 file changed, 37 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c index
> > > c8f44bcb298a..ab176401b727 100644
> > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> > > @@ -62,6 +62,10 @@ struct dw_hdmi_cec {
> > >
> > >     bool rx_done;
> > >     struct cec_notifier *notify;
> > >     int irq;
> > >
> > > +
> > > +   u8 regs_polarity;
> > > +   u8 regs_mask;
> > > +   u8 regs_mute_stat0;
> > >
> > >   };
> > >
> > >   static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int
> > > offset)
> > >
> > > @@ -306,11 +310,44 @@ static int dw_hdmi_cec_remove(struct
> > > platform_device *pdev)>
> > >     return 0;
> > >
> > >   }
> > >
> > > +static int __maybe_unused dw_hdmi_cec_resume(struct device *dev) {
> > > +   struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
> > > +
> > > +   /* Restore logical address */
> > > +   dw_hdmi_write(cec, cec->addresses & 255, HDMI_CEC_ADDR_L);
> > > +   dw_hdmi_write(cec, cec->addresses >> 8, HDMI_CEC_ADDR_H);
> > > +
> > > +   /* Restore interrupt status/mask register */
> > > +   dw_hdmi_write(cec, cec->regs_polarity, HDMI_CEC_POLARITY);
> > > +   dw_hdmi_write(cec, cec->regs_mask, HDMI_CEC_MASK);
> > > +   dw_hdmi_write(cec, cec->regs_mute_stat0,
> > > + HDMI_IH_MUTE_CEC_STAT0);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static int __maybe_unused dw_hdmi_cec_suspend(struct device *dev) {
> > > +   struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
> > > +
> > > +   /* store interrupt status/mask register */
> > > +    cec->regs_polarity = dw_hdmi_read(cec, HDMI_CEC_POLARITY);
> > > +    cec->regs_mask = dw_hdmi_read(cec, HDMI_CEC_MASK);
> > > +    cec->regs_mute_stat0 = dw_hdmi_read(cec,
> > > + HDMI_IH_MUTE_CEC_STAT0);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops dw_hdmi_cec_pm = {
> > > +   SET_SYSTEM_SLEEP_PM_OPS(dw_hdmi_cec_suspend,
> dw_hdmi_cec_resume)
> > > +};
> > > +
> > >
> > >   static struct platform_driver dw_hdmi_cec_driver = {
> > >
> > >     .probe  = dw_hdmi_cec_probe,
> > >     .remove = dw_hdmi_cec_remove,
> > >     .driver = {
> > >
> > >             .name = "dw-hdmi-cec",
> > >
> > > +           .pm = &dw_hdmi_cec_pm,
> > >
> > >     },
> > >
> > >   };
> > >   module_platform_driver(dw_hdmi_cec_driver);
> >
> > As Hans said on v1, why don't you call dw_hdmi_cec_enable(cec->adap,
> > false) in suspend and dw_hdmi_cec_enable(cec->adap, true) in resume ?
> >
> > With this, CEC engine is not disabled on suspend.
> 
> This should not be done, at least not unconditionally. CEC wakeup
> functionality is used by Crust firmware for Allwinner boards. In fact, DW HDMI
> CEC controller was designed with suspend/resume functionality in mind. If
> properly set, it can autonomously scan for preset CEC messages and generate
> interrupt when found.
> 
> Actually, I'm not fan of this patch, since it looks like workaround for power
> management firmware not restoring previous state. Or is this HW issue? In
> any case, Allwinner SoCs with DW-HDMI CEC don't need restoring any register,
> so it's certainly not a general issue.
> 
For NXP i.MX8MPlus SOC, CEC wakeup function is not required.
And the HDMI TX power domain could be gated off with CEC engine when suspend. 
Restore those registers in order to keep EARC/ARC in working after resume when HDMI TX power domain off in suspend.
It could be use by other SOC for extremely low power case.

B.R
Sandor
>  
> >
> > Do you plan to use the engine from the suspend code ?
> 
> As mentioned before, it's already done for Allwinner, so CEC should remain
> enabled.
> 
> Best regards,
> Jernej
> 
> >
> > Neil
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
index c8f44bcb298a..ab176401b727 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
@@ -62,6 +62,10 @@  struct dw_hdmi_cec {
 	bool rx_done;
 	struct cec_notifier *notify;
 	int irq;
+
+	u8 regs_polarity;
+	u8 regs_mask;
+	u8 regs_mute_stat0;
 };
 
 static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset)
@@ -306,11 +310,44 @@  static int dw_hdmi_cec_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused dw_hdmi_cec_resume(struct device *dev)
+{
+	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
+
+	/* Restore logical address */
+	dw_hdmi_write(cec, cec->addresses & 255, HDMI_CEC_ADDR_L);
+	dw_hdmi_write(cec, cec->addresses >> 8, HDMI_CEC_ADDR_H);
+
+	/* Restore interrupt status/mask register */
+	dw_hdmi_write(cec, cec->regs_polarity, HDMI_CEC_POLARITY);
+	dw_hdmi_write(cec, cec->regs_mask, HDMI_CEC_MASK);
+	dw_hdmi_write(cec, cec->regs_mute_stat0, HDMI_IH_MUTE_CEC_STAT0);
+
+	return 0;
+}
+
+static int __maybe_unused dw_hdmi_cec_suspend(struct device *dev)
+{
+	struct dw_hdmi_cec *cec = dev_get_drvdata(dev);
+
+	/* store interrupt status/mask register */
+	 cec->regs_polarity = dw_hdmi_read(cec, HDMI_CEC_POLARITY);
+	 cec->regs_mask = dw_hdmi_read(cec, HDMI_CEC_MASK);
+	 cec->regs_mute_stat0 = dw_hdmi_read(cec, HDMI_IH_MUTE_CEC_STAT0);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_hdmi_cec_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(dw_hdmi_cec_suspend, dw_hdmi_cec_resume)
+};
+
 static struct platform_driver dw_hdmi_cec_driver = {
 	.probe	= dw_hdmi_cec_probe,
 	.remove	= dw_hdmi_cec_remove,
 	.driver = {
 		.name = "dw-hdmi-cec",
+		.pm = &dw_hdmi_cec_pm,
 	},
 };
 module_platform_driver(dw_hdmi_cec_driver);