| Message ID | 52D6938A.9050806@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 15 January 2014 21:56:26 zhangfei wrote: > However, in our board cd =0 when card is deteced while cd=1 when card is > removed. > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be > set, as well as new property "caps2-mmc-cd-active-low". > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > @@ -73,6 +73,8 @@ Optional properties: > +* caps2-mmc-cd-active-low: cd pin is low when card active > + > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) > + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > + The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for legacy platforms. With DT parsing, you can normally specify the polarity of the GPIO line in the GPIO specifier in DT. Arnd
On Wednesday 15 January 2014 14:59:59 Arnd Bergmann wrote: > On Wednesday 15 January 2014 21:56:26 zhangfei wrote: > > However, in our board cd =0 when card is deteced while cd=1 when card is > > removed. > > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be > > set, as well as new property "caps2-mmc-cd-active-low". > > > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > @@ -73,6 +73,8 @@ Optional properties: > > +* caps2-mmc-cd-active-low: cd pin is low when card active > > + > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) > > + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > > + > > The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for > legacy platforms. With DT parsing, you can normally specify > the polarity of the GPIO line in the GPIO specifier in DT. I missed the fact that we already have a "cd-inverted" property as specified in bindings/mmc/mmc.txt. If your GPIO controller does not handle polarity, you can use that. Arnd
On Wed, January 15, 2014, Zhangfei wrote: > On 01/15/2014 08:26 PM, Seungwon Jeon wrote: > > >>>> @@ -1033,7 +1033,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc) > >>>> int present; > >>>> struct dw_mci_slot *slot = mmc_priv(mmc); > >>>> struct dw_mci_board *brd = slot->host->pdata; > >>>> - int gpio_cd = !mmc_gpio_get_cd(mmc); > >>>> + struct dw_mci *host = slot->host; > >>>> + int gpio_cd = mmc_gpio_get_cd(mmc); > >>>> > >>>> /* Use platform get_cd function, else try onboard card detect */ > >>>> if (brd->quirks & DW_MCI_QUIRK_BROKEN_CARD_DETECTION) > >>>> @@ -1041,11 +1042,12 @@ static int dw_mci_get_cd(struct mmc_host *mmc) > >>>> else if (brd->get_cd) > >>>> present = !brd->get_cd(slot->id); > >>>> else if (!IS_ERR_VALUE(gpio_cd)) > >>>> - present = !!gpio_cd; > >>>> + present = !gpio_cd; > >>> !!gpio_cd or gpio_cd is correct, isn't it? > >>> > >> > >> No, mmc_gpio_get_cd(mmc) has to revert. > > I'm missing something? > > If card is detected, mmc_gpio_get_cd() returns non-zero, right? > > I guess gpio_cd should be kept. > > > > Hmm, looks you are right. > Though not see clearly mmc_gpio_get_cd declaratoin, other drivers > directly set get_cd as mmc_gpio_get_cd. > .get_cd = mmc_gpio_get_cd > > However, in our board cd =0 when card is deteced while cd=1 when card is > removed. > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be > set, as well as new property "caps2-mmc-cd-active-low". Ok, you could do more. mmc_gpio_get_cd() is expected to return non-zero if card is detection. > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > @@ -73,6 +73,8 @@ Optional properties: > +* caps2-mmc-cd-active-low: cd pin is low when card active > + > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) > + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > + > > But it looks strange "cd-active-low" describing "CD_ACTIVE_HIGH" flag. > When card active, cd = 0, and ACTIVE_HIGH is required to make > mmc_gpio_get_cd return 1. I think your board seems not to use pull-up on GPIO line for card detection. So, MMC_CAP2_CD_ACTIVE_HIGH would be needed. Thanks, Seungwon Jeon > int mmc_gpio_get_cd(struct mmc_host *host) > { > return !gpio_get_value_cansleep(ctx->cd_gpio) ^ > !!(host->caps2 & MMC_CAP2_CD_ACTIVE_HIGH); > } > > Thanks > -- > 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 01/15/2014 10:20 PM, Arnd Bergmann wrote: > On Wednesday 15 January 2014 14:59:59 Arnd Bergmann wrote: >> On Wednesday 15 January 2014 21:56:26 zhangfei wrote: >>> However, in our board cd =0 when card is deteced while cd=1 when card is >>> removed. >>> In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be >>> set, as well as new property "caps2-mmc-cd-active-low". >>> >>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt >>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt >>> @@ -73,6 +73,8 @@ Optional properties: >>> +* caps2-mmc-cd-active-low: cd pin is low when card active >>> + >>> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >>> + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) >>> + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; >>> + >> >> The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for >> legacy platforms. With DT parsing, you can normally specify >> the polarity of the GPIO line in the GPIO specifier in DT. > > I missed the fact that we already have a "cd-inverted" property > as specified in bindings/mmc/mmc.txt. If your GPIO controller > does not handle polarity, you can use that. > Thanks Arnd GPIO controller drivers/gpio/gpio-pl061.c still not handle polarity, so use "cd-inverted" instead.
On Wed, Jan 15, 2014 at 02:59:59PM +0100, Arnd Bergmann wrote: > On Wednesday 15 January 2014 21:56:26 zhangfei wrote: > > However, in our board cd =0 when card is deteced while cd=1 when card is > > removed. > > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be > > set, as well as new property "caps2-mmc-cd-active-low". > > > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > @@ -73,6 +73,8 @@ Optional properties: > > +* caps2-mmc-cd-active-low: cd pin is low when card active > > + > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) > > + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > > + > > The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for > legacy platforms. With DT parsing, you can normally specify > the polarity of the GPIO line in the GPIO specifier in DT. Since when?
On Wednesday 15 January 2014 16:01:46 Russell King - ARM Linux wrote: > On Wed, Jan 15, 2014 at 02:59:59PM +0100, Arnd Bergmann wrote: > > On Wednesday 15 January 2014 21:56:26 zhangfei wrote: > > > However, in our board cd =0 when card is deteced while cd=1 when card is > > > removed. > > > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be > > > set, as well as new property "caps2-mmc-cd-active-low". > > > > > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > > @@ -73,6 +73,8 @@ Optional properties: > > > +* caps2-mmc-cd-active-low: cd pin is low when card active > > > + > > > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > > + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) > > > + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > > > + > > > > The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for > > legacy platforms. With DT parsing, you can normally specify > > the polarity of the GPIO line in the GPIO specifier in DT. > > Since when? I just looked through the bindings and found that about half of them allow passing polarity in the gpio specifier. I thought it was more than that and IIRC the outcome of a previous discussion was that when the gpio controller allows passing polarity, you should use that rather than a separate flag. Not a problem though, since we can use the cd-inverted and wp-inverted properties. Arnd
On Wed, Jan 15, 2014 at 05:07:49PM +0100, Arnd Bergmann wrote: > On Wednesday 15 January 2014 16:01:46 Russell King - ARM Linux wrote: > > On Wed, Jan 15, 2014 at 02:59:59PM +0100, Arnd Bergmann wrote: > > > On Wednesday 15 January 2014 21:56:26 zhangfei wrote: > > > > However, in our board cd =0 when card is deteced while cd=1 when card is > > > > removed. > > > > In order to mmc_gpio_get_cd return 1, MMC_CAP2_CD_ACTIVE_HIGH has to be > > > > set, as well as new property "caps2-mmc-cd-active-low". > > > > > > > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > > > > @@ -73,6 +73,8 @@ Optional properties: > > > > +* caps2-mmc-cd-active-low: cd pin is low when card active > > > > + > > > > > > > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > > > > + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) > > > > + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; > > > > + > > > > > > The MMC_CAP2_CD_ACTIVE_HIGH flag should only be required for > > > legacy platforms. With DT parsing, you can normally specify > > > the polarity of the GPIO line in the GPIO specifier in DT. > > > > Since when? > > I just looked through the bindings and found that about half > of them allow passing polarity in the gpio specifier. I thought > it was more than that and IIRC the outcome of a previous discussion > was that when the gpio controller allows passing polarity, you > should use that rather than a separate flag. > > Not a problem though, since we can use the cd-inverted and > wp-inverted properties. Well, having this in the gpio level as well as in subsystems like MMC is going to create confusion - and from the results of this patch _IS_ causing confusion. You've just encouraged one implementation to have things setup such that mmc_gpio_get_cd() returns false when a card is inserted, rather than it correctly returning true. The issue here is that we'll potentially now end up with _three_ inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and another in every driver which uses the GPIO layer inversion. This is hardly the right approach as it leads to multiple points where confusion about the inverted status can occur.
On 01/16/2014 12:22 AM, Russell King - ARM Linux wrote: > Well, having this in the gpio level as well as in subsystems like MMC > is going to create confusion - and from the results of this patch _IS_ > causing confusion. You've just encouraged one implementation to have > things setup such that mmc_gpio_get_cd() returns false when a card is > inserted, rather than it correctly returning true. > > The issue here is that we'll potentially now end up with _three_ > inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and > another in every driver which uses the GPIO layer inversion. This > is hardly the right approach as it leads to multiple points where > confusion about the inverted status can occur. > Thanks Russell, make sense. Double checked with the hardware guy, cd pin is high when card inserted on the board. The cd pin is originally connected to vout via a resister, and internally short to gound, which is broken when card inserted. Then Optional properties "caps2-mmc-cd-active-high" has to be added. --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +* caps2-mmc-cd-active-high: cd pin is high when card present @@ -2411,6 +2414,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host) if (of_find_property(np, "caps2-mmc-hs200-1_2v", NULL)) pdata->caps2 |= MMC_CAP2_HS200_1_2V_SDR; + if (of_find_property(np, "caps2-mmc-cd-active-high", NULL)) + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; + Thanks
On Wednesday 15 January 2014, Russell King - ARM Linux wrote: > The issue here is that we'll potentially now end up with three > inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and > another in every driver which uses the GPIO layer inversion. This > is hardly the right approach as it leads to multiple points where > confusion about the inverted status can occur. I did not mean to suggest adding a third inversion. We already have to deal with the one in the GPIO layer and the one in mmc_gpio_get_cd(), which I absolutely agree is unfortunate. As was pointed out already, the gpio driver used in this system does not support the inversion in the DT binding, so the only proper solution is to use the inversion from mmc_gpio_get_cd if MMC_CAP2_CD_ACTIVE_HIGH is set in host->caps2. This flag gets set from mmc_of_parse based on the presence of the "cd-inverted" flag, in the absence of the gpio flags. The dw_mmc driver does not use mmc_of_parse() at the moment, and while it probably should use that in the future, it should definitely read the same DT properties with the same semantics already and not introduce new properties. Arnd
On Thu, Jan 16, 2014 at 12:12:10PM +0100, Arnd Bergmann wrote: > On Wednesday 15 January 2014, Russell King - ARM Linux wrote: > > The issue here is that we'll potentially now end up with three > > inversions. One in the GPIO layers. One in mmc_gpio_get_cd(), and > > another in every driver which uses the GPIO layer inversion. This > > is hardly the right approach as it leads to multiple points where > > confusion about the inverted status can occur. > > I did not mean to suggest adding a third inversion. We already have > to deal with the one in the GPIO layer and the one in mmc_gpio_get_cd(), > which I absolutely agree is unfortunate. As was pointed out already, > the gpio driver used in this system does not support the inversion > in the DT binding, so the only proper solution is to use the > inversion from mmc_gpio_get_cd if MMC_CAP2_CD_ACTIVE_HIGH is set > in host->caps2. This flag gets set from mmc_of_parse based > on the presence of the "cd-inverted" flag, in the absence of the > gpio flags. The dw_mmc driver does not use mmc_of_parse() at the > moment, and while it probably should use that in the future, > it should definitely read the same DT properties with the > same semantics already and not introduce new properties. Arnd, The issue I'm pointing out is that _if_ you use the GPIO layer to do the inversion, then you end up with _three_ inversions of the signal, which is excessive and confusing - and makes the sense of mmc_gpio_get_cd() completely indeterminant without reading the DT files and the code.
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt @@ -73,6 +73,8 @@ Optional properties: +* caps2-mmc-cd-active-low: cd pin is low when card active + diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c + if (of_find_property(np, "caps2-mmc-cd-active-low", NULL)) + pdata->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH; + But it looks strange "cd-active-low" describing "CD_ACTIVE_HIGH" flag. When card active, cd = 0, and ACTIVE_HIGH is required to make mmc_gpio_get_cd return 1.