[PATCH] mmc: mmci: Add new VE MMCI variant

[PATCH] mmc: mmci: Add new VE MMCI variant

[Pawel Moll]

The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
and v116/216) contains a modified version of the PL180 MMCI, with
PeriphID Configuration value changed to 0x2.

This version adds an optional "hardware flow control" feature. When
enabled MMC card clock will be automatically disabled when FIFO is
about to over/underflow and re-enabled once the host retrieved some
data. This makes the controller immune to over/underrun errors caused
by big interrupt handling latencies.

This patch adds relevant device variant in the driver.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 drivers/mmc/host/mmci.c |   13 +++++++++++++
 drivers/mmc/host/mmci.h |    2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index edc3e9b..b2b550f 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -86,6 +86,14 @@ static struct variant_data variant_arm_extended_fifo = {
 	.pwrreg_powerup		= MCI_PWR_UP,
 };
 
+static struct variant_data variant_arm_extended_fifo_hwfc = {
+	.fifosize		= 128 * 4,
+	.fifohalfsize		= 64 * 4,
+	.clkreg_enable		= MCI_ARM_HWFCEN,
+	.datalength_bits	= 16,
+	.pwrreg_powerup		= MCI_PWR_UP,
+};
+
 static struct variant_data variant_u300 = {
 	.fifosize		= 16 * 4,
 	.fifohalfsize		= 8 * 4,
@@ -1628,6 +1636,11 @@ static struct amba_id mmci_ids[] = {
 		.data	= &variant_arm_extended_fifo,
 	},
 	{
+		.id	= 0x02041180,
+		.mask	= 0xff0fffff,
+		.data	= &variant_arm_extended_fifo_hwfc,
+	},
+	{
 		.id	= 0x00041181,
 		.mask	= 0x000fffff,
 		.data	= &variant_arm,
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index d437ccf..00d6d0c 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -28,6 +28,8 @@
 #define MCI_ST_UX500_NEG_EDGE	(1 << 13)
 #define MCI_ST_UX500_HWFCEN	(1 << 14)
 #define MCI_ST_UX500_CLK_INV	(1 << 15)
+/* Modified PL180 on Versatile Express platform */
+#define MCI_ARM_HWFCEN		(1 << 12)
 
 #define MMCIARGUMENT		0x008
 #define MMCICOMMAND		0x00c
-- 
1.7.10.4

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Russell King - ARM Linux]

On Fri, Dec 14, 2012 at 03:38:46PM +0000, Pawel Moll wrote:
> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> and v116/216) contains a modified version of the PL180 MMCI, with
> PeriphID Configuration value changed to 0x2.
> 
> This version adds an optional "hardware flow control" feature. When
> enabled MMC card clock will be automatically disabled when FIFO is
> about to over/underflow and re-enabled once the host retrieved some
> data. This makes the controller immune to over/underrun errors caused
> by big interrupt handling latencies.

Wrong.  It doesn't make it "immune", it just makes it less likely to
occur - you just need a heavier workload to provoke it.

"This makes the controller more immune to over/underrun errors caused
by longer interrupt handling latencies" would be a more accurate
statement.

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Pawel Moll]

On Fri, 2012-12-14 at 17:11 +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 14, 2012 at 03:38:46PM +0000, Pawel Moll wrote:
> > The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> > and v116/216) contains a modified version of the PL180 MMCI, with
> > PeriphID Configuration value changed to 0x2.
> > 
> > This version adds an optional "hardware flow control" feature. When
> > enabled MMC card clock will be automatically disabled when FIFO is
> > about to over/underflow and re-enabled once the host retrieved some
> > data. This makes the controller immune to over/underrun errors caused
> > by big interrupt handling latencies.
> 
> Wrong.  It doesn't make it "immune", it just makes it less likely to
> occur - you just need a heavier workload to provoke it.

Why do you think so? The MMC clock is cut off when the FIFO gets full,
so there will be no more data received - no overflow is possible.

Paweł

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Linus Walleij]

On Fri, Dec 14, 2012 at 6:11 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 14, 2012 at 03:38:46PM +0000, Pawel Moll wrote:
>> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
>> and v116/216) contains a modified version of the PL180 MMCI, with
>> PeriphID Configuration value changed to 0x2.
>>
>> This version adds an optional "hardware flow control" feature. When
>> enabled MMC card clock will be automatically disabled when FIFO is
>> about to over/underflow and re-enabled once the host retrieved some
>> data. This makes the controller immune to over/underrun errors caused
>> by big interrupt handling latencies.
>
> Wrong.  It doesn't make it "immune", it just makes it less likely to
> occur - you just need a heavier workload to provoke it.

That'd be what just adding DMA would do (Pawel mentioned this
being cooked for the Vexpress as well I think).

Yours,
Linus Walleij

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Linus Walleij]

On Fri, Dec 14, 2012 at 4:38 PM, Pawel Moll <pawel.moll@arm.com> wrote:

> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> and v116/216) contains a modified version of the PL180 MMCI, with
> PeriphID Configuration value changed to 0x2.
>
> This version adds an optional "hardware flow control" feature. When
> enabled MMC card clock will be automatically disabled when FIFO is
> about to over/underflow and re-enabled once the host retrieved some
> data. This makes the controller immune to over/underrun errors caused
> by big interrupt handling latencies.
>
> This patch adds relevant device variant in the driver.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

Code seems to do the right thing compared to what U300 and
Ux500 does with its similar HW flow control.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

On 14 December 2012 16:38, Pawel Moll <pawel.moll@arm.com> wrote:
> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> and v116/216) contains a modified version of the PL180 MMCI, with
> PeriphID Configuration value changed to 0x2.
>
> This version adds an optional "hardware flow control" feature. When
> enabled MMC card clock will be automatically disabled when FIFO is
> about to over/underflow and re-enabled once the host retrieved some
> data. This makes the controller immune to over/underrun errors caused
> by big interrupt handling latencies.
>
> This patch adds relevant device variant in the driver.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/mmc/host/mmci.c |   13 +++++++++++++
>  drivers/mmc/host/mmci.h |    2 ++
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index edc3e9b..b2b550f 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -86,6 +86,14 @@ static struct variant_data variant_arm_extended_fifo = {
>         .pwrreg_powerup         = MCI_PWR_UP,
>  };
>
> +static struct variant_data variant_arm_extended_fifo_hwfc = {
> +       .fifosize               = 128 * 4,
> +       .fifohalfsize           = 64 * 4,
> +       .clkreg_enable          = MCI_ARM_HWFCEN,

So enabling this in the variant will also effect how the clock is
being set/gated when the clock freq is 0.

Please have a look at "mmc: mmci: Gate the clock when freq is 0", a
patch I sent out as of 12 dec.
That patch is using the power register to gate the clock. Will that
work with this new version of the PL180 as well? If not, that patch
must be reworked.

> +       .datalength_bits        = 16,
> +       .pwrreg_powerup         = MCI_PWR_UP,
> +};
> +
>  static struct variant_data variant_u300 = {
>         .fifosize               = 16 * 4,
>         .fifohalfsize           = 8 * 4,
> @@ -1628,6 +1636,11 @@ static struct amba_id mmci_ids[] = {
>                 .data   = &variant_arm_extended_fifo,
>         },
>         {
> +               .id     = 0x02041180,
> +               .mask   = 0xff0fffff,
> +               .data   = &variant_arm_extended_fifo_hwfc,
> +       },
> +       {
>                 .id     = 0x00041181,
>                 .mask   = 0x000fffff,
>                 .data   = &variant_arm,
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index d437ccf..00d6d0c 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -28,6 +28,8 @@
>  #define MCI_ST_UX500_NEG_EDGE  (1 << 13)
>  #define MCI_ST_UX500_HWFCEN    (1 << 14)
>  #define MCI_ST_UX500_CLK_INV   (1 << 15)
> +/* Modified PL180 on Versatile Express platform */
> +#define MCI_ARM_HWFCEN         (1 << 12)
>
>  #define MMCIARGUMENT           0x008
>  #define MMCICOMMAND            0x00c
> --
> 1.7.10.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Ulf Hansson

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Pawel Moll]

Hello Ulf,

On Fri, 2012-12-21 at 11:05 +0000, Ulf Hansson wrote:
> On 14 December 2012 16:38, Pawel Moll <pawel.moll@arm.com> wrote:
> > The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> > and v116/216) contains a modified version of the PL180 MMCI, with
> > PeriphID Configuration value changed to 0x2.
> >
> > This version adds an optional "hardware flow control" feature. When
> > enabled MMC card clock will be automatically disabled when FIFO is
> > about to over/underflow and re-enabled once the host retrieved some
> > data. This makes the controller immune to over/underrun errors caused
> > by big interrupt handling latencies.
> >
> > This patch adds relevant device variant in the driver.
> >
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>

> So enabling this in the variant will also effect how the clock is
> being set/gated when the clock freq is 0.
> 
> Please have a look at "mmc: mmci: Gate the clock when freq is 0", a
> patch I sent out as of 12 dec.
> That patch is using the power register to gate the clock. Will that
> work with this new version of the PL180 as well? If not, that patch
> must be reworked.

I'm not quite sure what you meant here, but I see that v2 of your patch
does whatever it is supposed to do to ST variants only, so I guess your
objections don't apply any more?

Cheers!

Pawel

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

Hi Pawel,

On 24 January 2013 13:36, Pawel Moll <pawel.moll@arm.com> wrote:
> Hello Ulf,
>
> On Fri, 2012-12-21 at 11:05 +0000, Ulf Hansson wrote:
>> On 14 December 2012 16:38, Pawel Moll <pawel.moll@arm.com> wrote:
>> > The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
>> > and v116/216) contains a modified version of the PL180 MMCI, with
>> > PeriphID Configuration value changed to 0x2.
>> >
>> > This version adds an optional "hardware flow control" feature. When
>> > enabled MMC card clock will be automatically disabled when FIFO is
>> > about to over/underflow and re-enabled once the host retrieved some
>> > data. This makes the controller immune to over/underrun errors caused
>> > by big interrupt handling latencies.
>> >
>> > This patch adds relevant device variant in the driver.
>> >
>> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>
>> So enabling this in the variant will also effect how the clock is
>> being set/gated when the clock freq is 0.
>>
>> Please have a look at "mmc: mmci: Gate the clock when freq is 0", a
>> patch I sent out as of 12 dec.
>> That patch is using the power register to gate the clock. Will that
>> work with this new version of the PL180 as well? If not, that patch
>> must be reworked.
>
> I'm not quite sure what you meant here, but I see that v2 of your patch
> does whatever it is supposed to do to ST variants only, so I guess your
> objections don't apply any more?

Correct. ST-variants requires to use the POWER register to gate the
clock. So if this is not the case for VE you don't need to bother,
otherwise you should set "pwrreg_clkgate" for this variant as well.

Kind regards
Ulf Hansson

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Russell King - ARM Linux]

On Thu, Jan 24, 2013 at 01:48:39PM +0100, Ulf Hansson wrote:
> Correct. ST-variants requires to use the POWER register to gate the
> clock. So if this is not the case for VE you don't need to bother,
> otherwise you should set "pwrreg_clkgate" for this variant as well.

The ARM variant does gate the clock as well.  It's all described in the
TRM.

When power is turned off, the clock and power is removed from the card.
When power is applied, there is a defined sequence that must be gone
through (which is partly handled by the device and partly by the software)
through the OFF->POWER UP->POWER ON sequence.

The clock can be masked independently too via the clock enable bit in
the clock register.

However, it's interesting to note: experiments that Linus did with having
the primecell automatically masking the clock to the card did not prove
worthwhile (see the comment in the code).  We seem to be heading towards
doing this in software and people suggesting it is worthwhile...  Why is
this?

If the hardware can gate the clock and it doesn't produce the power
savings we expect, how does then gating the clock via software then
become worthwhile?  It doesn't make sense.

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

On 24 January 2013 13:59, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 24, 2013 at 01:48:39PM +0100, Ulf Hansson wrote:
>> Correct. ST-variants requires to use the POWER register to gate the
>> clock. So if this is not the case for VE you don't need to bother,
>> otherwise you should set "pwrreg_clkgate" for this variant as well.
>
> The ARM variant does gate the clock as well.  It's all described in the
> TRM.
>
> When power is turned off, the clock and power is removed from the card.
> When power is applied, there is a defined sequence that must be gone
> through (which is partly handled by the device and partly by the software)
> through the OFF->POWER UP->POWER ON sequence.
>
> The clock can be masked independently too via the clock enable bit in
> the clock register.
>
> However, it's interesting to note: experiments that Linus did with having
> the primecell automatically masking the clock to the card did not prove
> worthwhile (see the comment in the code).  We seem to be heading towards
> doing this in software and people suggesting it is worthwhile...  Why is
> this?
>
> If the hardware can gate the clock and it doesn't produce the power
> savings we expect, how does then gating the clock via software then
> become worthwhile?  It doesn't make sense.

For ST-variants it is clearly stated in specs that the power register
needs to be used to gate the clock. However for these variants the
power register is not used for controlling the power to the card, that
is handled by external regulators. In that sense the MMCI_POWER
register does not seems like the proper name at all for this register.
So, likely this is some legacy from the primecell. Just a guess
though. :-)

Kind regards
Ulf Hansson

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Linus Walleij]

On Thu, Jan 24, 2013 at 1:59 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> The clock can be masked independently too via the clock enable bit in
> the clock register.
>
> However, it's interesting to note: experiments that Linus did with having
> the primecell automatically masking the clock to the card did not prove
> worthwhile (see the comment in the code).  We seem to be heading towards
> doing this in software and people suggesting it is worthwhile...  Why is
> this?
>
> If the hardware can gate the clock and it doesn't produce the power
> savings we expect, how does then gating the clock via software then
> become worthwhile?  It doesn't make sense.

We're talking about this I guess:
http://marc.info/?l=linux-arm-kernel&m=123790711306850&w=4

In these experiments I actually gated not only the
enable bit (bit 8) to the MCI clock, but also the pclk to
the PrimeCell itself as well. This was due to the U300
clock framework doing that behind the scenes on
clk_disable(host->clk);

And the reason the clock framework was doing that
was that the U300 had the block clock and the card
clock wired to the same clock tap ...
The equivalent patch today would also call
amba_pclk_disable() when the card was inactive.

Thus we also got rid of all the switching leaks in the PL180
logic. (Containing two big state machines for example.)

It was an interesting comparison to just enable PwrSave
(bit 9) instead, and disable the other clock logic.

It turned out that this saving was significantly bigger when
gating the entire PrimeCell than the savings for just gating
the clock out to the card itself with bit 9.

But at this time I didn't have a card in the slot, so as to
eliminate the power consumed in the card (assuming
this would drown the overall current consumption).

So I have second thoughts about this. The experiment
was not testing the PwrSave bit with a card inserted, this
is the wrong thing to do. To determine if that bit is a good
thing to turn on, it needs to be tested with a card in the
slot.

The only really useful information is that the switching
logic inside the PL180 synthesized IP-core consumes
circa 200uA, so it could be useful to gate it at times.

Anyone wants to re-do the measurements?

Yours,
Linus Walleij

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

On 24 January 2013 18:12, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jan 24, 2013 at 1:59 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>
>> The clock can be masked independently too via the clock enable bit in
>> the clock register.
>>
>> However, it's interesting to note: experiments that Linus did with having
>> the primecell automatically masking the clock to the card did not prove
>> worthwhile (see the comment in the code).  We seem to be heading towards
>> doing this in software and people suggesting it is worthwhile...  Why is
>> this?
>>
>> If the hardware can gate the clock and it doesn't produce the power
>> savings we expect, how does then gating the clock via software then
>> become worthwhile?  It doesn't make sense.
>
> We're talking about this I guess:
> http://marc.info/?l=linux-arm-kernel&m=123790711306850&w=4
>
> In these experiments I actually gated not only the
> enable bit (bit 8) to the MCI clock, but also the pclk to
> the PrimeCell itself as well. This was due to the U300
> clock framework doing that behind the scenes on
> clk_disable(host->clk);
>
> And the reason the clock framework was doing that
> was that the U300 had the block clock and the card
> clock wired to the same clock tap ...
> The equivalent patch today would also call
> amba_pclk_disable() when the card was inactive.
>
> Thus we also got rid of all the switching leaks in the PL180
> logic. (Containing two big state machines for example.)
>
> It was an interesting comparison to just enable PwrSave
> (bit 9) instead, and disable the other clock logic.
>
> It turned out that this saving was significantly bigger when
> gating the entire PrimeCell than the savings for just gating
> the clock out to the card itself with bit 9.
>
> But at this time I didn't have a card in the slot, so as to
> eliminate the power consumed in the card (assuming
> this would drown the overall current consumption).
>
> So I have second thoughts about this. The experiment
> was not testing the PwrSave bit with a card inserted, this
> is the wrong thing to do. To determine if that bit is a good
> thing to turn on, it needs to be tested with a card in the
> slot.
>
> The only really useful information is that the switching
> logic inside the PL180 synthesized IP-core consumes
> circa 200uA, so it could be useful to gate it at times.

So in the runtime_pm callbacks, it you don't just want to do normal
clk gating/ungating with the clock API, which is done right now. You
also would like to actually gate the clock internally by using
clock/power register. Does that make sense?


>
> Anyone wants to re-do the measurements?
>
> Yours,
> Linus Walleij

Kind regards
Ulf Hansson

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Linus Walleij]

On Thu, Jan 24, 2013 at 9:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 24 January 2013 18:12, Linus Walleij <linus.walleij@linaro.org> wrote:

>> We're talking about this I guess:
>> http://marc.info/?l=linux-arm-kernel&m=123790711306850&w=4
>>
>> In these experiments I actually gated not only the
>> enable bit (bit 8) to the MCI clock, but also the pclk to
>> the PrimeCell itself as well. This was due to the U300
>> clock framework doing that behind the scenes on
>> clk_disable(host->clk);
>>
>> And the reason the clock framework was doing that
>> was that the U300 had the block clock and the card
>> clock wired to the same clock tap ...
>> The equivalent patch today would also call
>> amba_pclk_disable() when the card was inactive.
>>
>> Thus we also got rid of all the switching leaks in the PL180
>> logic. (Containing two big state machines for example.)
>>
>> It was an interesting comparison to just enable PwrSave
>> (bit 9) instead, and disable the other clock logic.
>>
>> It turned out that this saving was significantly bigger when
>> gating the entire PrimeCell than the savings for just gating
>> the clock out to the card itself with bit 9.
>>
>> But at this time I didn't have a card in the slot, so as to
>> eliminate the power consumed in the card (assuming
>> this would drown the overall current consumption).
>>
>> So I have second thoughts about this. The experiment
>> was not testing the PwrSave bit with a card inserted, this
>> is the wrong thing to do. To determine if that bit is a good
>> thing to turn on, it needs to be tested with a card in the
>> slot.
>>
>> The only really useful information is that the switching
>> logic inside the PL180 synthesized IP-core consumes
>> circa 200uA, so it could be useful to gate it at times.
>
> So in the runtime_pm callbacks, it you don't just want to do normal
> clk gating/ungating with the clock API, which is done right now. You
> also would like to actually gate the clock internally by using
> clock/power register. Does that make sense?

Well in the currently merged patches it will only cut of
MCLK through the clock API right?

Maybe it makes sense to also call amba_pclk_disable()
in these hooks so as to propagate up and gate off the
silicon pclk?

In the ST case, with SDIO support and all, this can not be
done on SDIO devices though, as that thing needs to
react to IRQs, and thus the logic must always be clocked.
But for blocks connected to cards/eMMCs where the device
is essentially passive, it seems like the right thing to do.

(Actually in a finer technology the power savings could of
course be greater than 200uA for doing this.)

Yours,
Linus Walleij

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

On 25 January 2013 10:35, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jan 24, 2013 at 9:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 24 January 2013 18:12, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> We're talking about this I guess:
>>> http://marc.info/?l=linux-arm-kernel&m=123790711306850&w=4
>>>
>>> In these experiments I actually gated not only the
>>> enable bit (bit 8) to the MCI clock, but also the pclk to
>>> the PrimeCell itself as well. This was due to the U300
>>> clock framework doing that behind the scenes on
>>> clk_disable(host->clk);
>>>
>>> And the reason the clock framework was doing that
>>> was that the U300 had the block clock and the card
>>> clock wired to the same clock tap ...
>>> The equivalent patch today would also call
>>> amba_pclk_disable() when the card was inactive.
>>>
>>> Thus we also got rid of all the switching leaks in the PL180
>>> logic. (Containing two big state machines for example.)
>>>
>>> It was an interesting comparison to just enable PwrSave
>>> (bit 9) instead, and disable the other clock logic.
>>>
>>> It turned out that this saving was significantly bigger when
>>> gating the entire PrimeCell than the savings for just gating
>>> the clock out to the card itself with bit 9.
>>>
>>> But at this time I didn't have a card in the slot, so as to
>>> eliminate the power consumed in the card (assuming
>>> this would drown the overall current consumption).
>>>
>>> So I have second thoughts about this. The experiment
>>> was not testing the PwrSave bit with a card inserted, this
>>> is the wrong thing to do. To determine if that bit is a good
>>> thing to turn on, it needs to be tested with a card in the
>>> slot.
>>>
>>> The only really useful information is that the switching
>>> logic inside the PL180 synthesized IP-core consumes
>>> circa 200uA, so it could be useful to gate it at times.
>>
>> So in the runtime_pm callbacks, it you don't just want to do normal
>> clk gating/ungating with the clock API, which is done right now. You
>> also would like to actually gate the clock internally by using
>> clock/power register. Does that make sense?
>
> Well in the currently merged patches it will only cut of
> MCLK through the clock API right?
>
> Maybe it makes sense to also call amba_pclk_disable()
> in these hooks so as to propagate up and gate off the
> silicon pclk?

That is already taken care off in the amba bus. So no need to worry :-)

>
> In the ST case, with SDIO support and all, this can not be
> done on SDIO devices though, as that thing needs to
> react to IRQs, and thus the logic must always be clocked.
> But for blocks connected to cards/eMMCs where the device
> is essentially passive, it seems like the right thing to do.
>

You are right, but until/if we implement SDIO_IRQ support for mmci, we
don't have to bother.

Morover, if the SDIO IRQ is rerouted to a GPIO IRQ when going to
runtime_suspend it will do the trick.

> (Actually in a finer technology the power savings could of
> course be greater than 200uA for doing this.)
>

Yes, new measurements would be great to do.

> Yours,
> Linus Walleij

Kind regards
Ulf Hansson

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Pawel Moll]

Hi Chris,

On Fri, 2012-12-14 at 15:38 +0000, Pawel Moll wrote:
> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> and v116/216) contains a modified version of the PL180 MMCI, with
> PeriphID Configuration value changed to 0x2.
> 
> This version adds an optional "hardware flow control" feature. When
> enabled MMC card clock will be automatically disabled when FIFO is
> about to over/underflow and re-enabled once the host retrieved some
> data. This makes the controller immune to over/underrun errors caused
> by big interrupt handling latencies.
> 
> This patch adds relevant device variant in the driver.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

This is just a polite, post-holiday, reminder about this patch... It has
been "Reviewed-by: Linus Walleij <linus.walleij@linaro.org>".

Cheers!

Pawel

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

On 24 January 2013 13:54, Pawel Moll <pawel.moll@arm.com> wrote:
> Hi Chris,
>
> On Fri, 2012-12-14 at 15:38 +0000, Pawel Moll wrote:
>> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
>> and v116/216) contains a modified version of the PL180 MMCI, with
>> PeriphID Configuration value changed to 0x2.
>>
>> This version adds an optional "hardware flow control" feature. When
>> enabled MMC card clock will be automatically disabled when FIFO is
>> about to over/underflow and re-enabled once the host retrieved some
>> data. This makes the controller immune to over/underrun errors caused
>> by big interrupt handling latencies.
>>
>> This patch adds relevant device variant in the driver.
>>
>> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>
> This is just a polite, post-holiday, reminder about this patch... It has
> been "Reviewed-by: Linus Walleij <linus.walleij@linaro.org>".
>
> Cheers!
>
> Pawel
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Of course you may have my Ack for this as well.

Normally we use Russell's tree for mmci patches. So I suppose you
should upload this patch into his patchtracker instead!?

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Ulf Hansson

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Ulf Hansson]

On 24 January 2013 14:04, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 24 January 2013 13:54, Pawel Moll <pawel.moll@arm.com> wrote:
>> Hi Chris,
>>
>> On Fri, 2012-12-14 at 15:38 +0000, Pawel Moll wrote:
>>> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
>>> and v116/216) contains a modified version of the PL180 MMCI, with
>>> PeriphID Configuration value changed to 0x2.
>>>
>>> This version adds an optional "hardware flow control" feature. When
>>> enabled MMC card clock will be automatically disabled when FIFO is
>>> about to over/underflow and re-enabled once the host retrieved some
>>> data. This makes the controller immune to over/underrun errors caused
>>> by big interrupt handling latencies.
>>>
>>> This patch adds relevant device variant in the driver.
>>>
>>> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>>
>> This is just a polite, post-holiday, reminder about this patch... It has
>> been "Reviewed-by: Linus Walleij <linus.walleij@linaro.org>".
>>
>> Cheers!
>>
>> Pawel
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Of course you may have my Ack for this as well.
>
> Normally we use Russell's tree for mmci patches. So I suppose you
> should upload this patch into his patchtracker instead!?
>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Kind regards
> Ulf Hansson

I was a bit late, please ignore my reply, expect for the ack of course:-)

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Chris Ball]

Hi Pawel,

On Fri, Dec 14 2012, Pawel Moll wrote:
> The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> and v116/216) contains a modified version of the PL180 MMCI, with
> PeriphID Configuration value changed to 0x2.
>
> This version adds an optional "hardware flow control" feature. When
> enabled MMC card clock will be automatically disabled when FIFO is
> about to over/underflow and re-enabled once the host retrieved some
> data. This makes the controller immune to over/underrun errors caused
> by big interrupt handling latencies.
>
> This patch adds relevant device variant in the driver.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  drivers/mmc/host/mmci.c |   13 +++++++++++++
>  drivers/mmc/host/mmci.h |    2 ++
>  2 files changed, 15 insertions(+)

Thanks for the reminder -- Russell's been reviewing and merging MMCI
patches lately, please can you add it to his patch tracker?

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: mmci: Add new VE MMCI variant

[Pawel Moll]

On Thu, 2013-01-24 at 12:57 +0000, Chris Ball wrote:
> Hi Pawel,
> 
> On Fri, Dec 14 2012, Pawel Moll wrote:
> > The Versatile Express IOFPGA as shipped on VECD 5.0 (bitfiles v108/208
> > and v116/216) contains a modified version of the PL180 MMCI, with
> > PeriphID Configuration value changed to 0x2.
> >
> > This version adds an optional "hardware flow control" feature. When
> > enabled MMC card clock will be automatically disabled when FIFO is
> > about to over/underflow and re-enabled once the host retrieved some
> > data. This makes the controller immune to over/underrun errors caused
> > by big interrupt handling latencies.
> >
> > This patch adds relevant device variant in the driver.
> >
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> >  drivers/mmc/host/mmci.c |   13 +++++++++++++
> >  drivers/mmc/host/mmci.h |    2 ++
> >  2 files changed, 15 insertions(+)
> 
> Thanks for the reminder -- Russell's been reviewing and merging MMCI
> patches lately, please can you add it to his patch tracker?

Sure, thanks!

Pawel