[U-Boot] [PATCH 0/4] exynos-dwmmc: check set init priority for boot channel

[U-Boot] [PATCH 0/4] exynos-dwmmc: check set init priority for boot channel

[Przemyslaw Marczak]

The dw mmc driver init priority was always the same: ch 0, ch 1, ch 2.
On some boards (e.g. Odroid XU3) the dwmmc driver is enabled for all
mmc channels. In this case, when boot device is switchable (SD/eMMC),
then the MMC boot device will be 0 or 1.
Change the init priority to boot device, always init the boot device as
mmc 0.
This fixes the issue with 'saveenv' command, because the MMC env device
number is always the same.

Przemyslaw Marczak (4):
  dm: gpio: extend gpio api by dm_gpio_set_pull()
  s5p: gpio: add implementation of dm_gpio_set_pull()
  mmc: exynos dwmmc: check boot mode before init dwmmc
  mmc: print SD/eMMC type for inited mmc devices

 drivers/gpio/gpio-uclass.c  | 12 ++++++++++++
 drivers/gpio/s5p_gpio.c     | 11 +++++++++++
 drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
 drivers/mmc/mmc.c           |  8 ++++++++
 include/asm-generic/gpio.h  | 12 ++++++++++++
 5 files changed, 53 insertions(+), 1 deletion(-)

-- 
1.9.1

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

This commits extends:
- dm gpio ops by: 'set_pull' call
- dm gpio uclass by: dm_gpio_set_pull() function

The pull mode is not defined so should be driver specific.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
CC: Simon Glass <sjg@chromium.org>
---
 drivers/gpio/gpio-uclass.c | 12 ++++++++++++
 include/asm-generic/gpio.h | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index a69bbd2..48b31c2 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -321,6 +321,18 @@ int dm_gpio_set_value(struct gpio_desc *desc, int value)
 	return 0;
 }
 
+int dm_gpio_set_pull(struct gpio_desc *desc, int pull)
+{
+	int ret;
+
+	ret = check_reserved(desc, "set_pull");
+	if (ret)
+		return ret;
+
+	gpio_get_ops(desc->dev)->set_pull(desc->dev, desc->offset, pull);
+	return 0;
+}
+
 int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
 {
 	struct udevice *dev = desc->dev;
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 3b96b82..7e0d426 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -241,6 +241,7 @@ struct dm_gpio_ops {
 				int value);
 	int (*get_value)(struct udevice *dev, unsigned offset);
 	int (*set_value)(struct udevice *dev, unsigned offset, int value);
+	int (*set_pull)(struct udevice *dev, unsigned offset, int pull);
 	/**
 	 * get_function() Get the GPIO function
 	 *
@@ -479,6 +480,7 @@ int gpio_free_list_nodev(struct gpio_desc *desc, int count);
 
 /**
  * dm_gpio_get_value() - Get the value of a GPIO
+
  *
  * This is the driver model version of the existing gpio_get_value() function
  * and should be used instead of that.
@@ -495,6 +497,16 @@ int dm_gpio_get_value(struct gpio_desc *desc);
 int dm_gpio_set_value(struct gpio_desc *desc, int value);
 
 /**
+ * dm_gpio_set_pull() - Set the pull-up/down value of a GPIO
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @pull:	GPIO pull value - driver specific
+ * @return 0 on success or -ve on error
+*/
+int dm_gpio_set_pull(struct gpio_desc *desc, int pull);
+
+/**
  * dm_gpio_set_dir() - Set the direction for a GPIO
  *
  * This sets up the direction according tot the provided flags. It will do
-- 
1.9.1

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Simon Glass]

+Stephen who might have an opinion on this.

Hi Przemyslaw,

On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> This commits extends:
> - dm gpio ops by: 'set_pull' call
> - dm gpio uclass by: dm_gpio_set_pull() function
>
> The pull mode is not defined so should be driver specific.

It's good to implement this, but I think you should try to have a
standard interface. You could define the options you want to support
and pass in a standard value.

Otherwise we are not really providing a driver abstraction, only an interface.

>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> CC: Simon Glass <sjg@chromium.org>
> ---
>  drivers/gpio/gpio-uclass.c | 12 ++++++++++++
>  include/asm-generic/gpio.h | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index a69bbd2..48b31c2 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -321,6 +321,18 @@ int dm_gpio_set_value(struct gpio_desc *desc, int value)
>         return 0;
>  }
>
> +int dm_gpio_set_pull(struct gpio_desc *desc, int pull)
> +{
> +       int ret;
> +
> +       ret = check_reserved(desc, "set_pull");
> +       if (ret)
> +               return ret;
> +
> +       gpio_get_ops(desc->dev)->set_pull(desc->dev, desc->offset, pull);

Should return this value.

> +       return 0;
> +}
> +
>  int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
>  {
>         struct udevice *dev = desc->dev;
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 3b96b82..7e0d426 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -241,6 +241,7 @@ struct dm_gpio_ops {
>                                 int value);
>         int (*get_value)(struct udevice *dev, unsigned offset);
>         int (*set_value)(struct udevice *dev, unsigned offset, int value);
> +       int (*set_pull)(struct udevice *dev, unsigned offset, int pull);
>         /**
>          * get_function() Get the GPIO function
>          *
> @@ -479,6 +480,7 @@ int gpio_free_list_nodev(struct gpio_desc *desc, int count);
>
>  /**
>   * dm_gpio_get_value() - Get the value of a GPIO
> +
>   *
>   * This is the driver model version of the existing gpio_get_value() function
>   * and should be used instead of that.
> @@ -495,6 +497,16 @@ int dm_gpio_get_value(struct gpio_desc *desc);
>  int dm_gpio_set_value(struct gpio_desc *desc, int value);
>
>  /**
> + * dm_gpio_set_pull() - Set the pull-up/down value of a GPIO
> + *
> + * @desc:      GPIO description containing device, offset and flags,
> + *             previously returned by gpio_request_by_name()
> + * @pull:      GPIO pull value - driver specific
> + * @return 0 on success or -ve on error
> +*/
> +int dm_gpio_set_pull(struct gpio_desc *desc, int pull);
> +
> +/**
>   * dm_gpio_set_dir() - Set the direction for a GPIO
>   *
>   * This sets up the direction according tot the provided flags. It will do
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello,

On 02/18/2015 06:01 AM, Simon Glass wrote:
> +Stephen who might have an opinion on this.
>
> Hi Przemyslaw,
>
> On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> This commits extends:
>> - dm gpio ops by: 'set_pull' call
>> - dm gpio uclass by: dm_gpio_set_pull() function
>>
>> The pull mode is not defined so should be driver specific.
>
> It's good to implement this, but I think you should try to have a
> standard interface. You could define the options you want to support
> and pass in a standard value.
>
> Otherwise we are not really providing a driver abstraction, only an interface.
>

Ok, I will change this.

>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> CC: Simon Glass <sjg@chromium.org>
>> ---
>>   drivers/gpio/gpio-uclass.c | 12 ++++++++++++
>>   include/asm-generic/gpio.h | 12 ++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
>> index a69bbd2..48b31c2 100644
>> --- a/drivers/gpio/gpio-uclass.c
>> +++ b/drivers/gpio/gpio-uclass.c
>> @@ -321,6 +321,18 @@ int dm_gpio_set_value(struct gpio_desc *desc, int value)
>>          return 0;
>>   }
>>
>> +int dm_gpio_set_pull(struct gpio_desc *desc, int pull)
>> +{
>> +       int ret;
>> +
>> +       ret = check_reserved(desc, "set_pull");
>> +       if (ret)
>> +               return ret;
>> +
>> +       gpio_get_ops(desc->dev)->set_pull(desc->dev, desc->offset, pull);
>
> Should return this value.
>

Ok.

>> +       return 0;
>> +}
>> +
>>   int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
>>   {
>>          struct udevice *dev = desc->dev;
>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
>> index 3b96b82..7e0d426 100644
>> --- a/include/asm-generic/gpio.h
>> +++ b/include/asm-generic/gpio.h
>> @@ -241,6 +241,7 @@ struct dm_gpio_ops {
>>                                  int value);
>>          int (*get_value)(struct udevice *dev, unsigned offset);
>>          int (*set_value)(struct udevice *dev, unsigned offset, int value);
>> +       int (*set_pull)(struct udevice *dev, unsigned offset, int pull);
>>          /**
>>           * get_function() Get the GPIO function
>>           *
>> @@ -479,6 +480,7 @@ int gpio_free_list_nodev(struct gpio_desc *desc, int count);
>>
>>   /**
>>    * dm_gpio_get_value() - Get the value of a GPIO
>> +
>>    *
>>    * This is the driver model version of the existing gpio_get_value() function
>>    * and should be used instead of that.
>> @@ -495,6 +497,16 @@ int dm_gpio_get_value(struct gpio_desc *desc);
>>   int dm_gpio_set_value(struct gpio_desc *desc, int value);
>>
>>   /**
>> + * dm_gpio_set_pull() - Set the pull-up/down value of a GPIO
>> + *
>> + * @desc:      GPIO description containing device, offset and flags,
>> + *             previously returned by gpio_request_by_name()
>> + * @pull:      GPIO pull value - driver specific
>> + * @return 0 on success or -ve on error
>> +*/
>> +int dm_gpio_set_pull(struct gpio_desc *desc, int pull);
>> +
>> +/**
>>    * dm_gpio_set_dir() - Set the direction for a GPIO
>>    *
>>    * This sets up the direction according tot the provided flags. It will do
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

Sorry for the mess with this patchset (gpio+mmc).

I was testing the mmc detection by checking one of emmc pin INPUT value, 
after set some pull value. And such detection works, but if no card is 
detected, then unfortunately mmc channel is not registered and mmc 
rescan command will not work for it, so I dropped this code.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Stephen Warren]

On 02/17/2015 10:01 PM, Simon Glass wrote:
> +Stephen who might have an opinion on this.
>
> Hi Przemyslaw,
>
> On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> This commits extends:
>> - dm gpio ops by: 'set_pull' call
>> - dm gpio uclass by: dm_gpio_set_pull() function
>>
>> The pull mode is not defined so should be driver specific.
>
> It's good to implement this, but I think you should try to have a
> standard interface. You could define the options you want to support
> and pass in a standard value.
>
> Otherwise we are not really providing a driver abstraction, only an interface.

I don't think that pull is a GPIO-related function/property. At least on 
Tegra, the GPIO controller allows you to set the pin direction and the 
output value and that's it. Configuring pull-up/down and other IO 
related properties is done in the pinmux controller instead. I don't 
think we want a standard API that has to touch both HW modules at once. 
What common code needs to manipulate a GPIO's pull-up/down setting? As 
precedent observe that pull-up/down isn't part of the Linux kernel's 
GPIO API, but rather that's part of the SoC-specific pinctrl driver, 
which controls pinmuxing etc.

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello,

On 02/18/2015 05:39 PM, Stephen Warren wrote:
> On 02/17/2015 10:01 PM, Simon Glass wrote:
>> +Stephen who might have an opinion on this.
>>
>> Hi Przemyslaw,
>>
>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>> <p.marczak@samsung.com> wrote:
>>> This commits extends:
>>> - dm gpio ops by: 'set_pull' call
>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>
>>> The pull mode is not defined so should be driver specific.
>>
>> It's good to implement this, but I think you should try to have a
>> standard interface. You could define the options you want to support
>> and pass in a standard value.
>>
>> Otherwise we are not really providing a driver abstraction, only an
>> interface.
>
> I don't think that pull is a GPIO-related function/property. At least on
> Tegra, the GPIO controller allows you to set the pin direction and the
> output value and that's it. Configuring pull-up/down and other IO
> related properties is done in the pinmux controller instead. I don't
> think we want a standard API that has to touch both HW modules at once.
> What common code needs to manipulate a GPIO's pull-up/down setting? As
> precedent observe that pull-up/down isn't part of the Linux kernel's
> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
> which controls pinmuxing etc.
>

This is a quite different than in the Exynos, where all the gpio 
functions and properties can be set by few registers within range of 
each gpio port base address. So in this case we don't touch another 
hardware module, we modify one of available gpio related registers.

Anyway, if we want to have a single and common gpio API in the future, 
then I think it is better to add pull option. And the driver will 
implement what is required, instead of provide common and private api 
for each driver.

For the various devices it is unclear, what should be pinmux and what 
should be gpio driver.

Moreover in my opinion from the single external pin point of view the 
pull up/down is the property of that pin.

Actually for Exynos, the pinmux is an abstraction and uses only GPIO 
driver api in U-Boot.

Do we need pinmux class?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Stephen Warren]

On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
> Hello,
>
> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>> +Stephen who might have an opinion on this.
>>>
>>> Hi Przemyslaw,
>>>
>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>> <p.marczak@samsung.com> wrote:
>>>> This commits extends:
>>>> - dm gpio ops by: 'set_pull' call
>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>
>>>> The pull mode is not defined so should be driver specific.
>>>
>>> It's good to implement this, but I think you should try to have a
>>> standard interface. You could define the options you want to support
>>> and pass in a standard value.
>>>
>>> Otherwise we are not really providing a driver abstraction, only an
>>> interface.
>>
>> I don't think that pull is a GPIO-related function/property. At least on
>> Tegra, the GPIO controller allows you to set the pin direction and the
>> output value and that's it. Configuring pull-up/down and other IO
>> related properties is done in the pinmux controller instead. I don't
>> think we want a standard API that has to touch both HW modules at once.
>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>> precedent observe that pull-up/down isn't part of the Linux kernel's
>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>> which controls pinmuxing etc.
>>
>
> This is a quite different than in the Exynos, where all the gpio
> functions and properties can be set by few registers within range of
> each gpio port base address. So in this case we don't touch another
> hardware module, we modify one of available gpio related registers.
>
> Anyway, if we want to have a single and common gpio API in the future,
> then I think it is better to add pull option.

Why? I'll ask again: What common driver code needs to manipulate pull-ups?

 > And the driver will
> implement what is required, instead of provide common and private api
> for each driver.

I'm not proposing driver-specific APIs, but rather having a common GPIO 
API and a common pinmux API. They need to be different since different 
HW modules may implement the functionality.

> For the various devices it is unclear, what should be pinmux and what
> should be gpio driver.

How about the following are GPIO:
* Set GPIO pin direction
* Read GPIO input
* Set GPIO output value

... and anything else is pinmux. That's the split in Linux and AFAIK it 
works out fine.

It'd be perfectly fine for the same driver code to implement both a GPIO 
and a pinmux driver, if the HW supports it.

> Moreover in my opinion from the single external pin point of view the
> pull up/down is the property of that pin.

It's a property of the same pin, but semantically it's not manipulating 
a GPIO-related function.

> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
> driver api in U-Boot.
>
> Do we need pinmux class?
>
> Best regards,

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello,

On 02/19/2015 06:09 PM, Stephen Warren wrote:
> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>> Hello,
>>
>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>> +Stephen who might have an opinion on this.
>>>>
>>>> Hi Przemyslaw,
>>>>
>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>> <p.marczak@samsung.com> wrote:
>>>>> This commits extends:
>>>>> - dm gpio ops by: 'set_pull' call
>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>
>>>>> The pull mode is not defined so should be driver specific.
>>>>
>>>> It's good to implement this, but I think you should try to have a
>>>> standard interface. You could define the options you want to support
>>>> and pass in a standard value.
>>>>
>>>> Otherwise we are not really providing a driver abstraction, only an
>>>> interface.
>>>
>>> I don't think that pull is a GPIO-related function/property. At least on
>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>> output value and that's it. Configuring pull-up/down and other IO
>>> related properties is done in the pinmux controller instead. I don't
>>> think we want a standard API that has to touch both HW modules at once.
>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>> which controls pinmuxing etc.
>>>
>>
>> This is a quite different than in the Exynos, where all the gpio
>> functions and properties can be set by few registers within range of
>> each gpio port base address. So in this case we don't touch another
>> hardware module, we modify one of available gpio related registers.
>>
>> Anyway, if we want to have a single and common gpio API in the future,
>> then I think it is better to add pull option.
>
> Why? I'll ask again: What common driver code needs to manipulate pull-ups?
>

Please look at driver: drivers/gpio/s5p_gpio.c

It's one driver related to one gpio hardware submodule and it takes care 
about standard gpio properties and also mux/pull/drv/rate.

And the exynos pinmux code is only a software abstraction:
arch/arm/cpu/armv7/exynos/pinmux.c


>  > And the driver will
>> implement what is required, instead of provide common and private api
>> for each driver.
>
> I'm not proposing driver-specific APIs, but rather having a common GPIO
> API and a common pinmux API. They need to be different since different
> HW modules may implement the functionality.
>

As in the above example, for the Exynos it's the one hw module, so it's 
simply.

>> For the various devices it is unclear, what should be pinmux and what
>> should be gpio driver.
>
> How about the following are GPIO:
> * Set GPIO pin direction
> * Read GPIO input
> * Set GPIO output value
>
> ... and anything else is pinmux. That's the split in Linux and AFAIK it
> works out fine.
>
> It'd be perfectly fine for the same driver code to implement both a GPIO
> and a pinmux driver, if the HW supports it.
>

Ok, I can drop this commit, since the current code works fine.

>> Moreover in my opinion from the single external pin point of view the
>> pull up/down is the property of that pin.
>
> It's a property of the same pin, but semantically it's not manipulating
> a GPIO-related function.
>
>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>> driver api in U-Boot.
>>
>> Do we need pinmux class?
>>
>> Best regards,
>
>

As I wrote in one of my previous e-mail, I was testing eMMC detect.
And setting the pull was required for this, before call the pinmux for 
eMMC pins.
But finally the eMMC detect seem to be not useful in case of the present 
'mmc rescan' command.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Stephen Warren]

On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
> Hello,
>
> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>> Hello,
>>>
>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>> +Stephen who might have an opinion on this.
>>>>>
>>>>> Hi Przemyslaw,
>>>>>
>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>> <p.marczak@samsung.com> wrote:
>>>>>> This commits extends:
>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>
>>>>>> The pull mode is not defined so should be driver specific.
>>>>>
>>>>> It's good to implement this, but I think you should try to have a
>>>>> standard interface. You could define the options you want to support
>>>>> and pass in a standard value.
>>>>>
>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>> interface.
>>>>
>>>> I don't think that pull is a GPIO-related function/property. At
>>>> least on
>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>> output value and that's it. Configuring pull-up/down and other IO
>>>> related properties is done in the pinmux controller instead. I don't
>>>> think we want a standard API that has to touch both HW modules at once.
>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>> which controls pinmuxing etc.
>>>>
>>>
>>> This is a quite different than in the Exynos, where all the gpio
>>> functions and properties can be set by few registers within range of
>>> each gpio port base address. So in this case we don't touch another
>>> hardware module, we modify one of available gpio related registers.
>>>
>>> Anyway, if we want to have a single and common gpio API in the future,
>>> then I think it is better to add pull option.
>>
>> Why? I'll ask again: What common driver code needs to manipulate
>> pull-ups?
>
> Please look at driver: drivers/gpio/s5p_gpio.c
>
> It's one driver related to one gpio hardware submodule and it takes care
> about standard gpio properties and also mux/pull/drv/rate.
>
> And the exynos pinmux code is only a software abstraction:
> arch/arm/cpu/armv7/exynos/pinmux.c

I didn't want to ask which driver implements the control of pullups, but 
rather which other driver needs to turn pullups on/off in a standard way 
across multiple SoCs.

In other words, do you expect code in common/ to need to call a "set pin 
pullup" function? If so, then we certainly need a standard API to 
manipulate pullups. However if no common code needs to manipulate 
pullups, then I'd argue we don't actually need a common API to do this, 
since there's no code that would call that common API.

>>  > And the driver will
>>> implement what is required, instead of provide common and private api
>>> for each driver.
>>
>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>> API and a common pinmux API. They need to be different since different
>> HW modules may implement the functionality.
>>
>
> As in the above example, for the Exynos it's the one hw module, so it's
> simply.
>
>>> For the various devices it is unclear, what should be pinmux and what
>>> should be gpio driver.
>>
>> How about the following are GPIO:
>> * Set GPIO pin direction
>> * Read GPIO input
>> * Set GPIO output value
>>
>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>> works out fine.
>>
>> It'd be perfectly fine for the same driver code to implement both a GPIO
>> and a pinmux driver, if the HW supports it.
>>
>
> Ok, I can drop this commit, since the current code works fine.
>
>>> Moreover in my opinion from the single external pin point of view the
>>> pull up/down is the property of that pin.
>>
>> It's a property of the same pin, but semantically it's not manipulating
>> a GPIO-related function.
>>
>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>> driver api in U-Boot.
>>>
>>> Do we need pinmux class?
>>>
>>> Best regards,
>>
>>
>
> As I wrote in one of my previous e-mail, I was testing eMMC detect.
> And setting the pull was required for this, before call the pinmux for
> eMMC pins.
> But finally the eMMC detect seem to be not useful in case of the present
> 'mmc rescan' command.

Why wouldn't the pinmux driver for the whole system simply apply the 
board's whole pinmux configuration before initializing any IO controller 
drivers? IO controller drivers shouldn't have to initialize 
board-/SoC-specific pinmux, but the board-/SoC-specfic code should do so.

At most, the eMMC driver should call a function such as pinmux_emmc(), 
and the board/SoC code should implement that as appropriate for that 
board. The eMMC driver shouldn't have to know about applying specific 
pullups/downs to specific pins (since those settings and pins are likely 
board-/SoC-specific, and drivers shouldn't know about 
board-/SoC-specific details). The only exception would be if the 
standard IO protocol requires pullups to be changed during regular 
operation. In which case, a specific callback from the driver could be 
added for each protocol-mandated configuration change, thus keeping the 
IO controller driver still completely isolated from details of the pins 
and pinmux APIs etc.

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Simon Glass]

Hi,

On 20 February 2015 at 10:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>
>> Hello,
>>
>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>
>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>
>>>> Hello,
>>>>
>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>
>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>
>>>>>> +Stephen who might have an opinion on this.
>>>>>>
>>>>>> Hi Przemyslaw,
>>>>>>
>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>>
>>>>>>> This commits extends:
>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>
>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>
>>>>>>
>>>>>> It's good to implement this, but I think you should try to have a
>>>>>> standard interface. You could define the options you want to support
>>>>>> and pass in a standard value.
>>>>>>
>>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>>> interface.
>>>>>
>>>>>
>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>> least on
>>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>> related properties is done in the pinmux controller instead. I don't
>>>>> think we want a standard API that has to touch both HW modules at once.
>>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>>> which controls pinmuxing etc.
>>>>>
>>>>
>>>> This is a quite different than in the Exynos, where all the gpio
>>>> functions and properties can be set by few registers within range of
>>>> each gpio port base address. So in this case we don't touch another
>>>> hardware module, we modify one of available gpio related registers.
>>>>
>>>> Anyway, if we want to have a single and common gpio API in the future,
>>>> then I think it is better to add pull option.
>>>
>>>
>>> Why? I'll ask again: What common driver code needs to manipulate
>>> pull-ups?
>>
>>
>> Please look at driver: drivers/gpio/s5p_gpio.c
>>
>> It's one driver related to one gpio hardware submodule and it takes care
>> about standard gpio properties and also mux/pull/drv/rate.
>>
>> And the exynos pinmux code is only a software abstraction:
>> arch/arm/cpu/armv7/exynos/pinmux.c
>
>
> I didn't want to ask which driver implements the control of pullups, but
> rather which other driver needs to turn pullups on/off in a standard way
> across multiple SoCs.
>
> In other words, do you expect code in common/ to need to call a "set pin
> pullup" function? If so, then we certainly need a standard API to manipulate
> pullups. However if no common code needs to manipulate pullups, then I'd
> argue we don't actually need a common API to do this, since there's no code
> that would call that common API.

We do currently use the GPIO to handle pullup/pulldown for some boards
so until we have a pinmux API (which might be a long while) it seems
reasonable for it to live there.

If not, does anyone plan to write such an API?

>
>
>>>  > And the driver will
>>>>
>>>> implement what is required, instead of provide common and private api
>>>> for each driver.
>>>
>>>
>>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>>> API and a common pinmux API. They need to be different since different
>>> HW modules may implement the functionality.
>>>
>>
>> As in the above example, for the Exynos it's the one hw module, so it's
>> simply.
>>
>>>> For the various devices it is unclear, what should be pinmux and what
>>>> should be gpio driver.
>>>
>>>
>>> How about the following are GPIO:
>>> * Set GPIO pin direction
>>> * Read GPIO input
>>> * Set GPIO output value
>>>
>>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>>> works out fine.
>>>
>>> It'd be perfectly fine for the same driver code to implement both a GPIO
>>> and a pinmux driver, if the HW supports it.
>>>
>>
>> Ok, I can drop this commit, since the current code works fine.
>>
>>>> Moreover in my opinion from the single external pin point of view the
>>>> pull up/down is the property of that pin.
>>>
>>>
>>> It's a property of the same pin, but semantically it's not manipulating
>>> a GPIO-related function.
>>>
>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>> driver api in U-Boot.
>>>>
>>>> Do we need pinmux class?
>>>>
>>>> Best regards,
>>>
>>>
>>>
>>
>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>> And setting the pull was required for this, before call the pinmux for
>> eMMC pins.
>> But finally the eMMC detect seem to be not useful in case of the present
>> 'mmc rescan' command.
>
>
> Why wouldn't the pinmux driver for the whole system simply apply the board's
> whole pinmux configuration before initializing any IO controller drivers? IO
> controller drivers shouldn't have to initialize board-/SoC-specific pinmux,
> but the board-/SoC-specfic code should do so.
>
> At most, the eMMC driver should call a function such as pinmux_emmc(), and
> the board/SoC code should implement that as appropriate for that board. The
> eMMC driver shouldn't have to know about applying specific pullups/downs to
> specific pins (since those settings and pins are likely board-/SoC-specific,
> and drivers shouldn't know about board-/SoC-specific details). The only

No this way lies madness. It is how things work on Jetson and Nyan.
Loads of opaque tables and no idea what the pins are connected to. It
has some value for pins that U-Boot doesn't use (so we are just
setting them up for Linux) but even then it is really opaque.

We can't even sent patches to the file because it is auto-generated
from a tool in another repo. Tiny differences between boards are
hidden because we repeat all the information again with just a line or
two of changes. I really don't want exynos to go that way.

> exception would be if the standard IO protocol requires pullups to be
> changed during regular operation. In which case, a specific callback from
> the driver could be added for each protocol-mandated configuration change,
> thus keeping the IO controller driver still completely isolated from details
> of the pins and pinmux APIs etc.

This is like the 'funcmux' in Tegra I think. I think this is more
useful and we should use it to set up all peripheral pins. We can
review the code, see changes, understand what they relate to, etc.

Anyway this all seems off-topic from this patch.

Unless someone plans to write a pinmux subsystem for U-Boot (which I
agree would be better) I think the general approach of this patch is
good.

Regards,
Simon

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello Simon,

On 02/20/2015 08:29 PM, Simon Glass wrote:
> Hi,
>
> On 20 February 2015 at 10:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>>
>>> Hello,
>>>
>>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>>
>>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>>
>>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> +Stephen who might have an opinion on this.
>>>>>>>
>>>>>>> Hi Przemyslaw,
>>>>>>>
>>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>>>
>>>>>>>> This commits extends:
>>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>>
>>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>>
>>>>>>>
>>>>>>> It's good to implement this, but I think you should try to have a
>>>>>>> standard interface. You could define the options you want to support
>>>>>>> and pass in a standard value.
>>>>>>>
>>>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>>>> interface.
>>>>>>
>>>>>>
>>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>>> least on
>>>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>>> related properties is done in the pinmux controller instead. I don't
>>>>>> think we want a standard API that has to touch both HW modules at once.
>>>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>>>> which controls pinmuxing etc.
>>>>>>
>>>>>
>>>>> This is a quite different than in the Exynos, where all the gpio
>>>>> functions and properties can be set by few registers within range of
>>>>> each gpio port base address. So in this case we don't touch another
>>>>> hardware module, we modify one of available gpio related registers.
>>>>>
>>>>> Anyway, if we want to have a single and common gpio API in the future,
>>>>> then I think it is better to add pull option.
>>>>
>>>>
>>>> Why? I'll ask again: What common driver code needs to manipulate
>>>> pull-ups?
>>>
>>>
>>> Please look at driver: drivers/gpio/s5p_gpio.c
>>>
>>> It's one driver related to one gpio hardware submodule and it takes care
>>> about standard gpio properties and also mux/pull/drv/rate.
>>>
>>> And the exynos pinmux code is only a software abstraction:
>>> arch/arm/cpu/armv7/exynos/pinmux.c
>>
>>
>> I didn't want to ask which driver implements the control of pullups, but
>> rather which other driver needs to turn pullups on/off in a standard way
>> across multiple SoCs.
>>
>> In other words, do you expect code in common/ to need to call a "set pin
>> pullup" function? If so, then we certainly need a standard API to manipulate
>> pullups. However if no common code needs to manipulate pullups, then I'd
>> argue we don't actually need a common API to do this, since there's no code
>> that would call that common API.
>
> We do currently use the GPIO to handle pullup/pulldown for some boards
> so until we have a pinmux API (which might be a long while) it seems
> reasonable for it to live there.
>
> If not, does anyone plan to write such an API?
>

Right, we uses this in most Exynos boards. But the boards uses direct 
calls to s5p gpio driver, without uclass.
I wonder if wouldn't it be better and faster to leave the board 
low-level init routines as they are now.

>>
>>
>>>>   > And the driver will
>>>>>
>>>>> implement what is required, instead of provide common and private api
>>>>> for each driver.
>>>>
>>>>
>>>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>>>> API and a common pinmux API. They need to be different since different
>>>> HW modules may implement the functionality.
>>>>
>>>
>>> As in the above example, for the Exynos it's the one hw module, so it's
>>> simply.
>>>
>>>>> For the various devices it is unclear, what should be pinmux and what
>>>>> should be gpio driver.
>>>>
>>>>
>>>> How about the following are GPIO:
>>>> * Set GPIO pin direction
>>>> * Read GPIO input
>>>> * Set GPIO output value
>>>>
>>>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>>>> works out fine.
>>>>
>>>> It'd be perfectly fine for the same driver code to implement both a GPIO
>>>> and a pinmux driver, if the HW supports it.
>>>>
>>>
>>> Ok, I can drop this commit, since the current code works fine.
>>>
>>>>> Moreover in my opinion from the single external pin point of view the
>>>>> pull up/down is the property of that pin.
>>>>
>>>>
>>>> It's a property of the same pin, but semantically it's not manipulating
>>>> a GPIO-related function.
>>>>
>>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>>> driver api in U-Boot.
>>>>>
>>>>> Do we need pinmux class?
>>>>>
>>>>> Best regards,
>>>>
>>>>
>>>>
>>>
>>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>>> And setting the pull was required for this, before call the pinmux for
>>> eMMC pins.
>>> But finally the eMMC detect seem to be not useful in case of the present
>>> 'mmc rescan' command.
>>
>>
>> Why wouldn't the pinmux driver for the whole system simply apply the board's
>> whole pinmux configuration before initializing any IO controller drivers? IO
>> controller drivers shouldn't have to initialize board-/SoC-specific pinmux,
>> but the board-/SoC-specfic code should do so.
>>
>> At most, the eMMC driver should call a function such as pinmux_emmc(), and
>> the board/SoC code should implement that as appropriate for that board. The
>> eMMC driver shouldn't have to know about applying specific pullups/downs to
>> specific pins (since those settings and pins are likely board-/SoC-specific,
>> and drivers shouldn't know about board-/SoC-specific details). The only
>
> No this way lies madness. It is how things work on Jetson and Nyan.
> Loads of opaque tables and no idea what the pins are connected to. It
> has some value for pins that U-Boot doesn't use (so we are just
> setting them up for Linux) but even then it is really opaque.
>
> We can't even sent patches to the file because it is auto-generated
> from a tool in another repo. Tiny differences between boards are
> hidden because we repeat all the information again with just a line or
> two of changes. I really don't want exynos to go that way.
>
>> exception would be if the standard IO protocol requires pullups to be
>> changed during regular operation. In which case, a specific callback from
>> the driver could be added for each protocol-mandated configuration change,
>> thus keeping the IO controller driver still completely isolated from details
>> of the pins and pinmux APIs etc.
>
> This is like the 'funcmux' in Tegra I think. I think this is more
> useful and we should use it to set up all peripheral pins. We can
> review the code, see changes, understand what they relate to, etc.
>
> Anyway this all seems off-topic from this patch.
>
> Unless someone plans to write a pinmux subsystem for U-Boot (which I
> agree would be better) I think the general approach of this patch is
> good.
>
> Regards,
> Simon
>

Ok, so there are two next versions of this patch-set.
Please decide, which one is better.

For me, at present, the current s5p_gpio api works fine for all the 
exynos based boards.
Introducing the pinmux uclass is not a quick task, now I'm trying to 
focus on pmic.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Simon Glass]

Hi,

On 23 February 2015 at 03:51, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>
> Hello Simon,
>
>
> On 02/20/2015 08:29 PM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 20 February 2015 at 10:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>>>
>>>>
>>>> Hello,
>>>>
>>>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>>>
>>>>>
>>>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> +Stephen who might have an opinion on this.
>>>>>>>>
>>>>>>>> Hi Przemyslaw,
>>>>>>>>
>>>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This commits extends:
>>>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>>>
>>>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It's good to implement this, but I think you should try to have a
>>>>>>>> standard interface. You could define the options you want to support
>>>>>>>> and pass in a standard value.
>>>>>>>>
>>>>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>>>>> interface.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>>>> least on
>>>>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>>>> related properties is done in the pinmux controller instead. I don't
>>>>>>> think we want a standard API that has to touch both HW modules at once.
>>>>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>>>>> which controls pinmuxing etc.
>>>>>>>
>>>>>>
>>>>>> This is a quite different than in the Exynos, where all the gpio
>>>>>> functions and properties can be set by few registers within range of
>>>>>> each gpio port base address. So in this case we don't touch another
>>>>>> hardware module, we modify one of available gpio related registers.
>>>>>>
>>>>>> Anyway, if we want to have a single and common gpio API in the future,
>>>>>> then I think it is better to add pull option.
>>>>>
>>>>>
>>>>>
>>>>> Why? I'll ask again: What common driver code needs to manipulate
>>>>> pull-ups?
>>>>
>>>>
>>>>
>>>> Please look at driver: drivers/gpio/s5p_gpio.c
>>>>
>>>> It's one driver related to one gpio hardware submodule and it takes care
>>>> about standard gpio properties and also mux/pull/drv/rate.
>>>>
>>>> And the exynos pinmux code is only a software abstraction:
>>>> arch/arm/cpu/armv7/exynos/pinmux.c
>>>
>>>
>>>
>>> I didn't want to ask which driver implements the control of pullups, but
>>> rather which other driver needs to turn pullups on/off in a standard way
>>> across multiple SoCs.
>>>
>>> In other words, do you expect code in common/ to need to call a "set pin
>>> pullup" function? If so, then we certainly need a standard API to manipulate
>>> pullups. However if no common code needs to manipulate pullups, then I'd
>>> argue we don't actually need a common API to do this, since there's no code
>>> that would call that common API.
>>
>>
>> We do currently use the GPIO to handle pullup/pulldown for some boards
>> so until we have a pinmux API (which might be a long while) it seems
>> reasonable for it to live there.
>>
>> If not, does anyone plan to write such an API?
>>
>
> Right, we uses this in most Exynos boards. But the boards uses direct calls to s5p gpio driver, without uclass.
> I wonder if wouldn't it be better and faster to leave the board low-level init routines as they are now.
>
>
>>>
>>>
>>>>>   > And the driver will
>>>>>>
>>>>>>
>>>>>> implement what is required, instead of provide common and private api
>>>>>> for each driver.
>>>>>
>>>>>
>>>>>
>>>>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>>>>> API and a common pinmux API. They need to be different since different
>>>>> HW modules may implement the functionality.
>>>>>
>>>>
>>>> As in the above example, for the Exynos it's the one hw module, so it's
>>>> simply.
>>>>
>>>>>> For the various devices it is unclear, what should be pinmux and what
>>>>>> should be gpio driver.
>>>>>
>>>>>
>>>>>
>>>>> How about the following are GPIO:
>>>>> * Set GPIO pin direction
>>>>> * Read GPIO input
>>>>> * Set GPIO output value
>>>>>
>>>>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>>>>> works out fine.
>>>>>
>>>>> It'd be perfectly fine for the same driver code to implement both a GPIO
>>>>> and a pinmux driver, if the HW supports it.
>>>>>
>>>>
>>>> Ok, I can drop this commit, since the current code works fine.
>>>>
>>>>>> Moreover in my opinion from the single external pin point of view the
>>>>>> pull up/down is the property of that pin.
>>>>>
>>>>>
>>>>>
>>>>> It's a property of the same pin, but semantically it's not manipulating
>>>>> a GPIO-related function.
>>>>>
>>>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>>>> driver api in U-Boot.
>>>>>>
>>>>>> Do we need pinmux class?
>>>>>>
>>>>>> Best regards,
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>>>> And setting the pull was required for this, before call the pinmux for
>>>> eMMC pins.
>>>> But finally the eMMC detect seem to be not useful in case of the present
>>>> 'mmc rescan' command.
>>>
>>>
>>>
>>> Why wouldn't the pinmux driver for the whole system simply apply the board's
>>> whole pinmux configuration before initializing any IO controller drivers? IO
>>> controller drivers shouldn't have to initialize board-/SoC-specific pinmux,
>>> but the board-/SoC-specfic code should do so.
>>>
>>> At most, the eMMC driver should call a function such as pinmux_emmc(), and
>>> the board/SoC code should implement that as appropriate for that board. The
>>> eMMC driver shouldn't have to know about applying specific pullups/downs to
>>> specific pins (since those settings and pins are likely board-/SoC-specific,
>>> and drivers shouldn't know about board-/SoC-specific details). The only
>>
>>
>> No this way lies madness. It is how things work on Jetson and Nyan.
>> Loads of opaque tables and no idea what the pins are connected to. It
>> has some value for pins that U-Boot doesn't use (so we are just
>> setting them up for Linux) but even then it is really opaque.
>>
>> We can't even sent patches to the file because it is auto-generated
>> from a tool in another repo. Tiny differences between boards are
>> hidden because we repeat all the information again with just a line or
>> two of changes. I really don't want exynos to go that way.
>>
>>> exception would be if the standard IO protocol requires pullups to be
>>> changed during regular operation. In which case, a specific callback from
>>> the driver could be added for each protocol-mandated configuration change,
>>> thus keeping the IO controller driver still completely isolated from details
>>> of the pins and pinmux APIs etc.
>>
>>
>> This is like the 'funcmux' in Tegra I think. I think this is more
>> useful and we should use it to set up all peripheral pins. We can
>> review the code, see changes, understand what they relate to, etc.
>>
>> Anyway this all seems off-topic from this patch.
>>
>> Unless someone plans to write a pinmux subsystem for U-Boot (which I
>> agree would be better) I think the general approach of this patch is
>> good.
>>
>> Regards,
>> Simon
>>
>
> Ok, so there are two next versions of this patch-set.
> Please decide, which one is better.
>
> For me, at present, the current s5p_gpio api works fine for all the exynos based boards.
> Introducing the pinmux uclass is not a quick task, now I'm trying to focus on pmic.

OK, then I think we should probably leave it as it is. If we add
pull-ups to driver model it should be done with pinctl as Stephen
says. I doubt this is a huge task, since we can likely port over the
code from Linux. But for now I think we should keep with the s5p API
until someone takes on pinctl.

Regards,
Simon

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello,

On 02/23/2015 04:30 PM, Simon Glass wrote:
> Hi,
>
> On 23 February 2015 at 03:51, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>>
>> Hello Simon,
>>
>>
>> On 02/20/2015 08:29 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 20 February 2015 at 10:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>>>>
>>>>>>
>>>>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> +Stephen who might have an opinion on this.
>>>>>>>>>
>>>>>>>>> Hi Przemyslaw,
>>>>>>>>>
>>>>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This commits extends:
>>>>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>>>>
>>>>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It's good to implement this, but I think you should try to have a
>>>>>>>>> standard interface. You could define the options you want to support
>>>>>>>>> and pass in a standard value.
>>>>>>>>>
>>>>>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>>>>>> interface.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>>>>> least on
>>>>>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>>>>> related properties is done in the pinmux controller instead. I don't
>>>>>>>> think we want a standard API that has to touch both HW modules at once.
>>>>>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>>>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>>>>>> which controls pinmuxing etc.
>>>>>>>>
>>>>>>>
>>>>>>> This is a quite different than in the Exynos, where all the gpio
>>>>>>> functions and properties can be set by few registers within range of
>>>>>>> each gpio port base address. So in this case we don't touch another
>>>>>>> hardware module, we modify one of available gpio related registers.
>>>>>>>
>>>>>>> Anyway, if we want to have a single and common gpio API in the future,
>>>>>>> then I think it is better to add pull option.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why? I'll ask again: What common driver code needs to manipulate
>>>>>> pull-ups?
>>>>>
>>>>>
>>>>>
>>>>> Please look at driver: drivers/gpio/s5p_gpio.c
>>>>>
>>>>> It's one driver related to one gpio hardware submodule and it takes care
>>>>> about standard gpio properties and also mux/pull/drv/rate.
>>>>>
>>>>> And the exynos pinmux code is only a software abstraction:
>>>>> arch/arm/cpu/armv7/exynos/pinmux.c
>>>>
>>>>
>>>>
>>>> I didn't want to ask which driver implements the control of pullups, but
>>>> rather which other driver needs to turn pullups on/off in a standard way
>>>> across multiple SoCs.
>>>>
>>>> In other words, do you expect code in common/ to need to call a "set pin
>>>> pullup" function? If so, then we certainly need a standard API to manipulate
>>>> pullups. However if no common code needs to manipulate pullups, then I'd
>>>> argue we don't actually need a common API to do this, since there's no code
>>>> that would call that common API.
>>>
>>>
>>> We do currently use the GPIO to handle pullup/pulldown for some boards
>>> so until we have a pinmux API (which might be a long while) it seems
>>> reasonable for it to live there.
>>>
>>> If not, does anyone plan to write such an API?
>>>
>>
>> Right, we uses this in most Exynos boards. But the boards uses direct calls to s5p gpio driver, without uclass.
>> I wonder if wouldn't it be better and faster to leave the board low-level init routines as they are now.
>>
>>
>>>>
>>>>
>>>>>>    > And the driver will
>>>>>>>
>>>>>>>
>>>>>>> implement what is required, instead of provide common and private api
>>>>>>> for each driver.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>>>>>> API and a common pinmux API. They need to be different since different
>>>>>> HW modules may implement the functionality.
>>>>>>
>>>>>
>>>>> As in the above example, for the Exynos it's the one hw module, so it's
>>>>> simply.
>>>>>
>>>>>>> For the various devices it is unclear, what should be pinmux and what
>>>>>>> should be gpio driver.
>>>>>>
>>>>>>
>>>>>>
>>>>>> How about the following are GPIO:
>>>>>> * Set GPIO pin direction
>>>>>> * Read GPIO input
>>>>>> * Set GPIO output value
>>>>>>
>>>>>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>>>>>> works out fine.
>>>>>>
>>>>>> It'd be perfectly fine for the same driver code to implement both a GPIO
>>>>>> and a pinmux driver, if the HW supports it.
>>>>>>
>>>>>
>>>>> Ok, I can drop this commit, since the current code works fine.
>>>>>
>>>>>>> Moreover in my opinion from the single external pin point of view the
>>>>>>> pull up/down is the property of that pin.
>>>>>>
>>>>>>
>>>>>>
>>>>>> It's a property of the same pin, but semantically it's not manipulating
>>>>>> a GPIO-related function.
>>>>>>
>>>>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>>>>> driver api in U-Boot.
>>>>>>>
>>>>>>> Do we need pinmux class?
>>>>>>>
>>>>>>> Best regards,
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>>>>> And setting the pull was required for this, before call the pinmux for
>>>>> eMMC pins.
>>>>> But finally the eMMC detect seem to be not useful in case of the present
>>>>> 'mmc rescan' command.
>>>>
>>>>
>>>>
>>>> Why wouldn't the pinmux driver for the whole system simply apply the board's
>>>> whole pinmux configuration before initializing any IO controller drivers? IO
>>>> controller drivers shouldn't have to initialize board-/SoC-specific pinmux,
>>>> but the board-/SoC-specfic code should do so.
>>>>
>>>> At most, the eMMC driver should call a function such as pinmux_emmc(), and
>>>> the board/SoC code should implement that as appropriate for that board. The
>>>> eMMC driver shouldn't have to know about applying specific pullups/downs to
>>>> specific pins (since those settings and pins are likely board-/SoC-specific,
>>>> and drivers shouldn't know about board-/SoC-specific details). The only
>>>
>>>
>>> No this way lies madness. It is how things work on Jetson and Nyan.
>>> Loads of opaque tables and no idea what the pins are connected to. It
>>> has some value for pins that U-Boot doesn't use (so we are just
>>> setting them up for Linux) but even then it is really opaque.
>>>
>>> We can't even sent patches to the file because it is auto-generated
>>> from a tool in another repo. Tiny differences between boards are
>>> hidden because we repeat all the information again with just a line or
>>> two of changes. I really don't want exynos to go that way.
>>>
>>>> exception would be if the standard IO protocol requires pullups to be
>>>> changed during regular operation. In which case, a specific callback from
>>>> the driver could be added for each protocol-mandated configuration change,
>>>> thus keeping the IO controller driver still completely isolated from details
>>>> of the pins and pinmux APIs etc.
>>>
>>>
>>> This is like the 'funcmux' in Tegra I think. I think this is more
>>> useful and we should use it to set up all peripheral pins. We can
>>> review the code, see changes, understand what they relate to, etc.
>>>
>>> Anyway this all seems off-topic from this patch.
>>>
>>> Unless someone plans to write a pinmux subsystem for U-Boot (which I
>>> agree would be better) I think the general approach of this patch is
>>> good.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> Ok, so there are two next versions of this patch-set.
>> Please decide, which one is better.
>>
>> For me, at present, the current s5p_gpio api works fine for all the exynos based boards.
>> Introducing the pinmux uclass is not a quick task, now I'm trying to focus on pmic.
>
> OK, then I think we should probably leave it as it is. If we add
> pull-ups to driver model it should be done with pinctl as Stephen
> says. I doubt this is a huge task, since we can likely port over the
> code from Linux. But for now I think we should keep with the s5p API
> until someone takes on pinctl.
>
> Regards,
> Simon
>

Great, in this case, can the v3 be accepted?

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Simon Glass]

Hi Przemyslaw,

On 23 February 2015 at 09:56, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Hello,
>
>
> On 02/23/2015 04:30 PM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 23 February 2015 at 03:51, Przemyslaw Marczak <p.marczak@samsung.com>
>> wrote:
>>>
>>>
>>> Hello Simon,
>>>
>>>
>>> On 02/20/2015 08:29 PM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> On 20 February 2015 at 10:50, Stephen Warren <swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> +Stephen who might have an opinion on this.
>>>>>>>>>>
>>>>>>>>>> Hi Przemyslaw,
>>>>>>>>>>
>>>>>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This commits extends:
>>>>>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>>>>>
>>>>>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It's good to implement this, but I think you should try to have a
>>>>>>>>>> standard interface. You could define the options you want to
>>>>>>>>>> support
>>>>>>>>>> and pass in a standard value.
>>>>>>>>>>
>>>>>>>>>> Otherwise we are not really providing a driver abstraction, only
>>>>>>>>>> an
>>>>>>>>>> interface.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>>>>>> least on
>>>>>>>>> Tegra, the GPIO controller allows you to set the pin direction and
>>>>>>>>> the
>>>>>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>>>>>> related properties is done in the pinmux controller instead. I
>>>>>>>>> don't
>>>>>>>>> think we want a standard API that has to touch both HW modules at
>>>>>>>>> once.
>>>>>>>>> What common code needs to manipulate a GPIO's pull-up/down setting?
>>>>>>>>> As
>>>>>>>>> precedent observe that pull-up/down isn't part of the Linux
>>>>>>>>> kernel's
>>>>>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl
>>>>>>>>> driver,
>>>>>>>>> which controls pinmuxing etc.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is a quite different than in the Exynos, where all the gpio
>>>>>>>> functions and properties can be set by few registers within range of
>>>>>>>> each gpio port base address. So in this case we don't touch another
>>>>>>>> hardware module, we modify one of available gpio related registers.
>>>>>>>>
>>>>>>>> Anyway, if we want to have a single and common gpio API in the
>>>>>>>> future,
>>>>>>>> then I think it is better to add pull option.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Why? I'll ask again: What common driver code needs to manipulate
>>>>>>> pull-ups?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Please look at driver: drivers/gpio/s5p_gpio.c
>>>>>>
>>>>>> It's one driver related to one gpio hardware submodule and it takes
>>>>>> care
>>>>>> about standard gpio properties and also mux/pull/drv/rate.
>>>>>>
>>>>>> And the exynos pinmux code is only a software abstraction:
>>>>>> arch/arm/cpu/armv7/exynos/pinmux.c
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I didn't want to ask which driver implements the control of pullups,
>>>>> but
>>>>> rather which other driver needs to turn pullups on/off in a standard
>>>>> way
>>>>> across multiple SoCs.
>>>>>
>>>>> In other words, do you expect code in common/ to need to call a "set
>>>>> pin
>>>>> pullup" function? If so, then we certainly need a standard API to
>>>>> manipulate
>>>>> pullups. However if no common code needs to manipulate pullups, then
>>>>> I'd
>>>>> argue we don't actually need a common API to do this, since there's no
>>>>> code
>>>>> that would call that common API.
>>>>
>>>>
>>>>
>>>> We do currently use the GPIO to handle pullup/pulldown for some boards
>>>> so until we have a pinmux API (which might be a long while) it seems
>>>> reasonable for it to live there.
>>>>
>>>> If not, does anyone plan to write such an API?
>>>>
>>>
>>> Right, we uses this in most Exynos boards. But the boards uses direct
>>> calls to s5p gpio driver, without uclass.
>>> I wonder if wouldn't it be better and faster to leave the board low-level
>>> init routines as they are now.
>>>
>>>
>>>>>
>>>>>
>>>>>>>    > And the driver will
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> implement what is required, instead of provide common and private
>>>>>>>> api
>>>>>>>> for each driver.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'm not proposing driver-specific APIs, but rather having a common
>>>>>>> GPIO
>>>>>>> API and a common pinmux API. They need to be different since
>>>>>>> different
>>>>>>> HW modules may implement the functionality.
>>>>>>>
>>>>>>
>>>>>> As in the above example, for the Exynos it's the one hw module, so
>>>>>> it's
>>>>>> simply.
>>>>>>
>>>>>>>> For the various devices it is unclear, what should be pinmux and
>>>>>>>> what
>>>>>>>> should be gpio driver.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> How about the following are GPIO:
>>>>>>> * Set GPIO pin direction
>>>>>>> * Read GPIO input
>>>>>>> * Set GPIO output value
>>>>>>>
>>>>>>> ... and anything else is pinmux. That's the split in Linux and AFAIK
>>>>>>> it
>>>>>>> works out fine.
>>>>>>>
>>>>>>> It'd be perfectly fine for the same driver code to implement both a
>>>>>>> GPIO
>>>>>>> and a pinmux driver, if the HW supports it.
>>>>>>>
>>>>>>
>>>>>> Ok, I can drop this commit, since the current code works fine.
>>>>>>
>>>>>>>> Moreover in my opinion from the single external pin point of view
>>>>>>>> the
>>>>>>>> pull up/down is the property of that pin.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It's a property of the same pin, but semantically it's not
>>>>>>> manipulating
>>>>>>> a GPIO-related function.
>>>>>>>
>>>>>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>>>>>> driver api in U-Boot.
>>>>>>>>
>>>>>>>> Do we need pinmux class?
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>>>>>> And setting the pull was required for this, before call the pinmux for
>>>>>> eMMC pins.
>>>>>> But finally the eMMC detect seem to be not useful in case of the
>>>>>> present
>>>>>> 'mmc rescan' command.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Why wouldn't the pinmux driver for the whole system simply apply the
>>>>> board's
>>>>> whole pinmux configuration before initializing any IO controller
>>>>> drivers? IO
>>>>> controller drivers shouldn't have to initialize board-/SoC-specific
>>>>> pinmux,
>>>>> but the board-/SoC-specfic code should do so.
>>>>>
>>>>> At most, the eMMC driver should call a function such as pinmux_emmc(),
>>>>> and
>>>>> the board/SoC code should implement that as appropriate for that board.
>>>>> The
>>>>> eMMC driver shouldn't have to know about applying specific
>>>>> pullups/downs to
>>>>> specific pins (since those settings and pins are likely
>>>>> board-/SoC-specific,
>>>>> and drivers shouldn't know about board-/SoC-specific details). The only
>>>>
>>>>
>>>>
>>>> No this way lies madness. It is how things work on Jetson and Nyan.
>>>> Loads of opaque tables and no idea what the pins are connected to. It
>>>> has some value for pins that U-Boot doesn't use (so we are just
>>>> setting them up for Linux) but even then it is really opaque.
>>>>
>>>> We can't even sent patches to the file because it is auto-generated
>>>> from a tool in another repo. Tiny differences between boards are
>>>> hidden because we repeat all the information again with just a line or
>>>> two of changes. I really don't want exynos to go that way.
>>>>
>>>>> exception would be if the standard IO protocol requires pullups to be
>>>>> changed during regular operation. In which case, a specific callback
>>>>> from
>>>>> the driver could be added for each protocol-mandated configuration
>>>>> change,
>>>>> thus keeping the IO controller driver still completely isolated from
>>>>> details
>>>>> of the pins and pinmux APIs etc.
>>>>
>>>>
>>>>
>>>> This is like the 'funcmux' in Tegra I think. I think this is more
>>>> useful and we should use it to set up all peripheral pins. We can
>>>> review the code, see changes, understand what they relate to, etc.
>>>>
>>>> Anyway this all seems off-topic from this patch.
>>>>
>>>> Unless someone plans to write a pinmux subsystem for U-Boot (which I
>>>> agree would be better) I think the general approach of this patch is
>>>> good.
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>
>>> Ok, so there are two next versions of this patch-set.
>>> Please decide, which one is better.
>>>
>>> For me, at present, the current s5p_gpio api works fine for all the
>>> exynos based boards.
>>> Introducing the pinmux uclass is not a quick task, now I'm trying to
>>> focus on pmic.
>>
>>
>> OK, then I think we should probably leave it as it is. If we add
>> pull-ups to driver model it should be done with pinctl as Stephen
>> says. I doubt this is a huge task, since we can likely port over the
>> code from Linux. But for now I think we should keep with the s5p API
>> until someone takes on pinctl.
>>
>> Regards,
>> Simon
>>
>
> Great, in this case, can the v3 be accepted?

v3 of what please? Can you give me a pointer?

Regards,
Simon

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello,

On 02/23/2015 06:50 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 23 February 2015 at 09:56, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello,
>>
>>
>> On 02/23/2015 04:30 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 23 February 2015 at 03:51, Przemyslaw Marczak <p.marczak@samsung.com>
>>> wrote:
>>>>
>>>>
>>>> Hello Simon,
>>>>
>>>>
>>>> On 02/20/2015 08:29 PM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 20 February 2015 at 10:50, Stephen Warren <swarren@wwwdotorg.org>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> +Stephen who might have an opinion on this.
>>>>>>>>>>>
>>>>>>>>>>> Hi Przemyslaw,
>>>>>>>>>>>
>>>>>>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This commits extends:
>>>>>>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>>>>>>
>>>>>>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> It's good to implement this, but I think you should try to have a
>>>>>>>>>>> standard interface. You could define the options you want to
>>>>>>>>>>> support
>>>>>>>>>>> and pass in a standard value.
>>>>>>>>>>>
>>>>>>>>>>> Otherwise we are not really providing a driver abstraction, only
>>>>>>>>>>> an
>>>>>>>>>>> interface.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>>>>>>> least on
>>>>>>>>>> Tegra, the GPIO controller allows you to set the pin direction and
>>>>>>>>>> the
>>>>>>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>>>>>>> related properties is done in the pinmux controller instead. I
>>>>>>>>>> don't
>>>>>>>>>> think we want a standard API that has to touch both HW modules at
>>>>>>>>>> once.
>>>>>>>>>> What common code needs to manipulate a GPIO's pull-up/down setting?
>>>>>>>>>> As
>>>>>>>>>> precedent observe that pull-up/down isn't part of the Linux
>>>>>>>>>> kernel's
>>>>>>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl
>>>>>>>>>> driver,
>>>>>>>>>> which controls pinmuxing etc.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is a quite different than in the Exynos, where all the gpio
>>>>>>>>> functions and properties can be set by few registers within range of
>>>>>>>>> each gpio port base address. So in this case we don't touch another
>>>>>>>>> hardware module, we modify one of available gpio related registers.
>>>>>>>>>
>>>>>>>>> Anyway, if we want to have a single and common gpio API in the
>>>>>>>>> future,
>>>>>>>>> then I think it is better to add pull option.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Why? I'll ask again: What common driver code needs to manipulate
>>>>>>>> pull-ups?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Please look at driver: drivers/gpio/s5p_gpio.c
>>>>>>>
>>>>>>> It's one driver related to one gpio hardware submodule and it takes
>>>>>>> care
>>>>>>> about standard gpio properties and also mux/pull/drv/rate.
>>>>>>>
>>>>>>> And the exynos pinmux code is only a software abstraction:
>>>>>>> arch/arm/cpu/armv7/exynos/pinmux.c
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I didn't want to ask which driver implements the control of pullups,
>>>>>> but
>>>>>> rather which other driver needs to turn pullups on/off in a standard
>>>>>> way
>>>>>> across multiple SoCs.
>>>>>>
>>>>>> In other words, do you expect code in common/ to need to call a "set
>>>>>> pin
>>>>>> pullup" function? If so, then we certainly need a standard API to
>>>>>> manipulate
>>>>>> pullups. However if no common code needs to manipulate pullups, then
>>>>>> I'd
>>>>>> argue we don't actually need a common API to do this, since there's no
>>>>>> code
>>>>>> that would call that common API.
>>>>>
>>>>>
>>>>>
>>>>> We do currently use the GPIO to handle pullup/pulldown for some boards
>>>>> so until we have a pinmux API (which might be a long while) it seems
>>>>> reasonable for it to live there.
>>>>>
>>>>> If not, does anyone plan to write such an API?
>>>>>
>>>>
>>>> Right, we uses this in most Exynos boards. But the boards uses direct
>>>> calls to s5p gpio driver, without uclass.
>>>> I wonder if wouldn't it be better and faster to leave the board low-level
>>>> init routines as they are now.
>>>>
>>>>
>>>>>>
>>>>>>
>>>>>>>>     > And the driver will
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> implement what is required, instead of provide common and private
>>>>>>>>> api
>>>>>>>>> for each driver.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I'm not proposing driver-specific APIs, but rather having a common
>>>>>>>> GPIO
>>>>>>>> API and a common pinmux API. They need to be different since
>>>>>>>> different
>>>>>>>> HW modules may implement the functionality.
>>>>>>>>
>>>>>>>
>>>>>>> As in the above example, for the Exynos it's the one hw module, so
>>>>>>> it's
>>>>>>> simply.
>>>>>>>
>>>>>>>>> For the various devices it is unclear, what should be pinmux and
>>>>>>>>> what
>>>>>>>>> should be gpio driver.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> How about the following are GPIO:
>>>>>>>> * Set GPIO pin direction
>>>>>>>> * Read GPIO input
>>>>>>>> * Set GPIO output value
>>>>>>>>
>>>>>>>> ... and anything else is pinmux. That's the split in Linux and AFAIK
>>>>>>>> it
>>>>>>>> works out fine.
>>>>>>>>
>>>>>>>> It'd be perfectly fine for the same driver code to implement both a
>>>>>>>> GPIO
>>>>>>>> and a pinmux driver, if the HW supports it.
>>>>>>>>
>>>>>>>
>>>>>>> Ok, I can drop this commit, since the current code works fine.
>>>>>>>
>>>>>>>>> Moreover in my opinion from the single external pin point of view
>>>>>>>>> the
>>>>>>>>> pull up/down is the property of that pin.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> It's a property of the same pin, but semantically it's not
>>>>>>>> manipulating
>>>>>>>> a GPIO-related function.
>>>>>>>>
>>>>>>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>>>>>>> driver api in U-Boot.
>>>>>>>>>
>>>>>>>>> Do we need pinmux class?
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>>>>>>> And setting the pull was required for this, before call the pinmux for
>>>>>>> eMMC pins.
>>>>>>> But finally the eMMC detect seem to be not useful in case of the
>>>>>>> present
>>>>>>> 'mmc rescan' command.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Why wouldn't the pinmux driver for the whole system simply apply the
>>>>>> board's
>>>>>> whole pinmux configuration before initializing any IO controller
>>>>>> drivers? IO
>>>>>> controller drivers shouldn't have to initialize board-/SoC-specific
>>>>>> pinmux,
>>>>>> but the board-/SoC-specfic code should do so.
>>>>>>
>>>>>> At most, the eMMC driver should call a function such as pinmux_emmc(),
>>>>>> and
>>>>>> the board/SoC code should implement that as appropriate for that board.
>>>>>> The
>>>>>> eMMC driver shouldn't have to know about applying specific
>>>>>> pullups/downs to
>>>>>> specific pins (since those settings and pins are likely
>>>>>> board-/SoC-specific,
>>>>>> and drivers shouldn't know about board-/SoC-specific details). The only
>>>>>
>>>>>
>>>>>
>>>>> No this way lies madness. It is how things work on Jetson and Nyan.
>>>>> Loads of opaque tables and no idea what the pins are connected to. It
>>>>> has some value for pins that U-Boot doesn't use (so we are just
>>>>> setting them up for Linux) but even then it is really opaque.
>>>>>
>>>>> We can't even sent patches to the file because it is auto-generated
>>>>> from a tool in another repo. Tiny differences between boards are
>>>>> hidden because we repeat all the information again with just a line or
>>>>> two of changes. I really don't want exynos to go that way.
>>>>>
>>>>>> exception would be if the standard IO protocol requires pullups to be
>>>>>> changed during regular operation. In which case, a specific callback
>>>>>> from
>>>>>> the driver could be added for each protocol-mandated configuration
>>>>>> change,
>>>>>> thus keeping the IO controller driver still completely isolated from
>>>>>> details
>>>>>> of the pins and pinmux APIs etc.
>>>>>
>>>>>
>>>>>
>>>>> This is like the 'funcmux' in Tegra I think. I think this is more
>>>>> useful and we should use it to set up all peripheral pins. We can
>>>>> review the code, see changes, understand what they relate to, etc.
>>>>>
>>>>> Anyway this all seems off-topic from this patch.
>>>>>
>>>>> Unless someone plans to write a pinmux subsystem for U-Boot (which I
>>>>> agree would be better) I think the general approach of this patch is
>>>>> good.
>>>>>
>>>>> Regards,
>>>>> Simon
>>>>>
>>>>
>>>> Ok, so there are two next versions of this patch-set.
>>>> Please decide, which one is better.
>>>>
>>>> For me, at present, the current s5p_gpio api works fine for all the
>>>> exynos based boards.
>>>> Introducing the pinmux uclass is not a quick task, now I'm trying to
>>>> focus on pmic.
>>>
>>>
>>> OK, then I think we should probably leave it as it is. If we add
>>> pull-ups to driver model it should be done with pinctl as Stephen
>>> says. I doubt this is a huge task, since we can likely port over the
>>> code from Linux. But for now I think we should keep with the s5p API
>>> until someone takes on pinctl.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> Great, in this case, can the v3 be accepted?
>
> v3 of what please? Can you give me a pointer?
>
> Regards,
> Simon
>

oops, sorry, I didn't change the cc list...
There was, v2 and v3 on the list. But the v3 is included in mmc tree 
pull request:

http://patchwork.ozlabs.org/patch/442639/

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello Stephen,

On 02/20/2015 06:50 PM, Stephen Warren wrote:
> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>> Hello,
>>
>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>> Hello,
>>>>
>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>> +Stephen who might have an opinion on this.
>>>>>>
>>>>>> Hi Przemyslaw,
>>>>>>
>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>> <p.marczak@samsung.com> wrote:
>>>>>>> This commits extends:
>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>
>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>
>>>>>> It's good to implement this, but I think you should try to have a
>>>>>> standard interface. You could define the options you want to support
>>>>>> and pass in a standard value.
>>>>>>
>>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>>> interface.
>>>>>
>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>> least on
>>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>> related properties is done in the pinmux controller instead. I don't
>>>>> think we want a standard API that has to touch both HW modules at
>>>>> once.
>>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>>> which controls pinmuxing etc.
>>>>>
>>>>
>>>> This is a quite different than in the Exynos, where all the gpio
>>>> functions and properties can be set by few registers within range of
>>>> each gpio port base address. So in this case we don't touch another
>>>> hardware module, we modify one of available gpio related registers.
>>>>
>>>> Anyway, if we want to have a single and common gpio API in the future,
>>>> then I think it is better to add pull option.
>>>
>>> Why? I'll ask again: What common driver code needs to manipulate
>>> pull-ups?
>>
>> Please look at driver: drivers/gpio/s5p_gpio.c
>>
>> It's one driver related to one gpio hardware submodule and it takes care
>> about standard gpio properties and also mux/pull/drv/rate.
>>
>> And the exynos pinmux code is only a software abstraction:
>> arch/arm/cpu/armv7/exynos/pinmux.c
>
> I didn't want to ask which driver implements the control of pullups, but
> rather which other driver needs to turn pullups on/off in a standard way
> across multiple SoCs.
>

Probably none.

> In other words, do you expect code in common/ to need to call a "set pin
> pullup" function? If so, then we certainly need a standard API to
> manipulate pullups. However if no common code needs to manipulate
> pullups, then I'd argue we don't actually need a common API to do this,
> since there's no code that would call that common API.
>

Ok, you are right.

>>>  > And the driver will
>>>> implement what is required, instead of provide common and private api
>>>> for each driver.
>>>
>>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>>> API and a common pinmux API. They need to be different since different
>>> HW modules may implement the functionality.
>>>
>>
>> As in the above example, for the Exynos it's the one hw module, so it's
>> simply.
>>
>>>> For the various devices it is unclear, what should be pinmux and what
>>>> should be gpio driver.
>>>
>>> How about the following are GPIO:
>>> * Set GPIO pin direction
>>> * Read GPIO input
>>> * Set GPIO output value
>>>
>>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>>> works out fine.
>>>
>>> It'd be perfectly fine for the same driver code to implement both a GPIO
>>> and a pinmux driver, if the HW supports it.
>>>
>>
>> Ok, I can drop this commit, since the current code works fine.
>>
>>>> Moreover in my opinion from the single external pin point of view the
>>>> pull up/down is the property of that pin.
>>>
>>> It's a property of the same pin, but semantically it's not manipulating
>>> a GPIO-related function.
>>>
>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>> driver api in U-Boot.
>>>>
>>>> Do we need pinmux class?
>>>>
>>>> Best regards,
>>>
>>>
>>
>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>> And setting the pull was required for this, before call the pinmux for
>> eMMC pins.
>> But finally the eMMC detect seem to be not useful in case of the present
>> 'mmc rescan' command.
>
> Why wouldn't the pinmux driver for the whole system simply apply the
> board's whole pinmux configuration before initializing any IO controller
> drivers? IO controller drivers shouldn't have to initialize
> board-/SoC-specific pinmux, but the board-/SoC-specfic code should do so.

This is clean, and the Exynos pinmux code is implemented in this way.

>
> At most, the eMMC driver should call a function such as pinmux_emmc(),
> and the board/SoC code should implement that as appropriate for that
> board. The eMMC driver shouldn't have to know about applying specific
> pullups/downs to specific pins (since those settings and pins are likely
> board-/SoC-specific, and drivers shouldn't know about
> board-/SoC-specific details). The only exception would be if the
> standard IO protocol requires pullups to be changed during regular
> operation. In which case, a specific callback from the driver could be
> added for each protocol-mandated configuration change, thus keeping the
> IO controller driver still completely isolated from details of the pins
> and pinmux APIs etc.
>

This is also clean, and the mmc driver for exynos, calls the pinmux with 
the device id and flag as an arguments. This works fine.

The exception which I need, was to set just one emmc pin as input with 
proper pull-up (which was defined in emmc dts node) and check the value.
This was temporary, before the proper pinmux call for emmc 
configuration, after card detect.

Of course this was a hack, and for each board, the pull value was 
defined in dts(tested on 2 boards). And this was my tests only, I didn't 
send this code to list, but now I see that probably better could be some 
board specific function like board_emmc_detected().

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 2/4] s5p: gpio: add implementation of dm_gpio_set_pull()

[Przemyslaw Marczak]

This commit adds implementation of driver model gpio pull
setting to s5p gpio driver.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Minkyu Kang <mk7.kang@samsung.com>
---
 drivers/gpio/s5p_gpio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c
index 0a245ba..5de96bf 100644
--- a/drivers/gpio/s5p_gpio.c
+++ b/drivers/gpio/s5p_gpio.c
@@ -231,6 +231,16 @@ static int exynos_gpio_set_value(struct udevice *dev, unsigned offset,
 
 	return 0;
 }
+
+static int exynos_gpio_set_pull(struct udevice *dev, unsigned offset,
+				int pull)
+{
+	struct exynos_bank_info *state = dev_get_priv(dev);
+
+	s5p_gpio_set_pull(state->bank, offset, pull);
+
+	return 0;
+}
 #endif /* nCONFIG_SPL_BUILD */
 
 /*
@@ -290,6 +300,7 @@ static const struct dm_gpio_ops gpio_exynos_ops = {
 	.direction_output	= exynos_gpio_direction_output,
 	.get_value		= exynos_gpio_get_value,
 	.set_value		= exynos_gpio_set_value,
+	.set_pull		= exynos_gpio_set_pull,
 	.get_function		= exynos_gpio_get_function,
 	.xlate			= exynos_gpio_xlate,
 };
-- 
1.9.1

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Przemyslaw Marczak]

Before this commit, the mmc devices were always registered
in the same order. So dwmmc channel 0 was registered as mmc 0,
channel 1 as mmc 1, etc.
In case of possibility to boot from more then one device,
the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.

This can be achieved by init boot device as first, so it will be
always registered as mmc 0. Thanks to this, the 'saveenv' command
will work fine for all mmc boot devices.

Exynos based boards usually uses mmc host channels configuration:
- 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
- 2 for 4bit - as an optional boot device (usually SD card slot)

And usually the boot order is defined by OM pin configuration,
which can be changed in a few ways, eg.
- Odroid U3     - eMMC card insertion -> first boot from eMMC
- Odroid X2/XU3 - boot priority jumper

By this commit, Exynos dwmmc driver will check the OM pin configuration,
and then try to init the boot device and register it as mmc 0.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Pantelis Antoniou <panto@intracom.gr>
Cc: Simon Glass <sjg@chromium.org>
Cc: Akshay Saraswat <akshay.s@samsung.com>
---
 drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
index dfa209b..91f0163 100644
--- a/drivers/mmc/exynos_dw_mmc.c
+++ b/drivers/mmc/exynos_dw_mmc.c
@@ -13,6 +13,7 @@
 #include <asm/arch/dwmmc.h>
 #include <asm/arch/clk.h>
 #include <asm/arch/pinmux.h>
+#include <asm/arch/power.h>
 #include <asm/gpio.h>
 #include <asm-generic/errno.h>
 
@@ -166,7 +167,6 @@ static int exynos_dwmci_get_config(const void *blob, int node,
 	if (host->dev_index == host->dev_id)
 		host->dev_index = host->dev_id - PERIPH_ID_SDMMC0;
 
-
 	/* Get the bus width from the device node */
 	host->buswidth = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
 	if (host->buswidth <= 0) {
@@ -229,12 +229,21 @@ int exynos_dwmmc_init(const void *blob)
 {
 	int compat_id;
 	int node_list[DWMMC_MAX_CH_NUM];
+	int boot_dev_node;
 	int err = 0, count;
 
 	compat_id = COMPAT_SAMSUNG_EXYNOS_DWMMC;
 
 	count = fdtdec_find_aliases_for_id(blob, "mmc",
 				compat_id, node_list, DWMMC_MAX_CH_NUM);
+
+	/* For DWMMC always set boot device as mmc 0 */
+	if (count >= 3 && get_boot_mode() == BOOT_MODE_SD) {
+		boot_dev_node = node_list[2];
+		node_list[2] = node_list[0];
+		node_list[0] = boot_dev_node;
+	}
+
 	err = exynos_dwmci_process_node(blob, node_list, count);
 
 	return err;
-- 
1.9.1

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Simon Glass]

Hi Przemyslaw,

On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> Before this commit, the mmc devices were always registered
> in the same order. So dwmmc channel 0 was registered as mmc 0,
> channel 1 as mmc 1, etc.
> In case of possibility to boot from more then one device,
> the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
>
> This can be achieved by init boot device as first, so it will be
> always registered as mmc 0. Thanks to this, the 'saveenv' command
> will work fine for all mmc boot devices.
>
> Exynos based boards usually uses mmc host channels configuration:
> - 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
> - 2 for 4bit - as an optional boot device (usually SD card slot)
>
> And usually the boot order is defined by OM pin configuration,
> which can be changed in a few ways, eg.
> - Odroid U3     - eMMC card insertion -> first boot from eMMC
> - Odroid X2/XU3 - boot priority jumper
>
> By this commit, Exynos dwmmc driver will check the OM pin configuration,
> and then try to init the boot device and register it as mmc 0.

I think a better way to do this would be to make
CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
selected at run-time.

However that would probably be better done when the drive rmodel
conversion is complete.

So this seems a reasonable patch given where we are.

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Pantelis Antoniou <panto@intracom.gr>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Akshay Saraswat <akshay.s@samsung.com>
> ---
>  drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> index dfa209b..91f0163 100644
> --- a/drivers/mmc/exynos_dw_mmc.c
> +++ b/drivers/mmc/exynos_dw_mmc.c
> @@ -13,6 +13,7 @@
>  #include <asm/arch/dwmmc.h>
>  #include <asm/arch/clk.h>
>  #include <asm/arch/pinmux.h>
> +#include <asm/arch/power.h>
>  #include <asm/gpio.h>
>  #include <asm-generic/errno.h>
>
> @@ -166,7 +167,6 @@ static int exynos_dwmci_get_config(const void *blob, int node,
>         if (host->dev_index == host->dev_id)
>                 host->dev_index = host->dev_id - PERIPH_ID_SDMMC0;
>
> -
>         /* Get the bus width from the device node */
>         host->buswidth = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
>         if (host->buswidth <= 0) {
> @@ -229,12 +229,21 @@ int exynos_dwmmc_init(const void *blob)
>  {
>         int compat_id;
>         int node_list[DWMMC_MAX_CH_NUM];
> +       int boot_dev_node;
>         int err = 0, count;
>
>         compat_id = COMPAT_SAMSUNG_EXYNOS_DWMMC;
>
>         count = fdtdec_find_aliases_for_id(blob, "mmc",
>                                 compat_id, node_list, DWMMC_MAX_CH_NUM);
> +
> +       /* For DWMMC always set boot device as mmc 0 */
> +       if (count >= 3 && get_boot_mode() == BOOT_MODE_SD) {
> +               boot_dev_node = node_list[2];
> +               node_list[2] = node_list[0];
> +               node_list[0] = boot_dev_node;
> +       }
> +
>         err = exynos_dwmci_process_node(blob, node_list, count);
>
>         return err;
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Przemyslaw Marczak]

Hello Simon,

On 02/18/2015 06:02 AM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Before this commit, the mmc devices were always registered
>> in the same order. So dwmmc channel 0 was registered as mmc 0,
>> channel 1 as mmc 1, etc.
>> In case of possibility to boot from more then one device,
>> the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
>>
>> This can be achieved by init boot device as first, so it will be
>> always registered as mmc 0. Thanks to this, the 'saveenv' command
>> will work fine for all mmc boot devices.
>>
>> Exynos based boards usually uses mmc host channels configuration:
>> - 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
>> - 2 for 4bit - as an optional boot device (usually SD card slot)
>>
>> And usually the boot order is defined by OM pin configuration,
>> which can be changed in a few ways, eg.
>> - Odroid U3     - eMMC card insertion -> first boot from eMMC
>> - Odroid X2/XU3 - boot priority jumper
>>
>> By this commit, Exynos dwmmc driver will check the OM pin configuration,
>> and then try to init the boot device and register it as mmc 0.
>
> I think a better way to do this would be to make
> CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
> selected at run-time.
>
> However that would probably be better done when the drive rmodel
> conversion is complete.
>
> So this seems a reasonable patch given where we are.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>

This was just a quick solution to solve the issue on XU3, when the same 
binary can boot from sd or eMMC slots.


>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>> Cc: Jaehoon Chung <jh80.chung@samsung.com>
>> Cc: Pantelis Antoniou <panto@intracom.gr>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Akshay Saraswat <akshay.s@samsung.com>
>> ---
>>   drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
>> index dfa209b..91f0163 100644
>> --- a/drivers/mmc/exynos_dw_mmc.c
>> +++ b/drivers/mmc/exynos_dw_mmc.c
>> @@ -13,6 +13,7 @@
>>   #include <asm/arch/dwmmc.h>
>>   #include <asm/arch/clk.h>
>>   #include <asm/arch/pinmux.h>
>> +#include <asm/arch/power.h>
>>   #include <asm/gpio.h>
>>   #include <asm-generic/errno.h>
>>
>> @@ -166,7 +167,6 @@ static int exynos_dwmci_get_config(const void *blob, int node,
>>          if (host->dev_index == host->dev_id)
>>                  host->dev_index = host->dev_id - PERIPH_ID_SDMMC0;
>>
>> -
>>          /* Get the bus width from the device node */
>>          host->buswidth = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
>>          if (host->buswidth <= 0) {
>> @@ -229,12 +229,21 @@ int exynos_dwmmc_init(const void *blob)
>>   {
>>          int compat_id;
>>          int node_list[DWMMC_MAX_CH_NUM];
>> +       int boot_dev_node;
>>          int err = 0, count;
>>
>>          compat_id = COMPAT_SAMSUNG_EXYNOS_DWMMC;
>>
>>          count = fdtdec_find_aliases_for_id(blob, "mmc",
>>                                  compat_id, node_list, DWMMC_MAX_CH_NUM);
>> +
>> +       /* For DWMMC always set boot device as mmc 0 */
>> +       if (count >= 3 && get_boot_mode() == BOOT_MODE_SD) {
>> +               boot_dev_node = node_list[2];
>> +               node_list[2] = node_list[0];
>> +               node_list[0] = boot_dev_node;
>> +       }
>> +
>>          err = exynos_dwmci_process_node(blob, node_list, count);
>>
>>          return err;
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>

Thank you for the review. I will send the second version soon.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Tom Rini]

On Wed, Feb 18, 2015 at 11:50:41AM +0100, Przemyslaw Marczak wrote:
> Hello Simon,
> 
> On 02/18/2015 06:02 AM, Simon Glass wrote:
> >Hi Przemyslaw,
> >
> >On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> >>Before this commit, the mmc devices were always registered
> >>in the same order. So dwmmc channel 0 was registered as mmc 0,
> >>channel 1 as mmc 1, etc.
> >>In case of possibility to boot from more then one device,
> >>the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
> >>
> >>This can be achieved by init boot device as first, so it will be
> >>always registered as mmc 0. Thanks to this, the 'saveenv' command
> >>will work fine for all mmc boot devices.
> >>
> >>Exynos based boards usually uses mmc host channels configuration:
> >>- 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
> >>- 2 for 4bit - as an optional boot device (usually SD card slot)
> >>
> >>And usually the boot order is defined by OM pin configuration,
> >>which can be changed in a few ways, eg.
> >>- Odroid U3     - eMMC card insertion -> first boot from eMMC
> >>- Odroid X2/XU3 - boot priority jumper
> >>
> >>By this commit, Exynos dwmmc driver will check the OM pin configuration,
> >>and then try to init the boot device and register it as mmc 0.
> >
> >I think a better way to do this would be to make
> >CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
> >selected at run-time.
> >
> >However that would probably be better done when the drive rmodel
> >conversion is complete.
> >
> >So this seems a reasonable patch given where we are.
> >
> >Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> 
> This was just a quick solution to solve the issue on XU3, when the
> same binary can boot from sd or eMMC slots.

XU3 isn't unique in this regard.  "am335x_evm" binaries runs on 4 very
different boards and we still just have to say that sometimes we default
to ENV in a place that isn't workable.
 
-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/76528fdb/attachment.sig>

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Przemyslaw Marczak]

Hello Tom,

On 02/19/2015 03:03 PM, Tom Rini wrote:
> On Wed, Feb 18, 2015 at 11:50:41AM +0100, Przemyslaw Marczak wrote:
>> Hello Simon,
>>
>> On 02/18/2015 06:02 AM, Simon Glass wrote:
>>> Hi Przemyslaw,
>>>
>>> On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>>>> Before this commit, the mmc devices were always registered
>>>> in the same order. So dwmmc channel 0 was registered as mmc 0,
>>>> channel 1 as mmc 1, etc.
>>>> In case of possibility to boot from more then one device,
>>>> the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
>>>>
>>>> This can be achieved by init boot device as first, so it will be
>>>> always registered as mmc 0. Thanks to this, the 'saveenv' command
>>>> will work fine for all mmc boot devices.
>>>>
>>>> Exynos based boards usually uses mmc host channels configuration:
>>>> - 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
>>>> - 2 for 4bit - as an optional boot device (usually SD card slot)
>>>>
>>>> And usually the boot order is defined by OM pin configuration,
>>>> which can be changed in a few ways, eg.
>>>> - Odroid U3     - eMMC card insertion -> first boot from eMMC
>>>> - Odroid X2/XU3 - boot priority jumper
>>>>
>>>> By this commit, Exynos dwmmc driver will check the OM pin configuration,
>>>> and then try to init the boot device and register it as mmc 0.
>>>
>>> I think a better way to do this would be to make
>>> CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
>>> selected at run-time.
>>>
>>> However that would probably be better done when the drive rmodel
>>> conversion is complete.
>>>
>>> So this seems a reasonable patch given where we are.
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>
>> This was just a quick solution to solve the issue on XU3, when the
>> same binary can boot from sd or eMMC slots.
>
> XU3 isn't unique in this regard.  "am335x_evm" binaries runs on 4 very
> different boards and we still just have to say that sometimes we default
> to ENV in a place that isn't workable.
>
>

Yes, I saw this issue on bb black. But it seems to be not a big problem 
to fix it.
When I finish the pmic and finally start work on adding it to the bb 
black, then maybe I will try to fix it somehow.

Regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Tom Rini]

On Thu, Feb 19, 2015 at 03:36:57PM +0100, Przemyslaw Marczak wrote:
> Hello Tom,
> 
> On 02/19/2015 03:03 PM, Tom Rini wrote:
> >On Wed, Feb 18, 2015 at 11:50:41AM +0100, Przemyslaw Marczak wrote:
> >>Hello Simon,
> >>
> >>On 02/18/2015 06:02 AM, Simon Glass wrote:
> >>>Hi Przemyslaw,
> >>>
> >>>On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> >>>>Before this commit, the mmc devices were always registered
> >>>>in the same order. So dwmmc channel 0 was registered as mmc 0,
> >>>>channel 1 as mmc 1, etc.
> >>>>In case of possibility to boot from more then one device,
> >>>>the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
> >>>>
> >>>>This can be achieved by init boot device as first, so it will be
> >>>>always registered as mmc 0. Thanks to this, the 'saveenv' command
> >>>>will work fine for all mmc boot devices.
> >>>>
> >>>>Exynos based boards usually uses mmc host channels configuration:
> >>>>- 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
> >>>>- 2 for 4bit - as an optional boot device (usually SD card slot)
> >>>>
> >>>>And usually the boot order is defined by OM pin configuration,
> >>>>which can be changed in a few ways, eg.
> >>>>- Odroid U3     - eMMC card insertion -> first boot from eMMC
> >>>>- Odroid X2/XU3 - boot priority jumper
> >>>>
> >>>>By this commit, Exynos dwmmc driver will check the OM pin configuration,
> >>>>and then try to init the boot device and register it as mmc 0.
> >>>
> >>>I think a better way to do this would be to make
> >>>CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
> >>>selected at run-time.
> >>>
> >>>However that would probably be better done when the drive rmodel
> >>>conversion is complete.
> >>>
> >>>So this seems a reasonable patch given where we are.
> >>>
> >>>Reviewed-by: Simon Glass <sjg@chromium.org>
> >>>
> >>
> >>This was just a quick solution to solve the issue on XU3, when the
> >>same binary can boot from sd or eMMC slots.
> >
> >XU3 isn't unique in this regard.  "am335x_evm" binaries runs on 4 very
> >different boards and we still just have to say that sometimes we default
> >to ENV in a place that isn't workable.
> 
> Yes, I saw this issue on bb black. But it seems to be not a big
> problem to fix it.
> When I finish the pmic and finally start work on adding it to the bb
> black, then maybe I will try to fix it somehow.

It's not hard (extending some of the concepts we have already) but I
want to hold that off until we have driver model support instead as part
of a "carrot and stick" approach ;)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/e00a6242/attachment.sig>

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Przemyslaw Marczak]

Hi,

On 02/19/2015 05:45 PM, Tom Rini wrote:
> On Thu, Feb 19, 2015 at 03:36:57PM +0100, Przemyslaw Marczak wrote:
>> Hello Tom,
>>
>> On 02/19/2015 03:03 PM, Tom Rini wrote:
>>> On Wed, Feb 18, 2015 at 11:50:41AM +0100, Przemyslaw Marczak wrote:
>>>> Hello Simon,
>>>>
>>>> On 02/18/2015 06:02 AM, Simon Glass wrote:
>>>>> Hi Przemyslaw,
>>>>>
>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>>>>>> Before this commit, the mmc devices were always registered
>>>>>> in the same order. So dwmmc channel 0 was registered as mmc 0,
>>>>>> channel 1 as mmc 1, etc.
>>>>>> In case of possibility to boot from more then one device,
>>>>>> the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
>>>>>>
>>>>>> This can be achieved by init boot device as first, so it will be
>>>>>> always registered as mmc 0. Thanks to this, the 'saveenv' command
>>>>>> will work fine for all mmc boot devices.
>>>>>>
>>>>>> Exynos based boards usually uses mmc host channels configuration:
>>>>>> - 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
>>>>>> - 2 for 4bit - as an optional boot device (usually SD card slot)
>>>>>>
>>>>>> And usually the boot order is defined by OM pin configuration,
>>>>>> which can be changed in a few ways, eg.
>>>>>> - Odroid U3     - eMMC card insertion -> first boot from eMMC
>>>>>> - Odroid X2/XU3 - boot priority jumper
>>>>>>
>>>>>> By this commit, Exynos dwmmc driver will check the OM pin configuration,
>>>>>> and then try to init the boot device and register it as mmc 0.
>>>>>
>>>>> I think a better way to do this would be to make
>>>>> CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
>>>>> selected at run-time.
>>>>>
>>>>> However that would probably be better done when the drive rmodel
>>>>> conversion is complete.
>>>>>
>>>>> So this seems a reasonable patch given where we are.
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>
>>>>
>>>> This was just a quick solution to solve the issue on XU3, when the
>>>> same binary can boot from sd or eMMC slots.
>>>
>>> XU3 isn't unique in this regard.  "am335x_evm" binaries runs on 4 very
>>> different boards and we still just have to say that sometimes we default
>>> to ENV in a place that isn't workable.
>>
>> Yes, I saw this issue on bb black. But it seems to be not a big
>> problem to fix it.
>> When I finish the pmic and finally start work on adding it to the bb
>> black, then maybe I will try to fix it somehow.
>
> It's not hard (extending some of the concepts we have already) but I
> want to hold that off until we have driver model support instead as part
> of a "carrot and stick" approach ;)
>

Ok, that's fine.

Regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

[U-Boot] [PATCH 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Tom Rini]

On Tue, Feb 17, 2015 at 10:02:03PM -0700, Simon Glass wrote:
> Hi Przemyslaw,
> 
> On 17 February 2015 at 06:09, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> > Before this commit, the mmc devices were always registered
> > in the same order. So dwmmc channel 0 was registered as mmc 0,
> > channel 1 as mmc 1, etc.
> > In case of possibility to boot from more then one device,
> > the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
> >
> > This can be achieved by init boot device as first, so it will be
> > always registered as mmc 0. Thanks to this, the 'saveenv' command
> > will work fine for all mmc boot devices.
> >
> > Exynos based boards usually uses mmc host channels configuration:
> > - 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
> > - 2 for 4bit - as an optional boot device (usually SD card slot)
> >
> > And usually the boot order is defined by OM pin configuration,
> > which can be changed in a few ways, eg.
> > - Odroid U3     - eMMC card insertion -> first boot from eMMC
> > - Odroid X2/XU3 - boot priority jumper
> >
> > By this commit, Exynos dwmmc driver will check the OM pin configuration,
> > and then try to init the boot device and register it as mmc 0.
> 
> I think a better way to do this would be to make
> CONFIG_SYS_MMC_ENV_DEV support an option where the device can be
> selected at run-time.
> 
> However that would probably be better done when the drive rmodel
> conversion is complete.

Yes, lets hold off on this until driver model is in and we can do this
more cleanly rather than whack at this problem right now (I've seen
solutions to this kind of problem on am335x for example and it's still
going to be better to wait I think).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/7a263834/attachment.sig>

[U-Boot] [PATCH 4/4] mmc: print SD/eMMC type for inited mmc devices

[Przemyslaw Marczak]

Depending on the boot priority, the eMMC/SD cards,
can be initialized with the same numbers for each boot.

To be sure which mmc device is SD and which is eMMC,
this info is printed by 'mmc list' command, when
the init is done.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Pantelis Antoniou <panto@intracom.gr>
---
 drivers/mmc/mmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index b8039cd..a13769e 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1693,11 +1693,19 @@ void print_mmc_devices(char separator)
 {
 	struct mmc *m;
 	struct list_head *entry;
+	char *mmc_type;
 
 	list_for_each(entry, &mmc_devices) {
 		m = list_entry(entry, struct mmc, link);
 
+		if (m->has_init)
+			mmc_type = IS_SD(m) ? "SD" : "eMMC";
+		else
+			mmc_type = NULL;
+
 		printf("%s: %d", m->cfg->name, m->block_dev.dev);
+		if (mmc_type)
+			printf(" (%s)", mmc_type);
 
 		if (entry->next != &mmc_devices) {
 			printf("%c", separator);
-- 
1.9.1

[U-Boot] [PATCH V2 0/4] exynos-dwmmc: check set init priority for boot channel

[Przemyslaw Marczak]

The dw mmc driver init priority was always the same: ch 0, ch 1, ch 2.
On some boards (e.g. Odroid XU3) the dwmmc driver is enabled for all
mmc channels. In this case, when boot device is switchable (SD/eMMC),
the default MMC device will be 0 or 1.
Change the init priority to boot device, always init the boot device as
mmc 0.
This fixes the issue with 'saveenv' command, because the MMC env device
number is always the same.

The patchset also adds gpio set pull option to gpio api.

Przemyslaw Marczak (4):
  dm: gpio: extend gpio api by dm_gpio_set_pull()
  s5p: gpio: add implementation of dm_gpio_set_pull()
  mmc: exynos dwmmc: check boot mode before init dwmmc
  mmc: print SD/eMMC type for inited mmc devices

 drivers/gpio/gpio-uclass.c  | 11 +++++++++++
 drivers/gpio/s5p_gpio.c     | 28 ++++++++++++++++++++++++++++
 drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
 drivers/mmc/mmc.c           |  8 ++++++++
 include/asm-generic/gpio.h  | 22 ++++++++++++++++++++++
 5 files changed, 79 insertions(+), 1 deletion(-)

-- 
1.9.1

[U-Boot] [PATCH V2 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

This commits extends:
- dm gpio ops by: 'set_pull' call
- dm gpio uclass by: dm_gpio_set_pull() function

The pull modes are defined by proper enum and can be:
- UP
- DOWN
- NONE
- UNKNOWN

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
CC: Simon Glass <sjg@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>

---
Changes v2:
- add enum with gpio pull mode
---
 drivers/gpio/gpio-uclass.c | 11 +++++++++++
 include/asm-generic/gpio.h | 22 ++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index a69bbd2..10f600b 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -321,6 +321,17 @@ int dm_gpio_set_value(struct gpio_desc *desc, int value)
 	return 0;
 }
 
+int dm_gpio_set_pull(struct gpio_desc *desc, int pull)
+{
+	int ret;
+
+	ret = check_reserved(desc, "set_pull");
+	if (ret)
+		return ret;
+
+	return gpio_get_ops(desc->dev)->set_pull(desc->dev, desc->offset, pull);
+}
+
 int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
 {
 	struct udevice *dev = desc->dev;
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 3b96b82..b353f80 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -108,6 +108,16 @@ enum gpio_func_t {
 	GPIOF_COUNT,
 };
 
+/* State of a GPIO pull */
+enum gpio_pull_t {
+	GPIOP_DOWN = 0,
+	GPIOP_UP,
+	GPIOP_NONE,
+	GPIOP_UNKNOWN,
+
+	GPIOP_COUNT,
+};
+
 struct udevice;
 
 struct gpio_desc {
@@ -241,6 +251,7 @@ struct dm_gpio_ops {
 				int value);
 	int (*get_value)(struct udevice *dev, unsigned offset);
 	int (*set_value)(struct udevice *dev, unsigned offset, int value);
+	int (*set_pull)(struct udevice *dev, unsigned offset, int pull);
 	/**
 	 * get_function() Get the GPIO function
 	 *
@@ -479,6 +490,7 @@ int gpio_free_list_nodev(struct gpio_desc *desc, int count);
 
 /**
  * dm_gpio_get_value() - Get the value of a GPIO
+
  *
  * This is the driver model version of the existing gpio_get_value() function
  * and should be used instead of that.
@@ -495,6 +507,16 @@ int dm_gpio_get_value(struct gpio_desc *desc);
 int dm_gpio_set_value(struct gpio_desc *desc, int value);
 
 /**
+ * dm_gpio_set_pull() - Set the pull-up/down value of a GPIO
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @pull:	GPIO pull value - one of enum gpio_pull_t
+ * @return 0 on success or -ve on error
+*/
+int dm_gpio_set_pull(struct gpio_desc *desc, int pull);
+
+/**
  * dm_gpio_set_dir() - Set the direction for a GPIO
  *
  * This sets up the direction according tot the provided flags. It will do
-- 
1.9.1

[U-Boot] [PATCH V2 2/4] s5p: gpio: add implementation of dm_gpio_set_pull()

[Przemyslaw Marczak]

This commit adds implementation of driver model gpio pull
setting to s5p gpio driver.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---
Changes v2:
- adjust code after added gpio pull enum to gpio api
---
 drivers/gpio/s5p_gpio.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c
index 0a245ba..ed531c6 100644
--- a/drivers/gpio/s5p_gpio.c
+++ b/drivers/gpio/s5p_gpio.c
@@ -129,6 +129,7 @@ static void s5p_gpio_set_pull(struct s5p_gpio_bank *bank, int gpio, int mode)
 	value &= ~PULL_MASK(gpio);
 
 	switch (mode) {
+	case S5P_GPIO_PULL_NONE:
 	case S5P_GPIO_PULL_DOWN:
 	case S5P_GPIO_PULL_UP:
 		value |= PULL_MODE(gpio, mode);
@@ -231,6 +232,32 @@ static int exynos_gpio_set_value(struct udevice *dev, unsigned offset,
 
 	return 0;
 }
+
+static int exynos_gpio_set_pull(struct udevice *dev, unsigned offset,
+				int pull)
+{
+	struct exynos_bank_info *state = dev_get_priv(dev);
+	int gpio_pull;
+
+	switch (pull) {
+	case GPIOP_NONE:
+		gpio_pull = S5P_GPIO_PULL_NONE;
+		break;
+	case GPIOP_DOWN:
+		gpio_pull = S5P_GPIO_PULL_DOWN;
+		break;
+	case GPIOP_UP:
+		gpio_pull = S5P_GPIO_PULL_UP;
+		break;
+	default:
+		return -EINVAL;
+		break;
+	}
+
+	s5p_gpio_set_pull(state->bank, offset, gpio_pull);
+
+	return 0;
+}
 #endif /* nCONFIG_SPL_BUILD */
 
 /*
@@ -290,6 +317,7 @@ static const struct dm_gpio_ops gpio_exynos_ops = {
 	.direction_output	= exynos_gpio_direction_output,
 	.get_value		= exynos_gpio_get_value,
 	.set_value		= exynos_gpio_set_value,
+	.set_pull		= exynos_gpio_set_pull,
 	.get_function		= exynos_gpio_get_function,
 	.xlate			= exynos_gpio_xlate,
 };
-- 
1.9.1

[U-Boot] [PATCH V2 3/4] mmc: exynos dwmmc: check boot mode before init dwmmc

[Przemyslaw Marczak]

Before this commit, the mmc devices were always registered
in the same order. So dwmmc channel 0 was registered as mmc 0,
channel 1 as mmc 1, etc.
In case of possibility to boot from more then one device,
the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.

This can be achieved by init boot device as first, so it will be
always registered as mmc 0. Thanks to this, the 'saveenv' command
will work fine for all mmc boot devices.

Exynos based boards usually uses mmc host channels configuration:
- 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
- 2 for 4bit - as an optional boot device (usually SD card slot)

And usually the boot order is defined by OM pin configuration,
which can be changed in a few ways, eg.
- Odroid U3     - eMMC card insertion -> first boot from eMMC
- Odroid X2/XU3 - boot priority jumper

By this commit, Exynos dwmmc driver will check the OM pin configuration,
and then try to init the boot device and register it as mmc 0.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Pantelis Antoniou <panto@intracom.gr>
Cc: Simon Glass <sjg@chromium.org>
Cc: Akshay Saraswat <akshay.s@samsung.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

---
Changes v2:
- none
---
 drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
index dfa209b..91f0163 100644
--- a/drivers/mmc/exynos_dw_mmc.c
+++ b/drivers/mmc/exynos_dw_mmc.c
@@ -13,6 +13,7 @@
 #include <asm/arch/dwmmc.h>
 #include <asm/arch/clk.h>
 #include <asm/arch/pinmux.h>
+#include <asm/arch/power.h>
 #include <asm/gpio.h>
 #include <asm-generic/errno.h>
 
@@ -166,7 +167,6 @@ static int exynos_dwmci_get_config(const void *blob, int node,
 	if (host->dev_index == host->dev_id)
 		host->dev_index = host->dev_id - PERIPH_ID_SDMMC0;
 
-
 	/* Get the bus width from the device node */
 	host->buswidth = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
 	if (host->buswidth <= 0) {
@@ -229,12 +229,21 @@ int exynos_dwmmc_init(const void *blob)
 {
 	int compat_id;
 	int node_list[DWMMC_MAX_CH_NUM];
+	int boot_dev_node;
 	int err = 0, count;
 
 	compat_id = COMPAT_SAMSUNG_EXYNOS_DWMMC;
 
 	count = fdtdec_find_aliases_for_id(blob, "mmc",
 				compat_id, node_list, DWMMC_MAX_CH_NUM);
+
+	/* For DWMMC always set boot device as mmc 0 */
+	if (count >= 3 && get_boot_mode() == BOOT_MODE_SD) {
+		boot_dev_node = node_list[2];
+		node_list[2] = node_list[0];
+		node_list[0] = boot_dev_node;
+	}
+
 	err = exynos_dwmci_process_node(blob, node_list, count);
 
 	return err;
-- 
1.9.1

[U-Boot] [PATCH V2 4/4] mmc: print SD/eMMC type for inited mmc devices

[Przemyslaw Marczak]

Depending on the boot priority, the eMMC/SD cards,
can be initialized with the same numbers for each boot.

To be sure which mmc device is SD and which is eMMC,
this info is printed by 'mmc list' command, when
the init is done.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Pantelis Antoniou <panto@intracom.gr>
Reviewed-by: Simon Glass <sjg@chromium.org>

---
Changes v2:
- none
---
 drivers/mmc/mmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index b8039cd..a13769e 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1693,11 +1693,19 @@ void print_mmc_devices(char separator)
 {
 	struct mmc *m;
 	struct list_head *entry;
+	char *mmc_type;
 
 	list_for_each(entry, &mmc_devices) {
 		m = list_entry(entry, struct mmc, link);
 
+		if (m->has_init)
+			mmc_type = IS_SD(m) ? "SD" : "eMMC";
+		else
+			mmc_type = NULL;
+
 		printf("%s: %d", m->cfg->name, m->block_dev.dev);
+		if (mmc_type)
+			printf(" (%s)", mmc_type);
 
 		if (entry->next != &mmc_devices) {
 			printf("%c", separator);
-- 
1.9.1

[U-Boot] [PATCH V3 0/2] exynos-dwmmc: set init priority for boot channel

[Przemyslaw Marczak]

The dw mmc driver init priority was always the same: ch 0, ch 1, ch 2.
On some boards (e.g. Odroid XU3) the dwmmc driver is enabled for all
mmc channels. In this case, when boot device is switchable (SD/eMMC),
the default MMC device will be 0 or 1.
Change the init priority to boot device, always init the boot device as
mmc 0.
This fixes the issue with 'saveenv' command, because the MMC env device
number is always the same.

Przemyslaw Marczak (2):
  mmc: exynos dwmmc: check boot mode before init dwmmc
  mmc: print SD/eMMC type for inited mmc devices

 drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
 drivers/mmc/mmc.c           |  8 ++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

-- 
1.9.1

[U-Boot] [PATCH V3 1/2] mmc: exynos dwmmc: check boot mode before init dwmmc

[Przemyslaw Marczak]

Before this commit, the mmc devices were always registered
in the same order. So dwmmc channel 0 was registered as mmc 0,
channel 1 as mmc 1, etc.
In case of possibility to boot from more then one device,
the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.

This can be achieved by init boot device as first, so it will be
always registered as mmc 0. Thanks to this, the 'saveenv' command
will work fine for all mmc boot devices.

Exynos based boards usually uses mmc host channels configuration:
- 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
- 2 for 4bit - as an optional boot device (usually SD card slot)

And usually the boot order is defined by OM pin configuration,
which can be changed in a few ways, eg.
- Odroid U3     - eMMC card insertion -> first boot from eMMC
- Odroid X2/XU3 - boot priority jumper

By this commit, Exynos dwmmc driver will check the OM pin configuration,
and then try to init the boot device and register it as mmc 0.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Akshay Saraswat <akshay.s@samsung.com>

---
Changes v2:
- none

Changes v3:
- Fix email to Pantelis
---
 drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
index dfa209b..91f0163 100644
--- a/drivers/mmc/exynos_dw_mmc.c
+++ b/drivers/mmc/exynos_dw_mmc.c
@@ -13,6 +13,7 @@
 #include <asm/arch/dwmmc.h>
 #include <asm/arch/clk.h>
 #include <asm/arch/pinmux.h>
+#include <asm/arch/power.h>
 #include <asm/gpio.h>
 #include <asm-generic/errno.h>
 
@@ -166,7 +167,6 @@ static int exynos_dwmci_get_config(const void *blob, int node,
 	if (host->dev_index == host->dev_id)
 		host->dev_index = host->dev_id - PERIPH_ID_SDMMC0;
 
-
 	/* Get the bus width from the device node */
 	host->buswidth = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
 	if (host->buswidth <= 0) {
@@ -229,12 +229,21 @@ int exynos_dwmmc_init(const void *blob)
 {
 	int compat_id;
 	int node_list[DWMMC_MAX_CH_NUM];
+	int boot_dev_node;
 	int err = 0, count;
 
 	compat_id = COMPAT_SAMSUNG_EXYNOS_DWMMC;
 
 	count = fdtdec_find_aliases_for_id(blob, "mmc",
 				compat_id, node_list, DWMMC_MAX_CH_NUM);
+
+	/* For DWMMC always set boot device as mmc 0 */
+	if (count >= 3 && get_boot_mode() == BOOT_MODE_SD) {
+		boot_dev_node = node_list[2];
+		node_list[2] = node_list[0];
+		node_list[0] = boot_dev_node;
+	}
+
 	err = exynos_dwmci_process_node(blob, node_list, count);
 
 	return err;
-- 
1.9.1

[U-Boot] [PATCH V3 1/2] mmc: exynos dwmmc: check boot mode before init dwmmc

[Pantelis Antoniou]

Hi Przemyslaw,

> On Feb 20, 2015, at 13:29 , Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> 
> Before this commit, the mmc devices were always registered
> in the same order. So dwmmc channel 0 was registered as mmc 0,
> channel 1 as mmc 1, etc.
> In case of possibility to boot from more then one device,
> the CONFIG_SYS_MMC_ENV_DEV should always point to right mmc device.
> 
> This can be achieved by init boot device as first, so it will be
> always registered as mmc 0. Thanks to this, the 'saveenv' command
> will work fine for all mmc boot devices.
> 
> Exynos based boards usually uses mmc host channels configuration:
> - 0, or 0+1 for 8 bit  - as a default boot device (usually eMMC)
> - 2 for 4bit - as an optional boot device (usually SD card slot)
> 
> And usually the boot order is defined by OM pin configuration,
> which can be changed in a few ways, eg.
> - Odroid U3     - eMMC card insertion -> first boot from eMMC
> - Odroid X2/XU3 - boot priority jumper
> 
> By this commit, Exynos dwmmc driver will check the OM pin configuration,
> and then try to init the boot device and register it as mmc 0.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Minkyu Kang <mk7.kang@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Akshay Saraswat <akshay.s@samsung.com>
> 
> ---
> Changes v2:
> - none
> 
> Changes v3:
> - Fix email to Pantelis
> ---
> drivers/mmc/exynos_dw_mmc.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> index dfa209b..91f0163 100644
> --- a/drivers/mmc/exynos_dw_mmc.c
> +++ b/drivers/mmc/exynos_dw_mmc.c
> @@ -13,6 +13,7 @@
> #include <asm/arch/dwmmc.h>
> #include <asm/arch/clk.h>
> #include <asm/arch/pinmux.h>
> +#include <asm/arch/power.h>
> #include <asm/gpio.h>
> #include <asm-generic/errno.h>
> 
> @@ -166,7 +167,6 @@ static int exynos_dwmci_get_config(const void *blob, int node,
> 	if (host->dev_index == host->dev_id)
> 		host->dev_index = host->dev_id - PERIPH_ID_SDMMC0;
> 
> -
> 	/* Get the bus width from the device node */
> 	host->buswidth = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
> 	if (host->buswidth <= 0) {
> @@ -229,12 +229,21 @@ int exynos_dwmmc_init(const void *blob)
> {
> 	int compat_id;
> 	int node_list[DWMMC_MAX_CH_NUM];
> +	int boot_dev_node;
> 	int err = 0, count;
> 
> 	compat_id = COMPAT_SAMSUNG_EXYNOS_DWMMC;
> 
> 	count = fdtdec_find_aliases_for_id(blob, "mmc",
> 				compat_id, node_list, DWMMC_MAX_CH_NUM);
> +
> +	/* For DWMMC always set boot device as mmc 0 */
> +	if (count >= 3 && get_boot_mode() == BOOT_MODE_SD) {
> +		boot_dev_node = node_list[2];
> +		node_list[2] = node_list[0];
> +		node_list[0] = boot_dev_node;
> +	}
> +
> 	err = exynos_dwmci_process_node(blob, node_list, count);
> 
> 	return err;
> -- 
> 1.9.1
> 

Applied, but ick? Driver model should make this go away.

? Pantelis

[U-Boot] [PATCH V3 2/2] mmc: print SD/eMMC type for inited mmc devices

[Przemyslaw Marczak]

Depending on the boot priority, the eMMC/SD cards,
can be initialized with the same numbers for each boot.

To be sure which mmc device is SD and which is eMMC,
this info is printed by 'mmc list' command, when
the init is done.

Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>

---
Changes v2:
- none

Changes v3:
- Fix email to Pantelis
---
 drivers/mmc/mmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index b8039cd..a13769e 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1693,11 +1693,19 @@ void print_mmc_devices(char separator)
 {
 	struct mmc *m;
 	struct list_head *entry;
+	char *mmc_type;
 
 	list_for_each(entry, &mmc_devices) {
 		m = list_entry(entry, struct mmc, link);
 
+		if (m->has_init)
+			mmc_type = IS_SD(m) ? "SD" : "eMMC";
+		else
+			mmc_type = NULL;
+
 		printf("%s: %d", m->cfg->name, m->block_dev.dev);
+		if (mmc_type)
+			printf(" (%s)", mmc_type);
 
 		if (entry->next != &mmc_devices) {
 			printf("%c", separator);
-- 
1.9.1

[U-Boot] [PATCH V3 2/2] mmc: print SD/eMMC type for inited mmc devices

[Pantelis Antoniou]

Hi Przemyslaw,

> On Feb 20, 2015, at 13:29 , Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> 
> Depending on the boot priority, the eMMC/SD cards,
> can be initialized with the same numbers for each boot.
> 
> To be sure which mmc device is SD and which is eMMC,
> this info is printed by 'mmc list' command, when
> the init is done.
> 
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> 
> ---
> Changes v2:
> - none
> 
> Changes v3:
> - Fix email to Pantelis
> ---
> drivers/mmc/mmc.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index b8039cd..a13769e 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1693,11 +1693,19 @@ void print_mmc_devices(char separator)
> {
> 	struct mmc *m;
> 	struct list_head *entry;
> +	char *mmc_type;
> 
> 	list_for_each(entry, &mmc_devices) {
> 		m = list_entry(entry, struct mmc, link);
> 
> +		if (m->has_init)
> +			mmc_type = IS_SD(m) ? "SD" : "eMMC";
> +		else
> +			mmc_type = NULL;
> +
> 		printf("%s: %d", m->cfg->name, m->block_dev.dev);
> +		if (mmc_type)
> +			printf(" (%s)", mmc_type);
> 
> 		if (entry->next != &mmc_devices) {
> 			printf("%c", separator);
> -- 
> 1.9.1
> 

Applied, thanks

? Pantelis