[U-Boot] [PATCH 0/4] exynos-dwmmc: check set init priority for boot channel
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(-)
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
+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
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,
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.
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,
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,
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,
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.
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
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,
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
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,
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
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,
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,
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, };
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;
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
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,
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.
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,
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 ;)
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,
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).
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);
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(-)
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
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, };
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;
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);
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(-)
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;
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
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);
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";elsemmc_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
participants (5)
-
Pantelis Antoniou -
Przemyslaw Marczak -
Simon Glass -
Stephen Warren -
Tom Rini