| Message ID | 1355499526-13790-1-git-send-email-pawel.moll@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- 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
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? -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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
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.
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. -- 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
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 -- 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
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 -- 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
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:-) -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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
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(+)