| Message ID | 1347018317-2222-1-git-send-email-lauri.hintsala@bluegiga.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 07, 2012 at 02:45:17PM +0300, Lauri Hintsala wrote: > This feature allows to use SDIO bus without wiring card detect > signal. This looks like a case of "broken-cd" ... > This is relevant in cases where SDIO device is connected to > SDIO bus and there is no way to disconnect device from bus (device > is always present). > ... while this sounds like a case of "non-removable". > Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com> > --- > drivers/mmc/host/mxs-mmc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c > index ad3fcea..992760e 100644 > --- a/drivers/mmc/host/mxs-mmc.c > +++ b/drivers/mmc/host/mxs-mmc.c > @@ -165,6 +165,7 @@ struct mxs_mmc_host { > int sdio_irq_en; > int wp_gpio; > bool wp_inverted; > + bool broken_cd; > }; > > static int mxs_mmc_get_ro(struct mmc_host *mmc) > @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc) > { > struct mxs_mmc_host *host = mmc_priv(mmc); > > + if (host->broken_cd) > + return -ENOSYS; > + The .get_cd caller does not check for error. This change makes no sense to me. Instead, the driver needs to set MMC_CAP_NEEDS_POLL in case of "broken-cd". Regards, Shawn > return !(readl(host->base + HW_SSP_STATUS(host)) & > BM_SSP_STATUS_CARD_DETECT); > } > @@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device *pdev) > &flags); > if (flags & OF_GPIO_ACTIVE_LOW) > host->wp_inverted = 1; > + > + if (of_get_property(np, "broken-cd", NULL)) > + host->broken_cd = 1; > } else { > if (pdata->flags & SLOTF_8_BIT_CAPABLE) > mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA; > -- > 1.7.9.5 >
Hi Shawn, On 09/10/2012 09:08 AM, Shawn Guo wrote: > On Fri, Sep 07, 2012 at 02:45:17PM +0300, Lauri Hintsala wrote: >> This feature allows to use SDIO bus without wiring card detect >> signal. > > This looks like a case of "broken-cd" ... > >> This is relevant in cases where SDIO device is connected to >> SDIO bus and there is no way to disconnect device from bus (device >> is always present). >> > ... while this sounds like a case of "non-removable". Right, I'll remove this comment. In our case the SDIO device is always physically connected to SDIO bus but the power of the device can be controller. So in SW point of view the device can be removed by switching the power off. >> Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com> >> --- >> drivers/mmc/host/mxs-mmc.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c >> index ad3fcea..992760e 100644 >> --- a/drivers/mmc/host/mxs-mmc.c >> +++ b/drivers/mmc/host/mxs-mmc.c >> @@ -165,6 +165,7 @@ struct mxs_mmc_host { >> int sdio_irq_en; >> int wp_gpio; >> bool wp_inverted; >> + bool broken_cd; >> }; >> >> static int mxs_mmc_get_ro(struct mmc_host *mmc) >> @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc) >> { >> struct mxs_mmc_host *host = mmc_priv(mmc); >> >> + if (host->broken_cd) >> + return -ENOSYS; >> + > > The .get_cd caller does not check for error. This change makes no > sense to me. In our case we don't want to read card detect bit and we have to continue MMC probing because card detect pin is routed nowhere. Could it be better to return zero instead of error? ENOSYS is used to indicate "unreachable card detect" in other drivers (e.g. atmel-mci, omap_hsmmc) and I followed them. > Instead, the driver needs to set MMC_CAP_NEEDS_POLL in > case of "broken-cd". MMC_CAP_NEEDS_POLL is always set in mxs-mmc. Best regards, Lauri > > Regards, > Shawn > >> return !(readl(host->base + HW_SSP_STATUS(host)) & >> BM_SSP_STATUS_CARD_DETECT); >> } >> @@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device *pdev) >> &flags); >> if (flags & OF_GPIO_ACTIVE_LOW) >> host->wp_inverted = 1; >> + >> + if (of_get_property(np, "broken-cd", NULL)) >> + host->broken_cd = 1; >> } else { >> if (pdata->flags & SLOTF_8_BIT_CAPABLE) >> mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA; >> -- >> 1.7.9.5 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Sep 10, 2012 at 01:04:00PM +0300, Lauri Hintsala wrote: > >>@@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc) > >> { > >> struct mxs_mmc_host *host = mmc_priv(mmc); > >> > >>+ if (host->broken_cd) > >>+ return -ENOSYS; > >>+ > > > >The .get_cd caller does not check for error. This change makes no > >sense to me. > > In our case we don't want to read card detect bit and we have to > continue MMC probing because card detect pin is routed nowhere. > > Could it be better to return zero instead of error? ENOSYS is used > to indicate "unreachable card detect" in other drivers (e.g. > atmel-mci, omap_hsmmc) and I followed them. > I was wrong. Even the kerneldoc of .get_cd suggests return -ENOSYS for that case. > > >Instead, the driver needs to set MMC_CAP_NEEDS_POLL in > >case of "broken-cd". > > MMC_CAP_NEEDS_POLL is always set in mxs-mmc. > Oops, I forgot that. But this is really something that needs to be fixed. Per my understanding, flag MMC_CAP_NEEDS_POLL should only be set when card-detect is not working. Also the card-detect in gpio support is missing. But for this patch: Acked-by: Shawn Guo <shawn.guo@linaro.org>
I think this describes three use cases which are different, as Shawn said we have here; * missing card detect support (card detect is not wired so it's impossible to tell, and the controller doesn't support the SD standard card detection) * non-removable device * broken card detect support The third one is what the property sounds like it describes, but this is not the use case you are describing at all. This kind of property naming is more like describing "card detect doesn't operate reliably" which is true of i.MX devices with certain versions of the eSDHC and uSDHC controllers, where non-GPIO card detection interferes with SDIO interrupts, but in this case a board designer should add a real card detect pin to the board (most more expensive push-push SD card slots come with a pin you can wire for CD). In cases where errata is published after boards are shipped, broken-cd is a meaningful description when described in the absense of a gpio cd description, or presence of some cd-handled-internally style property (forgive me for not cross-referencing the FSL definition for this property, we don't use it here at Genesi since all our boards have GPIO CD) It seems your device is a combination of the top two in the list, it is not down to a broken feature at all, but one that should be possible to not implement for devices which are permanently connected. A non-removable device should be able to be powered down, at least using runtime PM or clock gating (if this works, remember to whitelist the card for clock gating!) but card detect shouldn't be used in this case to detect if the card is powered or not (this is a problem for your card controller and card driver state tracker. The MMC subsystem already tracks this state fairly well for power management). I would refrain from calling a feature "broken card detect" if there is actually no breakage involved especially if it would be more prudent instead look into how to figure out how to track power management state properly without cluttering the device tree.
Hi, On 09/10/2012 05:58 PM, Matt Sealey wrote: > I think this describes three use cases which are different, as Shawn > said we have here; > > * missing card detect support (card detect is not wired so it's > impossible to tell, and the controller doesn't support the SD standard > card detection) > * non-removable device > * broken card detect support > > The third one is what the property sounds like it describes, but this > is not the use case you are describing at all. This kind of property > naming is more like describing "card detect doesn't operate reliably" > which is true of i.MX devices with certain versions of the eSDHC and > uSDHC controllers, where non-GPIO card detection interferes with SDIO > interrupts, but in this case a board designer should add a real card > detect pin to the board (most more expensive push-push SD card slots > come with a pin you can wire for CD). In cases where errata is > published after boards are shipped, broken-cd is a meaningful > description when described in the absense of a gpio cd description, or > presence of some cd-handled-internally style property (forgive me for > not cross-referencing the FSL definition for this property, we don't > use it here at Genesi since all our boards have GPIO CD) > > It seems your device is a combination of the top two in the list, it > is not down to a broken feature at all, but one that should be > possible to not implement for devices which are permanently connected. > A non-removable device should be able to be powered down, at least > using runtime PM or clock gating (if this works, remember to whitelist > the card for clock gating!) but card detect shouldn't be used in this > case to detect if the card is powered or not (this is a problem for > your card controller and card driver state tracker. The MMC subsystem > already tracks this state fairly well for power management). Do you mean the polling mode shouldn't been used to detect if the device is powered? Is there any examples how PM support could be implemented? I'm not familiar with PM subsystem. How is it done in practice? Should I play with fixed regulators? BR, Lauri > I would refrain from calling a feature "broken card detect" if there > is actually no breakage involved especially if it would be more > prudent instead look into how to figure out how to track power > management state properly without cluttering the device tree. >
Hi, Is it OK to use broken-cd? broken-cd feature is documented as "There is no card detection available; polling must be used". In this case the card detect is not broken but it is unrouted so it is unavailable. Documentation about broken-cd has been added by commit: https://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commitdiff;h=abe1e05da365350ac282ba5f6831aae13d97074e Any tips about power management implementation are welcome if I have to use PM instead of polling method. Regards, Lauri On 09/12/2012 05:06 PM, Lauri Hintsala wrote: > Hi, > > On 09/10/2012 05:58 PM, Matt Sealey wrote: >> I think this describes three use cases which are different, as Shawn >> said we have here; >> >> * missing card detect support (card detect is not wired so it's >> impossible to tell, and the controller doesn't support the SD standard >> card detection) >> * non-removable device >> * broken card detect support >> >> The third one is what the property sounds like it describes, but this >> is not the use case you are describing at all. This kind of property >> naming is more like describing "card detect doesn't operate reliably" >> which is true of i.MX devices with certain versions of the eSDHC and >> uSDHC controllers, where non-GPIO card detection interferes with SDIO >> interrupts, but in this case a board designer should add a real card >> detect pin to the board (most more expensive push-push SD card slots >> come with a pin you can wire for CD). In cases where errata is >> published after boards are shipped, broken-cd is a meaningful >> description when described in the absense of a gpio cd description, or >> presence of some cd-handled-internally style property (forgive me for >> not cross-referencing the FSL definition for this property, we don't >> use it here at Genesi since all our boards have GPIO CD) >> >> It seems your device is a combination of the top two in the list, it >> is not down to a broken feature at all, but one that should be >> possible to not implement for devices which are permanently connected. >> A non-removable device should be able to be powered down, at least >> using runtime PM or clock gating (if this works, remember to whitelist >> the card for clock gating!) but card detect shouldn't be used in this >> case to detect if the card is powered or not (this is a problem for >> your card controller and card driver state tracker. The MMC subsystem >> already tracks this state fairly well for power management). > > Do you mean the polling mode shouldn't been used to detect if the device > is powered? > > Is there any examples how PM support could be implemented? I'm not > familiar with PM subsystem. How is it done in practice? Should I play > with fixed regulators? > > BR, > Lauri > > >> I would refrain from calling a feature "broken card detect" if there >> is actually no breakage involved especially if it would be more >> prudent instead look into how to figure out how to track power >> management state properly without cluttering the device tree. >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Sep 17 2012, Lauri Hintsala wrote: > Is it OK to use broken-cd? broken-cd feature is documented as "There > is no card detection available; polling must be used". In this case > the card detect is not broken but it is unrouted so it is unavailable. > > Documentation about broken-cd has been added by commit: > https://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commitdiff;h=abe1e05da365350ac282ba5f6831aae13d97074e I'm having trouble following the problem; it sounds like perhaps all you need is: if (of_find_property(np, "non-removable", NULL)) mmc->caps |= MMC_CAP_NONREMOVABLE; Have you tried that? - Chris.
Hi, On 09/17/2012 05:01 PM, Chris Ball wrote: > Hi, > > On Mon, Sep 17 2012, Lauri Hintsala wrote: >> Is it OK to use broken-cd? broken-cd feature is documented as "There >> is no card detection available; polling must be used". In this case >> the card detect is not broken but it is unrouted so it is unavailable. >> >> Documentation about broken-cd has been added by commit: >> https://git.kernel.org/?p=linux/kernel/git/cjb/mmc.git;a=commitdiff;h=abe1e05da365350ac282ba5f6831aae13d97074e > > I'm having trouble following the problem; it sounds like perhaps all you > need is: > > if (of_find_property(np, "non-removable", NULL)) > mmc->caps |= MMC_CAP_NONREMOVABLE; > > Have you tried that? Yes I have and it does not work. Current mxs-mmc driver uses polling method and it reads the CARD_DETECT bit of STATUS register. In our case card detect is signal is unrouted so SDIO device is never detect. Another problem with NONREMOVABLE feature is in case where power cycle or reset is done for SDIO device. A device drive doesn't detect HW reset. After reset the device driver is still in running state and it does not reinitialize the device. So after reset device stops to work. Lauri
Hi, On 09/10/2012 04:50 PM, Shawn Guo wrote: > On Mon, Sep 10, 2012 at 01:04:00PM +0300, Lauri Hintsala wrote: >>>> @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc) >>>> { >>>> struct mxs_mmc_host *host = mmc_priv(mmc); >>>> >>>> + if (host->broken_cd) >>>> + return -ENOSYS; >>>> + ... > But for this patch: > > Acked-by: Shawn Guo <shawn.guo@linaro.org> Any chance to get this patch for 3.7? It makes possible to use the driver without a card detect signal. Regards, Lauri
diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c index ad3fcea..992760e 100644 --- a/drivers/mmc/host/mxs-mmc.c +++ b/drivers/mmc/host/mxs-mmc.c @@ -165,6 +165,7 @@ struct mxs_mmc_host { int sdio_irq_en; int wp_gpio; bool wp_inverted; + bool broken_cd; }; static int mxs_mmc_get_ro(struct mmc_host *mmc) @@ -187,6 +188,9 @@ static int mxs_mmc_get_cd(struct mmc_host *mmc) { struct mxs_mmc_host *host = mmc_priv(mmc); + if (host->broken_cd) + return -ENOSYS; + return !(readl(host->base + HW_SSP_STATUS(host)) & BM_SSP_STATUS_CARD_DETECT); } @@ -808,6 +812,9 @@ static int mxs_mmc_probe(struct platform_device *pdev) &flags); if (flags & OF_GPIO_ACTIVE_LOW) host->wp_inverted = 1; + + if (of_get_property(np, "broken-cd", NULL)) + host->broken_cd = 1; } else { if (pdata->flags & SLOTF_8_BIT_CAPABLE) mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA;
This feature allows to use SDIO bus without wiring card detect signal. This is relevant in cases where SDIO device is connected to SDIO bus and there is no way to disconnect device from bus (device is always present). Signed-off-by: Lauri Hintsala <lauri.hintsala@bluegiga.com> --- drivers/mmc/host/mxs-mmc.c | 7 +++++++ 1 file changed, 7 insertions(+)