sdhci: mmc: determining card type and dynamic clocks

sdhci: mmc: determining card type and dynamic clocks

[Philip Rakity]

Sometimes it is useful for the SD driver to know the type of card that is in use so that dynamic sd bus clocking can be enabled or disabled.
Dynamic clocks are a feature of sd 3.0 and we can support this in our v2.0 controller.

The problem is that some SDIO cards are not happy when the clocks are dynamically stopped.  I have not seen any issues with SD/mmc/eMMC cards.  I suspect the reason for the SDIO problems is that the cards need the clock since SDIO cards can interrupt the CPU for service and the other card types cannot.

The information about the type of card being used is known by the mmc layer when 

mmc_alloc_card(struct mmc_host *host, struct device_type *type) --- core/bus.c

is called but the sd driver cannot get to this information when set_ios() is called by the mmc layer since the host->card field is filled in too late.

I have added host->card = card to

struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
{
	struct mmc_card *card;

	card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
	if (!card)
		return ERR_PTR(-ENOMEM);

	card->host = host;

	device_initialize(&card->dev);

	card->dev.parent = mmc_classdev(host);
	card->dev.bus = &mmc_bus_type;
	card->dev.release = mmc_release_card;
	card->dev.type = type;
+     host->card = card;
	return card;
}

and this works for me provided the sd driver remembers to check for card being NULL  in the driver specific set_clock code.

My pseudo code is 

if (host->mmc->card == NULL)
     dynamic_clocks=FALSE;
else if host->mmc->card == MMC_TYPE_SDIO)
    	dynamic_clocks = TRUE;
else 
    	dynamic_clocks = FALSE;


My question is:  Is there a better way to pass the type of card to the sd driver?

Re: sdhci: mmc: determining card type and dynamic clocks

[Adrian Hunter]

On 21/10/10 14:12, ext Philip Rakity wrote:
>
> Sometimes it is useful for the SD driver to know the type of card that is in use so that dynamic sd bus clocking can be enabled or disabled.
> Dynamic clocks are a feature of sd 3.0 and we can support this in our v2.0 controller.
>
> The problem is that some SDIO cards are not happy when the clocks are dynamically stopped.  I have not seen any issues with SD/mmc/eMMC cards.  I suspect the reason for the SDIO problems is that the cards need the clock since SDIO cards can interrupt the CPU for service and the other card types cannot.
>
> The information about the type of card being used is known by the mmc layer when
>
> mmc_alloc_card(struct mmc_host *host, struct device_type *type) --- core/bus.c
>
> is called but the sd driver cannot get to this information when set_ios() is called by the mmc layer since the host->card field is filled in too late.
>
> I have added host->card = card to
>
> struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
> {
> 	struct mmc_card *card;
>
> 	card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
> 	if (!card)
> 		return ERR_PTR(-ENOMEM);
>
> 	card->host = host;
>
> 	device_initialize(&card->dev);
>
> 	card->dev.parent = mmc_classdev(host);
> 	card->dev.bus =&mmc_bus_type;
> 	card->dev.release = mmc_release_card;
> 	card->dev.type = type;
> +     host->card = card;
> 	return card;
> }
>
> and this works for me provided the sd driver remembers to check for card being NULL  in the driver specific set_clock code.
>
> My pseudo code is
>
> if (host->mmc->card == NULL)
>       dynamic_clocks=FALSE;
> else if host->mmc->card == MMC_TYPE_SDIO)
>      	dynamic_clocks = TRUE;
> else
>      	dynamic_clocks = FALSE;
>
>
> My question is:  Is there a better way to pass the type of card to the sd driver?
>

Have you considered passing the I/O state (dynamic clocks?) instead
of the card type?

Re: sdhci: mmc: determining card type and dynamic clocks

[Philip Rakity]

On Oct 21, 2010, at 5:08 AM, Adrian Hunter wrote:

> On 21/10/10 14:12, ext Philip Rakity wrote:
>> 
>> Sometimes it is useful for the SD driver to know the type of card that is in use so that dynamic sd bus clocking can be enabled or disabled.
>> Dynamic clocks are a feature of sd 3.0 and we can support this in our v2.0 controller.
>> 
>> The problem is that some SDIO cards are not happy when the clocks are dynamically stopped.  I have not seen any issues with SD/mmc/eMMC cards.  I suspect the reason for the SDIO problems is that the cards need the clock since SDIO cards can interrupt the CPU for service and the other card types cannot.
>> 
>> The information about the type of card being used is known by the mmc layer when
>> 
>> mmc_alloc_card(struct mmc_host *host, struct device_type *type) --- core/bus.c
>> 
>> is called but the sd driver cannot get to this information when set_ios() is called by the mmc layer since the host->card field is filled in too late.
>> 
>> I have added host->card = card to
>> 
>> struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
>> {
>> 	struct mmc_card *card;
>> 
>> 	card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
>> 	if (!card)
>> 		return ERR_PTR(-ENOMEM);
>> 
>> 	card->host = host;
>> 
>> 	device_initialize(&card->dev);
>> 
>> 	card->dev.parent = mmc_classdev(host);
>> 	card->dev.bus =&mmc_bus_type;
>> 	card->dev.release = mmc_release_card;
>> 	card->dev.type = type;
>> +     host->card = card;
>> 	return card;
>> }
>> 
>> and this works for me provided the sd driver remembers to check for card being NULL  in the driver specific set_clock code.
>> 
>> My pseudo code is
>> 
>> if (host->mmc->card == NULL)
>>      dynamic_clocks=FALSE;
>> else if host->mmc->card == MMC_TYPE_SDIO)
>>     	dynamic_clocks = TRUE;
>> else
>>     	dynamic_clocks = FALSE;
>> 
>> 
>> My question is:  Is there a better way to pass the type of card to the sd driver?
>> 
> 
> Have you considered passing the I/O state (dynamic clocks?) instead
> of the card type?
> 


Not sure I understand.  The SD driver knows the clock state but does not know the type of card that is being used.  
The pseudo code goes into the platform specific driver code in mmc/host.   I do not know how to make the clocks state 
dynamic / vs continuous visible in the mmc/core layer.   I am not sure that providing a hook in the mmc/core layer for 
core/ to switch the clock state is desirable.

One consideration is that the clocks at the sd driver layer should start as continuous and then switch to dynamic once
the type of card is known.  The other way around is not safe.

Re: sdhci: mmc: determining card type and dynamic clocks

[Nicolas Pitre]

On Thu, 21 Oct 2010, Philip Rakity wrote:

> 
> Sometimes it is useful for the SD driver to know the type of card that is in use so that dynamic sd bus clocking can be enabled or disabled.
> Dynamic clocks are a feature of sd 3.0 and we can support this in our v2.0 controller.
> 
> The problem is that some SDIO cards are not happy when the clocks are dynamically stopped.  I have not seen any issues with SD/mmc/eMMC cards.  I suspect the reason for the SDIO problems is that the cards need the clock since SDIO cards can interrupt the CPU for service and the other card types cannot.
> 
> The information about the type of card being used is known by the mmc layer when 
> 
> mmc_alloc_card(struct mmc_host *host, struct device_type *type) --- core/bus.c
> 
> is called but the sd driver cannot get to this information when set_ios() is called by the mmc layer since the host->card field is filled in too late.
> 
> I have added host->card = card to
> 
> struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
> {
> 	struct mmc_card *card;
> 
> 	card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
> 	if (!card)
> 		return ERR_PTR(-ENOMEM);
> 
> 	card->host = host;
> 
> 	device_initialize(&card->dev);
> 
> 	card->dev.parent = mmc_classdev(host);
> 	card->dev.bus = &mmc_bus_type;
> 	card->dev.release = mmc_release_card;
> 	card->dev.type = type;
> +     host->card = card;
> 	return card;
> }
> 
> and this works for me provided the sd driver remembers to check for card being NULL  in the driver specific set_clock code.
> 
> My pseudo code is 
> 
> if (host->mmc->card == NULL)
>      dynamic_clocks=FALSE;
> else if host->mmc->card == MMC_TYPE_SDIO)
>     	dynamic_clocks = TRUE;
> else 
>     	dynamic_clocks = FALSE;
> 
> 
> My question is:  Is there a better way to pass the type of card to the sd driver?

I don't like this.  Again we should strive to make the controller driver 
as simple as possible and let the core code handle this kind of clocking 
logic as much as possible.  The OMAP driver is again a terribly bad 
example to follow.

If there are cases where the clock still has to remain active with some 
cards, it is far better to take care of such exceptions in only one 
place instead of having to add a card quirk in every controller drivers.  
The controller driver should remain card agnostic as much as possible 
and not take decisions on its own based on card type.

Stopping the clock would also be a good thing even for those controllers 
without dynamic clock support. The core could certainly stop the clock 
manually after a period of inactivity for example.

So if your controller hardware has the ability to dynamically stop the 
clock on its own, then the driver should simply expose that knowledge to 
the core with a capability bit, and the core should tell the driver to 
enable that mode through mmc_set_ios() when applicable.


Nicolas

Re: sdhci: mmc: determining card type and dynamic clocks

[Philip Rakity]

On Oct 21, 2010, at 9:57 AM, Nicolas Pitre wrote:

> On Thu, 21 Oct 2010, Philip Rakity wrote:
> 
>> 
>> Sometimes it is useful for the SD driver to know the type of card that is in use so that dynamic sd bus clocking can be enabled or disabled.
>> Dynamic clocks are a feature of sd 3.0 and we can support this in our v2.0 controller.
>> 
>> The problem is that some SDIO cards are not happy when the clocks are dynamically stopped.  I have not seen any issues with SD/mmc/eMMC cards.  I suspect the reason for the SDIO problems is that the cards need the clock since SDIO cards can interrupt the CPU for service and the other card types cannot.
>> 
>> The information about the type of card being used is known by the mmc layer when 
>> 
>> mmc_alloc_card(struct mmc_host *host, struct device_type *type) --- core/bus.c
>> 
>> is called but the sd driver cannot get to this information when set_ios() is called by the mmc layer since the host->card field is filled in too late.
>> 
>> I have added host->card = card to
>> 
>> struct mmc_card *mmc_alloc_card(struct mmc_host *host, struct device_type *type)
>> {
>> 	struct mmc_card *card;
>> 
>> 	card = kzalloc(sizeof(struct mmc_card), GFP_KERNEL);
>> 	if (!card)
>> 		return ERR_PTR(-ENOMEM);
>> 
>> 	card->host = host;
>> 
>> 	device_initialize(&card->dev);
>> 
>> 	card->dev.parent = mmc_classdev(host);
>> 	card->dev.bus = &mmc_bus_type;
>> 	card->dev.release = mmc_release_card;
>> 	card->dev.type = type;
>> +     host->card = card;
>> 	return card;
>> }
>> 
>> and this works for me provided the sd driver remembers to check for card being NULL  in the driver specific set_clock code.
>> 
>> My pseudo code is 
>> 
>> if (host->mmc->card == NULL)
>>     dynamic_clocks=FALSE;
>> else if host->mmc->card == MMC_TYPE_SDIO)
>>    	dynamic_clocks = TRUE;
>> else 
>>    	dynamic_clocks = FALSE;
>> 
>> 
>> My question is:  Is there a better way to pass the type of card to the sd driver?
> 
> I don't like this.  Again we should strive to make the controller driver 
> as simple as possible and let the core code handle this kind of clocking 
> logic as much as possible.  The OMAP driver is again a terribly bad 
> example to follow.

Not looking at OMAP

> 
> If there are cases where the clock still has to remain active with some 
> cards, it is far better to take care of such exceptions in only one 
> place instead of having to add a card quirk in every controller drivers.  
> The controller driver should remain card agnostic as much as possible 
> and not take decisions on its own based on card type.

Was not going to add a quirk.  The low level platform specific code for
sdhci.c can handle this if it knows the card type.  It can program the
appropriate hardware registers.

> 
> Stopping the clock would also be a good thing even for those controllers 
> without dynamic clock support. The core could certainly stop the clock 
> manually after a period of inactivity for example.


agree except that stopping the clock for SDIO cards is not a good thing to do in our experience.
if the core can signal this via set_ios this works for me.  This is why I suggested the code telling
set_ios the card type which allowed the platform specific code to enable or not dynamic 
clocks.

> 
> So if your controller hardware has the ability to dynamically stop the 
> clock on its own, then the driver should simply expose that knowledge to 
> the core with a capability bit, and the core should tell the driver to 
> enable that mode through mmc_set_ios() when applicable.

sd 3.0 spec (not marvell specific) suppports dynamic clocking as well as marvell v2.0
controller.

I can add a CAP bit -- but for SDIO cards we will certainly set this to not disable the clocks.

100,000 foot summary
a) add new mmc->caps bit indicating dynamic clocks can be supported in the controller
b) add new field in ios that indicates if clocks should be dynamic
c) pass this to set_ios
d) add platform specific hook to set_ios to control the dynamic clocking option.

Is my understanding correct ?



> 
> 
> Nicolas

Re: sdhci: mmc: determining card type and dynamic clocks

[Nicolas Pitre]

On Thu, 21 Oct 2010, Philip Rakity wrote:

> 
> On Oct 21, 2010, at 9:57 AM, Nicolas Pitre wrote:
> 
> > I don't like this.  Again we should strive to make the controller driver 
> > as simple as possible and let the core code handle this kind of clocking 
> > logic as much as possible.  The OMAP driver is again a terribly bad 
> > example to follow.
> 
> Not looking at OMAP

I know... I'm just dropping a hint in the passing to those who may be 
concerned.

> > 
> > If there are cases where the clock still has to remain active with some 
> > cards, it is far better to take care of such exceptions in only one 
> > place instead of having to add a card quirk in every controller drivers.  
> > The controller driver should remain card agnostic as much as possible 
> > and not take decisions on its own based on card type.
> 
> Was not going to add a quirk.  The low level platform specific code for
> sdhci.c can handle this if it knows the card type.  It can program the
> appropriate hardware registers.

NO!!!  That's EXACTLY what I'm against!

> > Stopping the clock would also be a good thing even for those controllers 
> > without dynamic clock support. The core could certainly stop the clock 
> > manually after a period of inactivity for example.
> 
> 
> agree except that stopping the clock for SDIO cards is not a good thing to do in our experience.
> if the core can signal this via set_ios this works for me.  This is why I suggested the code telling
> set_ios the card type which allowed the platform specific code to enable or not dynamic 
> clocks.

NO!  The core should tell the platform specific code via set_ios() 
whether or not to enable dynamic clock, period.  The _decision_ to do so 
(is it a SDIO card, is it a broken SDHC card which requires the clock 
all the time, etc.) should remain in the core code.  The controller 
drivers have no business duplicating the code for making such decisions 
on their own.

> > So if your controller hardware has the ability to dynamically stop the 
> > clock on its own, then the driver should simply expose that knowledge to 
> > the core with a capability bit, and the core should tell the driver to 
> > enable that mode through mmc_set_ios() when applicable.
> 
> sd 3.0 spec (not marvell specific) suppports dynamic clocking as well as marvell v2.0
> controller.
> 
> I can add a CAP bit -- but for SDIO cards we will certainly set this to not disable the clocks.

Again, the "for SDIO cards" has to be decided in the *core* code, not in 
your driver.

> 100,000 foot summary
> a) add new mmc->caps bit indicating dynamic clocks can be supported in the controller
> b) add new field in ios that indicates if clocks should be dynamic
> c) pass this to set_ios
> d) add platform specific hook to set_ios to control the dynamic clocking option.
> 
> Is my understanding correct ?

Yes!  And then the core code may either set the dynamic clock bit in the 
ios structure if the host controller supports it, or manually turn off 
the clock after a delay of inactivity, but only when this makes sense 
i.e. not with SDIO cards (and even for SDIO that might be possible if no 
IRQ is claimed).


Nicolas