Re: [PATCH v2 2/2] mmc: sdhci-bcm2835: Actually enable the clock
On 05/29/2015 03:06 PM, Eric Anholt wrote:
> We're currently using a fixed frequency clock specified in the DT, so
> enabling is a no-op. However, the RPi firmware-based clocks driver
> can actually disable unused clocks, so when switching to use it we
> ended up losing our MMC clock once all devices were probed.
> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
> b/drivers/mmc/host/sdhci-bcm2835.c
> + ret = clk_prepare_enable(pltfm_host->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable host clk\n");
> + goto err;
> + }
Given that pltfm_host is a "struct sdhci_pltfm_host" i.e. a type
defined/handled by sdhci-pltfm.c , I'm rather surprised that
sdhci-pltfm.c doesn't do this itself. Wouldn't it make sense for it to
do so?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/11] mmc: pwrseq_simple: Add support for a delay
On 01/15/2015 09:12 AM, Tomeu Vizoso wrote: Signed-off-by: Tomeu Vizoso Ah, having read the explanation in the next patch, I think ... diff --git a/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt b/Documentation/devicetree/bindings/mmc/mmc,pwrseq-simple.txt +- delay : delay to wait after driving the reset gpio active [ms]. ... delay is the wrong name. reset-pulse-width or reset-pulse-length would be better. delay sounds like a delay after resetting the device before it can be accessed. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/11] mmc: pwrseq_simple: Add support for a delay
On 01/15/2015 09:12 AM, Tomeu Vizoso wrote: Signed-off-by: Tomeu Vizoso Some explanation of why such a delay might be useful would be ... useful! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible regression with commit 52221610d
On 12/14/2014 09:48 PM, Tim Kryger wrote: On Sat, Dec 13, 2014 at 11:22 PM, Bjorn Andersson wrote: ... Or simply; what is vmmc (in the code) supposed to represent? Hi Bjorn, VMMC is the supply that delivers power out to the SD card itself (aka VDD). It is not the internal power rail/power domain of the host controller within the SoC. I've seen this question come up quite a few times. Should Documentation/devicetree/bindings/mmc/mmc.txt document the vmmc/vqmmc regulators? I assume they're considered shared across all MMC/SDHCI controller bindings? Since that only covers DT, it might be nice to document vmmc-vs-vqmmc somewhere else too, such as right by the devm_regulator_get_optional() calls in mmc_regulator_get_supply() in drivers/mmc/core/core.c. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround
On 11/05/2014 12:00 AM, Scott Branden wrote:
> On 14-11-04 08:59 PM, Stephen Warren wrote:
>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>> Add a verify option to driver to print out an error message if a
>>> potential back to back write could cause a clock domain issue.
>>
>>> index f8c450a..11af27f 100644
>>
>>> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
>>> +struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> +struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>> +
>>> +if (bcm2835_host->previous_reg == reg) {
>>> +if ((reg != SDHCI_HOST_CONTROL)
>>> +&& (reg != SDHCI_CLOCK_CONTROL)) {
>>
>> The comment in patch 3 says the problem doesn't apply to the data
>> register. Why does this check for these two registers rather than data?
> This Verify workaround patch still a work in progress. I'm still
> getting more info from the silicon designers on the back-to-back
> register writes that are affect. The spew of 0x20 or 0x28 or 0x2c
> register writes are all ok locations that don't need to be worked
> around. This patch needs to be corrected with the proper register rules
> still.
FYI, I applied the series except for this patch, and everything
/appeared/ to work OK for a brief test (boot, log in, reboot). Still,
I'll hold off my Tested-by/acked-by until the comment in patch 3 and the
register list above match, and there's no log spew with everything applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12
On 11/05/2014 12:02 AM, Scott Branden wrote: > On 14-11-04 09:00 PM, Stephen Warren wrote: >> On 10/30/2014 12:36 AM, Scott Branden wrote: >>> SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this >>> controller. >> >> This seems fine, although any explanation of why this quirk is needed >> would be useful. >> > I don't know who to talk to at Arasan about this. Will try hunting > around a little for more info as to why this is needed to have eMMC and > SD work properly through our internal testing on other non-2835 chipset > that shares the same SDHCI controller as 2835. I thought I heard that this wasn't a bug in the controller itself, but rather an integration issue between the IP core and the register bus it's attached to. Consequently, it may be SoC-specific or at least have SoC-specific variations? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround
On 11/04/2014 11:55 PM, Scott Branden wrote:
> On 14-11-04 08:57 PM, Stephen Warren wrote:
>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>> The bcm2835 has clock domain issues when back to back writes to certain
>>> registers are written. The existing driver works around this issue with
>>> udelay. A more efficient method is to store the 8 and 16 bit writes
>>> to the registers affected and then write them as 32 bits at the
>>> appropriate
>>> time.
>>
>>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
>>> b/drivers/mmc/host/sdhci-bcm2835.c
>>
>>> static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val,
>>> int reg)
>>> {
>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>> -struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
>>> -u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
>>> -bcm2835_sdhci_readl(host, reg & ~3);
>>> +struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>
>> Is that type change for bcm2835_host really correct?
>
> Yes - at the top of the patch the structure has been expanded and named
> appropriately.
>
> -struct bcm2835_sdhci {
> -u32 shadow;
> +struct bcm2835_sdhci_host {
> +u32 shadow_cmd;
> +u32 shadow_blk;
> };
Ah yes, sorry for missing that.
>>> +} else {
>>> +/* Read reg, all other registers are not shadowed */
>>> +oldval = readl(host->ioaddr + (reg & ~3));
>>
>> Is there any reason to use readl() directly here rather than calling
>> bcm2835_readl()? ...
>
> Yes, bcm2835_readl does not need to be called in read-modify-write and
> shadow register situations and just adds overhead. All that needs to be
> called is readl. bcm2835_readl has some existing ugly code in it to
> modify the capabilities register on a read function. This info never
> needs to be for write as you can't overwrite the capabilities register.
To be honest, it seems better to do all the read/write through
consistent functions. One advantage of bcm2835_readl() is that it
consistently adds on the base address internally so you don't have to
write it out every time manually. Still, the code ought to work fine
after this change, so I guess it's OK.
> I hope to get rid of the capabilities hack in a future patch as this
> should never have been acceptable in upstreamed code to begin with. The
> capabilities override should have been passed in through a device tree
> entry.
It's a pretty common technique with precedent. I certainly don't agree
that it should be configured by DT. Arguably, DT makes sense to describe
board-to-board variations, but there's almost zero point putting data
into DT that is SoC description rather than board description; just put
it into the driver to avoid continually parsing the same data over and
over from DT just to get back to the same data that could have been
encoded into the driver. If the data varies between similar controllers,
an of_match table can easily be used to parameterize it based on
compatible value.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 5/5] mmc: sdhci-bcm2835: add sdhci quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12
On 10/30/2014 12:36 AM, Scott Branden wrote: > SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 is missing and needed for this controller. This seems fine, although any explanation of why this quirk is needed would be useful. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround
On 10/30/2014 12:36 AM, Scott Branden wrote:
> Add a verify option to driver to print out an error message if a
> potential back to back write could cause a clock domain issue.
> index f8c450a..11af27f 100644
> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> +
> + if (bcm2835_host->previous_reg == reg) {
> + if ((reg != SDHCI_HOST_CONTROL)
> + && (reg != SDHCI_CLOCK_CONTROL)) {
The comment in patch 3 says the problem doesn't apply to the data
register. Why does this check for these two registers rather than data?
> + dev_err(mmc_dev(host->mmc),
> + "back-to-back write to 0x%x\n", reg);
The continuation line should be indented at least one more level here.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround
On 10/30/2014 12:36 AM, Scott Branden wrote:
> The bcm2835 has clock domain issues when back to back writes to certain
> registers are written. The existing driver works around this issue with
> udelay. A more efficient method is to store the 8 and 16 bit writes
> to the registers affected and then write them as 32 bits at the appropriate
> time.
> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
> b/drivers/mmc/host/sdhci-bcm2835.c
> static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> - struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
> - u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
> - bcm2835_sdhci_readl(host, reg & ~3);
> + struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
Is that type change for bcm2835_host really correct?
> + } else {
> + /* Read reg, all other registers are not shadowed */
> + oldval = readl(host->ioaddr + (reg & ~3));
Is there any reason to use readl() directly here rather than calling
bcm2835_readl()? ...
> static void bcm2835_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
> {
> - u32 oldval = bcm2835_sdhci_readl(host, reg & ~3);
> + u32 oldval = readl(host->ioaddr + (reg & ~3));
... and here in particular, since this seems like an unrelated change?
> static int bcm2835_sdhci_probe(struct platform_device *pdev)
> {
> struct sdhci_host *host;
> - struct bcm2835_sdhci *bcm2835_host;
> + struct bcm2835_sdhci_host *bcm2835_host;
Is that type change for bcm2835_host really correct?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 2/5] mmc: sdhci-bcm2835: make shift calculations consistent
On 10/30/2014 12:36 AM, Scott Branden wrote: > Make the shift calculations consistent rather than having different > implementations to calculate the same thing. > diff --git a/drivers/mmc/host/sdhci-bcm2835.c > b/drivers/mmc/host/sdhci-bcm2835.c > +#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18) This should really be the following so people don't have to memorize operator precedence: #define REG_OFFSET_IN_BITS(reg) (((reg) << 3) & 0x18) (I've been bit by people mis-remembering precedence in very similar code...) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] mmc: shdci-bcm2835: add verify for 32-bit back-to-back workaround
On 10/30/2014 12:36 AM, Scott Branden wrote:
> Add a verify option to driver to print out an error message if a
> potential back to back write could cause a clock domain issue.
> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
> b/drivers/mmc/host/sdhci-bcm2835.c
> static inline void bcm2835_sdhci_writel(struct sdhci_host *host,
> u32 val, int reg)
> {
> +#ifdef CONFIG_MMC_SDHCI_BCM2835_VERIFY_WORKAROUND
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> +
> + if (bcm2835_host->previous_reg == reg) {
> + if ((reg != SDHCI_HOST_CONTROL)
> + && (reg != SDHCI_CLOCK_CONTROL)) {
> + dev_err(mmc_dev(host->mmc),
> + "back-to-back write to 0x%x\n", reg);
This fires a *ton* on reg 0x20 and 0x30 on my rev 2 model B with the
patches applied on top of next-20141031. Without the patches applied,
everything works fine. As far as I can tell, SD card accesses no longer
work (or perhaps there's just so much log spew over serial that it takes
more than 1.5 minutes to get to the login prompt).
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops
On 10/15/2014 10:43 AM, Scott Branden wrote:
> Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller.
> Removed udelay in write ops by using shadow registers for 16 bit
> accesses to 32-bit registers (where necessary).
> Optimized 32-bit operations when doing 8/16 register accesses.
That's 2 or 3 unrelated changes. They'd be better as separate patches,
so that any issues that arise can be bisected down to the smaller changes.
> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
> b/drivers/mmc/host/sdhci-bcm2835.c
> /*
> * The Arasan has a bugette whereby it may lose the content of successive
> + * writes to the same register that are within two SD-card clock cycles of
> + * each other (a clock domain crossing problem). Problem does not happen
> with
^ The?
See right > ^
> + * data.
Blank line to separate the paragraphs here, to be consistent with the
other paragraph break below?
> + * This wouldn't be a problem with the code except that we can only write the
> + * controller with 32-bit writes. So two different 16-bit registers in the
> + * written back to back creates the problem.
> *
> + * In reality, this only happens when a SDHCI_BLOCK_SIZE and
> SDHCI_BLOCK_COUNT
> + * are written followed by SDHCI_TRANSFER_MODE and SDHCI_COMMAND.
That seems like a rather risky assertion. Even if it's perfectly true
with the MMC core code right now, does the MMC core document a guarantee
that this will always be true? Even if we optimize the WAR for the issue
as you've done, I think we should still have code that validates that
the same register is never written back-to-back to detect this likely
very hard-to-debug problem.
> + * The BLOCK_SIZE and BLOCK_COUNT are meaningless until a command issued so
> + * the work around can be further optimized. We can keep shadow values of
> + * BLOCK_SIZE, BLOCK_COUNT, and TRANSFER_MODE until a COMMAND is issued.
> + * Then, write the BLOCK_SIZE+BLOCK_COUNT in a single 32-bit write followed
> + * by the TRANSFER+COMMAND in another 32-bit write.
> */
After this patch, the entire WAR for this issue is contained within
bcm2835_sdhci_writew(). It might be a good idea to move the comment next
to that function so it's more at hand to explain the code that's there.
Or at least add a comment to that function the to mention the location
of the explanation for the complex code.
> static inline u32 bcm2835_sdhci_readl(struct sdhci_host *host, int reg)
> {
> u32 val = readl(host->ioaddr + reg);
> @@ -71,76 +57,83 @@ static inline u32 bcm2835_sdhci_readl(struct sdhci_host
> *host, int reg)
> return val;
> }
>
> -static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
> -{
... (entire function deleted)
> -}
This patch could be a lot smaller if it didn't re-order the functions at
the same time. It makes the patch harder to understand. If you must
re-order the functions, perhaps make that a separate patch that does
nothing else, so that the actual code changes are easier to see?
> static u16 bcm2835_sdhci_readw(struct sdhci_host *host, int reg)
> {
> - u32 val = bcm2835_sdhci_readl(host, (reg & ~3));
> - u32 word_num = (reg >> 1) & 1;
> - u32 word_shift = word_num * 16;
> - u32 word = (val >> word_shift) & 0x;
> -
> + u32 val = bcm2835_sdhci_readl(host->ioaddr, (reg & ~3));
The change from host to host->ioaddr ends up passing the wrong value to
bcm2835_sdhci_readl(). This causes the kernel to crash during boot.
The compiler doesn't warn about this because host->ioaddr is void, so
can be automatically converted to struct sdhci_host *.
> + u16 word = val >> (reg << 3 & 0x18) & 0x;
> return word;
> }
To be honest, I think the existing code is a bit clearer, since it uses
variables with names to explain all the intermediate values. Assuming
the compiler is competent (which admittedly I haven't checked) I would
expect the same code to be generated either way, or at least something
pretty similar. Did you measure the benefit of the optimization?
> +static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val, int reg)
> {
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
> + u32 word_shift = reg << 3 & 0x18;
> + u32 mask = 0x << word_shift;
> + u32 oldval;
> + u32 newval;
> +
> + if (reg == SDHCI_COMMAND) {
> + if (bcm2835_host->shadow_blk != 0) {
> + writel(bcm2835_host->shadow_blk,
> +host->ioaddr + SDHCI_BLOCK_SIZE);
> + bcm2835_host->shadow_blk = 0;
> + }
Is it absolutely guaranteed that there's never a need to write 0 to that
register? I can see that no data transfer command is likely to transfer
0 blocks. I assume no other type of command uses that register as a
parameter?
--
To unsubscr
Re: [PATCH 1/1] mmc: sdhci-bcm2835: added quirk and removed udelay in write ops
On 10/14/2014 08:01 PM, Scott Branden wrote: > Added quirk SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12 present in controller. > Removed udelay in write ops by using shadow registers for 16 bit > accesses to 32-bit registers (where necessary). > Optimized 32-bit operations when doing 8/16 register accesses. I'm going to assume this is identical to the patch you sent 8/15? So, I'll ignore this copy... -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: don't request CD IRQ until mmc_start_host()
From: Stephen Warren
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host->detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host->mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() ->
mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists if
mmc_gpio_request_cd() is called directly before mmc_start_host().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host->detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it. For most SDHCI hosts at least, the typical call path that
does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
mmc_start_host(). Therefore, remove the call to mmc_gpiod_request_cd_irq()
from mmc_gpio_request_cd(). This also matches mmc_gpio*d*_request_cd(),
which already doesn't call mmc_gpiod_request_cd_irq().
However, some host controller drivers call mmc_gpio_request_cd() after
mmc_start_host() has already been called, and assume that this will also
call mmc_gpiod_request_cd_irq(). Update those drivers to explicitly call
mmc_gpiod_request_cd_irq() themselves. Ideally, these drivers should be
modified to move their call to mmc_gpio_request_cd() before their call
to mmc_add_host(). However that's too large a change for stable.
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
-> mmc_stop_host(), which does free the IRQ and cancel the work queue.
CC: Russell King
Cc: Adrian Hunter
Cc: Alexandre Courbot
Cc: Linus Walleij
Signed-off-by: Stephen Warren
---
Note: This is a patch for v3.17 (and perhaps earlier); linux-next doesn't
have this problem due to other code re-structing. However, I think those
patches are too large to backport.
v3: Explicitly call mmc_gpiod_request_cd_irq() from drivers that call
mmc_gpio_request_cd() after calling mmc_add_host(), rather than modifying
the function signature of mmc_gpio_request_cd().
v2: Rather than completely removing mmc_gpio_request_cd()'s call to
mmc_gpiod_request_cd_irq(), make the call conditional.
---
drivers/mmc/core/slot-gpio.c| 2 --
drivers/mmc/host/mmc_spi.c | 1 +
drivers/mmc/host/sdhci-sirf.c | 1 +
drivers/mmc/host/tmio_mmc_pio.c | 1 +
4 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 5f89cb83d5f0..187f48a5795a 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -221,8 +221,6 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int
gpio,
ctx->override_cd_active_level = true;
ctx->cd_gpio = gpio_to_desc(gpio);
- mmc_gpiod_request_cd_irq(host);
-
return 0;
}
EXPORT_SYMBOL(mmc_gpio_request_cd);
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index cc8d4a6099cd..e4a07546f8b6 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1436,6 +1436,7 @@ static int mmc_spi_probe(struct spi_device *spi)
host->pdata->cd_debounce);
if (status != 0)
goto fail_add_host;
+ mmc_gpiod_request_cd_irq(mmc);
}
if (host->pdata && host->pdata->flags & MMC_SPI_USE_RO_GPIO) {
diff --git a/dri
[PATCH V2] mmc: don't request CD IRQ until mmc_start_host()
From: Stephen Warren
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host->detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host->mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() ->
mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists if
mmc_gpio_request_cd() is called directly before mmc_start_host().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host->detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it. For most SDHCI hosts at least, the typical call path that
does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
mmc_start_host().
Ideally, we would solve this by removing the call to
mmc_gpiod_request_cd_irq() from mmc_gpio_request_cd(). This would match
mmc_gpio*d*_request_cd(), which already doesn't call
mmc_gpiod_request_cd_irq(). However, we need to keep the call and make it
conditional; some host controller drivers call mmc_gpio_request_cd()
after mmc_start_host() has already been called, and assume that this will
also call mmc_gpiod_request_cd_irq(). Unfortunately, the only way I could
find to differentiate the two cases was to add an extra parameter to
mmc_gpio_request_cd() to provide this information.
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
-> mmc_stop_host(), which does free the IRQ and cancel the work queue.
CC: Russell King
Cc: Adrian Hunter
Cc: Alexandre Courbot
Cc: Linus Walleij
Signed-off-by: Stephen Warren
---
Note: This is a patch for v3.17 (and perhaps earlier); linux-next doesn't
have this problem due to other code re-structing. However, I think those
patches are too large to backport.
v2: Rather than completely removing mmc_gpio_request_cd()'s call to
mmc_gpiod_request_cd_irq(), make the call conditional.
---
drivers/mmc/core/host.c| 2 +-
drivers/mmc/core/slot-gpio.c | 13 +++--
drivers/mmc/host/jz4740_mmc.c | 3 ++-
drivers/mmc/host/mmc_spi.c | 2 +-
drivers/mmc/host/mmci.c| 2 +-
drivers/mmc/host/mvsdio.c | 2 +-
drivers/mmc/host/sdhci-esdhc-imx.c | 3 ++-
drivers/mmc/host/sdhci-pxav3.c | 2 +-
drivers/mmc/host/sdhci-sirf.c | 2 +-
drivers/mmc/host/sdhci-spear.c | 3 ++-
drivers/mmc/host/sh_mmcif.c| 2 +-
drivers/mmc/host/tmio_mmc_pio.c| 2 +-
include/linux/mmc/slot-gpio.h | 2 +-
13 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 95cceae96944..217d6c99c11d 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -374,7 +374,7 @@ int mmc_of_parse(struct mmc_host *host)
if (!(flags & OF_GPIO_ACTIVE_LOW))
gpio_inv_cd = true;
- ret = mmc_gpio_request_cd(host, gpio, 0);
+ ret = mmc_gpio_request_cd(host, gpio, 0, false);
if (ret < 0) {
dev_err(host->parent,
"Failed to request CD GPIO #%d: %d!\n",
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 5f89cb83d5f0..b63af8392dc1 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/d
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 09/18/2014 12:49 AM, Adrian Hunter wrote:
On 09/18/2014 08:25 AM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren wrote:
From: Stephen Warren
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host->detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host->mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() ->
mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host->detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
-> mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
("mmc: slot-gpio: Add GPIO descriptor based CD GPIO API"), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
That comment is wrong. mmc_gpio_request_cd() has always set up the irq.
Uggh, yes. I did misinterpret your patch again, so that one paragraph is
just wrong.
Aside from that though, I do think my patch is a step in the correct
direction. It just needs some thought how to avoid the other issue you
mentioned - that some drivers rely on calling mmc_gpio_request_cd()
after the call to mmc_start().
Perhaps the logic should not be to remove mmc_gpio_request_cd()'s call
to mmc_gpiod_request_cd_irq(), but rather to make it conditional upon
mmc_start_host() having already been called; I assume that state that
can easily be checked to determine that.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 09/17/2014 11:25 PM, Adrian Hunter wrote:
On 09/17/2014 10:57 PM, Stephen Warren wrote:
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren wrote:
From: Stephen Warren
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host->detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host->mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() ->
mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263
debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host->detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
-> mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
("mmc: slot-gpio: Add GPIO descriptor based CD GPIO API"), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
CC: Russell King
Cc: Adrian Hunter
Cc: Alexandre Courbot
Cc: Linus Walleij
Signed-off-by: Stephen Warren
Hi Stephen,
Thanks for looking into this. It seems like this issue has been
present for quite a while.
I believe your patch should have a stable tag for 3.15+ as well,
unless you object I will add it.
Yes, that probably makes sense, thanks.
Doesn't this patch break the drivers that call mmc_gpio_request_cd() after
mmc_add_host() like mmc_spi.c or sdhci-sirf.c or tmio_mmc_pio.c ?
Oh, if there are drivers that do that, this patch might cause an issue.
But why are they doing that? Shouldn't all the drivers set up the same
kinds of resources in the same order and way?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: don't request CD IRQ until mmc_start_host()
On 09/17/2014 01:55 PM, Ulf Hansson wrote:
On 12 September 2014 19:18, Stephen Warren wrote:
From: Stephen Warren
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host->detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host->mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() ->
mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host->detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
-> mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
("mmc: slot-gpio: Add GPIO descriptor based CD GPIO API"), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
CC: Russell King
Cc: Adrian Hunter
Cc: Alexandre Courbot
Cc: Linus Walleij
Signed-off-by: Stephen Warren
Hi Stephen,
Thanks for looking into this. It seems like this issue has been
present for quite a while.
I believe your patch should have a stable tag for 3.15+ as well,
unless you object I will add it.
Yes, that probably makes sense, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: don't request CD IRQ until mmc_start_host()
From: Stephen Warren
As soon as the CD IRQ is requested, it can trigger, since it's an
externally controlled event. If it does, delayed_work host->detect will
be scheduled.
Many host controller probe()s are roughly structured as:
*_probe() {
host = sdhci_pltfm_init();
mmc_of_parse(host->mmc);
rc = sdhci_add_host(host);
if (rc) {
sdhci_pltfm_free();
return rc;
}
In 3.17, CD IRQs can are enabled quite early via *_probe() ->
mmc_of_parse() -> mmc_gpio_request_cd() -> mmc_gpiod_request_cd_irq().
Note that in linux-next, mmc_of_parse() calls mmc_gpio*d*_request_cd()
rather than mmc_gpio_request_cd(), and mmc_gpio*d*_request_cd() doesn't
call mmc_gpiod_request_cd_irq(). However, this issue still exists for
any other direct users of mmc_gpio_request_cd().
sdhci_add_host() may fail part way through (e.g. due to deferred
probe for a vmmc regulator), and sdhci_pltfm_free() does nothing to
unrequest the CD IRQ nor cancel the delayed_work. sdhci_pltfm_free() is
coded to assume that if sdhci_add_host() failed, then the delayed_work
cannot (or should not) have been triggered.
This can lead to the following with CONFIG_DEBUG_OBJECTS_* enabled, when
kfree(host) is eventually called inside sdhci_pltfm_free():
WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4()
ODEBUG: free active (active state 0) object type: timer_list hint:
delayed_work_timer_fn+0x0/0x18
The object being complained about is host->detect.
There's no need to request the CD IRQ so early; mmc_start_host() already
requests it, and I *assume* that mmc_start_host() is called somehow for
all host controllers. For SDHCI hosts at least, the typical call path
that does this is: *_probe() -> sdhci_add_host() -> mmc_add_host() ->
mmc_start_host(). So, remove the call to mmc_gpiod_request_cd_irq() from
mmc_gpio_request_cd(). This matches mmc_gpio*d*_request_cd(), which
already doesn't call mmc_gpiod_request_cd_irq().
This solves the problem (eliminates the kernel error message above),
since it guarantees that the IRQ can't trigger before mmc_start_host()
is called.
The critical point here is that once sdhci_add_host() calls
mmc_add_host() -> mmc_start_host(), sdhci_add_host() is coded not to
fail. In other words, if there's a chance that mmc_start_host() may have
been called, and CD IRQs triggered, and the delayed_work scheduled,
sdhci_add_host() won't fail, and so cleanup is no longer via
sdhci_pltfm_free() (which doesn't free the IRQ or cancel the work queue)
but instead must be via sdhci_remove_host(), which calls mmc_remove_host()
-> mmc_stop_host(), which does free the IRQ and cancel the work queue.
This fixes what I might conclude to be a mistake in commit 740a221ef0e5
("mmc: slot-gpio: Add GPIO descriptor based CD GPIO API"), which added the
call from mmc_start_host() to mmc_gpiod_request_cd_irq(), but also added
incorrectly added a call from mmc_gpio_request_cd() to
mmc_gpiod_request_cd_irq().
CC: Russell King
Cc: Adrian Hunter
Cc: Alexandre Courbot
Cc: Linus Walleij
Signed-off-by: Stephen Warren
---
drivers/mmc/core/slot-gpio.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c
index 5f89cb83d5f0..187f48a5795a 100644
--- a/drivers/mmc/core/slot-gpio.c
+++ b/drivers/mmc/core/slot-gpio.c
@@ -221,8 +221,6 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int
gpio,
ctx->override_cd_active_level = true;
ctx->cd_gpio = gpio_to_desc(gpio);
- mmc_gpiod_request_cd_irq(host);
-
return 0;
}
EXPORT_SYMBOL(mmc_gpio_request_cd);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mmc: freeing host while host->detect work queue is still active
On 09/11/2014 04:03 PM, Stephen Warren wrote: Running Fedora rawhides's 3.17.0-0.rc4.git2.1.fc22.armv7hl kernel on Jetson TK1 (an ARM board containing Tegra SoC), I see the following during boot most times the Tegra SDHCI driver defers probe for the SD slot: (and indeed I can reproduce the same issue in plain old v3.17-rc4, so this isn't at all Fedora specific) [8.377719] sdhci-tegra 700b0400.sdhci: Got CD GPIO #170. [8.377780] sdhci-tegra 700b0400.sdhci: Got WP GPIO #132. [8.377898] mmc1: Unknown controller version (3). You may experience problems. [8.379796] sdhci-tegra 700b0400.sdhci: No vmmc regulator found [8.380225] [ cut here ] [8.380243] WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4() [8.380261] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x18 [8.380308] Modules linked in: ehci_tegra(+) sdhci_tegra sdhci_pltfm sdhci phy_tegra_usb mmc_core i2c_tegra tegra_drm drm_kms_helper drm host1x [8.380319] CPU: 2 PID: 6 Comm: kworker/u8:0 Not tainted 3.17.0-0.rc4.git2.1.fc22.armv7hl #1 [8.380336] Workqueue: deferwq deferred_probe_work_func [8.380358] [] (unwind_backtrace) from [] (show_stack+0x18/0x1c) [8.380374] [] (show_stack) from [] (dump_stack+0x84/0xb0) [8.380395] [] (dump_stack) from [] (warn_slowpath_common+0x70/0x94) [8.380413] [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x34/0x44) [8.380426] [] (warn_slowpath_fmt) from [] (debug_print_object+0x8c/0xb4) [8.380439] [] (debug_print_object) from [] (debug_check_no_obj_freed+0xf4/0x1dc) [8.380453] [] (debug_check_no_obj_freed) from [] (kfree+0x160/0x308) [8.380465] [] (kfree) from [] (device_release+0x64/0x98) [8.380479] [] (device_release) from [] (kobject_release+0x12c/0x19c) [8.380498] [] (kobject_release) from [] (sdhci_tegra_probe+0x190/0x1bc [sdhci_tegra]) [8.380810] [] (sdhci_tegra_probe [sdhci_tegra]) from [] (platform_drv_probe+0x34/0x68) [8.380825] [] (platform_drv_probe) from [] (driver_probe_device+0x13c/0x33c) [8.380838] [] (driver_probe_device) from [] (bus_for_each_drv+0x8c/0x9c) [8.380852] [] (bus_for_each_drv) from [] (device_attach+0x70/0x94) [8.380865] [] (device_attach) from [] (bus_probe_device+0x30/0xa4) [8.380878] [] (bus_probe_device) from [] (deferred_probe_work_func+0x88/0xb8) [8.380894] [] (deferred_probe_work_func) from [] (process_one_work+0x2a0/0x5f8) [8.380908] [] (process_one_work) from [] (worker_thread+0x2d0/0x40c) [8.380922] [] (worker_thread) from [] (kthread+0xd4/0xe8) [8.380936] [] (kthread) from [] (ret_from_fork+0x14/0x20) [8.380941] ---[ end trace b1f4b0fe632eb3a4 ]--- The problem is as follows: The following sequence of calls requests the CD (Change Detect) IRQ for the SD slot: sdhci_tegra_probe() sdhci_tegra_parse_dt() mmc_of_parse() mmc_gpio_request_cd() devm_request_threaded_irq() The IRQ is triggered by a pin on the SD slot, so could be in any state, and might fire immediately. This causes the IRQ handler mmc_gpio_cd_irqt() to run, which calls mmc_detect_change() which calls mmc_schedule_delayed_work(&host->detect, ...); Thus, the work queue can be immediately active. However, if later part of sdhci_tegra_probe() fails, e.g. sdhci_add_host() -> mmc_regulator_get_supply() -> devm_regulator_get_optional() -> -EPROBE_DEFER, then sdhci_tegra_probe() needs to tear everything down, and so calls sdhci_pltfm_free() -> sdhci_free_host() -> mmc_free_host() -> put_device(&host->class_dev) -> mmc_host_classdev_release() -> kfree(host). However, host->detect is part of host and may still be active at this point, which is what triggers the ODETECT log spew mentioned above. This doesn't happen in linux-next, because mmc_gpiod_request_cd() doesn't set up the IRQ handler. Rather this happens inside mmc_gpiod_request_cd_irq() which is called from mmc_start_host(), which I believe is well after deferred probe could occur. Some possible options for fixing this in 3.17 are: a) Back-port whatever changes in mainline stopped mmc_of_parse()() from requesting the IRQ. That is commit 740a221ef0e5 "mmc: slot-gpio: Add GPIO descriptor based CD GPIO API". I haven't investigated how many other commits would be required for that commit to apply, but I'm nervous it'd be too many to apply to 3.17. Sorry, I quoted the wrong commit ID there. That commit is already present in 3.17 (and some earlier). That commit adds the separate mmc_gpiod_request_cd_irq() function that's necessary to solve the problem, and adds a call to it from mmc_start_host(). However, it still leaves a call to mmc_gpiod_request_cd_irq() in mmc_gpio_request_cd(), thus leaving the problem in place. The following commit from linux-next actually solves the problem for me, by having mmc_of_parse() call mmc_gpiod_re
mmc: freeing host while host->detect work queue is still active
Running Fedora rawhides's 3.17.0-0.rc4.git2.1.fc22.armv7hl kernel on Jetson TK1 (an ARM board containing Tegra SoC), I see the following during boot most times the Tegra SDHCI driver defers probe for the SD slot: [8.377719] sdhci-tegra 700b0400.sdhci: Got CD GPIO #170. [8.377780] sdhci-tegra 700b0400.sdhci: Got WP GPIO #132. [8.377898] mmc1: Unknown controller version (3). You may experience problems. [8.379796] sdhci-tegra 700b0400.sdhci: No vmmc regulator found [8.380225] [ cut here ] [8.380243] WARNING: CPU: 2 PID: 6 at lib/debugobjects.c:263 debug_print_object+0x8c/0xb4() [8.380261] ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x18 [8.380308] Modules linked in: ehci_tegra(+) sdhci_tegra sdhci_pltfm sdhci phy_tegra_usb mmc_core i2c_tegra tegra_drm drm_kms_helper drm host1x [8.380319] CPU: 2 PID: 6 Comm: kworker/u8:0 Not tainted 3.17.0-0.rc4.git2.1.fc22.armv7hl #1 [8.380336] Workqueue: deferwq deferred_probe_work_func [8.380358] [] (unwind_backtrace) from [] (show_stack+0x18/0x1c) [8.380374] [] (show_stack) from [] (dump_stack+0x84/0xb0) [8.380395] [] (dump_stack) from [] (warn_slowpath_common+0x70/0x94) [8.380413] [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x34/0x44) [8.380426] [] (warn_slowpath_fmt) from [] (debug_print_object+0x8c/0xb4) [8.380439] [] (debug_print_object) from [] (debug_check_no_obj_freed+0xf4/0x1dc) [8.380453] [] (debug_check_no_obj_freed) from [] (kfree+0x160/0x308) [8.380465] [] (kfree) from [] (device_release+0x64/0x98) [8.380479] [] (device_release) from [] (kobject_release+0x12c/0x19c) [8.380498] [] (kobject_release) from [] (sdhci_tegra_probe+0x190/0x1bc [sdhci_tegra]) [8.380810] [] (sdhci_tegra_probe [sdhci_tegra]) from [] (platform_drv_probe+0x34/0x68) [8.380825] [] (platform_drv_probe) from [] (driver_probe_device+0x13c/0x33c) [8.380838] [] (driver_probe_device) from [] (bus_for_each_drv+0x8c/0x9c) [8.380852] [] (bus_for_each_drv) from [] (device_attach+0x70/0x94) [8.380865] [] (device_attach) from [] (bus_probe_device+0x30/0xa4) [8.380878] [] (bus_probe_device) from [] (deferred_probe_work_func+0x88/0xb8) [8.380894] [] (deferred_probe_work_func) from [] (process_one_work+0x2a0/0x5f8) [8.380908] [] (process_one_work) from [] (worker_thread+0x2d0/0x40c) [8.380922] [] (worker_thread) from [] (kthread+0xd4/0xe8) [8.380936] [] (kthread) from [] (ret_from_fork+0x14/0x20) [8.380941] ---[ end trace b1f4b0fe632eb3a4 ]--- The problem is as follows: The following sequence of calls requests the CD (Change Detect) IRQ for the SD slot: sdhci_tegra_probe() sdhci_tegra_parse_dt() mmc_of_parse() mmc_gpio_request_cd() devm_request_threaded_irq() The IRQ is triggered by a pin on the SD slot, so could be in any state, and might fire immediately. This causes the IRQ handler mmc_gpio_cd_irqt() to run, which calls mmc_detect_change() which calls mmc_schedule_delayed_work(&host->detect, ...); Thus, the work queue can be immediately active. However, if later part of sdhci_tegra_probe() fails, e.g. sdhci_add_host() -> mmc_regulator_get_supply() -> devm_regulator_get_optional() -> -EPROBE_DEFER, then sdhci_tegra_probe() needs to tear everything down, and so calls sdhci_pltfm_free() -> sdhci_free_host() -> mmc_free_host() -> put_device(&host->class_dev) -> mmc_host_classdev_release() -> kfree(host). However, host->detect is part of host and may still be active at this point, which is what triggers the ODETECT log spew mentioned above. This doesn't happen in linux-next, because mmc_gpiod_request_cd() doesn't set up the IRQ handler. Rather this happens inside mmc_gpiod_request_cd_irq() which is called from mmc_start_host(), which I believe is well after deferred probe could occur. Some possible options for fixing this in 3.17 are: a) Back-port whatever changes in mainline stopped mmc_of_parse()() from requesting the IRQ. That is commit 740a221ef0e5 "mmc: slot-gpio: Add GPIO descriptor based CD GPIO API". I haven't investigated how many other commits would be required for that commit to apply, but I'm nervous it'd be too many to apply to 3.17. b) Fix the -EPROBE_DEFER cleanup path to explicitly free the IRQ (so it can't schedule the work queue any more), and then cancel the work queue (so it isn't active and doesn't trigger the ODETECT message). A naive patch might be like: diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index 31969436d77c..8125f916be4a 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -586,6 +586,9 @@ void mmc_free_host(struct mmc_host *host) idr_remove(&mmc_host_idr, host->index); spin_unlock(&mmc_host_lock); + mmc_gpiod_free_cd(host); + cancel_delayed_work_sync(&host->detect); + put_device(&host->class_dev); } However, that's problematic, since
Re: [PATCH 23/38] mmc: sdhci: convert sdhci_set_uhs_signaling() into a library function
On 06/19/2014 06:28 AM, Russell King - ARM Linux wrote: > On Mon, Jun 16, 2014 at 02:17:30PM +0200, Ulf Hansson wrote: >> Anyway, we did get some folks to test the patches and was thus fairly >> confident that we could merge them. Chris asked me to try to collect >> them in a PR for him, so I did. Sorry if I managed to screw some >> things up, there were several conflicts and actual regressions, which >> I tried to take care of. >> >> The mmc people were also very helping in sending patches to fixup >> related regressions, immediately after we merged your patchset. Thus >> together I think we managed to pull it off. > > I tend to look through slightly less rose-tinted glasses. > > The fact is... there's loads of ARM platforms which now fail in Olof's > build/boot testing, and they all seem to have a very similar pattern: > > hummingboard: > [1.149688] sdhci: Secure Digital Host Controller Interface driver > [1.155901] sdhci: Copyright(c) Pierre Ossman > ... > [1.253630] Waiting for root device /dev/mmcblk0p2... > [ 60.325469] imx-sdma 20ec000.sdma: firmware not found > ~$off > # PYBOOT: Exception: timeout > > jetson: > [2.261355] Waiting for root device /dev/mmcblk0p1... > > wandboard: > [1.186870] sdhci: Secure Digital Host Controller Interface driver > [1.193075] sdhci: Copyright(c) Pierre Ossman > ... > [1.291064] Waiting for root device /dev/mmcblk0p2... Any SDHCI failures in Linus' tree (but not linux-next) that occur only in multi_v7_defconfig are likely solved by: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/264012.html [PATCH] ARM: multi_v7_defconfig: re-enable SDHCI drivers > Whether these are caused by the patch set or not is anyone's guess, > because we (a) don't know what's causing these failures, and (b) > my patch series was never tested on anything but iMX6. I thought that I'd tested at least some of it on Tegra. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: bcm_defconfig: re-enable BCM Kona SDHCI driver
On 06/16/2014 03:55 PM, Russell King - ARM Linux wrote: > On Mon, Jun 16, 2014 at 01:56:16PM -0400, Matt Porter wrote: >> Since 5d01b7684b7e "mmc: simplify SDHCI Kconfig dependencies", >> MMC_SDHCI_PLTFM is no longer selected by the BCM Kona SDHCI >> driver which is enabled in this defconfig. This results in the >> SDHCI driver not being built and a boot failure. Explictly >> enable MMC_SDHCI_PLTFM in bcm_defconfig so the BCM Kona SDHCI >> driver is built. > > I really don't understand why MMC_SDHCI_PLTFM isn't selected by the > sub-drivers. sdhci-pltfm.c is a library module, which is only > useful with one of the sub-drivers enabled. > > So, rather than providing a multitude of dependent options, why not > present people with: > > + sdhci drivers >+ sdhci pci >+ sdhci acpi >+ Arasan >+ Freescale eSDHC >+ Nintendo Wii >+ Dove > > and leave MMC_SDHCI_PLTFM as a hidden option whose purpose is to > enable building of sdhci-pltfm.c. CC'ing Arnd, as author of 5d01b7684b7e "mmc: simplify SDHCI Kconfig dependencies". I also forgot to do that on the patch that fixed multi_v7_defconfig for this:-( -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: multi_v7_defconfig: re-enable SDHCI drivers
From: Stephen Warren
Following 5d01b7684b7e "mmc: simplify SDHCI Kconfig dependencies",
SDHCI drivers that use MMC_SDHCI_PLTFM no longer select it, but
instead depend on it. This means that multi_v7_defconfig no longer
selects it, and hence many SDHCI drivers are no longer enabled.
Explicitly enable MMC_SDHCI_PLTFM to solve this.
Fixes: 5d01b7684b7e ("mmc: simplify SDHCI Kconfig dependencies")
Signed-off-by: Stephen Warren
---
This is a fix for 3.16, and based directly on v3.16-rc1.
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig
b/arch/arm/configs/multi_v7_defconfig
index e2d62048e198..17d9462b9fb9 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -300,6 +300,7 @@ CONFIG_MMC=y
CONFIG_MMC_BLOCK_MINORS=16
CONFIG_MMC_ARMMMCI=y
CONFIG_MMC_SDHCI=y
+CONFIG_MMC_SDHCI_PLTFM=y
CONFIG_MMC_SDHCI_OF_ARASAN=y
CONFIG_MMC_SDHCI_ESDHC_IMX=y
CONFIG_MMC_SDHCI_DOVE=y
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: adding aliases to mmc ... again
On 05/23/2014 02:29 AM, Michael Olbrich wrote: > On Thu, May 22, 2014 at 01:23:27PM -0600, Stephen Warren wrote: >> On 05/22/2014 12:21 PM, Sascha Hauer wrote: >>> On Thu, May 22, 2014 at 10:16:35AM -0600, Stephen Warren wrote: >>>> On 05/22/2014 09:30 AM, Sascha Hauer wrote: >>>>> Hi all, >>>>> >>>>> The wish to have persistent MMC block device names for passing a suitable >>>>> root=/dev/mmcblkX option came up several times already and has been >>>>> discussed >>>>> at least in these threads: >>>>> >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/109984.html >>>>> https://www.mail-archive.com/linux-mmc@vger.kernel.org/msg22104.html >>>>> http://comments.gmane.org/gmane.linux.kernel.mmc/21519 >>>>> >>>>> Several patches have been proposed to nail the slot index to a known >>>>> number. >>>>> Arguments against these patches were: >>>>> >>>>> - Use an initrd and locate the correct root device there >>>>> even Thomas who suggested this admitted this would be painful to do >>>>> - use root=UUID= or root=PARTUUID= >>>>> This generally works but has an important downside. With the UUID >>>>> approach devices which should boot from the internal eMMC may start >>>>> booting from an external SD slot when somebody deliberately inserts >>>>> a SD card with the same UUID. >>>>> >>>>> The following patches should have the technical issues fixed. It works >>>>> by counting the mmc aliases in the devicetree during initialization of >>>>> the mmc framework. Those slot numbers will never be assigned to other >>>>> hosts. >>>> >>>> Does it solve the following, which AFAIK has always been the primary >>>> argument against aligning block device IDs with controller IDs: >>>> >>>> - User inserts SD card into MMC controller ID (or alias) 1. >>>> - /dev/mmcblk1 now exists >>>> - Mount /dev/mmcblk1 on /mnt/tmp >>>> - User removes that SD card >>>> - /dev/mmcblk1 still exists, since it's mounted so can't be deleted >>>> after the card removal. >>>> - User inserts SD card into MMC controller ID (or alias) 1. >>>> - /dev/mmcblkN (N is something other than 1) now exists >>>> >>>> Now, the block device ID must be != the original ID, since two block >>>> devices exist. > > We could limit this to non-removable devices. This problem cannot occur > there. Would that be acceptable? That might work. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: adding aliases to mmc ... again
On 05/23/2014 03:23 AM, Sascha Hauer wrote: ... > Speaking of which my preferred solution is another one. As a bootloader > developer it really annoys me that I don't have the possibility to tell > the kernel to boot a particular device. What I really want to do is to > pass a devicetree phandle to the kernel for the rootfs (Or a device path > for the EFI/ACPI guys). This would solve a whole lot of problems here. Why not implement a root= kernel cmdline option that provides exactly that? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: adding aliases to mmc ... again
On 05/22/2014 12:21 PM, Sascha Hauer wrote: > On Thu, May 22, 2014 at 10:16:35AM -0600, Stephen Warren wrote: >> On 05/22/2014 09:30 AM, Sascha Hauer wrote: >>> Hi all, >>> >>> The wish to have persistent MMC block device names for passing a suitable >>> root=/dev/mmcblkX option came up several times already and has been >>> discussed >>> at least in these threads: >>> >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/109984.html >>> https://www.mail-archive.com/linux-mmc@vger.kernel.org/msg22104.html >>> http://comments.gmane.org/gmane.linux.kernel.mmc/21519 >>> >>> Several patches have been proposed to nail the slot index to a known number. >>> Arguments against these patches were: >>> >>> - Use an initrd and locate the correct root device there >>> even Thomas who suggested this admitted this would be painful to do >>> - use root=UUID= or root=PARTUUID= >>> This generally works but has an important downside. With the UUID >>> approach devices which should boot from the internal eMMC may start >>> booting from an external SD slot when somebody deliberately inserts >>> a SD card with the same UUID. >>> >>> The following patches should have the technical issues fixed. It works >>> by counting the mmc aliases in the devicetree during initialization of >>> the mmc framework. Those slot numbers will never be assigned to other >>> hosts. >> >> Does it solve the following, which AFAIK has always been the primary >> argument against aligning block device IDs with controller IDs: >> >> - User inserts SD card into MMC controller ID (or alias) 1. >> - /dev/mmcblk1 now exists >> - Mount /dev/mmcblk1 on /mnt/tmp >> - User removes that SD card >> - /dev/mmcblk1 still exists, since it's mounted so can't be deleted >> after the card removal. >> - User inserts SD card into MMC controller ID (or alias) 1. >> - /dev/mmcblkN (N is something other than 1) now exists >> >> Now, the block device ID must be != the original ID, since two block >> devices exist. > > No, it shouldn't solve that, but it's out of scope for this patch. All > it solves is to reliably find the rootfs. If the card containing your > rootfs is removed you are in trouble anyway and it won't help if it gets > the same mmcblkno once it's plugged again. For other devices which don't > contain the rootfs there are enough possibilities to find them in userspace. I don't think there should be any special cases for the root fs. I don't think the disadvantage of the UUID= or PARTUUID= issue you mention enough is really an issue in general. If it is for some platform, then the root fs should be cryto-signed and validated to prevent someone from using the wrong root fs. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: adding aliases to mmc ... again
On 05/22/2014 09:30 AM, Sascha Hauer wrote: > Hi all, > > The wish to have persistent MMC block device names for passing a suitable > root=/dev/mmcblkX option came up several times already and has been discussed > at least in these threads: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/109984.html > https://www.mail-archive.com/linux-mmc@vger.kernel.org/msg22104.html > http://comments.gmane.org/gmane.linux.kernel.mmc/21519 > > Several patches have been proposed to nail the slot index to a known number. > Arguments against these patches were: > > - Use an initrd and locate the correct root device there > even Thomas who suggested this admitted this would be painful to do > - use root=UUID= or root=PARTUUID= > This generally works but has an important downside. With the UUID > approach devices which should boot from the internal eMMC may start > booting from an external SD slot when somebody deliberately inserts > a SD card with the same UUID. > > The following patches should have the technical issues fixed. It works > by counting the mmc aliases in the devicetree during initialization of > the mmc framework. Those slot numbers will never be assigned to other > hosts. Does it solve the following, which AFAIK has always been the primary argument against aligning block device IDs with controller IDs: - User inserts SD card into MMC controller ID (or alias) 1. - /dev/mmcblk1 now exists - Mount /dev/mmcblk1 on /mnt/tmp - User removes that SD card - /dev/mmcblk1 still exists, since it's mounted so can't be deleted after the card removal. - User inserts SD card into MMC controller ID (or alias) 1. - /dev/mmcblkN (N is something other than 1) now exists Now, the block device ID must be != the original ID, since two block devices exist. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] mmc: tegra: disable UHS modes
On 04/16/2014 05:08 PM, Andrew Bresticker wrote: > Program TEGRA_SDHCI_VENDOR_MISC_CTRL so that UHS modes aren't advertised > in SDHCI_CAPABILITIES_1. While the Tegra SDHCI controller does support > these modes, they require Tegra-specific tuning and calibration routines > which the driver does not support yet. What's the status of patches 1 and 2 in this series? I assumed they'd be applied to the MMC tree. I've had them applied to my local development tree for a while now, so in case this helps: Tested-by: Stephen Warren Acked-by: Stephen Warren (I thought I wrote that before, but I can't find it, so I must have forgotten to) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/38] MMC updates, plus CuBox-i WiFi support
On 04/23/2014 12:55 PM, Russell King - ARM Linux wrote: > All, > > This is where I'm at with trying to clean up the SDHCI mess, and sort out > issues I've noticed when trying to get UHS support working on CuBoxes. > This is my full patch set, but I recommend not applying patch 37 as there > appears to be a hardware issue preventing it working reliably. > > In any case, patches 0-33 inclusive are the SDHCI clean up, based against > v3.15-rc1. Patches 34 and 35 are Olof's Wifi support, 36 is my fix against > those, and 38 adds the Broadcom Wifi and BT support for CuBox. > > The questions over how to handle these devices were never properly settled, > so I recommend against merging patch 34 onwards. As for the rest, I'm not > planning on any further work, so it may be a good idea for people to > consider testing them with a view to getting them merged. The series, Tested-by: Stephen Warren (On an NVIDIA Tegra "Jetson TK1" board, with the patches applied on top of next-20140428, also with Andrew Bresticker's Tegra SDHCI patches "mmc: tegra: disable UHS modes" and "mmc: tegra: fix reporting of base clock frequency" applied) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 33/38] mmc: sdhci: fix SDHCI dependencies
On 04/23/2014 01:08 PM, Russell King wrote: > Signed-off-by: Russell King A patch description would be useful so I can work out why... > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > config MMC_SDHCI > - tristate "Secure Digital Host Controller Interface support" > - depends on HAS_DMA > + tristate > help > This selects the generic Secure Digital Host Controller Interface. > It is used by manufacturers such as Texas Instruments(R), Ricoh(R) ... it's useful to remove that depends from here and ... > config MMC_SDHCI_TEGRA > tristate "SDHCI platform support for the Tegra SD/MMC Controller" > - depends on ARCH_TEGRA > - depends on MMC_SDHCI_PLTFM > + depends on ARCH_TEGRA && HAS_DMA ... duplicate it in all the HW-specific drivers. On Tegra (and I believe at least any ARM system), I think HAS_DMA is always true, so is there a need for "&& HAS_DMA" at all, now that this statement is pushed into arch-/HW-specific options? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] ARM: tegra: fix Venice2 SD card VQMMC supply
On 04/16/2014 05:08 PM, Andrew Bresticker wrote: > VDDIO_SDMMC3 is the VQMMC (I/O) supply, not the VMMC (core) supply, > for the SD slot on Venice2. I've applied this (one patch) to Tegra's for-3.16/dt branch. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] ARM: tegra: fix Venice2 VQMMC regulators
On 04/15/2014 06:29 PM, Andrew Bresticker wrote:
>> sdhci@0,700b0600 {
>> status = "okay";
>> bus-width = <8>;
>> + vqmmc-supply = <&vddio_1v8>;
>
> It turns out this bit isn't needed as the initialization failures are
> instead due to +3V3_RUN toggling, which Stephen has fixed:
> http://patchwork.ozlabs.org/patch/339386/
Indeed, if I apply patches 1, 2, and 4 from this series, revert the hunk
above, apply my Venice2 regulator patch, and finally apply your AS3722
regulator GPIO patch, then everything works fine for me:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] ARM: tegra: fix Venice2 VQMMC regulators
On 04/14/2014 07:42 PM, Andrew Bresticker wrote:
> VDDIO_SDMMC3 is the VQMMC supply, not the VMMC supply, for the SD
> slot. Add 1.8V_VDDIO as the VQMMC supply for the eMMC.
Is there documentation re: what vmmc-supply and vqmmc-supply actually
refer to? I looked a long while ago and couldn't find any, and didn't
get an answer when I asked.
Neither
Documentation/devicetree/bindings/mmc/{mmc.txt,nvidia,tegra20-sdhci.txt}
seem to say:-(
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] mmc: sdhci: defer probing on regulator_get_optional() failures
On 04/14/2014 07:42 PM, Andrew Bresticker wrote:
> If regulator_get_optional() returns EPROBE_DEFER, it indicates
> that the regulator may show up later (e.g. the DT property is
> present but the corresponding regulator may not have probed).
> Instead of continuing without the regulator, return EPROBE_DEFER
> from sdhci_add_host(). Also, fix regulator leaks in the error
> paths in sdhci_add_host().
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> if (IS_ERR_OR_NULL(host->vqmmc)) {
This is a pre-existing condition, but ick: Why doesn't this test either
IS_ERR() /or/ == NULL, but not both. On error, the regulator API should
either return and error-point or return NULL, so that client code
shouldn't need to check for both.
> +put_vmmc:
> + if (host->vmmc)
> + regulator_put(host->vmmc);
> +put_vqmmc:
> + if (host->vqmmc)
> + regulator_put(host->vqmmc);
If IS_ERR_OR_NULL() really is required above, it should be used here
too. More likely, I hope you need to replace if (host->vmmc) with if
(!IS_ERR(host->vmmc)).
I wonder if fixing this would help solve the crashes that Alex saw?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] mmc: tegra: fix reporting of base clock frequency
On 04/14/2014 07:42 PM, Andrew Bresticker wrote: > Tegra SDHCI controllers, by default, report a base clock frequency > of 208Mhz in SDHCI_CAPABILTIES which may or may not be equal to the > actual base clock frequency. Some explanation of why this "may or may not be equal to the actual base clock frequency" would be nice. Presumably, it's because the clock frequency is supplied by the clock controller module, and configuring that happens externally to the SD controller, so the SD HW has no knowledge of the actual frequency, and hence simply reports a hard-coded maximum possible clock frequency? > While this can be overridden by setting > BASE_CLK_FREQ in VENDOR_CLOCK_CTRL on Tegra30 and later SoCs, just > set SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN and supply a get_max_clock() > callback to get the actual rate of the base clock. It's not clear to me from the function name that sdhci_pltfm_clk_get_max_clock() simply calls clk_get() on the actual clock. It might be nice to mention that in the commit description. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/2] mmc: use SD/MMC host ID for block device name ID
On 04/01/2014 02:35 PM, ste...@agner.ch wrote: > From: Stefan Agner > > By using the SD/MMC host device ID as a starting point for block > device numbering, one can reliably predict the first block device > name (at least for the first controller). That's not true. There's no guarantee that a device name/ID gets released as soon as the SD card is removed; something might still have it mounted for example. The correct solution here is to use filesystem or partition UUIDs to identify the device/partition, not to attempt to assign static device IDs. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 31/31] mmc: sdhci-tegra: get rid of special PRESENT_STATE register handling
On 02/18/2014 08:11 AM, Russell King wrote: > sdhci-tegra provides a get_ro method, which overrides the checking > of the write protect bit in the PRESENT_STATE register in sdhci.c: > > if (host->flags & SDHCI_DEVICE_DEAD) > is_readonly = 0; > else if (host->ops->get_ro) > is_readonly = host->ops->get_ro(host); > else > is_readonly = !(sdhci_readl(host, SDHCI_PRESENT_STATE) > & SDHCI_WRITE_PROTECT); > > This means it's pointless detecting accesses to this register and > manually setting the SDHCI_WRITE_PROTECT as it has no effect. > > This means that the whole of tegra_sdhci_readl() can be removed and > we can use the builtin sdhci readl functionality here. Acked-by: Stephen Warren > =-DO NOT APPLY-= Is that just because it's an RFC and you want to make sure it doesn't get accepted early, or are you explicitly trying to stop people applying this, testing it, and giving Tested-by? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH REPOST3] mmc: tegra: add support for Tegra124
From: Stephen Warren
Tegra124's MMC controller is very similar to earlier SoC generations,
and can be supported by the same driver.
However, there are some non-backwards-compatible HW differences, and
hence a new DT compatible value must be used to describe the HW. This
patch updates the driver to support that new compatible value.
That said, the HW differences are only relevant when enabling certain
high-performance transfer modes. Since the driver is currently very
simple and doesn't enable those modes, we don't actually need to address
any of these HW differences in the code yet, hence the simple nature of
this patch.
Signed-off-by: Stephen Warren
Reviewed-by: Thierry Reding
---
drivers/mmc/host/sdhci-tegra.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 5b7b2eba8a54..a835898a68dd 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -198,6 +198,7 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = {
};
static const struct of_device_id sdhci_tegra_dt_match[] = {
+ { .compatible = "nvidia,tegra124-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra114-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra30-sdhci", .data = &soc_data_tegra30 },
{ .compatible = "nvidia,tegra20-sdhci", .data = &soc_data_tegra20 },
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: don't return 1 for max_discard
On 12/19/2013 02:05 AM, Dong Aisheng wrote:
> On Thu, Dec 19, 2013 at 7:00 AM, Stephen Warren wrote:
>> On 12/18/2013 03:27 PM, Stephen Warren wrote:
>>> From: Stephen Warren
>>>
>>> In mmc_do_calc_max_discard(), if only a single erase block can be
>>> discarded within the host controller's timeout, don't allow discard
>>> operations at all.
>>>
>>> Previously, the code allowed sector-at-a-time discard (rather than
>>> erase-block-at-a-time), which was chronically slow.
>>>
>>> Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
>>> in qty == 1, which is immediately returned. This causes discard to
>>> operate a single sector at a time, which is chronically slow. With this
>>> patch in place, discard operates a single erase block at a time, which
>>> is reasonably fast.
>>
>> Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
>> host controllers specify maximum discard timeout", followed by:
>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 050eb262485c..35c5b5d86c99 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
>>> unsigned int from,
>>> cmd.opcode = MMC_ERASE;
>>> cmd.arg = arg;
>>> cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>> - cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
>>> err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>> if (err) {
>>> pr_err("mmc_erase: erase error %d, status %#x\n",
>>> @@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
>>> unsigned int from,
>>> if (mmc_host_is_spi(card->host))
>>> goto out;
>>>
>>> - timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
>>> + timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg,
>>> qty));
>>> do {
>>> memset(&cmd, 0, sizeof(struct mmc_command));
>>> cmd.opcode = MMC_SEND_STATUS;
>>
>> That certainly also seems to solve the problem on my board...
>
> Is the change you mean here only include above 3 lines changes?
> If yes, it's strange to me how does this solve your issue?
> It actually does not change the max_discard_to/max_discard_bytes.
> It still discards only one sector one time if it does as before
It's a revert of the patch which introduced max_discard_to, so all that
logic goes away completely, *then* the 3 lines above, which move the
timeout away from command submission (which is what I believe is
implemented by the controller's timeout HW) and to the CMD13 polling
operation (where we can implement whatever timeout we want, in the kernel).
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: don't return 1 for max_discard
On 12/19/2013 02:01 AM, Adrian Hunter wrote:
> On 19/12/13 01:00, Stephen Warren wrote:
>> On 12/18/2013 03:27 PM, Stephen Warren wrote:
>>> From: Stephen Warren
>>>
>>> In mmc_do_calc_max_discard(), if only a single erase block can be
>>> discarded within the host controller's timeout, don't allow discard
>>> operations at all.
>>>
>>> Previously, the code allowed sector-at-a-time discard (rather than
>>> erase-block-at-a-time), which was chronically slow.
>>>
>>> Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
>>> in qty == 1, which is immediately returned. This causes discard to
>>> operate a single sector at a time, which is chronically slow. With this
>>> patch in place, discard operates a single erase block at a time, which
>>> is reasonably fast.
>>
>> Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
>> host controllers specify maximum discard timeout", followed by:
>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 050eb262485c..35c5b5d86c99 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card,
>>> unsigned int from,
>>> cmd.opcode = MMC_ERASE;
>>> cmd.arg = arg;
>>> cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>> - cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
>>> err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>> if (err) {
>>> pr_err("mmc_erase: erase error %d, status %#x\n",
>>> @@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card,
>>> unsigned int from,
>>> if (mmc_host_is_spi(card->host))
>>> goto out;
>>>
>>> - timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
>>> + timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg,
>>> qty));
>>> do {
>>> memset(&cmd, 0, sizeof(struct mmc_command));
>>> cmd.opcode = MMC_SEND_STATUS;
>>
>> That certainly also seems to solve the problem on my board...
>
> But large erases will timeout when they should have been split into smaller
> chunks.
>
> A generic solution needs to be able to explain what happens when the host
> controller *does* timeout.
I thought the whole point of this discussion was that the timeout in the
first code segment above only applies to command submission, whereas
completion of the erase operation itself was determined by polling using
CMD13, which still uses mmc_erase_timeout(). As such, I don't /think/
that "large erases will timeout" here, unless I'm significantly
misunderstanding something?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: don't return 1 for max_discard
On 12/19/2013 01:39 AM, Dong Aisheng wrote: > On Thu, Dec 19, 2013 at 6:27 AM, Stephen Warren wrote: >> From: Stephen Warren >> >> In mmc_do_calc_max_discard(), if only a single erase block can be >> discarded within the host controller's timeout, don't allow discard >> operations at all. >> >> Previously, the code allowed sector-at-a-time discard (rather than >> erase-block-at-a-time), which was chronically slow. >> >> Without this patch, on the NVIDIA Tegra Cardhu board, the loops result >> in qty == 1, which is immediately returned. This causes discard to >> operate a single sector at a time, which is chronically slow. With this >> patch in place, discard operates a single erase block at a time, which >> is reasonably fast. >> >> Cc: Adrian Hunter >> Cc: Dong Aisheng >> Cc: Ulf Hansson >> Cc: Vladimir Zapolskiy >> Fixes: e056a1b5b67b "(mmc: queue: let host controllers specify maximum >> discard timeout") >> Signed-off-by: Stephen Warren >> --- >> drivers/mmc/core/core.c | 21 +++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 57a2b403bf8e..eb952ca634ea 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -2150,8 +2150,25 @@ static unsigned int mmc_do_calc_max_discard(struct >> mmc_card *card, >> if (!qty) >> return 0; >> >> - if (qty == 1) >> - return 1; >> + /* >> +* Discard operations may not be aligned to an erase block. In an >> +* unaligned case, we need to issue 1 more discard operation to HW >> +* than strictly calculated by: >> +* sectors_to_erase / sectors_per_discard_operation >> +* >> +* To account for this in the timeout calculations, we assume we can >> +* actually discard one less erase block than fits into the HW >> +* timeout. This explains the --qty below. >> +* >> +* When only a single discard block operation fits into the timeout, >> +* disallow any discard operations at all. For example, discarding >> one >> +* sector at a time is so chronically slow as to be useless. However, >> +* make an exception for SD cards without an erase shift, since qty >> +* isn't multiplied up by an erase block size in the code below for >> +* that case. >> +*/ >> + if (qty == 1 && !(!card->erase_shift && mmc_card_sd(card))) >> + return 0; >> > > How about when qty == 2? > Erase 2 sectors may have no much difference from 1 sector per one time > for a SD card, > it may still chronically slow, i guess. > So i wonder it may not sovle the issues totally. When qty==2, the number of sectors gets multiplied by the number of sectors in an erase block, so it isn't chronically slow. It's only slow with qty==1 because without this patch, the multiplication gets skipped and "1" returned rather then "1 << card->erase_shift". -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: don't return 1 for max_discard
On 12/18/2013 03:27 PM, Stephen Warren wrote:
> From: Stephen Warren
>
> In mmc_do_calc_max_discard(), if only a single erase block can be
> discarded within the host controller's timeout, don't allow discard
> operations at all.
>
> Previously, the code allowed sector-at-a-time discard (rather than
> erase-block-at-a-time), which was chronically slow.
>
> Without this patch, on the NVIDIA Tegra Cardhu board, the loops result
> in qty == 1, which is immediately returned. This causes discard to
> operate a single sector at a time, which is chronically slow. With this
> patch in place, discard operates a single erase block at a time, which
> is reasonably fast.
Alternatively, is the real fix a revert of e056a1b5b67b "mmc: queue: let
host controllers specify maximum discard timeout", followed by:
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 050eb262485c..35c5b5d86c99 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1950,7 +1950,6 @@ static int mmc_do_erase(struct mmc_card *card, unsigned
> int from,
> cmd.opcode = MMC_ERASE;
> cmd.arg = arg;
> cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> - cmd.cmd_timeout_ms = mmc_erase_timeout(card, arg, qty);
> err = mmc_wait_for_cmd(card->host, &cmd, 0);
> if (err) {
> pr_err("mmc_erase: erase error %d, status %#x\n",
> @@ -1962,7 +1961,7 @@ static int mmc_do_erase(struct mmc_card *card, unsigned
> int from,
> if (mmc_host_is_spi(card->host))
> goto out;
>
> - timeout = jiffies + msecs_to_jiffies(MMC_CORE_TIMEOUT_MS);
> + timeout = jiffies + msecs_to_jiffies(mmc_erase_timeout(card, arg,
> qty));
> do {
> memset(&cmd, 0, sizeof(struct mmc_command));
> cmd.opcode = MMC_SEND_STATUS;
That certainly also seems to solve the problem on my board...
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: core: don't return 1 for max_discard
From: Stephen Warren In mmc_do_calc_max_discard(), if only a single erase block can be discarded within the host controller's timeout, don't allow discard operations at all. Previously, the code allowed sector-at-a-time discard (rather than erase-block-at-a-time), which was chronically slow. Without this patch, on the NVIDIA Tegra Cardhu board, the loops result in qty == 1, which is immediately returned. This causes discard to operate a single sector at a time, which is chronically slow. With this patch in place, discard operates a single erase block at a time, which is reasonably fast. Cc: Adrian Hunter Cc: Dong Aisheng Cc: Ulf Hansson Cc: Vladimir Zapolskiy Fixes: e056a1b5b67b "(mmc: queue: let host controllers specify maximum discard timeout") Signed-off-by: Stephen Warren --- drivers/mmc/core/core.c | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 57a2b403bf8e..eb952ca634ea 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2150,8 +2150,25 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, if (!qty) return 0; - if (qty == 1) - return 1; + /* +* Discard operations may not be aligned to an erase block. In an +* unaligned case, we need to issue 1 more discard operation to HW +* than strictly calculated by: +* sectors_to_erase / sectors_per_discard_operation +* +* To account for this in the timeout calculations, we assume we can +* actually discard one less erase block than fits into the HW +* timeout. This explains the --qty below. +* +* When only a single discard block operation fits into the timeout, +* disallow any discard operations at all. For example, discarding one +* sector at a time is so chronically slow as to be useless. However, +* make an exception for SD cards without an erase shift, since qty +* isn't multiplied up by an erase block size in the code below for +* that case. +*/ + if (qty == 1 && !(!card->erase_shift && mmc_card_sd(card))) + return 0; /* Convert qty to sectors */ if (card->erase_shift) -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: max_discard anomaly on certain Sandisk eMMC
On 12/17/2013 08:32 PM, Dong Aisheng wrote: > On Wed, Dec 18, 2013 at 1:27 AM, Stephen Warren wrote: >> On 12/17/2013 02:25 AM, Dong Aisheng wrote: >>> Hi Stephen, >>> >>> On Sat, Dec 14, 2013 at 6:43 AM, Stephen Warren >>> wrote: >>>> On one of my eMMC devices, I see the following results from calling >>>> mmc_do_calc_max_discard() with various parameters: >>>> >>>> [3.057263] MMC_DISCARD_ARG max_discard 1 >>>> [3.057266] MMC_ERASE_ARG max_discard 4096 >>>> [3.057267] MMC_TRIM_ARGmax_discard 1 >>>> >>>> This causes mmc_calc_max_discard() to return 1, which makes the discard >>>> IOCTL extremely slow. >>>> >>> >>> IMX met the similar issue. >>> http://www.spinics.net/lists/linux-mmc/msg23375.html >>> It's caused by the max_discard_to supported by host is too small. >>> >>> I submitted the fix patches: >>> http://www.spinics.net/lists/arm-kernel/msg294924.html >>> Please see if it helps for you, especially patch #5. >>> It could increase the max_discard_to if Tegra has same problem. >> ... >> Even on the boards where your patch solves the problem, isn't it just a >> temporary measure; as soon as we upstream the changes to enable the >> faster transfer modes, we'll have a faster SDCLK, and hence again be >> limited in the discard size, perhaps down to a single sector again. > > Actually my patch is intend to fix 1) IMX incorrect max timeout issue > 2) should not use max_clock > to calculate max_discard_to issue for using SDCLK as timeout clock. > The issue discussed here is a different issue that the card timeout > may still be bigger than > the host capability and how to use discard for such case. > So your problem may exist if you meet some more big timeout cards. > For IMX, when running at 50Mhz, the max timeout is more than 5s. > It looks bigger enough currently and i tested many eMMC cards(Samsung, > Toshiba, Sandisk, Hynix) > and all they worked well with discard after fix. > I don't know which eMMC cards you meet the issue and don't know what > is Tegra max timeout. > Just for SD3.0 cards working on 200Mhz, i observed one Toshiba > SDHC U1 card could not do discard, since its AU erase timeout is 2s+ > which exceeds the host > capability 1335ms. Thus discard is automatically disabled. > But another Sandisk SDXC can still work well since it has small > ERASE_OFFSET as 1s. On the more recent Tegra boards, the eMMC devices appear to have an erase timeout of 4200ms for a single erase block! That's more than the ~2600ms max controller timeout at 48MHz on Tegra:-( (that is unless Tegra also supports more than 27 bits of timeout register) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: don't decrement qty when calculating max_discard
On 12/18/2013 04:08 AM, Adrian Hunter wrote: > On 17/12/13 20:02, Stephen Warren wrote: >> From: Stephen Warren >> >> In mmc_do_calc_max_discard(), if any value has been assigned to qty, >> that value must have passed the timeout checks in the loop. Hence, >> qty is the maximum number of erase blocks that fit within the timeout, >> not the first value that does not fit into the timeout. In turn, this >> means we don't need any special case for (qty == 1); any value of qty >> needs to be multiplied by the card's erase shift, and we don't need to >> decrement qty before doing so. >> >> Without this patch, on the NVIDIA Tegra Cardhu board, the loops result >> in qty == 1, which is immediately returned. This causes discard to >> operate a single sector at a time, which is chronically slow. With this >> patch in place, discard operates a single erase block at a time, which >> is reasonably fast. ... > The quantity is decreased by 1 to account for the fact that the erase can > cross the boundary between 1 erase block and another. i.e. even though the > size is 1 erase block it touches 2 erase blocks. Don't erases have to be aligned to the erase block alignment; surely that's what the eMMC's preferred erase alignment is all about? If that isn't the case, a comment in the code would be extremely useful... -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: core: don't decrement qty when calculating max_discard
From: Stephen Warren In mmc_do_calc_max_discard(), if any value has been assigned to qty, that value must have passed the timeout checks in the loop. Hence, qty is the maximum number of erase blocks that fit within the timeout, not the first value that does not fit into the timeout. In turn, this means we don't need any special case for (qty == 1); any value of qty needs to be multiplied by the card's erase shift, and we don't need to decrement qty before doing so. Without this patch, on the NVIDIA Tegra Cardhu board, the loops result in qty == 1, which is immediately returned. This causes discard to operate a single sector at a time, which is chronically slow. With this patch in place, discard operates a single erase block at a time, which is reasonably fast. Cc: Adrian Hunter Cc: Dong Aisheng Cc: Ulf Hansson Cc: Vladimir Zapolskiy Fixes: e056a1b5b67b "(mmc: queue: let host controllers specify maximum discard timeout") Signed-off-by: Stephen Warren --- If this makes sense, I wonder if it should be Cc: stable? --- drivers/mmc/core/core.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 57a2b403bf8e..dd793cf4ef46 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2150,16 +2150,13 @@ static unsigned int mmc_do_calc_max_discard(struct mmc_card *card, if (!qty) return 0; - if (qty == 1) - return 1; - /* Convert qty to sectors */ if (card->erase_shift) - max_discard = --qty << card->erase_shift; + max_discard = qty << card->erase_shift; else if (mmc_card_sd(card)) max_discard = qty; else - max_discard = --qty * card->erase_size; + max_discard = qty * card->erase_size; return max_discard; } -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: max_discard anomaly on certain Sandisk eMMC
On 12/17/2013 02:25 AM, Dong Aisheng wrote: > Hi Stephen, > > On Sat, Dec 14, 2013 at 6:43 AM, Stephen Warren wrote: >> On one of my eMMC devices, I see the following results from calling >> mmc_do_calc_max_discard() with various parameters: >> >> [3.057263] MMC_DISCARD_ARG max_discard 1 >> [3.057266] MMC_ERASE_ARG max_discard 4096 >> [3.057267] MMC_TRIM_ARGmax_discard 1 >> >> This causes mmc_calc_max_discard() to return 1, which makes the discard >> IOCTL extremely slow. >> > > IMX met the similar issue. > http://www.spinics.net/lists/linux-mmc/msg23375.html > It's caused by the max_discard_to supported by host is too small. > > I submitted the fix patches: > http://www.spinics.net/lists/arm-kernel/msg294924.html > Please see if it helps for you, especially patch #5. > It could increase the max_discard_to if Tegra has same problem. Thanks for the pointer! Yes, Tegra has SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK set, and has a max clock of 208MHz specified in HW, yet we only run the HW at 48MHz upstream, since we haven't actually implemented any of the advanced/faster transfer rates yet. Hence that patch does avoid/solve the issue on 2 of my boards. However, the patch doesn't solve the problem on 2 other boards, since the eMMC device on those boards specifies a much larger timeout, which still causes max_discard to be set to 0. It sounds like the real solution is what was discussed elsewhere in this thread; to use command polling for erases? Even on the boards where your patch solves the problem, isn't it just a temporary measure; as soon as we upstream the changes to enable the faster transfer modes, we'll have a faster SDCLK, and hence again be limited in the discard size, perhaps down to a single sector again. (Incidentally, I think the code should be limiting to a single erase block, not a single sector. I'll send a separate patch to fix that, and Cc everyone here). -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: max_discard anomaly on certain Sandisk eMMC
On 12/13/2013 03:43 PM, Stephen Warren wrote: > On one of my eMMC devices, I see the following results from calling > mmc_do_calc_max_discard() with various parameters: > > [3.057263] MMC_DISCARD_ARG max_discard 1 > [3.057266] MMC_ERASE_ARG max_discard 4096 > [3.057267] MMC_TRIM_ARGmax_discard 1 > > This causes mmc_calc_max_discard() to return 1, which makes the discard > IOCTL extremely slow. Further investigation shows that if I make a few hacks that essentially revert e056a1b5b67b "mmc: queue: let host controllers specify maximum discard timeout": diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c index 357bbc54fe4b..e66af930d0e3 100644 --- a/drivers/mmc/card/queue.c +++ b/drivers/mmc/card/queue.c @@ -167,13 +167,15 @@ static void mmc_queue_setup_discard(struct request_queue *q, return; queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q); - q->limits.max_discard_sectors = max_discard; + q->limits.max_discard_sectors = UINT_MAX; if (card->erased_byte == 0 && !mmc_can_discard(card)) q->limits.discard_zeroes_data = 1; q->limits.discard_granularity = card->pref_erase << 9; /* granularity must not be greater than max. discard */ +#if 0 if (card->pref_erase > max_discard) q->limits.discard_granularity = 0; +#endif if (mmc_can_secure_erase_trim(card)) queue_flag_set_unlocked(QUEUE_FLAG_SECDISCARD, q); } I end up with: $ cat /sys/.../block/mmcblk1/queue# cat discard_granularity 2097152 $ cat /sys/.../block/mmcblk1/queue# cat discard_max_bytes 2199023255040 $ cat /sys/.../block/mmcblk1/queue# cat discard_zeroes_data 1 With those values, mke2fs is fast, and I validated that "blkdiscard" works; I filled a large partition with /dev/urandom, executed "blkdiscard" on the 4M at the start, and saw zeroes when reading the discarded part back. This implies that the issue is simply the operation of mmc_calc_max_discard(), rather than the eMMC device mis-reporting its discard abilities, doesn't it? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH REPOST2] mmc: tegra: add support for Tegra124
From: Stephen Warren
Tegra124's MMC controller is very similar to earlier SoC generations,
and can be supported by the same driver.
However, there are some non-backwards-compatible HW differences, and
hence a new DT compatible value must be used to describe the HW. This
patch updates the driver to support that new compatible value.
That said, the HW differences are only relevant when enabling certain
high-performance transfer modes. Since the driver is currently very
simple and doesn't enable those modes, we don't actually need to address
any of these HW differences in the code yet, hence the simple nature of
this patch.
Signed-off-by: Stephen Warren
Reviewed-by: Thierry Reding
---
drivers/mmc/host/sdhci-tegra.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 5b7b2eba8a54..a835898a68dd 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -198,6 +198,7 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = {
};
static const struct of_device_id sdhci_tegra_dt_match[] = {
+ { .compatible = "nvidia,tegra124-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra114-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra30-sdhci", .data = &soc_data_tegra30 },
{ .compatible = "nvidia,tegra20-sdhci", .data = &soc_data_tegra20 },
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
max_discard anomaly on certain Sandisk eMMC
On one of my eMMC devices, I see the following results from calling
mmc_do_calc_max_discard() with various parameters:
[3.057263] MMC_DISCARD_ARG max_discard 1
[3.057266] MMC_ERASE_ARG max_discard 4096
[3.057267] MMC_TRIM_ARGmax_discard 1
This causes mmc_calc_max_discard() to return 1, which makes the discard
IOCTL extremely slow.
For almost all my other eMMC devices, either:
* Both arguments to mmc_do_calc_max_discard() yield zero. Hence, the
discard IOCTL is not supported.
* Both arguments to mmc_do_calc_max_discard() yield some reasonable
large value. Hence, the discard IOCTL executes reasonably quickly.
Do you think that TRIM_ARG result is expected, or is the eMMC firmware
simply buggy?
If I modify mmc_calc_max_discard() to simply ignore the TRIM_ARG result
and always use the ERASE_ARG result, I see no errors when executing
discard operations from either mke2fs, or from the blkdiscard utility. I
have no idea if the discard operation is doing anything useful though.
As an aside, another eMMC device (with same manfid/oemid/name) I have
returns the same 1 for TRIM_ARG, but returns 0 for ERASE_ARG, and hence
discard is disabled, so I don't see this problem:
[1.835747] MMC_DISCARD_ARG max_discard 1
[1.839779] MMC_ERASE_ARG max_discard 0
[1.843791] MMC_TRIM_ARGmax_discard 1
To solve my slow discard operations, I'm tempted to modify
mmc_calc_max_discard() as follows:
if (max_discard == 1)
max_discard = 0;
... but I'm not sure if that would be seen as a regression, since it'd
disable the discard operation completely on theoretically working (but
perhaps practically useless) systems.
Alternatively, perhaps I should replace:
if (mmc_can_trim(card)) {
max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
if (max_trim < max_discard)
max_discard = max_trim;
with:
if (mmc_can_trim(card)) {
max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
if (max_trim > 1 && max_trim < max_discard)
max_discard = max_trim;
Alternatively, should I install a quirk for the specific eMMC device,
which guards one of the changes above, or completely ignores the
TRIM_ARG result?
The eMMC device is question is:
manfid = 0x45
oemid = 0x100
name = SEM16G
Strangely, this is apparently a Sandisk eMMC device, yet there already
exist some quirks for a set of similarly named Sandisk devices, yet they
are triggered by manfid == 2, not 0x45. I'm not sure why Sandisk uses
two separate manufacturer IDs...
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH REPOST] mmc: tegra: add support for Tegra124
From: Stephen Warren
Tegra124's MMC controller is very similar to earlier SoC generations,
and can be supported by the same driver.
However, there are some non-backwards-compatible HW differences, and
hence a new DT compatible value must be used to describe the HW. This
patch updates the driver to support that new compatible value.
That said, the HW differences are only relevant when enabling certain
high-performance transfer modes. Since the driver is currently very
simple and doesn't enable those modes, we don't actually need to address
any of these HW differences in the code yet, hence the simple nature of
this patch.
Signed-off-by: Stephen Warren
Reviewed-by: Thierry Reding
---
drivers/mmc/host/sdhci-tegra.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 5b7b2eba8a54..a835898a68dd 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -198,6 +198,7 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = {
};
static const struct of_device_id sdhci_tegra_dt_match[] = {
+ { .compatible = "nvidia,tegra124-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra114-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra30-sdhci", .data = &soc_data_tegra30 },
{ .compatible = "nvidia,tegra20-sdhci", .data = &soc_data_tegra20 },
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: tegra: add support for Tegra124
From: Stephen Warren
Tegra124's MMC controller is very similar to earlier SoC generations,
and can be supported by the same driver.
However, there are some non-backwards-compatible HW differences, and
hence a new DT compatible value must be used to describe the HW. This
patch updates the driver to support that new compatible value.
That said, the HW differences are only relevant when enabling certain
high-performance transfer modes. Since the driver is currently very
simple and doesn't enable those modes, we don't actually need to address
any of these HW differences in the code yet, hence the simple nature of
this patch.
Signed-off-by: Stephen Warren
---
drivers/mmc/host/sdhci-tegra.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 5b7b2eba8a54..a835898a68dd 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -198,6 +198,7 @@ static struct sdhci_tegra_soc_data soc_data_tegra114 = {
};
static const struct of_device_id sdhci_tegra_dt_match[] = {
+ { .compatible = "nvidia,tegra124-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra114-sdhci", .data = &soc_data_tegra114 },
{ .compatible = "nvidia,tegra30-sdhci", .data = &soc_data_tegra30 },
{ .compatible = "nvidia,tegra20-sdhci", .data = &soc_data_tegra20 },
--
1.8.1.5
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Shutdown regression due to mmc workqueue/locking changes
When I run "shutdown -h now" in next-20131030, I get a hung task. git bisect shows that the first problem that shows up is due to 35a8fca "mmc: sdhci: Turn tuning timeout timer into delayed work", which yields the following crash immediately that poweroff happens: > [ 76.314514] BUG: scheduling while atomic: halt/1367/0x0002 > [ 76.320614] Modules linked in: > [ 76.323802] CPU: 2 PID: 1367 Comm: halt Not tainted > 3.12.0-rc2-00141-g35a8fca #147 > [ 76.331682] [] (unwind_backtrace+0x0/0xf8) from [] > (show_stack+0x10/0x14) > [ 76.340501] [] (show_stack+0x10/0x14) from [] > (dump_stack+0x80/0xc4) > [ 76.348860] [] (dump_stack+0x80/0xc4) from [] > (__schedule_bug+0x48/0x60) > [ 76.357559] [] (__schedule_bug+0x48/0x60) from [] > (__schedule+0x64c/0x6d0) > [ 76.366404] [] (__schedule+0x64c/0x6d0) from [] > (schedule_timeout+0x174/0x1ec) > [ 76.375552] [] (schedule_timeout+0x174/0x1ec) from [] > (wait_for_common+0xb8/0x14c) > [ 76.385074] [] (wait_for_common+0xb8/0x14c) from [] > (__flush_work+0xa4/0x12c) > [ 76.394078] [] (__flush_work+0xa4/0x12c) from [] > (sdhci_do_set_ios+0x90/0x6fc) > [ 76.403220] [] (sdhci_do_set_ios+0x90/0x6fc) from [] > (sdhci_set_ios+0x20/0x2c) > [ 76.412380] [] (sdhci_set_ios+0x20/0x2c) from [] > (mmc_power_off+0x70/0x90) > [ 76.421212] [] (mmc_power_off+0x70/0x90) from [] > (_mmc_suspend+0xb4/0x274) > [ 76.430037] [] (_mmc_suspend+0xb4/0x274) from [] > (mmc_bus_shutdown+0x40/0x68) > [ 76.439129] [] (mmc_bus_shutdown+0x40/0x68) from [] > (device_shutdown+0x34/0x188) > [ 76.448478] [] (device_shutdown+0x34/0x188) from [] > (kernel_power_off+0x2c/0x70) > [ 76.457814] [] (kernel_power_off+0x2c/0x70) from [] > (SyS_reboot+0x108/0x1d4) > [ 76.466812] [] (SyS_reboot+0x108/0x1d4) from [] > (ret_fast_syscall+0x0/0x30) A few commits later, 769303b "mmc: sdhci: Turn host->lock into a mutex" changes this to a hang, followed by a hung task being detected: > [ 240.870402] INFO: task kworker/1:1:70 blocked for more than 120 seconds. > [ 240.877203] Not tainted 3.12.0-rc7-next-20131030 #133 > [ 240.882886] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables > this message. > [ 240.890824] kworker/1:1D c0554ec00 70 2 0x > [ 240.897345] Workqueue: events sdhci_tuning_timeout_work > [ 240.902734] [] (__schedule+0x300/0x674) from [] > (schedule_preempt_disabled+0x24/0x34) > [ 240.912461] [] (schedule_preempt_disabled+0x24/0x34) from > [] (__mutex_lock_slowpath+0x17c/0x374) > [ 240.923146] [] (__mutex_lock_slowpath+0x17c/0x374) from > [] (mutex_lock+0xc/0x24) > [ 240.932430] [] (mutex_lock+0xc/0x24) from [] > (sdhci_tuning_timeout_work+0x14/0x2c) > [ 240.941890] [] (sdhci_tuning_timeout_work+0x14/0x2c) from > [] (process_one_work+0xfc/0x34c) > [ 240.952033] [] (process_one_work+0xfc/0x34c) from [] > (process_scheduled_works+0x24/0x34) > [ 240.961998] [] (process_scheduled_works+0x24/0x34) from > [] (worker_thread+0x228/0x384) > [ 240.971787] [] (worker_thread+0x228/0x384) from [] > (kthread+0xc4/0xe0) > [ 240.980160] [] (kthread+0xc4/0xe0) from [] > (ret_from_fork+0x14/0x3c) > [ 240.988366] INFO: task halt:1272 blocked for more than 120 seconds. > [ 240.994724] Not tainted 3.12.0-rc7-next-20131030 #133 > [ 241.000373] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables > this message. > [ 241.008273] halt D c0554ec00 1272 1270 0x > [ 241.014820] [] (__schedule+0x300/0x674) from [] > (schedule_timeout+0x174/0x1ec) > [ 241.023919] [] (schedule_timeout+0x174/0x1ec) from [] > (wait_for_common+0xb8/0x14c) > [ 241.033359] [] (wait_for_common+0xb8/0x14c) from [] > (__flush_work+0xa4/0x12c) > [ 241.042362] [] (__flush_work+0xa4/0x12c) from [] > (sdhci_do_set_ios+0x8c/0x6e8) > [ 241.051448] [] (sdhci_do_set_ios+0x8c/0x6e8) from [] > (sdhci_set_ios+0x20/0x2c) > [ 241.060553] [] (sdhci_set_ios+0x20/0x2c) from [] > (mmc_power_off+0x5c/0x7c) > [ 241.069275] [] (mmc_power_off+0x5c/0x7c) from [] > (_mmc_suspend+0x194/0x2a0) > [ 241.078110] [] (_mmc_suspend+0x194/0x2a0) from [] > (mmc_bus_shutdown+0x40/0x68) > [ 241.087339] [] (mmc_bus_shutdown+0x40/0x68) from [] > (device_shutdown+0x34/0x188) > [ 241.096627] [] (device_shutdown+0x34/0x188) from [] > (kernel_power_off+0x2c/0x70) > [ 241.105893] [] (kernel_power_off+0x2c/0x70) from [] > (SyS_reboot+0x108/0x1d4) > [ 241.114814] [] (SyS_reboot+0x108/0x1d4) from [] > (ret_fast_syscall+0x0/0x30) This is on the Tegra114 Dalmore board, which is an ARM system. Looking at the backtraces, I'd guess that all Tegra devices are affected, but haven't tested that. I haven't looked at the commits to see if there's anything obviously wrong, but perhaps you can suggest something to try? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdhci-esdhc-imx: Allow the usage of mmc aliases
On 09/22/2013 08:38 PM, Fabio Estevam wrote: > From: Fabio Estevam > > On embedded devices, there is often a combination of removable mmc > devices (e.g. MMC/SD cards) and hard wired ones (e.g. eMMC). > Depending on the hardware configuration, the 'mmcblkN' node might > change if the removable device is available or not at boot time. > > E.g. if the removable device is attached at boot time, it might > become mmxblk0. And the hard wired one mmcblk1. But if the removable > device isn't there at boot time, the hard wired one will become > mmcblk0. This makes it somehow difficult to hard code the root device > to the non-removable device and boot fast. > > Allow the sdhci-esdhc-imx driver to retrieve the mmc aliases, so that we can > map via the device tree which mmcblk corresponds to the rootfs. > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c > - devidx = find_first_zero_bit(dev_use, max_devices); > + devidx = find_next_zero_bit(dev_use, max_devices, card->host->devidx); I'm not sure if this works; what if the SD card gets detected/removed without recycling the device name/ID a few times before the fixed eMMC is detected? So, in the perfect case this will achieve what you want, but not in some unusual cases. So I don't think it's a good idea to rely on this. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding aliases to mmc
On 09/20/2013 10:37 AM, Dirk Behme wrote: > Am 20.09.2013 18:05, schrieb Stephen Warren: >> On 09/18/2013 11:22 PM, Dirk Behme wrote: >> ... >>> If you have an embedded system were you just care a little about boot >>> time you don't want to do anything like U-Boot's "part uuid" every time >>> you boot. Or even worse, you just have a minimalistic boot loader (e.g. >>> U-Boot's SPL) which doesn't know anything about UUIDs and file systems. >>> >>> As mentioned above, no I don't think UUIDs work for production embedded >>> systems. >> >> As I said above, whatever generates the filesystem image can easily >> embed the appropriate UUID in the system's boot scripts or bootloader >> environment. There's no need to run the "part" command at run-time if >> there's a more appropriate flow for your situation. > > Using a simple boot loader as an example, there are no boot scripts or > boot loader environment. The only thing the boot loader does is loading > the device tree and the kernel into RAM. Where do you want to embed an > UUID in such a product? I think the initrd would be typical. > To my understanding, the UUID is different for each SD card/eMMC, correct? Yes by default. Although for an embedded product with a fixed eMMC, there's no reason you couldn't make every device have the same UUID for the fixed device. Obviously you wouldn't want to do that for any removable device. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding aliases to mmc
On 09/20/2013 12:30 AM, Chaiken, Alison wrote: > Stephen Warren writes: >> Patches to make mmc block devices have static names have been proposed >> in the past and rejected. I think the main reason is that the block >> device names are (or can be) dynamic, so anything that assumes a >> particular naming scheme is simply broken. > > Why may network devices have static IPs and yet storage devices can't have > static names? Ethernet devices have MAC addresses as their unique ID. Block devices have PARTUUIDs and filesystems have UUIDs. The IP address is keyed to the MAC address. (In desktop distributions these days, I believe that MAC address typically drives device name via renaming, and then device name indexes the network configuration, which then determines the IP. Alternatively, MAC address drives IP address via DHCP). To my mind, using UUIDs as the key rather than device name is exactly the same situation, not a different one. ... > I understand that there is a widespread desire to institute a new level of > quality control in the device-tree, but if we are agreeing on a new rule, > let's be explicit about it. (please word-wrap your email) I don't believe this has anything at all to do with device tree; my first experience with this issue was long before DT. The issue is that block device IDs cannot be equal to controller IDs, since: * Mount an SD card -> assigned an ID at run-time * Physically unplug it -> ID kept since it's still referenced * Plug a new card back in -> assigned a new ID at run-time Therefore, there's no 1:1 match possible between SD/MMC controllers and SD/MMC block device names. It's dynamic. The same is true of USB mass storage devices too, I believe. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding aliases to mmc
On 09/18/2013 11:22 PM, Dirk Behme wrote: ... > If you have an embedded system were you just care a little about boot > time you don't want to do anything like U-Boot's "part uuid" every time > you boot. Or even worse, you just have a minimalistic boot loader (e.g. > U-Boot's SPL) which doesn't know anything about UUIDs and file systems. > > As mentioned above, no I don't think UUIDs work for production embedded > systems. As I said above, whatever generates the filesystem image can easily embed the appropriate UUID in the system's boot scripts or bootloader environment. There's no need to run the "part" command at run-time if there's a more appropriate flow for your situation. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] mmc: sdhci-bcm2835: Use sdhci_pltfm_unregister instead of open coded
On 09/17/2013 02:00 AM, Axel Lin wrote: > This avoid duplicated implementation. Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding aliases to mmc
On 09/18/2013 11:01 AM, Dirk Behme wrote: > Am 18.09.2013 17:17, schrieb Stephen Warren: >> On 09/17/2013 12:04 PM, Fabio Estevam wrote: >>> Hi Dirk, >>> >>> I have adapted your patch at: >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/111022.html >>> >>> >>> and tested it on 3.12-rc1 on a mx6qsabresd board. >>> >>> Do you have plans to submit it? Maybe as a RFC? >>> >>> It solves the mmcblkX order issue on my tests and it would be nice we >>> could have this problem addressed. >> >> Patches to make mmc block devices have static names have been proposed >> in the past and rejected. I think the main reason is that the block >> device names are (or can be) dynamic, so anything that assumes a >> particular naming scheme is simply broken. >> >> The correct solution is to use e.g. root=UUID=xxx or root=PARTUUID=xxx, >> or similar techniques for other filesystems (e.g. /etc/fstab) > > To my understanding this doesn't work if you need to have your rootfs on > a SD card/eMMC with different UUIDs on each board (SD card/eMMC)? That works fine. If you're using filesystem UUIDs (root=UUID=), then whatever generates your boot script (which is presumably stored in the filesystem you care about) needs to put the correct UUID into the script when the script is generated. This is presumably part of the install process, or part of a post kernel-install hook, just like generating grub.cfg is. If you're using partition UUIDs (root=PARTUUID=), then you can follow that same scheme too... ... or if using a recent U-Boot, run the "part uuid" command to determine the partition UUID at run-time, and use that to construct the kernel command-line. Any bootloader could implement equivalent functionality. In fact, any bootloader that can read ext* (or whatever filesystem) could in fact read the filesystem UUID too, and apply the same technique to root=UUID= too. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Adding aliases to mmc
On 09/17/2013 12:04 PM, Fabio Estevam wrote: > Hi Dirk, > > I have adapted your patch at: > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/111022.html > > and tested it on 3.12-rc1 on a mx6qsabresd board. > > Do you have plans to submit it? Maybe as a RFC? > > It solves the mmcblkX order issue on my tests and it would be nice we > could have this problem addressed. Patches to make mmc block devices have static names have been proposed in the past and rejected. I think the main reason is that the block device names are (or can be) dynamic, so anything that assumes a particular naming scheme is simply broken. The correct solution is to use e.g. root=UUID=xxx or root=PARTUUID=xxx, or similar techniques for other filesystems (e.g. /etc/fstab) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] arm: dts: socfpga: Add support for SD/MMC
On 09/14/2013 06:30 AM, Tomasz Figa wrote: ... > Just as a side note, correct name is Synopsys, not Synopsis. There are > multiple places around Documentation/devicetree where this typo is > present[1]. Should we consider correcting this or the typo will have > to stay? > > [1] git grep -i synopsis Documentation/devicetree/ > Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:* Samsung Exynos > specific extensions to the Synopsis Designware Mobile > Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:The Synopsis > designware mobile storage host controller is used to interface > Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:differences between > the core Synopsis dw mshc controller properties described > Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:by > synopsis-dw-mshc.txt and the properties used by the Samsung Exynos specific > Documentation/devicetree/bindings/mmc/exynos-dw-mshc.txt:extensions to the > Synopsis Designware Mobile Storage Host Controller. > Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt:* Rockchip > specific extensions to the Synopsis Designware Mobile > Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt:The Synopsis > designware mobile storage host controller is used to interface > Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt:differences > between the core Synopsis dw mshc controller properties described > Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt:by > synopsis-dw-mshc.txt and the properties used by the Rockchip specific > Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt:extensions to the > Synopsis Designware Mobile Storage Host Controller. > Documentation/devicetree/bindings/mmc/synopsis-dw-mshc.txt:* Synopsis > Designware Mobile Storage Host Controller > Documentation/devicetree/bindings/mmc/synopsis-dw-mshc.txt:The Synopsis > designware mobile storage host controller is used to interface > Documentation/devicetree/bindings/mmc/synopsis-dw-mshc.txt:properties used by > the Synopsis Designware Mobile Storage Host Controller. > Documentation/devicetree/bindings/mmc/synopsis-dw-mshc.txt: - > snps,dw-mshc: for controllers compliant with synopsis dw-mshc. > Documentation/devicetree/bindings/pci/designware-pcie.txt:* Synopsis > Designware PCIe interface Those typos only appear to exist in free-form text and not in any property names/values. As such, they can all easily be fixed without impacting any DT content or ABI. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] arm: dts: socfpga: Add support for SD/MMC
On 09/09/2013 09:33 AM, dingu...@altera.com wrote: > From: Dinh Nguyen > > Add bindings for SD/MMC for SOCFPGA. > diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt > b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt > +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile > + Storage Host Controller > + > +The Synopsis designware mobile storage host controller is used to interface > +a SoC with storage medium such as eMMC or SD/MMC cards. This file documents > +differences between the core Synopsis dw mshc controller properties described > +by synopsis-dw-mshc.txt and the properties used by the SOCFPGA specific > +extensions to the Synopsis Designware Mobile Storage Host Controller. > + > +Required Properties: > + > +* compatible: should be > +- "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA > + specific extensions. > + > +* samsung,dw-mshc-sdr-timing: See exynos-dw-mshc.txt for more information > about > + this binding. s/binding/property/ It's odd that this isn't "synopsis," rather than "samsung," if the property is generic across all/some uses of the Synopsis core. But, I guess this is fine. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 2/3] ARM: socfpga: dts: Add support for SD/MMC
On 08/23/2013 05:01 PM, Dinh Nguyen wrote:
> On Fri, 2013-08-23 at 16:29 -0600, Stephen Warren wrote:
>> On 08/23/2013 09:44 AM, dingu...@altera.com wrote:
>>> From: Dinh Nguyen
>>>
>>> Add bindings for SD/MMC for SOCFPGA.
>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>
>>> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is
>>> where
>>> + this where the register that controls the CIU clock phases
>>> + reside.
>>> +
>>> +* altr,ciu-clk-offset: The order of the cells should be:
>>> + - First Cell: Offset of the register in the system_mgr node that
>>> controls
>>> + the smpsel bits.
>>> + - Second Cell: Shift value of the drvsel bits.
>>> + - Third Cell: Shift value of the smpsel bits.
>>
>> This almost solves the issues I was thinking of. A few more thoughts though:
>>
>> * What if the sysmgr node has multiple reg entries. Is the offset cell
>> in altr,ciu-clk-offset an offset from the first reg entry, or across all
>> reg entries? It might be better to specify this as a reg index plus
>> offset, or allow the sysmgr node to define the format (#sysmgr-cells
>> perhaps).
>>
>> * What if the drvsel and smpsel bits are in different registers, even
>> different sysmgr blocks? Wouldn't it be better to have 2 separate
>> properties, each one defining the location of one bit-field?
>>
>> * bikeshed: altr,ciu-clk-offset isn't a great name; the value is more
>> than just an offset.
>>
>> Putting those together, I might expect the following properties:
>>
>> sysmgr: sysmgr {
>> /* binding for sysmgr node must specify what those 3 cells are */
>> #sysmgr-cells = <3>;
>> }
>>
>> dwmmc {
>> altr,drvsel-reg-field = <
>> &sysmgr /* sysmgr phandle */
>> 0 /* reg index */
>> 0 /* reg offset */
>> 0 /* field bit position */
>> 3 /* field bit size */>;
>> altr,smpsel-reg-field = <
>> &sysmgr /* sysmgr phandle */
>> 0 /* reg index */
>> 0 /* reg offset */
>> 3 /* field bit position */
>> 3 /* field bit size */>;
>> };
>>
>> That would allow the whole sysmgr concept to be completely generic.
>>
>> But, this is a bit like representing raw register I/O in DT, which has
>> been frowned upon in the past.
>>
>> Finally, what if the values for drvsel, smpsel are different in
>> different sysmgr implementations? Do you need a property that defines
>> that values?
>>
>> Another option might be to define a semantic API between the two, such
>> that you only need a sysmgr=<&sysmgr> property, yet the driver for the
>> sysmgr node exposes a function sysmgr_set_dwmmc_drvsel_smpsel(struct
>> device_node *sysmgr_node, uint drvsel, uint smpsel); Now, the sysmgr
>> driver would have to implement that on any SoC that supported a dwmmc.
>
> I was trying to avoid adding a driver for the sysmgr, as it really does
> not represent any type of device. It is a merely a register region with
> miscellaneous registers that controls other IPs in the SOC.
>
> I'm thinking perhaps I can set this register in the arch specific file,
> then the SD/MMC driver would not need to bother with it at all?
The problem with hard-coding this in the MMC driver is that it makes the
MMC driver depend on information outside the MMC HW block. If you do
that, you may as well just write the "syscon" registers directly in the
MMC driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 2/3] ARM: socfpga: dts: Add support for SD/MMC
On 08/23/2013 09:44 AM, dingu...@altera.com wrote:
> From: Dinh Nguyen
>
> Add bindings for SD/MMC for SOCFPGA.
> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
> + this where the register that controls the CIU clock phases
> + reside.
> +
> +* altr,ciu-clk-offset: The order of the cells should be:
> + - First Cell: Offset of the register in the system_mgr node that
> controls
> + the smpsel bits.
> + - Second Cell: Shift value of the drvsel bits.
> + - Third Cell: Shift value of the smpsel bits.
This almost solves the issues I was thinking of. A few more thoughts though:
* What if the sysmgr node has multiple reg entries. Is the offset cell
in altr,ciu-clk-offset an offset from the first reg entry, or across all
reg entries? It might be better to specify this as a reg index plus
offset, or allow the sysmgr node to define the format (#sysmgr-cells
perhaps).
* What if the drvsel and smpsel bits are in different registers, even
different sysmgr blocks? Wouldn't it be better to have 2 separate
properties, each one defining the location of one bit-field?
* bikeshed: altr,ciu-clk-offset isn't a great name; the value is more
than just an offset.
Putting those together, I might expect the following properties:
sysmgr: sysmgr {
/* binding for sysmgr node must specify what those 3 cells are */
#sysmgr-cells = <3>;
}
dwmmc {
altr,drvsel-reg-field = <
&sysmgr /* sysmgr phandle */
0 /* reg index */
0 /* reg offset */
0 /* field bit position */
3 /* field bit size */>;
altr,smpsel-reg-field = <
&sysmgr /* sysmgr phandle */
0 /* reg index */
0 /* reg offset */
3 /* field bit position */
3 /* field bit size */>;
};
That would allow the whole sysmgr concept to be completely generic.
But, this is a bit like representing raw register I/O in DT, which has
been frowned upon in the past.
Finally, what if the values for drvsel, smpsel are different in
different sysmgr implementations? Do you need a property that defines
that values?
Another option might be to define a semantic API between the two, such
that you only need a sysmgr=<&sysmgr> property, yet the driver for the
sysmgr node exposes a function sysmgr_set_dwmmc_drvsel_smpsel(struct
device_node *sysmgr_node, uint drvsel, uint smpsel); Now, the sysmgr
driver would have to implement that on any SoC that supported a dwmmc.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC
On 08/21/2013 01:48 PM, Dinh Nguyen wrote: > On Fri, 2013-08-16 at 16:36 -0600, Stephen Warren wrote: >> On 08/14/2013 10:48 AM, dingu...@altera.com wrote: >>> From: Dinh Nguyen >>> >>> Add bindings for SD/MMC for SOCFPGA. >> >>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >> >>> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is >>> where >>> + this where the register that controls the CIU clock phases >>> + reside. >> >> On the surface, this binding series seems OK, but I do have a question: >> how is the sysmgr phandle used? >> >> I assume there's some register in this syscon device that resets or >> enables or otherwise controls this MSHC module. How does the code know >> which register it is? The phandle in the altr,sysmgr property would >> usually be followed by a/some cell(s) that encode this information, so >> that the MSHC driver doesn't have to know anything about the layout of >> the syscon registers, and so the sysconf driver doesn't have to know >> anything about the identity of the MSHC client device. > > There is a #define SYSMGR_SDMMCGRP_CTRL_OFFSET that is in > dw_mmc-socfpga.c. This defines the offset from the base address that the > sysmgr phandle will give me. Hmmm. That doesn't sound good. That means that the SDMMC driver knows internal details about some other HW module. It'd be better if either: a) The sysmgr driver was required to provide an API to the SDMMC driver to set up the CIU register as requested. or: b) The CIU register details were represented in DT. Either of these would allow the SDMMC driver to operate unchanged on an SoC with a different sysmgr register layout. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC
On 08/14/2013 10:48 AM, dingu...@altera.com wrote: > From: Dinh Nguyen > > Add bindings for SD/MMC for SOCFPGA. > diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt > b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt > +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where > + this where the register that controls the CIU clock phases > + reside. On the surface, this binding series seems OK, but I do have a question: how is the sysmgr phandle used? I assume there's some register in this syscon device that resets or enables or otherwise controls this MSHC module. How does the code know which register it is? The phandle in the altr,sysmgr property would usually be followed by a/some cell(s) that encode this information, so that the MSHC driver doesn't have to know anything about the layout of the syscon registers, and so the sysconf driver doesn't have to know anything about the identity of the MSHC client device. That way, the MSHC driver will work fine if a HW designer has dropped the MSHC IP block into a completely different SoC with a different syscon register layout. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: dw_mmc-exynos: Common bindings for dw-mshc timing
On 08/13/2013 11:29 PM, Olof Johansson wrote: > On Mon, Aug 12, 2013 at 05:15:01PM -0600, Stephen Warren wrote: >> On 08/08/2013 04:55 PM, dingu...@altera.com wrote: >>> From: Dinh Nguyen >>> >>> Remove the "samsung" in "samsung,dw-mshc-ciu-div", >>> "samsung,dw-mshc-sdr-timing", >>> and "samsung,dw-mshc-ddr-timing". These characteristics are not applicable >>> to >>> just Samsung platforms, but to any platform that uses the Synopsis SD/MMC >>> IP. >> >> But the properties aren't globally standard; they apply specifically >> anywhere that this particular Synopsis IP is present. As such, I think >> at least a "synopsis," prefix is warranted. > > A prefix is only really needed when a vendor is making a vendor-specific > extension on a shared binding. Since the whole binding is > synopsis-specific, I don't think there's need for a prefix here. Oh! That definitely isn't the rule that's been applied before; I've been asked to add "nvidia," prefixes to properties in entirely custom bindings... -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] ARM: socfpga: dts: Add support for SD/MMC
On 08/13/2013 02:17 PM, Dinh Nguyen wrote:
> On Tue, 2013-08-13 at 14:10 -0600, Stephen Warren wrote:
>> On 08/13/2013 01:58 PM, Dinh Nguyen wrote:
>>> On Tue, 2013-08-13 at 13:52 -0600, Stephen Warren wrote:
>>>> On 08/12/2013 09:49 AM, dingu...@altera.com wrote:
>>>>> From: Dinh Nguyen
>>>>>
>>>>> Add bindings for SD/MMC for SOCFPGA.
>>>>> Add "syscon" to the "altr,sys-mgr" binding.
>>
>>>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi
>>>>> b/arch/arm/boot/dts/socfpga.dtsi
>>>>
>>>>> sysmgr@ffd08000 {
>>>>> - compatible = "altr,sys-mgr";
>>>>> + compatible = "altr,sys-mgr", "syscon";
>>>>
>>>> That seems like an unrelated change?
>>>
>>> This patch is to enable SD/MMC for SOCFPGA. The "syscon" is needed in
>>> dw_mmc-socfpga.c as the clock phases for the SD IP is controlled by
>>> registers in "altr,sys-mgr".
>>
>> But nothing in the dwmmc nodes refers to the syscon node, so how can the
>> syscon driver possibly provide services to the MMC driver; the MMC
>> driver won't be able to find it. I hope there's no back-door assumption
>> that some syscon exists, and that there's only one of them...
>
> In dw_mmc-socfpga.c:
>
> priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>
> then, uses regmap_write() to set the correct values from
> "altr,dw-mshc-sdr-timing".
Uggh. There should really be a phandle from the MMC node to the syscon
node to specify the connectivity. What if there ends up being 2 HW
modules that are sys-mgr?
At the very least, the binding for any device that's affected by the
sys-mgr should specify that some other node must exist with that
compatible value, to provide XXX services for it.
But, I wonder if the altr,dw-mshc-sdr-timing property shouldn't be part
of the syscon node itself?
(Probably best to ignore my ack for now given this new information)
> This was a suggestion from Arnd. It seems that this part of patch is
> causing alot of confusion, I should just split the patches out?
A separate patch to change that compatible value probably would make
sense; by the sound of it, that change would also be required for a lot
of other devices on the SoC; it's not just specific to MMC?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] ARM: socfpga: dts: Add support for SD/MMC
On 08/13/2013 01:58 PM, Dinh Nguyen wrote:
> On Tue, 2013-08-13 at 13:52 -0600, Stephen Warren wrote:
>> On 08/12/2013 09:49 AM, dingu...@altera.com wrote:
>>> From: Dinh Nguyen
>>>
>>> Add bindings for SD/MMC for SOCFPGA.
>>> Add "syscon" to the "altr,sys-mgr" binding.
>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>
>>> sysmgr@ffd08000 {
>>> - compatible = "altr,sys-mgr";
>>> + compatible = "altr,sys-mgr", "syscon";
>>
>> That seems like an unrelated change?
>
> This patch is to enable SD/MMC for SOCFPGA. The "syscon" is needed in
> dw_mmc-socfpga.c as the clock phases for the SD IP is controlled by
> registers in "altr,sys-mgr".
But nothing in the dwmmc nodes refers to the syscon node, so how can the
syscon driver possibly provide services to the MMC driver; the MMC
driver won't be able to find it. I hope there's no back-door assumption
that some syscon exists, and that there's only one of them...
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] ARM: socfpga: dts: Add support for SD/MMC
On 08/12/2013 09:49 AM, dingu...@altera.com wrote:
> From: Dinh Nguyen
>
> Add bindings for SD/MMC for SOCFPGA.
> Add "syscon" to the "altr,sys-mgr" binding.
> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
I think it's reasonable to define one binding as being based on another
plus some additions, so, the binding,
Acked-by: Stephen Warren
> +Example:
> + dwmmc0@ff704000 {
> + compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
> + altr,dw-mshc-sdr-timing = <0 3>;
> + };
It'd be nice to provide a complete example though, rather than only
including the properties that this binding adds to the base binding.
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> sysmgr@ffd08000 {
> - compatible = "altr,sys-mgr";
> + compatible = "altr,sys-mgr", "syscon";
That seems like an unrelated change?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: dw_mmc-exynos: Common bindings for dw-mshc timing
On 08/08/2013 04:55 PM, dingu...@altera.com wrote: > From: Dinh Nguyen > > Remove the "samsung" in "samsung,dw-mshc-ciu-div", > "samsung,dw-mshc-sdr-timing", > and "samsung,dw-mshc-ddr-timing". These characteristics are not applicable to > just Samsung platforms, but to any platform that uses the Synopsis SD/MMC IP. But the properties aren't globally standard; they apply specifically anywhere that this particular Synopsis IP is present. As such, I think at least a "synopsis," prefix is warranted. And the driver should probably support both the old and new property names for backwards-compatibility? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] ARM: socfpga: dts: Add support for SD/MMC
On 08/09/2013 04:41 PM, Dinh Nguyen wrote: > On Fri, 2013-08-09 at 15:00 -0600, Stephen Warren wrote: >> On 08/08/2013 05:10 PM, Dinh Nguyen wrote: >>> On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote: >> ... >>>> Why is there a need to directly represent the divider anywhere? The >>>> driver can find the rate of the input clock, and I assume it knows what >>>> rate it wants the clock to run at, so can't it calculate the divider >>>> based on those two pieces of information? >>> >>> CC: Chris Ball >>>> >>>> Or, does the driver really not know what rate it wants the clock to be >>>> after the internal divider? If not, then I think that *rate* should be >>>> recorded in DT, not the divider to obtain that rate. >>>> >>> >>> I believe that this is the case, that the driver does not know what rate >>> it will clock the SD card at. I think internally we did have a "bus_hz" >>> in DT a while back. I guess I don't really understand why it would be >>> better to have a *rate* DT entry instead of a fixed-divider entry? >> >> The value of the divider depends on two things: >> >> 1) Input clock rate. >> 2) Desired rate after applying the internal divider. >> >> The input clock rate may vary, either between SoCs the IP is integrated >> into, or even at run-time perhaps base on clk_set_rate() etc. >> >> If the MMC driver knows the clock rate it wants to run at, it can >> calculate the divider easily; it can automatically adjust to any input >> clock that its environment may provide. >> >> If the MMC driver is simply told "use this divider", that's encoding >> assumptions about the rate of the input clock which might not be valid;. >> Encoding the desire clock rate within the MMC HW block allows the >> divider to be calculated based on the actual environment. > > Thanks Stephen. That makes sense. > > Chris, Jaehoon, and Seungwon, do you have any inputs? If not I will go > down the path of have the "bus-hz" in the DTS node for the clock rate of > the CIU clock. Then I would also make the same change to dw_mmc-exynos > but would need your help on what the rate would be. > > For SOCFPGA, its 12.5 MHZ. I'm not sure "bus-hz" is the right name; it's not clear from that name that it refers to an internal clock inside the MMC controller rather than the SD bus out to the card itself. Perhaps "controller-clock-hz" or something like that? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] ARM: socfpga: dts: Add support for SD/MMC
On 08/08/2013 05:10 PM, Dinh Nguyen wrote: > On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote: ... >> Why is there a need to directly represent the divider anywhere? The >> driver can find the rate of the input clock, and I assume it knows what >> rate it wants the clock to run at, so can't it calculate the divider >> based on those two pieces of information? > > CC: Chris Ball >> >> Or, does the driver really not know what rate it wants the clock to be >> after the internal divider? If not, then I think that *rate* should be >> recorded in DT, not the divider to obtain that rate. >> > > I believe that this is the case, that the driver does not know what rate > it will clock the SD card at. I think internally we did have a "bus_hz" > in DT a while back. I guess I don't really understand why it would be > better to have a *rate* DT entry instead of a fixed-divider entry? The value of the divider depends on two things: 1) Input clock rate. 2) Desired rate after applying the internal divider. The input clock rate may vary, either between SoCs the IP is integrated into, or even at run-time perhaps base on clk_set_rate() etc. If the MMC driver knows the clock rate it wants to run at, it can calculate the divider easily; it can automatically adjust to any input clock that its environment may provide. If the MMC driver is simply told "use this divider", that's encoding assumptions about the rate of the input clock which might not be valid;. Encoding the desire clock rate within the MMC HW block allows the divider to be calculated based on the actual environment. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] ARM: socfpga: dts: Add support for SD/MMC
On 08/08/2013 02:54 PM, Dinh Nguyen wrote: > On Thu, 2013-08-08 at 14:37 -0600, Stephen Warren wrote: >> On 08/08/2013 02:32 PM, Dinh Nguyen wrote: >>> On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote: >>>> On 08/05/2013 02:43 PM, dingu...@altera.com wrote: >>>>> From: Dinh Nguyen >>>>> >>>>> Add bindings for SD/MMC for SOCFPGA. >>>>> Add "syscon" to the "altr,sys-mgr" binding. >>>> >>>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>>>> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>>>> new file mode 100644 >>>>> index 000..dc14922 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>>>> @@ -0,0 +1,48 @@ >>>>> +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile >>>>> + Storage Host Controller >>>>> + >>>>> +Required Properties: >>>>> + >>>>> +* compatible: should be >>>>> + - "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA >>>>> + specific extensions. >>>>> + >>>>> +* altr,dw-mshc-ciu-div: Specifies the divider value for the card >>>>> interface >>>>> + unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the >>>>> divider >>>>> + value is fixed at 3, which means parent_clock/4. >>>> >>>> This feels like something that should be represented using the common >>>> clock API; a driver should query the rate of its input clock, and then >>>> calculate the MMC block's internal divider based on that (perhaps also >>>> call clk_set_rate() on the input clock?). >>> >>> This means a change to the dw_mmc driver, which I can look into for the >>> next round? I have promised Pawel to consolidate the bindings for both >>> exynos and socfpga in the next round already. I will also look into >>> using the common clock API for the MMC as well. >>> >>> This patch is the only thing that is preventing from SD/MMC working for >>> SOCFPGA in the mainline, can I get your Ack if I look into doing this >>> for 3.13 for both the exynos and socfpga driver, and address your latter >>> comments? >> >> The problem is that if the binding supports or requires that property >> now, it has to at least support it forever. Given that we're getting >> serious about DT ABI now, we should be only introducing DT bindings that >> we believe are complete and corrrect, rather than bindings which we >> actively expect to be temporary and to change incompatibly later. >> > Right ok. Then I guess I will have to look into consolidating the > bindings sooner rather than later. > > I went back to look at the driver again, and I think it is doing as you > are suggesting: > > host->bus_hz is getting its input value from the common clock API. > "altr,dw-mshc-ciu-div" is specifying the internal divider that is in the > SD/MMC IP itself. > > I guess I'm still not clear on how I can represent the SD/MMC divider in > in the context of the common clock API. Why is there a need to directly represent the divider anywhere? The driver can find the rate of the input clock, and I assume it knows what rate it wants the clock to run at, so can't it calculate the divider based on those two pieces of information? Or, does the driver really not know what rate it wants the clock to be after the internal divider? If not, then I think that *rate* should be recorded in DT, not the divider to obtain that rate. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] ARM: socfpga: dts: Add support for SD/MMC
On 08/08/2013 02:32 PM, Dinh Nguyen wrote: > On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote: >> On 08/05/2013 02:43 PM, dingu...@altera.com wrote: >>> From: Dinh Nguyen >>> >>> Add bindings for SD/MMC for SOCFPGA. >>> Add "syscon" to the "altr,sys-mgr" binding. >> >>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>> new file mode 100644 >>> index 000..dc14922 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt >>> @@ -0,0 +1,48 @@ >>> +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile >>> + Storage Host Controller >>> + >>> +Required Properties: >>> + >>> +* compatible: should be >>> + - "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA >>> + specific extensions. >>> + >>> +* altr,dw-mshc-ciu-div: Specifies the divider value for the card interface >>> + unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the >>> divider >>> + value is fixed at 3, which means parent_clock/4. >> >> This feels like something that should be represented using the common >> clock API; a driver should query the rate of its input clock, and then >> calculate the MMC block's internal divider based on that (perhaps also >> call clk_set_rate() on the input clock?). > > This means a change to the dw_mmc driver, which I can look into for the > next round? I have promised Pawel to consolidate the bindings for both > exynos and socfpga in the next round already. I will also look into > using the common clock API for the MMC as well. > > This patch is the only thing that is preventing from SD/MMC working for > SOCFPGA in the mainline, can I get your Ack if I look into doing this > for 3.13 for both the exynos and socfpga driver, and address your latter > comments? The problem is that if the binding supports or requires that property now, it has to at least support it forever. Given that we're getting serious about DT ABI now, we should be only introducing DT bindings that we believe are complete and corrrect, rather than bindings which we actively expect to be temporary and to change incompatibly later. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] ARM: socfpga: dts: Add support for SD/MMC
On 08/05/2013 02:43 PM, dingu...@altera.com wrote:
> From: Dinh Nguyen
>
> Add bindings for SD/MMC for SOCFPGA.
> Add "syscon" to the "altr,sys-mgr" binding.
> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> new file mode 100644
> index 000..dc14922
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> @@ -0,0 +1,48 @@
> +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile
> + Storage Host Controller
> +
> +Required Properties:
> +
> +* compatible: should be
> + - "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA
> + specific extensions.
> +
> +* altr,dw-mshc-ciu-div: Specifies the divider value for the card interface
> + unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the
> divider
> + value is fixed at 3, which means parent_clock/4.
This feels like something that should be represented using the common
clock API; a driver should query the rate of its input clock, and then
calculate the MMC block's internal divider based on that (perhaps also
call clk_set_rate() on the input clock?).
> +Example:
> + dwmmc0@ff704000 {
> + compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
> + reg = <0xff704000 0x1000>;
> + interrupts = <0 139 4>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + num-slots = <1>;
> + supports-highspeed;
> + fifo-depth = <0x400>;
Those properties aren't defined in this document anywhere. I guess this
binding is meant to "inherit" from that described in
"synopsis-dw-mshc.txt"? If so, that should be stated explicitly.
A similar comment applies to the clocks properties in the *.dtsi changes.
> + altr,dw-mshc-ciu-div = <3>;
> + altr,dw-mshc-sdr-timing = <0 3>;
Indentation issue.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: add Device Tree properties for UHS modes
On 07/29/2013 12:20 AM, Guennadi Liakhovetski wrote: > Hi Stephen > > On Fri, 26 Jul 2013, Stephen Warren wrote: > >> On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote: >>> Hi Stephen >>> >>> On Fri, 26 Jul 2013, Stephen Warren wrote: >>> >>>> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote: >>>>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and >>>>> for supported by the host in DDR mode VccQ values. Adding them to DT will >>>>> automatically enable respective MMC host capabilities. >>>> >>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt >>>>> b/Documentation/devicetree/bindings/mmc/mmc.txt >>>> >>>>> +- uhs-sdr12: the host supports UHS SDR12 mode >>>>> +- uhs-sdr25: the host supports UHS SDR25 mode >>>>> +- uhs-sdr50: the host supports UHS SDR50 mode >>>>> +- uhs-sdr104: the host supports UHS SDR104 mode >>>>> +- uhs-ddr50: the host supports UHS DDR50 mode >>>>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ >>>>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ >>>> >>>> Surely the driver for the host controller already knows this, so there's >>>> no need to represent it in DT? >>> >>> Not necessarily. One driver can handle several versions of the same chip, >>> and some versions of the chip implement some of those features, others >>> don't. >> >> Certainly. >> >>> So, your choice is really whether to specify a chip version in the >>> compatible string or these properties. >> >> There's no choice there. The compatible property needs to specify all of >> the following: >> >> * A value which describes the exact chip the IP block is in. This can be >> matched on to enable any quirks that need to be handled due to >> integration of the IP into the particular chip. This value will list the >> chip version. An example might be tegra20-sdhci. >> >> * A value which describes the IP block version (if that IP block has a >> version independent of the SoC that contains it, as e.g. a Synopsis IP >> block might). A totally made-up example might be synopsis-dwc2-1.0.0 >> >> * Various more generic values that describe the HW, and that define a n >> interface to the HW that is specific and complete enough that HW can >> program to. An example might be usb-ehci (assuming a chip that doesn't >> have so many quirks that a driver has to know more than just "it's an >> EHCI controller). > > Yes, all these certainly make sense. As far as I understand, we are still > in the process of defining good clear rules for DTs, there is an "ABI" > discussion currently running on ALKML and IIRC this is also going to be a > topic for one of coming conferences. So, hopefully we're approaching a > state of a greater clarity about DT. I don't think the rules for the compatible property are up in the air; they've been pretty well communicated since at least the start of my involvement in DT perhaps 2 years ago. >> Further classes of compatible value might exist, but you get the idea. >> >> All of those values have to exist in the DT right from the very start, >> so that the first DT is usable with a future kernel, which might decide >> to vary the exact compatible value(s) it matches on, provided they're >> all documented in the DT binding ABI, in order to enable/disable new >> sets of quirsk. > > Makes sense too, sure. > >>> Now, when you consider that >>> multiple drivers have to decide upon those, and sometimes you don't have >>> an exact IP version of the SD/MMC controller but only the SoC version, you >>> choice becomes: >> >> That would be a bug in the DT, given my assertions above. > > Sorry, what exactly would be a bug? Not having an exact IP version? Yes. > But > you did write above - "if that IP block has a version independent of the > SoC that contains it." In my case it doesn't really. Those IPs only exist > within those SoCs. I think this has been well covered in the rest of the thread, but if the IP block doesn't have a version independent from the SoC itself (because the SoC is developed as a whole, rather than being pieced together out of a pool of IP blocks for example), then the IP block's version *is* the SoC version. In my list of "sources" of compatible values above, it's possible that the values for multiple of those bullet p
Re: [PATCH 1/2] mmc: add Device Tree properties for UHS modes
On 07/29/2013 07:30 AM, Guennadi Liakhovetski wrote: > On Mon, 29 Jul 2013, Pawel Moll wrote: > >> On Mon, 2013-07-29 at 13:05 +0100, Guennadi Liakhovetski wrote: >>> No, that's exactly the problem. We absolutely do not want to write >>> compatible="vendor,soc-v1"; in a .dts file for SoC v2 just because we >>> currently think, that we don't have to distinguish between those SoC >>> versions in this specific driver. I think we all know it quite well, that >>> there are (practically) always differences - sometimes documented, >>> sometimes undocumented. And if you later find out, that you do have to >>> differentiate in the driver - it's too late. Even if we disregard the >>> argument of ugliness of having to set compatibility with soc-v1, soc-v2, >>> soc-v3 in different DT nodes on an SoC v4. >> >> First of all I think your example calls for more than one compatible >> string - if it seems that soc,v2 is almost like soc,v1, make it >> compatible = "soc-v2", "soc-v1" and don't touch the driver (as in: keep >> it compatible with "soc-v1" only). Then, when the realisation comes, you >> can simply add the "soc-v2" of_device_id with .data pointing at new >> features. > > Yes, this is also a possibility, but it seems only a little bit better. > Why should a DT node on SoC vN have a compatible string with SoC vK for > some random for an abstract user absolutely unrelated SoC version?... Precisely because the HW /is/ compatible. If the HW interface (registers) in two chips A and B is such that a driver for chip A can be expected to run unmodified on chip B too (albeit perhaps without yet supporting any new features, or perhaps not running at full performance etc.), then chip B /is/ compatible with chip A, and it makes sense to mark it so in DT. That way, an old kernel which had support for chip A can run on chip B. A newer kernel might come along later with explicit support for chip B, and hence run faster and/or expose new features, but the driver for chip A can still work on chip B. > And > that vK would be different for different devices... So on SoC v5 MMC can > be compatible with with v1, sound with v2, camera with v3... Don't you > think it would look like a mess? I don't consider that a mess, no. It's nice evolutionary HW design:-) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: add Device Tree properties for UHS modes
On 07/26/2013 02:23 PM, Guennadi Liakhovetski wrote: > Hi Stephen > > On Fri, 26 Jul 2013, Stephen Warren wrote: > >> On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote: >>> Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and >>> for supported by the host in DDR mode VccQ values. Adding them to DT will >>> automatically enable respective MMC host capabilities. >> >>> diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt >>> b/Documentation/devicetree/bindings/mmc/mmc.txt >> >>> +- uhs-sdr12: the host supports UHS SDR12 mode >>> +- uhs-sdr25: the host supports UHS SDR25 mode >>> +- uhs-sdr50: the host supports UHS SDR50 mode >>> +- uhs-sdr104: the host supports UHS SDR104 mode >>> +- uhs-ddr50: the host supports UHS DDR50 mode >>> +- ddr-1v2: the host can support DDR, using 1.2V VccQ >>> +- ddr-1v8: the host can support DDR, using 1.8V VccQ >> >> Surely the driver for the host controller already knows this, so there's >> no need to represent it in DT? > > Not necessarily. One driver can handle several versions of the same chip, > and some versions of the chip implement some of those features, others > don't. Certainly. > So, your choice is really whether to specify a chip version in the > compatible string or these properties. There's no choice there. The compatible property needs to specify all of the following: * A value which describes the exact chip the IP block is in. This can be matched on to enable any quirks that need to be handled due to integration of the IP into the particular chip. This value will list the chip version. An example might be tegra20-sdhci. * A value which describes the IP block version (if that IP block has a version independent of the SoC that contains it, as e.g. a Synopsis IP block might). A totally made-up example might be synopsis-dwc2-1.0.0 * Various more generic values that describe the HW, and that define a n interface to the HW that is specific and complete enough that HW can program to. An example might be usb-ehci (assuming a chip that doesn't have so many quirks that a driver has to know more than just "it's an EHCI controller). Further classes of compatible value might exist, but you get the idea. All of those values have to exist in the DT right from the very start, so that the first DT is usable with a future kernel, which might decide to vary the exact compatible value(s) it matches on, provided they're all documented in the DT binding ABI, in order to enable/disable new sets of quirsk. > Now, when you consider that > multiple drivers have to decide upon those, and sometimes you don't have > an exact IP version of the SD/MMC controller but only the SoC version, you > choice becomes: That would be a bug in the DT, given my assertions above. > either _standard_ _common_ properties as above, or > compatibility strings virtually in each driver for each SoC version. My preference would certainly be to derive the data from compatible strings here. I'd prefer more that DT represent board differences rather than SoC differences; drivers can encompass the SoC differences with minimal code in general. Related, is it really true that zero driver involvement is required to enable these UHS features? If absolutely any HW can enable them without any driver support at all, then perhaps it's still reasonable to create DT properties to enable them. However, if driver support is required to make those features actually work, the driver had better be validating that support actually works on the HW it's running on before enabling the feature (and can therefore pass the validation results to the SD core rather than relying on DT properties to be set). I'm pretty sure that our downstream Tegra SDHCI driver contains a lot of code to make UHS actually work, even though the HW does support it, and hence with this patch applied, the DT would request that it be enabled. > That's why I decided to use explicit properties for those. Example: > sh_mmcif driver supports MMCIF IP blocks on various Renesas sh-, r-mobile > and r-car SuperH and ARM SoCs. On some of them DDR50 is supported, on > others it isn't. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: socfpga: dts: Add support for SD/MMC
On 07/26/2013 02:44 PM, Dinh Nguyen wrote:
> On Fri, 2013-07-26 at 14:02 -0600, Stephen Warren wrote:
>> On 07/26/2013 01:33 PM, Dinh Nguyen wrote:
>>> On Fri, 2013-07-26 at 11:24 -0600, Stephen Warren wrote:
>>>> On 07/25/2013 04:04 PM, dingu...@altera.com wrote:
>>>>> From: Dinh Nguyen
>>>>>
>>>>> Add bindings for SD/MMC for SOCFPGA.
>>>>> Add "syscon" to the "altr,sys-mgr" binding.
>>
>>>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>>>> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>
>>>>> +Example:
>>>>> +
>>>>> + The MSHC controller node can be split into two portions, SoC specific
>>>>> and
>>>>> + board specific portions, as listed below.
>>>>
>>>> That doesn't sound like a good idea. There should be one DT node for
>>>> each logical block. The internal construction of the Linux drivers
>>>> (presumably you have entirely separate code to handle the two nodes in
>>>> Linux so far?) should not influence the DT construction at all.
>>>
>>> In the end, there is only 1 DT node for each logical block:
>>
>> Oh right, I see you were intending to show the distinction between the
>> SoC .dtsi and board .dts file. I hadn't realized that. I don't think
>> it's common to do that in the examples, so I would recommend just
>> merging the whole example together myself.
>
> I'll merge it.
>
>>
>>> dwmmc0@ff704000 {
>>> compatible = "altr,socfpga-dw-mshc";
>>
>> That should include the baseline synopsis compatible value too.
>
> We don't need the baseline synopsis compatible because of
> dw_mci_pltfm_register() call.
It's not a matter of whether it's strictly necessary for the SW to work
right now. The compatible property should include entries for everything
that the HW is actually compatible with.
Of course, if a plain driver for the raw synopsis controller/binding
wouldn't actually work on this HW at all, without explicit knowledge of
the extra details of the more HW-specific binding, then that's a good
argument for leaving it out of compatible.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: socfpga: dts: Add support for SD/MMC
On 07/26/2013 01:33 PM, Dinh Nguyen wrote:
> On Fri, 2013-07-26 at 11:24 -0600, Stephen Warren wrote:
>> On 07/25/2013 04:04 PM, dingu...@altera.com wrote:
>>> From: Dinh Nguyen
>>>
>>> Add bindings for SD/MMC for SOCFPGA.
>>> Add "syscon" to the "altr,sys-mgr" binding.
>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
>>> +Example:
>>> +
>>> + The MSHC controller node can be split into two portions, SoC specific and
>>> + board specific portions, as listed below.
>>
>> That doesn't sound like a good idea. There should be one DT node for
>> each logical block. The internal construction of the Linux drivers
>> (presumably you have entirely separate code to handle the two nodes in
>> Linux so far?) should not influence the DT construction at all.
>
> In the end, there is only 1 DT node for each logical block:
Oh right, I see you were intending to show the distinction between the
SoC .dtsi and board .dts file. I hadn't realized that. I don't think
it's common to do that in the examples, so I would recommend just
merging the whole example together myself.
> dwmmc0@ff704000 {
> compatible = "altr,socfpga-dw-mshc";
That should include the baseline synopsis compatible value too.
> reg = <0xff704000 0x1000>;
> interrupts = <0x 0x008b 0x0004>;
> fifo-depth = <0x0400>;
> #address-cells = <0x0001>;
> #size-cells = <0x>;
> clocks = <0x0016 0x0017>;
> clock-names = "biu", "ciu";
> num-slots = <0x0001>;
> supports-highspeed;
> broken-cd;
> altr,dw-mshc-ciu-div = <0x0003>;
> altr,dw-mshc-sdr-timing = <0x 0x0003>;
> slot@0 {
> reg = <0x>;
> bus-width = <0x0004>;
> };
> };
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: socfpga: dts: Add support for SD/MMC
On 07/25/2013 04:04 PM, dingu...@altera.com wrote:
> From: Dinh Nguyen
>
> Add bindings for SD/MMC for SOCFPGA.
> Add "syscon" to the "altr,sys-mgr" binding.
> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile
> + Storage Host Controller
If these are extensions to an existing binding, it feels like the
documentation should be part of that binding, although suppose it's
reasonable to create a HW-specific "sub-class" of an existing binding.
> +Required Properties:
> +
> +* compatible: should be
> + - "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA
> + specific extensions.
The "altr" vendor prefix doesn't appear in
Documentation/devicetree/bindings/vendor-prefixes.txt. Is there another
patch in flight to add it?
> +* altr,dw-mshc-ciu-div: Specifies the divider value for the card interface
> + unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the
> divider
> + value is fixed at 3, which means parent_clock/4.
Should the clocks be represented using the common clock DT binding? If
this register is something that will always be handled entirely
internally to the HW module, then perhaps there's no need. If the driver
is going to need to call clk_set_rate() at all, then representing the
clock using the standard bindings seems better, unless you expect the
driver to call clk_set_rate(desired_rate * internal_divider) instead of
clk_set_rate(desired_rate) everywhere. That might not be unreasonable
though.
> +Required properties for a slot:
> +
> +* bus-width: Data width for card slot. 4-bit or 8-bit data.
Isn't that already part of the standard MMC bindings, and hence not
something you need to duplicate here?
> +Example:
> +
> + The MSHC controller node can be split into two portions, SoC specific and
> + board specific portions, as listed below.
That doesn't sound like a good idea. There should be one DT node for
each logical block. The internal construction of the Linux drivers
(presumably you have entirely separate code to handle the two nodes in
Linux so far?) should not influence the DT construction at all.
> + dwmmc0@ff704000 {
> + compatible = "altr,socfpga-dw-mshc";
> + reg = <0xff704000 0x1000>;
> + interrupts = <0 139 4>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + dwmmc0@ff704000 {
> + num-slots = <1>;
> + supports-highspeed;
> + broken-cd;
> + fifo-depth = <0x400>;
> + altr,dw-mshc-ciu-div = <3>;
> + altr,dw-mshc-sdr-timing = <0 3>;
> +
> + slot@0 {
> + reg = <0>;
> + bus-width = <4>;
> + };
> + };
> +
You might want to trim that trailing blank line.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: add Device Tree properties for UHS modes
On 07/26/2013 09:51 AM, Guennadi Liakhovetski wrote: > Add DT properties for UHS SDR12, SDR25, SDR50, SDR104 and DDR50 modes and > for supported by the host in DDR mode VccQ values. Adding them to DT will > automatically enable respective MMC host capabilities. > diff --git a/Documentation/devicetree/bindings/mmc/mmc.txt > b/Documentation/devicetree/bindings/mmc/mmc.txt > +- uhs-sdr12: the host supports UHS SDR12 mode > +- uhs-sdr25: the host supports UHS SDR25 mode > +- uhs-sdr50: the host supports UHS SDR50 mode > +- uhs-sdr104: the host supports UHS SDR104 mode > +- uhs-ddr50: the host supports UHS DDR50 mode > +- ddr-1v2: the host can support DDR, using 1.2V VccQ > +- ddr-1v8: the host can support DDR, using 1.8V VccQ Surely the driver for the host controller already knows this, so there's no need to represent it in DT? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/2] Set mmc(blk)X with DT alias
On 07/24/2013 02:09 AM, Steffen Trumtrar wrote: > Hi! > > Embedded devices often use multiple SD/MMC devices as boot/rootfs disks. > Some of them are removable, some not. If the removable cards are not > present, but are probed before the non-removable ones, the indexing > scheme changes. This makes it harder to hard-code the rootfs in the > cmdline. > > First solution I came up with was the alias-node in DT. > I guess my implementation is pretty hacky and ugly, but you can get lost > pretty fast in the whole mmc stack. > For example, the second patch should use "card->host->index" instead of > parsing > the alias again, I guess. I'm not sure why it currently doesn't though. This has been discussed a few times before and rejected IIRC. One issue is that block device ID is actually decoupled from host controller ID anyway, e.g. if a removable device is mounted, removed, and then re-plugged the new device can get a different ID, so this approach doesn't really work in all cases anyway. > So, if there is a better place or solution to specify a reliable ordering > of mmc devices, please let me hear it. root=UUID=xxx or root=PARTUUID=xxx are the best solution. With recent U-Boot, you can enable and use the "part" command to find the partition UUID automatically, and hence not need to hard-code anything. Something similar could presumably be implemented for other bootloaders. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Fixup Oops for SDIO shutdown
On 07/02/2013 04:53 AM, Ulf Hansson wrote: > Commit "mmc: core: Handle card shutdown from mmc_bus" introduced an > Oops in the shutdown sequence for SDIO. > > The drv pointer, does not exist for SDIO since the probing of the SDIO > card from the mmc_bus perspective is expected to fail by returning > -ENODEV. > > This patch adds the proper check for the pointer before calling it. Tested-by: Stephen Warren Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Crash during reboot due to 7628774 "mmc: core: Handle card shutdown from mmc_bus"
It looks like commit 7628774 "mmc: core: Handle card shutdown from
mmc_bus" causes a crash during "reboot" at least on Tegra platforms. The
issue appears to be in:
> static void mmc_bus_shutdown(struct device *dev)
> {
> struct mmc_driver *drv = to_mmc_driver(dev->driver);
> struct mmc_card *card = mmc_dev_to_card(dev);
> struct mmc_host *host = card->host;
> int ret;
>
> drv->shutdown(card);
Since drv == NULL.
Is this expected (and hence there's a missing guard on that function
call), or is the root-cause some other issue?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: lockdep dump on devtree_lock (involving esdhc)
On 06/11/2013 05:33 PM, Scott Wood wrote: > I get the following lockdump output on p2020rdb using > v3.10-rc5-43-g34376a5. While it's not particularly polite for the > esdhc driver to be calling OF functions while holding another lock which > can be acquired from interrupt context, why is devtree_lock usually > acquired in an irqsafe manner but sometimes not? > > Both types of usage were added by the same commit: > > commit d6d3c4e656513dcea61ce900f0ecb9ca820ee7cd > Author: Thomas Gleixner > Date: Wed Feb 6 15:30:56 2013 -0500 > > OF: convert devtree lock from rw_lock to raw spinlock > > Stephen, you asked about this here: > http://lkml.indiana.edu/hypermail/linux/kernel/1302.1/01383.html > > Did you ever get an answer? I believe that was fixed by c31a0c0 "of: fix recursive locking in of_get_next_available_child()". -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH V3 6/8] mmc: tegra: handle mmc_of_parse() errors during probe
On 06/09/2013 02:14 PM, Simon Baatz wrote: > Signed-off-by: Simon Baatz Tested-by: Stephen Warren (On Seaboard/Springbank board, i.e. Tegra20) > diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c > +err_parse_dt: > err_power_req: > err_alloc_tegra_host: Nit: It'd be nice if that new label got inserted into the middle of the list, so err_power_req, err_parse_dt, err_alloc_tegra_host. That way if we ever needed to add separate code there, they'd be in the right order already. Still, this isn't worth fixing unless you have to repost for some other reason I guess. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/10] pinctrl: add pinctrl driver for Rockchip SoCs
On 06/05/2013 01:01 AM, Linus Walleij wrote: > On Tue, Jun 4, 2013 at 2:05 PM, Heiko Stübner wrote: ... >> The only problem is the pull stuff mentioned above that is either pull up or >> down without the driver having knowledge about it. And generic_pinconf only >> knows about them separately right now. > > Create a separate patch adding PIN_CONFIG_BIAS_PULL_AUTO > to include/linux/pinctrl/pinconf-generic.h, don't forget the > kerneldoc, and patching drivers/pinctrl/pinconf-generic.c. "AUTO" sounds really rather generic. Based on just the word "AUTO", I have no idea if it's a HW- or SW-supplied default, or what the algorithm is for determining the automatically selected value. Perhaps s/AUTO/PIN_DEFAULT/ or something like that? While the concept is simple enough, it's unusual enough that such a patch would hopefully have a comment containing a full explanation of exactly what this option means. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: mmc: bcm281xx SDHCI driver
On 05/08/2013 11:55 PM, Christian Daudt wrote: > Add SDHCI driver for the Broadcom 281xx SoCs. Also > add bindings for it into bcm281xx dts files. > Still missing: > - power managemement > diff --git a/drivers/mmc/host/sdhci-bcm-kona.c > b/drivers/mmc/host/sdhci-bcm-kona.c > + /* > + * Back-to-Back register write needs a delay of 1ms at bootup (min 10uS) > + * Back-to-Back writes to same register needs delay when SD bus clock > + * is very low w.r.t AHB clock, mainly during boot-time and during card > + * insert-removal. > + */ > + udelay(1000); > + sdhci_writel(host, val, KONA_SDHOST_CORECTRL); This sounds very similar to the workaround in drivers/mmc/host/sdhci-bcm2835.c. Is this the same IP block supported by that driver? Perhaps not, since bcm2835 apparently needs other WARs such as always using 32-bit IO and hence needing custom read/write w/b functions to do read-modify-write which isn't here? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] ARM: tegra: DT-related fixes needed by the MMC tree
On 03/11/2013 02:44 PM, Stephen Warren wrote: > In order to convert the Tegra MMC driver to using mmc_of_parse(), some > bugs in the Tegra device-tree content need to be fixed first; it's > currently wrong but unused, and mmc_of_parse() causes that data to be > used for the first time. Chris, I don't see this pull request nor the followup patch in the mmc-next branch yet. Are you still planning on pulling it in? Let me know if you need me to resend them. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] mmc: sdhci_pltfm: Constify sdhci_pltfm_data
On 03/13/2013 12:26 PM, Lars-Peter Clausen wrote: > The sdhci_pltfm_data struct is never modified within the sdhci_pltfm module. > So > make the pdata parameter to sdhci_pltfm_init and sdhci_pltfm_register const. > This allows drivers to declare their sdhci_pltfm_data struct as const. > > This patch also makes the sdhci_pltfm_data declarations const where possible. The Tegra parts of the series, Acked-by: Stephen Warren -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] mmc: sdhci_pltfm: Constify sdhci_pltfm_data
On 03/12/2013 04:52 AM, Lars-Peter Clausen wrote:
> The sdhci_pltfm_data struct is never modified within the sdhci_pltfm module.
> So
> make the pdata parameter to sdhci_pltfm_init and sdhci_pltfm_register const.
> This allows drivers to declare their sdhci_pltfm_data struct as const.
>
> This patch also makes the sdhci_pltfm_data declarations const where possible.
> No changes since the last submission, just rebased onto of the latest mmc-next
> diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
> -static struct sdhci_pltfm_data sdhci_tegra20_pdata = {
> +static const struct sdhci_pltfm_data sdhci_tegra20_pdata = {
> -static struct sdhci_pltfm_data sdhci_tegra30_pdata = {
> +static const struct sdhci_pltfm_data sdhci_tegra30_pdata = {
Commit 5ebf255 "mmc: sdhci-tegra: add basic support for Tegra114"
recently added sdhci_tegra114_pdata, which would need the same treatment.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: tegra: use mmc_of_parse to get the support of standard MMC DT bindings
From: Joseph Lo
Updating the sdhci-tegra driver to use mmc_of_parse to support standard
MMC DT bindings. Then we can remove the redundant code that already support
in generic MMC core.
Signed-off-by: Joseph Lo
Tested-by: Thierry Reding
Signed-off-by: Stephen Warren
---
Chris, this patch needs to be applied after the preceding pull request.
Thanks very much.
drivers/mmc/host/sdhci-tegra.c | 92 +---
1 file changed, 10 insertions(+), 82 deletions(-)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c
index 08b06e9..f2ede63 100644
--- a/drivers/mmc/host/sdhci-tegra.c
+++ b/drivers/mmc/host/sdhci-tegra.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
@@ -44,10 +45,7 @@ struct sdhci_tegra_soc_data {
struct sdhci_tegra {
const struct sdhci_tegra_soc_data *soc_data;
- int cd_gpio;
- int wp_gpio;
int power_gpio;
- int is_8bit;
};
static u32 tegra_sdhci_readl(struct sdhci_host *host, int reg)
@@ -107,23 +105,9 @@ static void tegra_sdhci_writel(struct sdhci_host *host,
u32 val, int reg)
static unsigned int tegra_sdhci_get_ro(struct sdhci_host *host)
{
- struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
- struct sdhci_tegra *tegra_host = pltfm_host->priv;
-
- if (!gpio_is_valid(tegra_host->wp_gpio))
- return -1;
-
- return gpio_get_value(tegra_host->wp_gpio);
+ return mmc_gpio_get_ro(host->mmc);
}
-static irqreturn_t carddetect_irq(int irq, void *data)
-{
- struct sdhci_host *sdhost = (struct sdhci_host *)data;
-
- tasklet_schedule(&sdhost->card_tasklet);
- return IRQ_HANDLED;
-};
-
static void tegra_sdhci_reset_exit(struct sdhci_host *host, u8 mask)
{
struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -145,12 +129,11 @@ static void tegra_sdhci_reset_exit(struct sdhci_host
*host, u8 mask)
static int tegra_sdhci_buswidth(struct sdhci_host *host, int bus_width)
{
- struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
- struct sdhci_tegra *tegra_host = pltfm_host->priv;
u32 ctrl;
ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
- if (tegra_host->is_8bit && bus_width == MMC_BUS_WIDTH_8) {
+ if ((host->mmc->caps & MMC_CAP_8_BIT_DATA) &&
+ (bus_width == MMC_BUS_WIDTH_8)) {
ctrl &= ~SDHCI_CTRL_4BITBUS;
ctrl |= SDHCI_CTRL_8BITBUS;
} else {
@@ -216,19 +199,15 @@ static const struct of_device_id sdhci_tegra_dt_match[] =
{
};
MODULE_DEVICE_TABLE(of, sdhci_dt_ids);
-static void sdhci_tegra_parse_dt(struct device *dev,
- struct sdhci_tegra *tegra_host)
+static void sdhci_tegra_parse_dt(struct device *dev)
{
struct device_node *np = dev->of_node;
- u32 bus_width;
+ struct sdhci_host *host = dev_get_drvdata(dev);
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_tegra *tegra_host = pltfm_host->priv;
- tegra_host->cd_gpio = of_get_named_gpio(np, "cd-gpios", 0);
- tegra_host->wp_gpio = of_get_named_gpio(np, "wp-gpios", 0);
tegra_host->power_gpio = of_get_named_gpio(np, "power-gpios", 0);
-
- if (of_property_read_u32(np, "bus-width", &bus_width) == 0 &&
- bus_width == 8)
- tegra_host->is_8bit = 1;
+ mmc_of_parse(host->mmc);
}
static int sdhci_tegra_probe(struct platform_device *pdev)
@@ -260,7 +239,7 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
tegra_host->soc_data = soc_data;
pltfm_host->priv = tegra_host;
- sdhci_tegra_parse_dt(&pdev->dev, tegra_host);
+ sdhci_tegra_parse_dt(&pdev->dev);
if (gpio_is_valid(tegra_host->power_gpio)) {
rc = gpio_request(tegra_host->power_gpio, "sdhci_power");
@@ -272,37 +251,6 @@ static int sdhci_tegra_probe(struct platform_device *pdev)
gpio_direction_output(tegra_host->power_gpio, 1);
}
- if (gpio_is_valid(tegra_host->cd_gpio)) {
- rc = gpio_request(tegra_host->cd_gpio, "sdhci_cd");
- if (rc) {
- dev_err(mmc_dev(host->mmc),
- "failed to allocate cd gpio\n");
- goto err_cd_req;
- }
- gpio_direction_input(tegra_host->cd_gpio);
-
- rc = request_irq(gpio_to_irq(tegra_host->cd_gpio),
-carddetect_irq,
-IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
-mmc_hostname(host->mmc), host);
-
- if (rc) {
- dev_err(mmc_dev(host->mmc), "r
[GIT PULL] ARM: tegra: DT-related fixes needed by the MMC tree
In order to convert the Tegra MMC driver to using mmc_of_parse(), some bugs in the Tegra device-tree content need to be fixed first; it's currently wrong but unused, and mmc_of_parse() causes that data to be used for the first time. The following changes since commit 6dbe51c251a327e012439c4772097a13df43c5b8: Linux 3.9-rc1 are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git tegra-for-3.10-fixes-for-mmc for you to fetch changes up to 908ab9368866e6edf0edebdd546adefd5f3128f9: ARM: dts: tegra: fix the activate polarity of cd-gpio in mmc host Joseph Lo (1): ARM: dts: tegra: fix the activate polarity of cd-gpio in mmc host arch/arm/boot/dts/tegra20-colibri-512.dtsi |2 +- arch/arm/boot/dts/tegra20-harmony.dts |4 ++-- arch/arm/boot/dts/tegra20-paz00.dts|2 +- arch/arm/boot/dts/tegra20-seaboard.dts |2 +- arch/arm/boot/dts/tegra20-tamonten.dtsi|2 +- arch/arm/boot/dts/tegra20-trimslice.dts|2 +- arch/arm/boot/dts/tegra20-ventana.dts |2 +- arch/arm/boot/dts/tegra20-whistler.dts |1 + arch/arm/boot/dts/tegra30-beaver.dts |2 +- arch/arm/boot/dts/tegra30-cardhu.dtsi |2 +- 10 files changed, 11 insertions(+), 10 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register
On 03/08/2013 09:27 PM, Kevin Liu wrote: > 2013/3/9 Stephen Warren : >> On 03/07/2013 09:52 PM, Kevin Liu wrote: >>> commit 6c56e7a0 provide a function mmc_of_parse for standard MMC >>> device-tree binding parser centrally. So just call it with >>> sdhci_get_of_property together in sdhci_pltfm_register. >> >> BTW, this patch seems to do two unrelated things; firstly it changes the >> indentation level throughout sdhci_get_of_property(), and secondly makes >> the change described in the commit description. > > That's true. I originally want to do the first thing incidentally :-) > So do you want to split this patch into two? It's Chris's call, but I think it'd be more usual to make this two separate patches. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register
n 03/08/2013 09:24 PM, Kevin Liu wrote: > 2013/3/9 Stephen Warren : >> On 03/08/2013 08:07 AM, Kevin Liu wrote: >>> commit 6c56e7a0 provide a function mmc_of_parse for standard MMC >>> device-tree binding parser centrally. So just call it with >>> sdhci_get_of_property together in sdhci_pltfm_register. >> >>> diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c >> >>> @@ -212,6 +213,7 @@ int sdhci_pltfm_register(struct platform_device *pdev, >>> if (IS_ERR(host)) >>> return PTR_ERR(host); >>> >>> + mmc_of_parse(host->mmc); >> >> A few drivers already call mmc_of_parse() themselves. This change will >> make that call happen twice. Mostly this won't be an issue, but there >> are a couple gpio_request() calls in there, the error-handling for which >> in mmc_of_parse() will spew error messages if attempted twice. I also >> have a patch in the Tegra tree that adds a call to mmc_of_parse() into >> the Tegra driver, and that relies on fixing some bugs in the device >> tree; the CD GPIO polarity was previously specified incorrectly in the DT... >> > > Stephen, > > I don't think so. I add calling mmc_of_parse in sdhci_pltfm_register > rather than sdhci_pltfm_init. Ah yes, you're right. So, there's no issue. Chris, I do think I'll still send you a pull request to move the Tegra SDHCI change that's currently in the Tegra tree into the MMC tree, just in case there do turn out to be any MMC core cleanup/... issues this kernel cycle. Best to be safe! I suspect that Tegra might be able to be converted to using sdhci_pltfm_register() rather than sdhci_pltfm_init() after Kevin's change, but I'll have to investigate more to be certain. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: sdhci-pltfm: add calling mmc_of_parse in sdhci_pltfm_register
On 03/07/2013 09:52 PM, Kevin Liu wrote: > commit 6c56e7a0 provide a function mmc_of_parse for standard MMC > device-tree binding parser centrally. So just call it with > sdhci_get_of_property together in sdhci_pltfm_register. BTW, this patch seems to do two unrelated things; firstly it changes the indentation level throughout sdhci_get_of_property(), and secondly makes the change described in the commit description. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
