From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tuomas Tynkkynen Date: Wed, 20 Dec 2017 15:34:47 +0200 Subject: [U-Boot] [PATCH 1/2] mmc: sunxi: Support cd-inverted DT property In-Reply-To: <20171220122608.yinlyqijwaladred@flea.lan> References: <20171218200942.23586-1-tuomas@tuxera.com> <20171220122608.yinlyqijwaladred@flea.lan> Message-ID: <1513776887.5797.11.camel@tuxera.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, 2017-12-20 at 13:26 +0100, Maxime Ripard wrote: > On Mon, Dec 18, 2017 at 10:09:41PM +0200, Tuomas Tynkkynen wrote: > > Commit 8620f384098b ("dm: sunxi: Linksprite_pcDuino3: Correct > > polarity > > of MMC card detect") claims that the Pcduino3 device tree has an > > incorrect polarity for the card detect pin, but the actual problem > > is > > that unlike the Linux MMC driver, the U-Boot driver isn't > > respecting the > > cd-invert property (see > > Documentation/devicetree/bindings/mmc/mmc.txt) > > which tells that the card detect signal level should be inverted. > > > > Fix this properly by adding support for the cd-inverted property > > while > > reverting the original commit at the same time. While at it, I > > noticed > > the driver always enables pullups for the card detect line, which > > is not > > right if the card detect GPIO is active-high, so fix that as well. > > > > Signed-off-by: Tuomas Tynkkynen > > --- > > arch/arm/dts/sun7i-a20-pcduino3.dts | 2 +- > > drivers/mmc/sunxi_mmc.c | 12 ++++++++++-- > > 2 files changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/dts/sun7i-a20-pcduino3.dts > > b/arch/arm/dts/sun7i-a20-pcduino3.dts > > index 37b1e0ee9b..1a8b39be1d 100644 > > --- a/arch/arm/dts/sun7i-a20-pcduino3.dts > > +++ b/arch/arm/dts/sun7i-a20-pcduino3.dts > > @@ -164,7 +164,7 @@ > > pinctrl-0 = <&mmc0_pins_a>, > > <&mmc0_cd_pin_reference_design>; > > vmmc-supply = <®_vcc3v3>; > > bus-width = <4>; > > - cd-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>; /* PH1 */ > > + cd-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */ > > cd-inverted; > > status = "okay"; > > }; > > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > > index 4edb4be46c..7cc7303570 100644 > > --- a/drivers/mmc/sunxi_mmc.c > > +++ b/drivers/mmc/sunxi_mmc.c > > @@ -30,6 +30,7 @@ struct sunxi_mmc_priv { > > uint32_t *mclkreg; > > unsigned fatal_err; > > struct gpio_desc cd_gpio; /* Change Detect GPIO */ > > + int cd_inverted; > > struct sunxi_mmc *reg; > > struct mmc_config cfg; > > }; > > @@ -545,7 +546,7 @@ static int sunxi_mmc_getcd(struct udevice *dev) > > struct sunxi_mmc_priv *priv = dev_get_priv(dev); > > > > if (dm_gpio_is_valid(&priv->cd_gpio)) > > - return dm_gpio_get_value(&priv->cd_gpio); > > + return dm_gpio_get_value(&priv->cd_gpio) ^ priv- > > >cd_inverted; > > > > return 1; > > } > > @@ -606,8 +607,15 @@ static int sunxi_mmc_probe(struct udevice > > *dev) > > if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv- > > >cd_gpio, > > GPIOD_IS_IN)) { > > int cd_pin = gpio_get_number(&priv->cd_gpio); > > + int cd_state = priv->cd_gpio.flags & > > GPIOD_ACTIVE_LOW ? 0 : 1; > > Isn't that change itself enough to get what you wanted? > > You really shouldn't be doing anything else. Was that the correct part you meant to quote? The cd_state was only for the somewhat pull-up change... Sorry about that, here's the minimal (probably whitespace-damaged) change: diff --git a/arch/arm/dts/sun7i-a20-pcduino3.dts b/arch/arm/dts/sun7i-a20-pcduino3.dts index 37b1e0ee9b..1a8b39be1d 100644 --- a/arch/arm/dts/sun7i-a20-pcduino3.dts +++ b/arch/arm/dts/sun7i-a20-pcduino3.dts @@ -164,7 +164,7 @@ pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_reference_design>; vmmc-supply = <®_vcc3v3>; bus-width = <4>; - cd-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>; /* PH1 */ + cd-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */ cd-inverted; status = "okay"; }; diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index 4edb4be46c..a974543148 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -30,6 +30,7 @@ struct sunxi_mmc_priv { uint32_t *mclkreg; unsigned fatal_err; struct gpio_desc cd_gpio; /* Change Detect GPIO */ + int cd_inverted; struct sunxi_mmc *reg; struct mmc_config cfg; }; @@ -545,7 +546,7 @@ static int sunxi_mmc_getcd(struct udevice *dev) struct sunxi_mmc_priv *priv = dev_get_priv(dev); if (dm_gpio_is_valid(&priv->cd_gpio)) - return dm_gpio_get_value(&priv->cd_gpio); + return dm_gpio_get_value(&priv->cd_gpio) ^ priv->cd_inverted; return 1; } @@ -607,6 +608,8 @@ static int sunxi_mmc_probe(struct udevice *dev) GPIOD_IS_IN)) { int cd_pin = gpio_get_number(&priv->cd_gpio); + priv->cd_inverted = dev_read_bool(dev, "cd-inverted"); + sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); } So: if the DT specifies GPIO_ACTIVE_LOW, dm_gpio_get_value() does its own inversion of the GPIO level. Then, on top of that we do another inversion if the "cd-inverted" property was specified. This matches what the Linux MMC driver does. Does that make sense? Thanks.