Regarding SDHI clocks on RZ/G2L

Regarding SDHI clocks on RZ/G2L

[Biju Das]

Hi All,

As per the RZ/G2L clock list document, SDHI has 4 clocks and  IMCLK2 should not be turn off during suspend. 

1)IMCLK  Main Clock 1
2)IMCLK2 Main Clock 2, this clk should be not be turned off during suspend state, otherwise card detection won't work.
3)CLK_HS High speed clock
4)ACLK	Bus clock

Multi clock PM domain support for SDHI is available in RZ/G2L and we are filtering this clock not to add
PM domain.

But on the SDHI driver, we are handing cd clocks for RZ/A devices. Unfortunately we are turning of this clock
during suspend state. 

Q1) Is it expected behaviour for this device?

Q2)What is the best way to handle cd clocks for RZ/G2L?

1) Handle it in SDHI driver? ie, enable it during probe only once and avoid turning it off

or

2) Add this clock as critical clock, so it will be turned on permanently and don't handle it in SDHI driver?


Regards,
Biju

RE: Regarding SDHI clocks on RZ/G2L

[Chris Brandt]

Hi Biju,

> But on the SDHI driver, we are handing cd clocks for RZ/A devices.
> Unfortunately we are turning of this clock
> during suspend state.

I think it is fine to shut off the cd clock in a suspend state because plug/unplug a SD card is not really something that should wake up a system.

SHDI should not be a 'wake up' event source in a system, so we don't need it running during a suspend state.
Things like interrupt pins and GPIO are wake up triggers.

From a power perspective, I don't think it will save much power to turn all the clocks off. So your #1 or #2 would be fine.

As for #2, it's not really a 'critical clock' for the system.....it is more of a 'clock we don't care about'.

I would say go with the one that is the easiest and makes the least code changes. If adding these to the SDHI driver make it very ugly, then we should just add them as critical clock and be done with it.

That's my opinion.

Chris


> -----Original Message-----
> From: Biju Das
> Sent: Sunday, July 18, 2021 6:26 AM
> To: Wolfram Sang <wsa+renesas@sang-engineering.com>; Geert Uytterhoeven
> <geert+renesas@glider.be>; Linux-Renesas <linux-renesas-soc@vger.kernel.org>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>; Chris Brandt
> <Chris.Brandt@renesas.com>; Chris Paterson <Chris.Paterson2@renesas.com>
> Subject: Regarding SDHI clocks on RZ/G2L
> 
> Hi All,
> 
> As per the RZ/G2L clock list document, SDHI has 4 clocks and  IMCLK2 should
> not be turn off during suspend.
> 
> 1)IMCLK  Main Clock 1
> 2)IMCLK2 Main Clock 2, this clk should be not be turned off during suspend
> state, otherwise card detection won't work.
> 3)CLK_HS High speed clock
> 4)ACLK	Bus clock
> 
> Multi clock PM domain support for SDHI is available in RZ/G2L and we are
> filtering this clock not to add
> PM domain.
> 
> But on the SDHI driver, we are handing cd clocks for RZ/A devices.
> Unfortunately we are turning of this clock
> during suspend state.
> 
> Q1) Is it expected behaviour for this device?
> 
> Q2)What is the best way to handle cd clocks for RZ/G2L?
> 
> 1) Handle it in SDHI driver? ie, enable it during probe only once and avoid
> turning it off
> 
> or
> 
> 2) Add this clock as critical clock, so it will be turned on permanently and
> don't handle it in SDHI driver?
> 
> 
> Regards,
> Biju

RE: Regarding SDHI clocks on RZ/G2L

[Biju Das]

Hi Chris,

> Subject: RE: Regarding SDHI clocks on RZ/G2L
> 
> Hi Biju,
> 
> > But on the SDHI driver, we are handing cd clocks for RZ/A devices.
> > Unfortunately we are turning of this clock during suspend state.
> 
> I think it is fine to shut off the cd clock in a suspend state because
> plug/unplug a SD card is not really something that should wake up a
> system.

Some of the Mobile SoC's, SDHI is  a wakeup source for deep sleep. 
For that use case, We need to configure CD/WP pin as GPIO rather than function.
So looks like we don't have any use case like that here.


> SHDI should not be a 'wake up' event source in a system, so we don't need
> it running during a suspend state.

Agreed, if cd pin is configured as function. But for some use case It can be a wakeup source,
If it is configured as GPIO.

Run time PM enabled is in SDHI. So it will turn off both the clocks during suspend.

RZ/G2L document says we should not turn off cd clock during suspend. Otherwise card detection
Won't work. So whether SDHI can reliably work in this case? Basically if there is no activity,
this module can go into suspend state. If the cd clock turned off and card detection fails
during resume, how the SDHI functionality going to work? 


> Things like interrupt pins and GPIO are wake up triggers.
> 
> From a power perspective, I don't think it will save much power to turn
> all the clocks off. So your #1 or #2 would be fine.
> 
> As for #2, it's not really a 'critical clock' for the system.....it is
> more of a 'clock we don't care about'.

OK.

> I would say go with the one that is the easiest and makes the least code
> changes. If adding these to the SDHI driver make it very ugly, 

Easiest is to handle it in SDHI driver. May be to address "cd" clock for RZ/G2L SoC
we need special handling for suspend/resume callbacks.

@Wolfram Sang, What do you think?


Note:-
Currently I am configuring CD pin as gpio rather than function due to some reset issue[1]. After reset
cd detection fails. But it works ok, if it configured as GPIO.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/renesas_sdhi_core.c?h=v5.14-rc2#n580

Cheers,
Biju


> should just add them as critical clock and be done with it.





> > -----Original Message-----
> > From: Biju Das
> > Sent: Sunday, July 18, 2021 6:26 AM
> > To: Wolfram Sang <wsa+renesas@sang-engineering.com>; Geert
> > Uytterhoeven <geert+renesas@glider.be>; Linux-Renesas
> > <linux-renesas-soc@vger.kernel.org>
> > Cc: Phil Edworthy <phil.edworthy@renesas.com>; Chris Brandt
> > <Chris.Brandt@renesas.com>; Chris Paterson
> > <Chris.Paterson2@renesas.com>
> > Subject: Regarding SDHI clocks on RZ/G2L
> >
> > Hi All,
> >
> > As per the RZ/G2L clock list document, SDHI has 4 clocks and  IMCLK2
> > should not be turn off during suspend.
> >
> > 1)IMCLK  Main Clock 1
> > 2)IMCLK2 Main Clock 2, this clk should be not be turned off during
> > suspend state, otherwise card detection won't work.
> > 3)CLK_HS High speed clock
> > 4)ACLK	Bus clock
> >
> > Multi clock PM domain support for SDHI is available in RZ/G2L and we
> > are filtering this clock not to add PM domain.
> >
> > But on the SDHI driver, we are handing cd clocks for RZ/A devices.
> > Unfortunately we are turning of this clock during suspend state.
> >
> > Q1) Is it expected behaviour for this device?
> >
> > Q2)What is the best way to handle cd clocks for RZ/G2L?
> >
> > 1) Handle it in SDHI driver? ie, enable it during probe only once and
> > avoid turning it off
> >
> > or
> >
> > 2) Add this clock as critical clock, so it will be turned on
> > permanently and don't handle it in SDHI driver?
> >
> >
> > Regards,
> > Biju

RE: Regarding SDHI clocks on RZ/G2L

[Chris Brandt]

Hi Biju,

> -----Original Message-----
> From: Biju Das
> Sent: Tuesday, July 20, 2021 11:55 AM

> RZ/G2L document says we should not turn off cd clock during suspend.
> Otherwise card detection

From a hardware standpoint....yes.

> Won't work. So whether SDHI can reliably work in this case? Basically if
> there is no activity,
> this module can go into suspend state. If the cd clock turned off and card
> detection fails
> during resume, how the SDHI functionality going to work?

What I am saying is that from a system design standpoint, you need to wake up the system first before you can use the SD card (even detecting a card insert/remove).


> Note:-
> Currently I am configuring CD pin as gpio rather than function due to some
> reset issue[1]. After reset
> cd detection fails. But it works ok, if it configured as GPIO.

> 1) Handle it in SDHI driver? ie, enable it during probe only once and
> > avoid turning it off
> >
> > or
> >
> > 2) Add this clock as critical clock, so it will be turned on
> > permanently and don't handle it in SDHI driver?

So does that mean your #1 and #2 both do not work unless the pin is configured as GPIO?

Chris

RE: Regarding SDHI clocks on RZ/G2L

[Biju Das]

Hi Chris,

> Subject: RE: Regarding SDHI clocks on RZ/G2L
> 
> Hi Biju,
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: Tuesday, July 20, 2021 11:55 AM
> 
> > RZ/G2L document says we should not turn off cd clock during suspend.
> > Otherwise card detection
> 
> From a hardware standpoint....yes.
> 
> > Won't work. So whether SDHI can reliably work in this case? Basically
> > if there is no activity, this module can go into suspend state. If the
> > cd clock turned off and card detection fails during resume, how the
> > SDHI functionality going to work?
> 
> What I am saying is that from a system design standpoint, you need to wake
> up the system first before you can use the SD card (even detecting a card
> insert/remove).

Agreed. That is the usual flow.

> 
> 
> > Note:-
> > Currently I am configuring CD pin as gpio rather than function due to
> > some reset issue[1]. After reset cd detection fails. But it works ok,
> > if it configured as GPIO.
> 
> > 1) Handle it in SDHI driver? ie, enable it during probe only once and
> > > avoid turning it off
> > >
> > > or
> > >
> > > 2) Add this clock as critical clock, so it will be turned on
> > > permanently and don't handle it in SDHI driver?
> 
> So does that mean your #1 and #2 both do not work unless the pin is
> configured as GPIO?

With upstream kernel it doesn't work, because of the reset handling patch[1]
4.19 it works, where it doesn't have this patch.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/mmc/host/renesas_sdhi_core.c?h=v5.14-rc2&id=b4d86f37eacb724690d0d300576b82806bc743d5

Regards,
Biju