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

[Neil Armstrong]

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@xxxxxxx>
> > > ---
> > >
> > >    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