Re: [PATCH] ARM: tegra: enable console framebuffer rotation
On 05/08/2014 12:57 AM, Stephen Warren wrote: On 05/06/2014 09:18 PM, Alexandre Courbot wrote: Console rotation is needed for devices like Tegra Note 7 and NVIDIA SHIELD to get the boot console in the expected orientation. I've squashed this into Tegra's for-3.16/defconfig branch. Can you please also update multi_v7_defconfig, and send that change to arm-soc (a...@kernel.org) to be applied. Thanks. I omitted doing this for now because the devices that require this option (TN7/SHIELD) need a custom build with appended DTB and/or command-line anyway. Therefore they cannot use a multi-mach kernel and might as well be built against Tegra's defconfig. Does your remark above still apply in spite of this? Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: enable console framebuffer rotation
On 05/08/2014 12:57 AM, Stephen Warren wrote: On 05/06/2014 09:18 PM, Alexandre Courbot wrote: Console rotation is needed for devices like Tegra Note 7 and NVIDIA SHIELD to get the boot console in the expected orientation. I've squashed this into Tegra's for-3.16/defconfig branch. Can you please also update multi_v7_defconfig, and send that change to arm-soc (a...@kernel.org) to be applied. Thanks. I omitted doing this for now because the devices that require this option (TN7/SHIELD) need a custom build with appended DTB and/or command-line anyway. Therefore they cannot use a multi-mach kernel and might as well be built against Tegra's defconfig. Does your remark above still apply in spite of this? Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpiolib: return -ENOENT if no GPIO mapping exists
On 12/11/2013 12:28 AM, Andy Shevchenko wrote:
On Tue, 2013-12-10 at 23:37 +0900, Alexandre Courbot wrote:
Some devices drivers make use of optional GPIO parameters. For such
drivers, it is important to discriminate between the case where no
GPIO mapping has been defined for the function they are requesting, and
the case where a mapping exists but an error occured while resolving it
or when acquiring the GPIO.
This patch changes the family of gpiod_get() functions such that they
will return -ENOENT if and only if no GPIO mapping is defined for the
requested function. Other error codes are used when an actual error
occured during the GPIO resolution.
One minor comment below.
Independently on that:
Reviewed-by: Andy Shevchenko
Thanks!
Signed-off-by: Alexandre Courbot
---
Changes since v1:
- Rebase on top of Linus' devel tree
- Fixed error reporting as suggested by Andy
---
Documentation/gpio/consumer.txt | 6 +-
drivers/gpio/gpiolib.c | 33 -
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 07c74a3..e42f77d 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -38,7 +38,11 @@ device that displays digits), an additional index argument
can be specified:
const char *con_id, unsigned int idx)
Both functions return either a valid GPIO descriptor, or an error code
checkable
-with IS_ERR(). They will never return a NULL pointer.
+with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
+if and only if no GPIO has been assigned to the device/function/index triplet,
+other error codes are used for cases where a GPIO has been assigned but an
error
+occured while trying to acquire it. This is useful to discriminate between mere
+errors and an absence of GPIO for optional GPIO parameters.
Device-managed variants of these functions are also defined:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 12e47df..32a6e3a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2367,7 +2367,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
unsigned int idx,
enum gpio_lookup_flags *flags)
{
- struct gpio_desc *desc = ERR_PTR(-ENODEV);
+ struct gpio_desc *desc = ERR_PTR(-ENOENT);
struct gpiod_lookup_table *table;
struct gpiod_lookup *p;
@@ -2389,19 +2389,21 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
chip = find_chip_by_name(p->chip_label);
if (!chip) {
- dev_warn(dev, "cannot find GPIO chip %s\n",
-p->chip_label);
- continue;
+ dev_err(dev, "cannot find GPIO chip %s\n",
+ p->chip_label);
+ return ERR_PTR(-ENODEV);
}
if (chip->ngpio <= p->chip_hwnum) {
- dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
-chip->label, chip->ngpio);
- continue;
+ dev_err(dev, "requested GPIO %d is out of range [0..%d]"
+ " for chip %s\n", idx, chip->ngpio, chip->label);
You may leave string literals over 80 character border. chackpatch.pl
will not complain about.
I might put a) 'dev_warn(dev,', b) '"..."', and c) 'the rest'
on three separate lines.
Oh right, I didn't know you could do that, thanks.
Having the message on a single line is indeed better for grepping. v3 on
the way...
+ return ERR_PTR(-EINVAL);
}
desc = gpiochip_offset_to_desc(chip, p->chip_hwnum);
*flags = p->flags;
+
+ return desc;
}
return desc;
@@ -2413,7 +2415,8 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
* @con_id: function within the GPIO consumer
*
* Return the GPIO descriptor corresponding to the function con_id of device
- * dev, or an IS_ERR() condition if an error occured.
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occured while trying to acquire the GPIO.
*/
struct gpio_desc *__must_check gpiod_get(struct device *dev, const char
*con_id)
{
@@ -2430,7 +2433,9 @@ EXPORT_SYMBOL_GPL(gpiod_get);
* This variant of gpiod_get() allows to access GPIOs other than the first
* defined one for functions that define several GPIOs.
*
- * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
+ * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or another IS_ERR() code if an
Re: [PATCH v2] gpiolib: return -ENOENT if no GPIO mapping exists
On 12/11/2013 12:28 AM, Andy Shevchenko wrote:
On Tue, 2013-12-10 at 23:37 +0900, Alexandre Courbot wrote:
Some devices drivers make use of optional GPIO parameters. For such
drivers, it is important to discriminate between the case where no
GPIO mapping has been defined for the function they are requesting, and
the case where a mapping exists but an error occured while resolving it
or when acquiring the GPIO.
This patch changes the family of gpiod_get() functions such that they
will return -ENOENT if and only if no GPIO mapping is defined for the
requested function. Other error codes are used when an actual error
occured during the GPIO resolution.
One minor comment below.
Independently on that:
Reviewed-by: Andy Shevchenko andriy.shevche...@linux.intel.com
Thanks!
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
Changes since v1:
- Rebase on top of Linus' devel tree
- Fixed error reporting as suggested by Andy
---
Documentation/gpio/consumer.txt | 6 +-
drivers/gpio/gpiolib.c | 33 -
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 07c74a3..e42f77d 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -38,7 +38,11 @@ device that displays digits), an additional index argument
can be specified:
const char *con_id, unsigned int idx)
Both functions return either a valid GPIO descriptor, or an error code
checkable
-with IS_ERR(). They will never return a NULL pointer.
+with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
+if and only if no GPIO has been assigned to the device/function/index triplet,
+other error codes are used for cases where a GPIO has been assigned but an
error
+occured while trying to acquire it. This is useful to discriminate between mere
+errors and an absence of GPIO for optional GPIO parameters.
Device-managed variants of these functions are also defined:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 12e47df..32a6e3a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2367,7 +2367,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
unsigned int idx,
enum gpio_lookup_flags *flags)
{
- struct gpio_desc *desc = ERR_PTR(-ENODEV);
+ struct gpio_desc *desc = ERR_PTR(-ENOENT);
struct gpiod_lookup_table *table;
struct gpiod_lookup *p;
@@ -2389,19 +2389,21 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
chip = find_chip_by_name(p-chip_label);
if (!chip) {
- dev_warn(dev, cannot find GPIO chip %s\n,
-p-chip_label);
- continue;
+ dev_err(dev, cannot find GPIO chip %s\n,
+ p-chip_label);
+ return ERR_PTR(-ENODEV);
}
if (chip-ngpio = p-chip_hwnum) {
- dev_warn(dev, GPIO chip %s has %d GPIOs\n,
-chip-label, chip-ngpio);
- continue;
+ dev_err(dev, requested GPIO %d is out of range [0..%d]
+ for chip %s\n, idx, chip-ngpio, chip-label);
You may leave string literals over 80 character border. chackpatch.pl
will not complain about.
I might put a) 'dev_warn(dev,', b) '...', and c) 'the rest'
on three separate lines.
Oh right, I didn't know you could do that, thanks.
Having the message on a single line is indeed better for grepping. v3 on
the way...
+ return ERR_PTR(-EINVAL);
}
desc = gpiochip_offset_to_desc(chip, p-chip_hwnum);
*flags = p-flags;
+
+ return desc;
}
return desc;
@@ -2413,7 +2415,8 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
* @con_id: function within the GPIO consumer
*
* Return the GPIO descriptor corresponding to the function con_id of device
- * dev, or an IS_ERR() condition if an error occured.
+ * dev, -ENOENT if no GPIO has been assigned to the requested function, or
+ * another IS_ERR() code if an error occured while trying to acquire the GPIO.
*/
struct gpio_desc *__must_check gpiod_get(struct device *dev, const char
*con_id)
{
@@ -2430,7 +2433,9 @@ EXPORT_SYMBOL_GPL(gpiod_get);
* This variant of gpiod_get() allows to access GPIOs other than the first
* defined one for functions that define several GPIOs.
*
- * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
+ * Return a valid GPIO descriptor, -ENOENT if no GPIO has been assigned to the
+ * requested function and/or index, or
Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists
On 12/09/2013 09:40 PM, Mika Westerberg wrote: On Fri, Dec 06, 2013 at 11:06:56AM +0900, Alexandre Courbot wrote: Some devices drivers make use of optional GPIO parameters. For such drivers, it is important to discriminate between the case where no GPIO mapping has been defined for the function they are requesting, and the case where a mapping exists but an error occured while resolving it or when acquiring the GPIO. This patch changes the family of gpiod_get() functions such that they will return -ENOENT if and only if no GPIO mapping is defined for the requested function. Other error codes are used when an actual error occured during the GPIO resolution. Signed-off-by: Alexandre Courbot --- I think this change should be merged early as not having it may prevent some users to switch to gpiod. I stumbled upon this issue while considering porting a simple driver (pwm_bl) that has an optional GPIO parameter. Mika, Andy: if Linus agrees with this change, could you take care of having -ENOENT returned as well for the ACPI and SFI GPIOs lookup? Sure. I have a patch for this already so once this gets merged, I'll send out the ACPI version. Please feel free to send it now, as it should not break anything if your patch is merged before mine anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists
On 12/09/2013 07:28 PM, Andy Shevchenko wrote:
On Fri, 2013-12-06 at 11:06 +0900, Alexandre Courbot wrote:
Some devices drivers make use of optional GPIO parameters. For such
drivers, it is important to discriminate between the case where no
GPIO mapping has been defined for the function they are requesting, and
the case where a mapping exists but an error occured while resolving it
or when acquiring the GPIO.
This patch changes the family of gpiod_get() functions such that they
will return -ENOENT if and only if no GPIO mapping is defined for the
requested function. Other error codes are used when an actual error
occured during the GPIO resolution.
I like the idea.
One minor comment below (in code).
Signed-off-by: Alexandre Courbot
---
I think this change should be merged early as not having it may prevent
some users to switch to gpiod. I stumbled upon this issue while
considering porting a simple driver (pwm_bl) that has an optional GPIO
parameter.
Mika, Andy: if Linus agrees with this change, could you take care of
having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?
I have already switched to -ENOENT, so, consider done.
Great, thanks!
My understanding of ACPI was not sufficient to allow me to do it myself.
SFI OTOH should be trivial as it is a simple table.
Documentation/gpio/consumer.txt | 6 +-
drivers/gpio/gpiolib.c | 31 ---
2 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 07c74a3765a0..e42f77d8d4ca 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -38,7 +38,11 @@ device that displays digits), an additional index argument
can be specified:
const char *con_id, unsigned int idx)
Both functions return either a valid GPIO descriptor, or an error code
checkable
-with IS_ERR(). They will never return a NULL pointer.
+with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
+if and only if no GPIO has been assigned to the device/function/index triplet,
+other error codes are used for cases where a GPIO has been assigned but an
error
+occured while trying to acquire it. This is useful to discriminate between mere
+errors and an absence of GPIO for optional GPIO parameters.
Device-managed variants of these functions are also defined:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5fad38fcd701..e96d4a90c0c3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2358,7 +2358,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
unsigned int idx,
enum gpio_lookup_flags *flags)
{
- struct gpio_desc *desc = ERR_PTR(-ENODEV);
+ struct gpio_desc *desc = ERR_PTR(-ENOENT);
struct gpiod_lookup_table *table;
struct gpiod_lookup *p;
@@ -2380,19 +2380,21 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
chip = find_chip_by_name(p->chip_label);
if (!chip) {
- dev_warn(dev, "cannot find GPIO chip %s\n",
-p->chip_label);
- continue;
+ dev_err(dev, "cannot find GPIO chip %s\n",
+ p->chip_label);
+ return ERR_PTR(-ENODEV);
}
if (chip->ngpio <= p->chip_hwnum) {
- dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
-chip->label, chip->ngpio);
- continue;
+ dev_err(dev, "requested GPIO %d but chip %s has %d\n",
The proposed message may confuse user. This lead to question in my head:
"what gpio chip has that referred by %d at the end of line".
Maybe something like "requested GPIO %d is out of range [0..%d] for chip
%s\n" ?
I agree it would be better. My concern here is to have the line fit
within 80 characters. :P
But you're right, I will improve this in v2.
Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SFI: remove wrong FSF address from the license
On 12/09/2013 06:32 PM, Andy Shevchenko wrote: Remove old and currently wrong address of the FSF from license parts of the code. The address is not wrong, it is just that you don't want to see it become wrong should it change in the future. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SFI: remove wrong FSF address from the license
On 12/09/2013 06:32 PM, Andy Shevchenko wrote: Remove old and currently wrong address of the FSF from license parts of the code. The address is not wrong, it is just that you don't want to see it become wrong should it change in the future. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists
On 12/09/2013 07:28 PM, Andy Shevchenko wrote:
On Fri, 2013-12-06 at 11:06 +0900, Alexandre Courbot wrote:
Some devices drivers make use of optional GPIO parameters. For such
drivers, it is important to discriminate between the case where no
GPIO mapping has been defined for the function they are requesting, and
the case where a mapping exists but an error occured while resolving it
or when acquiring the GPIO.
This patch changes the family of gpiod_get() functions such that they
will return -ENOENT if and only if no GPIO mapping is defined for the
requested function. Other error codes are used when an actual error
occured during the GPIO resolution.
I like the idea.
One minor comment below (in code).
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
I think this change should be merged early as not having it may prevent
some users to switch to gpiod. I stumbled upon this issue while
considering porting a simple driver (pwm_bl) that has an optional GPIO
parameter.
Mika, Andy: if Linus agrees with this change, could you take care of
having -ENOENT returned as well for the ACPI and SFI GPIOs lookup?
I have already switched to -ENOENT, so, consider done.
Great, thanks!
My understanding of ACPI was not sufficient to allow me to do it myself.
SFI OTOH should be trivial as it is a simple table.
Documentation/gpio/consumer.txt | 6 +-
drivers/gpio/gpiolib.c | 31 ---
2 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 07c74a3765a0..e42f77d8d4ca 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -38,7 +38,11 @@ device that displays digits), an additional index argument
can be specified:
const char *con_id, unsigned int idx)
Both functions return either a valid GPIO descriptor, or an error code
checkable
-with IS_ERR(). They will never return a NULL pointer.
+with IS_ERR() (they will never return a NULL pointer). -ENOENT will be returned
+if and only if no GPIO has been assigned to the device/function/index triplet,
+other error codes are used for cases where a GPIO has been assigned but an
error
+occured while trying to acquire it. This is useful to discriminate between mere
+errors and an absence of GPIO for optional GPIO parameters.
Device-managed variants of these functions are also defined:
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5fad38fcd701..e96d4a90c0c3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2358,7 +2358,7 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
unsigned int idx,
enum gpio_lookup_flags *flags)
{
- struct gpio_desc *desc = ERR_PTR(-ENODEV);
+ struct gpio_desc *desc = ERR_PTR(-ENOENT);
struct gpiod_lookup_table *table;
struct gpiod_lookup *p;
@@ -2380,19 +2380,21 @@ static struct gpio_desc *gpiod_find(struct device *dev,
const char *con_id,
chip = find_chip_by_name(p-chip_label);
if (!chip) {
- dev_warn(dev, cannot find GPIO chip %s\n,
-p-chip_label);
- continue;
+ dev_err(dev, cannot find GPIO chip %s\n,
+ p-chip_label);
+ return ERR_PTR(-ENODEV);
}
if (chip-ngpio = p-chip_hwnum) {
- dev_warn(dev, GPIO chip %s has %d GPIOs\n,
-chip-label, chip-ngpio);
- continue;
+ dev_err(dev, requested GPIO %d but chip %s has %d\n,
The proposed message may confuse user. This lead to question in my head:
what gpio chip has that referred by %d at the end of line.
Maybe something like requested GPIO %d is out of range [0..%d] for chip
%s\n ?
I agree it would be better. My concern here is to have the line fit
within 80 characters. :P
But you're right, I will improve this in v2.
Thanks,
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: return -ENOENT when no GPIO mapping exists
On 12/09/2013 09:40 PM, Mika Westerberg wrote: On Fri, Dec 06, 2013 at 11:06:56AM +0900, Alexandre Courbot wrote: Some devices drivers make use of optional GPIO parameters. For such drivers, it is important to discriminate between the case where no GPIO mapping has been defined for the function they are requesting, and the case where a mapping exists but an error occured while resolving it or when acquiring the GPIO. This patch changes the family of gpiod_get() functions such that they will return -ENOENT if and only if no GPIO mapping is defined for the requested function. Other error codes are used when an actual error occured during the GPIO resolution. Signed-off-by: Alexandre Courbot acour...@nvidia.com --- I think this change should be merged early as not having it may prevent some users to switch to gpiod. I stumbled upon this issue while considering porting a simple driver (pwm_bl) that has an optional GPIO parameter. Mika, Andy: if Linus agrees with this change, could you take care of having -ENOENT returned as well for the ACPI and SFI GPIOs lookup? Sure. I have a patch for this already so once this gets merged, I'll send out the ACPI version. Please feel free to send it now, as it should not break anything if your patch is merged before mine anyway. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv4] ARM: tegra: add gpiod_lookup table for paz00
On 12/03/2013 09:49 PM, Heikki Krogerus wrote:
This makes it possible to request the gpio descriptors in
rfkill_gpio driver regardless of the platform.
Signed-off-by: Heikki Krogerus
---
arch/arm/mach-tegra/board-paz00.c | 11 +++
1 file changed, 11 insertions(+)
Changes since v3:
-
Rebased on top of Alexandre's patch "gpio: better lookup method for
platform GPIOs".
diff --git a/arch/arm/mach-tegra/board-paz00.c
b/arch/arm/mach-tegra/board-paz00.c
index 06f0240..e4dec9f 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c
@@ -18,6 +18,7 @@
*/
#include
+#include
#include
#include "board.h"
@@ -36,7 +37,17 @@ static struct platform_device wifi_rfkill_device = {
},
};
+static struct gpiod_lookup_table wifi_gpio_lookup = {
+ .dev_id = "rfkill_gpio",
+ .table = {
+ GPIO_LOOKUP_IDX("tegra-gpio", 25, NULL, 0, 0),
+ GPIO_LOOKUP_IDX("tegra-gpio", 85, NULL, 1, 0),
nit: the flags could be changed to GPIO_ACTIVE_HIGH (which evaluates to
0) to be more explicit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv4] ARM: tegra: add gpiod_lookup table for paz00
On 12/03/2013 09:49 PM, Heikki Krogerus wrote:
This makes it possible to request the gpio descriptors in
rfkill_gpio driver regardless of the platform.
Signed-off-by: Heikki Krogerus heikki.kroge...@linux.intel.com
---
arch/arm/mach-tegra/board-paz00.c | 11 +++
1 file changed, 11 insertions(+)
Changes since v3:
-
Rebased on top of Alexandre's patch gpio: better lookup method for
platform GPIOs.
diff --git a/arch/arm/mach-tegra/board-paz00.c
b/arch/arm/mach-tegra/board-paz00.c
index 06f0240..e4dec9f 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c
@@ -18,6 +18,7 @@
*/
#include linux/platform_device.h
+#include linux/gpio/driver.h
#include linux/rfkill-gpio.h
#include board.h
@@ -36,7 +37,17 @@ static struct platform_device wifi_rfkill_device = {
},
};
+static struct gpiod_lookup_table wifi_gpio_lookup = {
+ .dev_id = rfkill_gpio,
+ .table = {
+ GPIO_LOOKUP_IDX(tegra-gpio, 25, NULL, 0, 0),
+ GPIO_LOOKUP_IDX(tegra-gpio, 85, NULL, 1, 0),
nit: the flags could be changed to GPIO_ACTIVE_HIGH (which evaluates to
0) to be more explicit.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: better lookup method for platform GPIOs
On 11/29/2013 12:54 AM, Andy Shevchenko wrote:
On Thu, Nov 28, 2013 at 10:46 AM, Alexandre Courbot wrote:
Change the format of the platform GPIO lookup tables to make them less
confusing and improve lookup efficiency.
The previous format was a single linked-list that required to compare
the device name and function ID of every single GPIO defined for each
lookup. Switch that to a list of per-device tables, so that the lookup
can be done in two steps, omitting the GPIOs that are not relevant for a
particular device.
The matching rules are now defined as follows:
- The device name must match *exactly*, and can be NULL for GPIOs not
assigned to a particular device,
- If the function ID in the lookup table is NULL, the con_id argument of
gpiod_get() will not be used for lookup. However, if it is defined, it
must match exactly.
- The index must always match.
Thanks for that, since I'm also was a bit confused of those dev_id/con_id stuff.
Few comments below (mostly about style).
--- a/Documentation/gpio/board.txt
+++ b/Documentation/gpio/board.txt
@@ -88,16 +89,20 @@ Note that GPIO_LOOKUP() is just a shortcut to
GPIO_LOOKUP_IDX() where idx = 0.
A lookup table can then be defined as follows:
- struct gpiod_lookup gpios_table[] = {
- GPIO_LOOKUP_IDX("gpio.0", 15, "foo.0", "led", 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio.0", 16, "foo.0", "led", 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio.0", 17, "foo.0", "led", 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP("gpio.0", 1, "foo.0", "power", GPIO_ACTIVE_LOW),
- };
+struct gpiod_lookup_table gpios_table = {
+ .dev_id = "foo.0",
+ .size = 4,
+ .table = {
+ GPIO_LOOKUP_IDX("gpio.0", 15, "led", 0, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio.0", 16, "led", 1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio.0", 17, "led", 2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("gpio.0", 1, "power", GPIO_ACTIVE_LOW),
Can you use deeper indentation for GPIO_* lines here?
Fixed.
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2326,72 +2322,77 @@ static struct gpio_desc *acpi_find_gpio(struct device
*dev, const char *con_id,
return desc;
}
-static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
- unsigned int idx,
- enum gpio_lookup_flags *flags)
+static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
{
const char *dev_id = dev ? dev_name(dev) : NULL;
- struct gpio_desc *desc = ERR_PTR(-ENODEV);
- unsigned int match, best = 0;
- struct gpiod_lookup *p;
+ struct gpiod_lookup_table *table;
mutex_lock(_lookup_lock);
- list_for_each_entry(p, _lookup_list, list) {
- match = 0;
+ list_for_each_entry(table, _lookup_list, list) {
+ if (table->dev_id && dev_id && strcmp(table->dev_id, dev_id))
Maybe check !dev_id outside of loop?
And create two loops, one for each case? Might complicate the code for
little benefit IMHO, but please elaborate if I missed your point.
+ continue;
- if (p->dev_id) {
- if (!dev_id || strcmp(p->dev_id, dev_id))
- continue;
+ if (dev_id != table->dev_id)
+ continue;
- match += 2;
- }
+ return table;
What about
if (dev_id == table->dev_id)
return table;
?
Actually my algorithm is broken to start with - and dangerous, as the
missed mutex_unlock() you spotted later testifies. I will rewrite it in
a (hopefully) sounder way.
+static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
+ unsigned int idx,
+ enum gpio_lookup_flags *flags)
+{
+ struct gpio_desc *desc = ERR_PTR(-ENODEV);
+ struct gpiod_lookup_table *table;
+ int i;
- if (match > best) {
- struct gpio_chip *chip;
Looks like redundant empty line.
Fixed.
- chip = find_chip_by_name(p->chip_label);
+ table = gpiod_find_lookup_table(dev);
+ if (!table)
+ return desc;
- if (!chip) {
- dev_warn(dev, "cannot find GPIO chip %s\n",
-p->chip_label);
- continue;
- }
+ for (i = 0; i < table->size; i++) {
+ struct gpio_chip *chip;
+ struct gpiod_lookup *p = >table[i];
- if (chip->ngpio <= p->chip_hwnum) {
- dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
-chip->label, chip->ngpio);
+ if (p->idx != idx)
+ continue;
+
+
Re: [PATCH] gpio: better lookup method for platform GPIOs
On 11/29/2013 08:57 PM, Heikki Krogerus wrote:
Hi,
On Thu, Nov 28, 2013 at 05:46:28PM +0900, Alexandre Courbot wrote:
@@ -88,16 +89,20 @@ Note that GPIO_LOOKUP() is just a shortcut to
GPIO_LOOKUP_IDX() where idx = 0.
A lookup table can then be defined as follows:
- struct gpiod_lookup gpios_table[] = {
- GPIO_LOOKUP_IDX("gpio.0", 15, "foo.0", "led", 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio.0", 16, "foo.0", "led", 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX("gpio.0", 17, "foo.0", "led", 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP("gpio.0", 1, "foo.0", "power", GPIO_ACTIVE_LOW),
- };
+struct gpiod_lookup_table gpios_table = {
+ .dev_id = "foo.0",
+ .size = 4,
+ .table = {
+ GPIO_LOOKUP_IDX("gpio.0", 15, "led", 0, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio.0", 16, "led", 1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("gpio.0", 17, "led", 2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP("gpio.0", 1, "power", GPIO_ACTIVE_LOW),
+ },
+};
Instead of using the size variable, wouldn't it be more clear to
expect the array to be null terminated?
This is a zero-length array, its entries are not pointers but flattened
lookup entries. Thus you cannot simply null-terminate it. It would be
possible to use { NULL } as a terminator, but this would expand into a
whole gpiod_lookup and is not very pleasant esthetically-speaking. So I
think the size member is maybe better suited here.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: better lookup method for platform GPIOs
On 11/29/2013 08:57 PM, Heikki Krogerus wrote:
Hi,
On Thu, Nov 28, 2013 at 05:46:28PM +0900, Alexandre Courbot wrote:
@@ -88,16 +89,20 @@ Note that GPIO_LOOKUP() is just a shortcut to
GPIO_LOOKUP_IDX() where idx = 0.
A lookup table can then be defined as follows:
- struct gpiod_lookup gpios_table[] = {
- GPIO_LOOKUP_IDX(gpio.0, 15, foo.0, led, 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(gpio.0, 16, foo.0, led, 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(gpio.0, 17, foo.0, led, 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP(gpio.0, 1, foo.0, power, GPIO_ACTIVE_LOW),
- };
+struct gpiod_lookup_table gpios_table = {
+ .dev_id = foo.0,
+ .size = 4,
+ .table = {
+ GPIO_LOOKUP_IDX(gpio.0, 15, led, 0, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX(gpio.0, 16, led, 1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX(gpio.0, 17, led, 2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(gpio.0, 1, power, GPIO_ACTIVE_LOW),
+ },
+};
Instead of using the size variable, wouldn't it be more clear to
expect the array to be null terminated?
This is a zero-length array, its entries are not pointers but flattened
lookup entries. Thus you cannot simply null-terminate it. It would be
possible to use { NULL } as a terminator, but this would expand into a
whole gpiod_lookup and is not very pleasant esthetically-speaking. So I
think the size member is maybe better suited here.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpio: better lookup method for platform GPIOs
On 11/29/2013 12:54 AM, Andy Shevchenko wrote:
On Thu, Nov 28, 2013 at 10:46 AM, Alexandre Courbot acour...@nvidia.com wrote:
Change the format of the platform GPIO lookup tables to make them less
confusing and improve lookup efficiency.
The previous format was a single linked-list that required to compare
the device name and function ID of every single GPIO defined for each
lookup. Switch that to a list of per-device tables, so that the lookup
can be done in two steps, omitting the GPIOs that are not relevant for a
particular device.
The matching rules are now defined as follows:
- The device name must match *exactly*, and can be NULL for GPIOs not
assigned to a particular device,
- If the function ID in the lookup table is NULL, the con_id argument of
gpiod_get() will not be used for lookup. However, if it is defined, it
must match exactly.
- The index must always match.
Thanks for that, since I'm also was a bit confused of those dev_id/con_id stuff.
Few comments below (mostly about style).
--- a/Documentation/gpio/board.txt
+++ b/Documentation/gpio/board.txt
@@ -88,16 +89,20 @@ Note that GPIO_LOOKUP() is just a shortcut to
GPIO_LOOKUP_IDX() where idx = 0.
A lookup table can then be defined as follows:
- struct gpiod_lookup gpios_table[] = {
- GPIO_LOOKUP_IDX(gpio.0, 15, foo.0, led, 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(gpio.0, 16, foo.0, led, 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(gpio.0, 17, foo.0, led, 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP(gpio.0, 1, foo.0, power, GPIO_ACTIVE_LOW),
- };
+struct gpiod_lookup_table gpios_table = {
+ .dev_id = foo.0,
+ .size = 4,
+ .table = {
+ GPIO_LOOKUP_IDX(gpio.0, 15, led, 0, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX(gpio.0, 16, led, 1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX(gpio.0, 17, led, 2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(gpio.0, 1, power, GPIO_ACTIVE_LOW),
Can you use deeper indentation for GPIO_* lines here?
Fixed.
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2326,72 +2322,77 @@ static struct gpio_desc *acpi_find_gpio(struct device
*dev, const char *con_id,
return desc;
}
-static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
- unsigned int idx,
- enum gpio_lookup_flags *flags)
+static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
{
const char *dev_id = dev ? dev_name(dev) : NULL;
- struct gpio_desc *desc = ERR_PTR(-ENODEV);
- unsigned int match, best = 0;
- struct gpiod_lookup *p;
+ struct gpiod_lookup_table *table;
mutex_lock(gpio_lookup_lock);
- list_for_each_entry(p, gpio_lookup_list, list) {
- match = 0;
+ list_for_each_entry(table, gpio_lookup_list, list) {
+ if (table-dev_id dev_id strcmp(table-dev_id, dev_id))
Maybe check !dev_id outside of loop?
And create two loops, one for each case? Might complicate the code for
little benefit IMHO, but please elaborate if I missed your point.
+ continue;
- if (p-dev_id) {
- if (!dev_id || strcmp(p-dev_id, dev_id))
- continue;
+ if (dev_id != table-dev_id)
+ continue;
- match += 2;
- }
+ return table;
What about
if (dev_id == table-dev_id)
return table;
?
Actually my algorithm is broken to start with - and dangerous, as the
missed mutex_unlock() you spotted later testifies. I will rewrite it in
a (hopefully) sounder way.
+static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
+ unsigned int idx,
+ enum gpio_lookup_flags *flags)
+{
+ struct gpio_desc *desc = ERR_PTR(-ENODEV);
+ struct gpiod_lookup_table *table;
+ int i;
- if (match best) {
- struct gpio_chip *chip;
Looks like redundant empty line.
Fixed.
- chip = find_chip_by_name(p-chip_label);
+ table = gpiod_find_lookup_table(dev);
+ if (!table)
+ return desc;
- if (!chip) {
- dev_warn(dev, cannot find GPIO chip %s\n,
-p-chip_label);
- continue;
- }
+ for (i = 0; i table-size; i++) {
+ struct gpio_chip *chip;
+ struct gpiod_lookup *p = table-table[i];
- if (chip-ngpio = p-chip_hwnum) {
- dev_warn(dev, GPIO chip %s has %d GPIOs\n,
-chip-label, chip-ngpio);
+ if (p-idx != idx)
+ continue;
+
+ if (p-con_id) {
+
Re: [PATCH] gpiolib: use platform GPIO mappings as fallback
On 11/29/2013 06:29 PM, Linus Walleij wrote: On Sat, Nov 23, 2013 at 11:34 AM, Alexandre Courbot wrote: For platforms that use device tree or ACPI as the standard way to look GPIOs up, allow the platform-defined GPIO mappings to be used as a fallback. This may be useful for platforms that need extra GPIOs mappings not defined by the firmware. Signed-off-by: Alexandre Courbot Applied for fixes. I was considering applying it for the next kernel but it depends on the former patches and we should expect a bit of extra fuzz now with the new API. Thanks - I'd rather have all the problems coming in a row than incrementally as we introduce potentially incompatible changes. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: use platform GPIO mappings as fallback
On 11/29/2013 06:29 PM, Linus Walleij wrote: On Sat, Nov 23, 2013 at 11:34 AM, Alexandre Courbot acour...@nvidia.com wrote: For platforms that use device tree or ACPI as the standard way to look GPIOs up, allow the platform-defined GPIO mappings to be used as a fallback. This may be useful for platforms that need extra GPIOs mappings not defined by the firmware. Signed-off-by: Alexandre Courbot acour...@nvidia.com Applied for fixes. I was considering applying it for the next kernel but it depends on the former patches and we should expect a bit of extra fuzz now with the new API. Thanks - I'd rather have all the problems coming in a row than incrementally as we introduce potentially incompatible changes. Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 7/7] ARM: tegra: support Trusted Foundations by default
On 11/29/2013 04:47 AM, Dave Martin wrote: On Thu, Nov 28, 2013 at 04:58:33PM +, Stephen Warren wrote: On 11/27/2013 11:02 PM, Alexandre Courbot wrote: On Wed, Nov 27, 2013 at 1:47 AM, Dave Martin wrote: On Tue, Nov 26, 2013 at 10:35:58AM +0900, Alexandre Courbot wrote: On Tue, Nov 26, 2013 at 9:06 AM, Olof Johansson wrote: On Sun, Nov 24, 2013 at 03:30:52PM +0900, Alexandre Courbot wrote: Support for Trusted Foundations is light and allows the kernel to run on a wider range of devices, so enable it by default. Signed-off-by: Alexandre Courbot Reviewed-by: Tomasz Figa Reviewed-by: Stephen Warren --- arch/arm/configs/tegra_defconfig | 1 + 1 file changed, 1 insertion(+) I think we want this enabled on multi_v7_defconfig too? Send a separate patch for that once this is merged though. Will do. Should it just be default y if one of the relevant CONFIG_ARCH_TEGRA_*_SOC is selected? That way, it's automatically included if relevant, and automatically excluded if not -- regardless of whether the kernel is multiplatform or not. So basically, that would mean setting the default to 'y' since the option is not available unless a supported platform is included? I'm fine this way too, if Stephen also agrees. Fine by me. Sure, as long as the result is right, that should be fine. Ok, then please ignore this patch in this series and I will send a new one that changes the CONFIG_TRUSTED_FOUNDATION's default setting once we have confirmation from Russell that this series can be merged. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 7/7] ARM: tegra: support Trusted Foundations by default
On 11/29/2013 04:47 AM, Dave Martin wrote: On Thu, Nov 28, 2013 at 04:58:33PM +, Stephen Warren wrote: On 11/27/2013 11:02 PM, Alexandre Courbot wrote: On Wed, Nov 27, 2013 at 1:47 AM, Dave Martin dave.mar...@arm.com wrote: On Tue, Nov 26, 2013 at 10:35:58AM +0900, Alexandre Courbot wrote: On Tue, Nov 26, 2013 at 9:06 AM, Olof Johansson o...@lixom.net wrote: On Sun, Nov 24, 2013 at 03:30:52PM +0900, Alexandre Courbot wrote: Support for Trusted Foundations is light and allows the kernel to run on a wider range of devices, so enable it by default. Signed-off-by: Alexandre Courbot acour...@nvidia.com Reviewed-by: Tomasz Figa t.f...@samsung.com Reviewed-by: Stephen Warren swar...@nvidia.com --- arch/arm/configs/tegra_defconfig | 1 + 1 file changed, 1 insertion(+) I think we want this enabled on multi_v7_defconfig too? Send a separate patch for that once this is merged though. Will do. Should it just be default y if one of the relevant CONFIG_ARCH_TEGRA_*_SOC is selected? That way, it's automatically included if relevant, and automatically excluded if not -- regardless of whether the kernel is multiplatform or not. So basically, that would mean setting the default to 'y' since the option is not available unless a supported platform is included? I'm fine this way too, if Stephen also agrees. Fine by me. Sure, as long as the result is right, that should be fine. Ok, then please ignore this patch in this series and I will send a new one that changes the CONFIG_TRUSTED_FOUNDATION's default setting once we have confirmation from Russell that this series can be merged. Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/6] net: rfkill: gpio: convert to descriptor-based GPIO interface
On 11/26/2013 07:05 PM, Mika Westerberg wrote: From: Heikki Krogerus Convert to the safer gpiod_* family of API functions. Reviewed-by: Alexandre Courbot -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/6] ARM: tegra: add gpiod_lookup table for paz00
On 11/27/2013 05:33 AM, Stephen Warren wrote:
On 11/26/2013 03:05 AM, Mika Westerberg wrote:
From: Heikki Krogerus
This makes it possible to request the gpio descriptors in
rfkill_gpio driver regardless of the platform.
V2 of the series did the following:
static struct rfkill_gpio_platform_data wifi_rfkill_platform_data = {
.name = "wifi_rfkill",
- .reset_gpio = 25, /* PD1 */
- .shutdown_gpio = 85, /* PK5 */
.type = RFKILL_TYPE_WLAN,
};
I assume that will happen sometime, along with removing those two fields
from the struct definition.
This needs to happen after patch 2 of this series, but would be nice to
have indeed. Would suggest to do it in a separate patch if this series
needs no more revision.
Reviewed-by: Alexandre Courbot
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/6] ARM: tegra: add gpiod_lookup table for paz00
On 11/27/2013 05:33 AM, Stephen Warren wrote:
On 11/26/2013 03:05 AM, Mika Westerberg wrote:
From: Heikki Krogerus heikki.kroge...@linux.intel.com
This makes it possible to request the gpio descriptors in
rfkill_gpio driver regardless of the platform.
V2 of the series did the following:
static struct rfkill_gpio_platform_data wifi_rfkill_platform_data = {
.name = wifi_rfkill,
- .reset_gpio = 25, /* PD1 */
- .shutdown_gpio = 85, /* PK5 */
.type = RFKILL_TYPE_WLAN,
};
I assume that will happen sometime, along with removing those two fields
from the struct definition.
This needs to happen after patch 2 of this series, but would be nice to
have indeed. Would suggest to do it in a separate patch if this series
needs no more revision.
Reviewed-by: Alexandre Courbot acour...@nvidia.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/6] net: rfkill: gpio: convert to descriptor-based GPIO interface
On 11/26/2013 07:05 PM, Mika Westerberg wrote: From: Heikki Krogerus heikki.kroge...@linux.intel.com Convert to the safer gpiod_* family of API functions. Reviewed-by: Alexandre Courbot acour...@nvidia.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: add missing declarations
On 11/25/2013 06:28 PM, Mika Westerberg wrote: On Mon, Nov 25, 2013 at 06:07:10PM +0900, Alexandre Courbot wrote: On Mon, Nov 25, 2013 at 5:58 PM, Mika Westerberg wrote: On Sat, Nov 23, 2013 at 02:54:29PM +0900, Alexandre Courbot wrote: Add missing declarations and include files to avoid warnings during compilation of include/gpio/driver.h. It would be good to have those warnings included in the changelog as well. As in, copy-pasting the compiler's output? Isn't the change explicit enough? If you just do 'git log' you can't see the change itself and that's where the changelog should help. If I see some warnings when I build the kernel, the first thing I usually do is to check 'git log' against linux-next and see if someone has already fixed that warning (grepping the warning message from changelog). Ah, I can see several examples of what you mention indeed. Will add these and send a v2. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] net: rfkill: gpio: remove gpio conversion support
On 11/25/2013 06:02 PM, Heikki Krogerus wrote: On Mon, Nov 25, 2013 at 05:47:38PM +0900, Alex Courbot wrote: On 11/25/2013 05:41 PM, Heikki Krogerus wrote: Adding the lookup table in first patch and then changing the driver in the second creates a point to the history where this driver stops working on this platform, which is something I'm not willing to do. Does it? If you just add a lookup table and keep using the integer-based GPIO interface, then your lookup table will not be used by anyone and will basically be a no-op. Then you can switch to the GPIO descriptor interface and take advantage of the lookup table. Unless I missed something there should not be any point that breaks in the git history. (to be clear: the first patch should *only* contain the lookup table, and the second be a merge of the current patches 1 and 3 of this series.) OK, I agree. If I don't remove the old gpio numbers in in the first patch, there is no problem. We can do this with the two patches. Yep, that's what I meant. :) It would make the series considerably easier to understand by removing its temporary code. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 6/7] gpio / ACPI: get rid of acpi_gpio.h
On 11/25/2013 05:54 PM, Mika Westerberg wrote:
On Sat, Nov 23, 2013 at 06:21:03PM +0900, Alexandre Courbot wrote:
On Fri, Nov 22, 2013 at 9:14 PM, Mika Westerberg
wrote:
Now that all users of acpi_gpio.h have been moved to user either the GPIO
s/user/use
OK.
descriptor interface or to the internal gpiolib.h we can get rid of
gpiolib.h will also need to be renamed if you follow my suggestion below.
acpi_gpio.h entirely.
Once this is done the only interface to get GPIOs to drivers enumerated
from ACPI namespace is the descriptor based interface.
Oh, do I like this. :)
Signed-off-by: Mika Westerberg
---
drivers/gpio/gpiolib-acpi.c | 4 +++-
drivers/gpio/gpiolib.c | 1 -
drivers/gpio/gpiolib.h | 23 +++
include/linux/acpi_gpio.h | 45 -
4 files changed, 26 insertions(+), 47 deletions(-)
delete mode 100644 include/linux/acpi_gpio.h
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index cb2da66fbbfe..6c158d9a6efa 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -12,11 +12,13 @@
#include
#include
+#include
#include
-#include
#include
#include
+#include "gpiolib.h"
+
struct acpi_gpio_evt_pin {
struct list_head node;
acpi_handle *evt_handle;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 70abccf0d144..a6b82413c290 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -10,7 +10,6 @@
#include
#include
#include
-#include
#include
#include
#include
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2ed23ab8298c..82be586c1f90 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -12,12 +12,35 @@
#ifndef GPIOLIB_H
#define GPIOLIB_H
+#include
+#include
+
+/**
+ * struct acpi_gpio_info - ACPI GPIO specific information
+ * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
+ * @active_low: in case of @gpioint, the pin is active low
+ */
+struct acpi_gpio_info {
+ bool gpioint;
+ bool active_low;
+};
+
#ifdef CONFIG_ACPI
void acpi_gpiochip_add(struct gpio_chip *chip);
void acpi_gpiochip_remove(struct gpio_chip *chip);
+
+struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
+ struct acpi_gpio_info *info);
#else
static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
+
+static inline struct gpio_desc *
+acpi_get_gpiod_by_index(struct device *dev, int index,
+ struct acpi_gpio_info *info)
+{
+ return ERR_PTR(-ENOSYS);
+}
#endif
Since this header contains purely ACPI-related declarations so far, I wonder
if it should not be renamed acpi.h?
Well, for now it contains only ACPI related stuff but once you guys start
tearing DT interfaces away from drivers, this header is expected to grow :)
So idea is to have gpiolib internal/private header where we can add stuff
taht is not going to be exported to drivers.
I just wondered if these headers should not be split by GPIO providers,
but if you prefer it that way I'm fine with it too. We can move stuff
later if needed.
Acked-by: Alexandre Courbot
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] net: rfkill: gpio: remove gpio conversion support
On 11/25/2013 05:41 PM, Heikki Krogerus wrote: On Sat, Nov 23, 2013 at 05:59:30PM +0900, Alexandre Courbot wrote: Wouldn't it be possible (and simpler) to move patch 2 of your series to first position, and then to merge patch 1 and 3 together in second position? It seems to me that you are basically undoing much of the work of your first patch here (notably by removing rfkill_gpio_convert_to_desc() which ends up having a very short life) and that this could be avoided if you defined the platform lookup tables first. Doing so would avoid prevent you from using gpio_to_desc() which you should never ever use anyway. :P Adding the lookup table in first patch and then changing the driver in the second creates a point to the history where this driver stops working on this platform, which is something I'm not willing to do. Does it? If you just add a lookup table and keep using the integer-based GPIO interface, then your lookup table will not be used by anyone and will basically be a no-op. Then you can switch to the GPIO descriptor interface and take advantage of the lookup table. Unless I missed something there should not be any point that breaks in the git history. (to be clear: the first patch should *only* contain the lookup table, and the second be a merge of the current patches 1 and 3 of this series.) But, we can make one patch out of all three if everybody is OK with that. IIUC platform changes should be distinct from drivers whenever possible, so this is probably not the best choice here. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] net: rfkill: gpio: remove gpio conversion support
On 11/25/2013 05:41 PM, Heikki Krogerus wrote: On Sat, Nov 23, 2013 at 05:59:30PM +0900, Alexandre Courbot wrote: Wouldn't it be possible (and simpler) to move patch 2 of your series to first position, and then to merge patch 1 and 3 together in second position? It seems to me that you are basically undoing much of the work of your first patch here (notably by removing rfkill_gpio_convert_to_desc() which ends up having a very short life) and that this could be avoided if you defined the platform lookup tables first. Doing so would avoid prevent you from using gpio_to_desc() which you should never ever use anyway. :P Adding the lookup table in first patch and then changing the driver in the second creates a point to the history where this driver stops working on this platform, which is something I'm not willing to do. Does it? If you just add a lookup table and keep using the integer-based GPIO interface, then your lookup table will not be used by anyone and will basically be a no-op. Then you can switch to the GPIO descriptor interface and take advantage of the lookup table. Unless I missed something there should not be any point that breaks in the git history. (to be clear: the first patch should *only* contain the lookup table, and the second be a merge of the current patches 1 and 3 of this series.) But, we can make one patch out of all three if everybody is OK with that. IIUC platform changes should be distinct from drivers whenever possible, so this is probably not the best choice here. Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 6/7] gpio / ACPI: get rid of acpi_gpio.h
On 11/25/2013 05:54 PM, Mika Westerberg wrote:
On Sat, Nov 23, 2013 at 06:21:03PM +0900, Alexandre Courbot wrote:
On Fri, Nov 22, 2013 at 9:14 PM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
Now that all users of acpi_gpio.h have been moved to user either the GPIO
s/user/use
OK.
descriptor interface or to the internal gpiolib.h we can get rid of
gpiolib.h will also need to be renamed if you follow my suggestion below.
acpi_gpio.h entirely.
Once this is done the only interface to get GPIOs to drivers enumerated
from ACPI namespace is the descriptor based interface.
Oh, do I like this. :)
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
---
drivers/gpio/gpiolib-acpi.c | 4 +++-
drivers/gpio/gpiolib.c | 1 -
drivers/gpio/gpiolib.h | 23 +++
include/linux/acpi_gpio.h | 45 -
4 files changed, 26 insertions(+), 47 deletions(-)
delete mode 100644 include/linux/acpi_gpio.h
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index cb2da66fbbfe..6c158d9a6efa 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -12,11 +12,13 @@
#include linux/errno.h
#include linux/gpio/consumer.h
+#include linux/gpio/driver.h
#include linux/export.h
-#include linux/acpi_gpio.h
#include linux/acpi.h
#include linux/interrupt.h
+#include gpiolib.h
+
struct acpi_gpio_evt_pin {
struct list_head node;
acpi_handle *evt_handle;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 70abccf0d144..a6b82413c290 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -10,7 +10,6 @@
#include linux/seq_file.h
#include linux/gpio.h
#include linux/of_gpio.h
-#include linux/acpi_gpio.h
#include linux/idr.h
#include linux/slab.h
#include linux/acpi.h
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2ed23ab8298c..82be586c1f90 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -12,12 +12,35 @@
#ifndef GPIOLIB_H
#define GPIOLIB_H
+#include linux/err.h
+#include linux/device.h
+
+/**
+ * struct acpi_gpio_info - ACPI GPIO specific information
+ * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
+ * @active_low: in case of @gpioint, the pin is active low
+ */
+struct acpi_gpio_info {
+ bool gpioint;
+ bool active_low;
+};
+
#ifdef CONFIG_ACPI
void acpi_gpiochip_add(struct gpio_chip *chip);
void acpi_gpiochip_remove(struct gpio_chip *chip);
+
+struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
+ struct acpi_gpio_info *info);
#else
static inline void acpi_gpiochip_add(struct gpio_chip *chip) { }
static inline void acpi_gpiochip_remove(struct gpio_chip *chip) { }
+
+static inline struct gpio_desc *
+acpi_get_gpiod_by_index(struct device *dev, int index,
+ struct acpi_gpio_info *info)
+{
+ return ERR_PTR(-ENOSYS);
+}
#endif
Since this header contains purely ACPI-related declarations so far, I wonder
if it should not be renamed acpi.h?
Well, for now it contains only ACPI related stuff but once you guys start
tearing DT interfaces away from drivers, this header is expected to grow :)
So idea is to have gpiolib internal/private header where we can add stuff
taht is not going to be exported to drivers.
I just wondered if these headers should not be split by GPIO providers,
but if you prefer it that way I'm fine with it too. We can move stuff
later if needed.
Acked-by: Alexandre Courbot acour...@nvidia.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/7] net: rfkill: gpio: remove gpio conversion support
On 11/25/2013 06:02 PM, Heikki Krogerus wrote: On Mon, Nov 25, 2013 at 05:47:38PM +0900, Alex Courbot wrote: On 11/25/2013 05:41 PM, Heikki Krogerus wrote: Adding the lookup table in first patch and then changing the driver in the second creates a point to the history where this driver stops working on this platform, which is something I'm not willing to do. Does it? If you just add a lookup table and keep using the integer-based GPIO interface, then your lookup table will not be used by anyone and will basically be a no-op. Then you can switch to the GPIO descriptor interface and take advantage of the lookup table. Unless I missed something there should not be any point that breaks in the git history. (to be clear: the first patch should *only* contain the lookup table, and the second be a merge of the current patches 1 and 3 of this series.) OK, I agree. If I don't remove the old gpio numbers in in the first patch, there is no problem. We can do this with the two patches. Yep, that's what I meant. :) It would make the series considerably easier to understand by removing its temporary code. Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] gpiolib: add missing declarations
On 11/25/2013 06:28 PM, Mika Westerberg wrote: On Mon, Nov 25, 2013 at 06:07:10PM +0900, Alexandre Courbot wrote: On Mon, Nov 25, 2013 at 5:58 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Sat, Nov 23, 2013 at 02:54:29PM +0900, Alexandre Courbot wrote: Add missing declarations and include files to avoid warnings during compilation of include/gpio/driver.h. It would be good to have those warnings included in the changelog as well. As in, copy-pasting the compiler's output? Isn't the change explicit enough? If you just do 'git log' you can't see the change itself and that's where the changelog should help. If I see some warnings when I build the kernel, the first thing I usually do is to check 'git log' against linux-next and see if someone has already fixed that warning (grepping the warning message from changelog). Ah, I can see several examples of what you mention indeed. Will add these and send a v2. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1.2] gpiolib: append SFI helpers for GPIO API
On 11/19/2013 06:24 PM, Linus Walleij wrote:
On Wed, Jun 5, 2013 at 3:58 PM, Andy Shevchenko
wrote:
To support some (legacy) firmwares and platforms let's make life easier for
their customers.
This patch extracts SFI GPIO API from arch/x86/platform/mrst/mrst.c.
Signed-off-by: Andy Shevchenko
So since this patch was ACKed the world has changed a bit and now
I want new changes (or maybe I was tired and not paying enough
attention at the time).
(...)
+int sfi_get_gpio_by_name(const char *name)
+{
+ struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
+ int i;
+
+ if (!pentry)
+ return -EINVAL;
+
+ for (i = 0; i < sfi_gpio_num_entry; i++, pentry++) {
+ if (!strncmp(name, pentry->pin_name, SFI_NAME_LEN))
+ return pentry->pin_no;
+ }
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(sfi_get_gpio_by_name);
Last merge window we merged the GPIO descriptor API and this
is now the recommended way to handle GPIOs and it is also
deployed into the ACPI and DT implementations.
So I'd like the signature of this function changed to return
a GPIO descriptor rather than an int so we don't stockpile more
stuff to refactor.
i.e.:
struct gpio_desc *sfi_get_gpio_by_name(const char *name);
--- /dev/null
+++ b/include/linux/sfi_gpio.h
Maybe that header could move to instead.
Alexandre what do you think?
Agreed - all the GPIO drivers into drivers/gpio, all the headers into
include/linux/gpio. Logical. :)
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1.2] gpiolib: append SFI helpers for GPIO API
On 11/19/2013 06:24 PM, Linus Walleij wrote:
On Wed, Jun 5, 2013 at 3:58 PM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
To support some (legacy) firmwares and platforms let's make life easier for
their customers.
This patch extracts SFI GPIO API from arch/x86/platform/mrst/mrst.c.
Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
So since this patch was ACKed the world has changed a bit and now
I want new changes (or maybe I was tired and not paying enough
attention at the time).
(...)
+int sfi_get_gpio_by_name(const char *name)
+{
+ struct sfi_gpio_table_entry *pentry = sfi_gpio_table;
+ int i;
+
+ if (!pentry)
+ return -EINVAL;
+
+ for (i = 0; i sfi_gpio_num_entry; i++, pentry++) {
+ if (!strncmp(name, pentry-pin_name, SFI_NAME_LEN))
+ return pentry-pin_no;
+ }
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(sfi_get_gpio_by_name);
Last merge window we merged the GPIO descriptor API and this
is now the recommended way to handle GPIOs and it is also
deployed into the ACPI and DT implementations.
So I'd like the signature of this function changed to return
a GPIO descriptor rather than an int so we don't stockpile more
stuff to refactor.
i.e.:
struct gpio_desc *sfi_get_gpio_by_name(const char *name);
--- /dev/null
+++ b/include/linux/sfi_gpio.h
Maybe that header could move to linux/gpio/sfi.h instead.
Alexandre what do you think?
Agreed - all the GPIO drivers into drivers/gpio, all the headers into
include/linux/gpio. Logical. :)
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: properly use FUSE clock
On 11/19/2013 08:48 AM, Stephen Warren wrote:
On 11/18/2013 04:43 AM, Thierry Reding wrote:
On Mon, Nov 18, 2013 at 07:40:47PM +0900, Alexandre Courbot wrote:
FUSE clock is enabled by most bootloaders, but we cannot expect
it to be on in all contexts (e.g. kexec).
This patch adds a FUSE clkdev to all Tegra platforms and makes
sure it is enabled before touching FUSE registers.
tegra_init_fuse() is invoked during very early boot and thus
cannot rely on the clock framework ; therefore the FUSE clock is
forcibly enabled using a register write in that function, and
remains that way until the clock framework can be used.
Signed-off-by: Alexandre Courbot ---
arch/arm/mach-tegra/fuse.c | 41
+++-
drivers/clk/tegra/clk-tegra114.c | 1 +
drivers/clk/tegra/clk-tegra124.c | 1 +
drivers/clk/tegra/clk-tegra20.c | 1 +
Isn't this missing the clock driver changes for Tegra30? Ah...
Tegra30 already has this clock defined. I wonder why only Tegra30
has it. grep says that fuse-tegra isn't used by any drivers, which
also indicates that perhaps we don't need the .dev_id in the first
place. We should be able to get by with just the .con_id = "fuse".
Also are there any reasons to keep this in one single patch? Since
none of the fuse clocks are used yet, I think the clock changes
could be a separate patch that can go in through the clock tree.
And there isn't even a hard runtime dependency, since if the Tegra
changes were to go in without the clock changes, then the fallback
code in this patch should still turn the clock on properly. It just
might not be turned off again, but isn't that something we can live
with for a short period of time? I think perhaps that could even be
improved, see further below.
I've added Mike on Cc, he'll need to either take the patch in
through his tree or Ack this one, so he needs to see it
eventually.
4 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-tegra/fuse.c
b/arch/arm/mach-tegra/fuse.c index 9a4e910c3796..3b9191b930b5
100644 --- a/arch/arm/mach-tegra/fuse.c +++
b/arch/arm/mach-tegra/fuse.c @@ -22,6 +22,7 @@ #include
#include #include
+#include #include
#include "fuse.h" @@ -54,6 +55,7 @@ int tegra_cpu_speedo_id; /*
only exist in Tegra30 and later */ int tegra_soc_speedo_id; enum
tegra_revision tegra_revision;
+static struct clk *fuse_clk; static int tegra_fuse_spare_bit;
static void (*tegra_init_speedo_data)(void);
@@ -77,6 +79,22 @@ static const char
*tegra_revision_name[TEGRA_REVISION_MAX] = { [TEGRA_REVISION_A04]
= "A04", };
+static void tegra_fuse_enable_clk(void) +{ + if
(IS_ERR(fuse_clk)) +fuse_clk = clk_get_sys("fuse-tegra",
"fuse"); +if (IS_ERR(fuse_clk)) + return;
Perhaps instead of just returning here, this should actually be
where the code to enable the clock should go.
+ clk_prepare_enable(fuse_clk); +} + +static void
tegra_fuse_disable_clk(void) +{ + if (IS_ERR(fuse_clk)) +
return;
And this is where we could disable it again. That way we should
get equal functionality in both cases.
That would need a shared lock with the clock code; at some point, the
clock will be registered, and the clock subsystem in control of the
enable bit. I think having a very early tegra_init_fuse() come along
and force the clock on, and then having the rest of the fuse code use
the clock object as soon as it's available, is the safest approach.
Of course, I suppose there's still a window where the following might
happen:
cpu 0:
- tegra_fuse_enable_clk entered
- fails to clk_get
cpu 1
- tegra clk driver is registered
- clk subsystem initcall disables all
unused clocks
- access a fuse register
-> badness
It seems to me that both solutions require a shared lock with the clock
code in order to be theoretically safe. The situation you described
requires a lock to be addressed ; and unless I missed something,
anything that could break with Thierry's proposal could also only do so
if we "hold" the clock when the tegra clk driver is registered.
However we can consider ourselves safe for both cases if we know for
sure that there is no fuse function in use when this happens. Since
of_clk_init() is called very early during boot with SMP and preemption
disabled, isn't that always the case?
I'm not sure how to protect against that, unless we simply assume that
all the fuse driver functions are guaranteed to happen early and
before the clk subsystem's initcall, so that can't happen?
That's probably the current behavior actually - the clk subsystem's
initcall disables the fuse clock, fuse functions are never used after
that, and we only notice when we try to kexec into another kernel that
the fuse clock is not enabled as expected.
With that in mind, it's probably a good idea to preemptively implement
proper clock enabling
Re: [PATCH] ARM: tegra: properly use FUSE clock
On 11/18/2013 08:43 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Mon, Nov 18, 2013 at 07:40:47PM +0900, Alexandre Courbot wrote:
FUSE clock is enabled by most bootloaders, but we cannot expect it to be
on in all contexts (e.g. kexec).
This patch adds a FUSE clkdev to all Tegra platforms and makes sure
it is enabled before touching FUSE registers. tegra_init_fuse() is
invoked during very early boot and thus cannot rely on the clock
framework ; therefore the FUSE clock is forcibly enabled using a
register write in that function, and remains that way until the
clock framework can be used.
Signed-off-by: Alexandre Courbot
---
arch/arm/mach-tegra/fuse.c | 41 +++-
drivers/clk/tegra/clk-tegra114.c | 1 +
drivers/clk/tegra/clk-tegra124.c | 1 +
drivers/clk/tegra/clk-tegra20.c | 1 +
Isn't this missing the clock driver changes for Tegra30? Ah... Tegra30
already has this clock defined. I wonder why only Tegra30 has it. grep
says that fuse-tegra isn't used by any drivers, which also indicates
that perhaps we don't need the .dev_id in the first place. We should be
able to get by with just the .con_id = "fuse".
Will fix that.
Also are there any reasons to keep this in one single patch? Since none
of the fuse clocks are used yet, I think the clock changes could be a
separate patch that can go in through the clock tree. And there isn't
even a hard runtime dependency, since if the Tegra changes were to go in
without the clock changes, then the fallback code in this patch should
still turn the clock on properly. It just might not be turned off again,
but isn't that something we can live with for a short period of time? I
think perhaps that could even be improved, see further below.
I've added Mike on Cc, he'll need to either take the patch in through
his tree or Ack this one, so he needs to see it eventually.
I will split the change into two patches - at first I thought it would
not be worth the trouble, but I overlooked the fact this needed to go
through the clock source tree.
4 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-tegra/fuse.c b/arch/arm/mach-tegra/fuse.c
index 9a4e910c3796..3b9191b930b5 100644
--- a/arch/arm/mach-tegra/fuse.c
+++ b/arch/arm/mach-tegra/fuse.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
#include
#include "fuse.h"
@@ -54,6 +55,7 @@ int tegra_cpu_speedo_id; /* only exist in
Tegra30 and later */
int tegra_soc_speedo_id;
enum tegra_revision tegra_revision;
+static struct clk *fuse_clk;
static int tegra_fuse_spare_bit;
static void (*tegra_init_speedo_data)(void);
@@ -77,6 +79,22 @@ static const char *tegra_revision_name[TEGRA_REVISION_MAX] =
{
[TEGRA_REVISION_A04] = "A04",
};
+static void tegra_fuse_enable_clk(void)
+{
+ if (IS_ERR(fuse_clk))
+ fuse_clk = clk_get_sys("fuse-tegra", "fuse");
+ if (IS_ERR(fuse_clk))
+ return;
Perhaps instead of just returning here, this should actually be where
the code to enable the clock should go.
+ clk_prepare_enable(fuse_clk);
+}
+
+static void tegra_fuse_disable_clk(void)
+{
+ if (IS_ERR(fuse_clk))
+ return;
And this is where we could disable it again. That way we should get
equal functionality in both cases.
What Stephen said, basically - but let me address that in the other mail.
Thanks for the review!
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: move firmware_ops to drivers/firmware
On 11/18/2013 08:58 PM, Catalin Marinas wrote: On Mon, Nov 18, 2013 at 03:05:59AM +, Alex Courbot wrote: On 11/18/2013 12:59 AM, Catalin Marinas wrote: On 17 November 2013 08:49, Alexandre Courbot wrote: The ARM tree includes a firmware_ops interface that is designed to implement support for simple, TrustZone-based firmwares but could also cover other use-cases. It has been suggested that this interface might be useful to other architectures (e.g. arm64) and that it should be moved out of arch/arm. NAK. I'm for code sharing with arm via common locations but this API goes against the ARMv8 firmware standardisation efforts like PSCI, encouraging each platform to define there own non-standard interface. I have to say, I pretty much agree with your NAK. The reason for this patch is that the suggestion to move firmware_ops out of arch/arm is the last (I hope) thing that prevents my Trusted Foundation support series from being merged. Moving it into drivers shouldn't be a workaround. Nice try ;). Hehe. I thought that just sending a patch would settle the issue one way or the other and avoid a huge discussion. Woke up this morning to see how wrong I was. Now if we can all agree: * that ARMv8 will only use PSCI Or spin-table (which does not require secure calls). Otherwise, if secure firmware is present, SoCs should use PSCI (as the only firmware standard currently supported in the arm64 kernel). However, things evolve and we may have other needs in the future or PSCI may not be sufficient or we get newer PSCI revisions. This can be extended but my requirement is to decouple booting standard from SoC support (together with the aim of having no SoC-specific code under arch/arm64). I really don't see why SoCs can't agree on one (or very few) standard booting protocol (and legacy argument doesn't work since the ARMv8 firmware needs to be converted to AArch64 anyway). * that there is no use-case of this interface outside of arch/arm as of today (and none foreseen in the near future) The firmware_ops are only used under arch/arm so far, I don't see any drivers doing anything with it. Also, l2x0_init is ARMv7 only. On arm64, support for PSCI is handled via cpu_operations in the latest kernel. That's an arm64 abstraction and is extensible (but we want to keep tight control of this, hence no register_cpu_ops function). * that the firmware_ops interface is quite ARMv7-specific anyway, This was introduced to allow SoC code to enable hooks for SoC-specific firmware calls like cpu_idle, l2x0_init. By standardising the interface and decoupling it from SoC code on arm64, we don't need per-SoC firmware_ops. Of course, trusted foundations interface could be plugged into cpu_ops on arm64 but I will NAK it on the grounds of not using the PSCI API, nor the SMC calling convention (and it's easy to fix when porting to ARMv8). If a supported standard API is used, then there is no need for additional code in the kernel. BTW, is legacy code the reason for not converting the SMC # to PSCI? It's already supported on ARMv7, so you may not have much code left to merge in the kernel ;). The problem here is twofold: 1) we are just consumers of the TrustZone secure monitor who receive a binary and do not have any control over its calling conventions. I agree that it would be trivial to make it compatible with PSCI, but it's just not something we can make by ourselves (TF does not even follow the SMC calling convention). If this problem is to be addressed, it should be done by forcing the TrustZone secure monitors providers to follow PSCI. 2) devices have already shipped with this firmware. Are we going to just renounce supporting them, even though the necessary support is lightweight and fits within already existing interfaces? I certainly do hope that for ARMv8 things will be different and more standardized. But that's not something that can be guaranteed unless ARM strongly enforces it to firmware vendors. In case such a non-standard firmware gets used again, I *do* hope that using cpu_ops will be preferred over saying "this device cannot be supported in mainline, ever". The kernel already supports non-standard hardware, BIOS, ACPI through hacks that are *way* more horrible than that. This should certainly not be encouraged, but that's not a valid reason to forbid otherwise perfectly fine devices to run mainline IMHO. * that should a need to move it (for whatever reason) occur later, it will be easy to do (as this patch hopefully demonstrates). I agree, it's not hard to unify this but so far I haven't seen a good reason. Same here. arm64 has its own cpu_operations. Other archs have different needs and if we move this to a common place it will just become a messy placeholder for function pointers from which each arch will only use a subset. Not to mention that if we follow the logic completely, we should then implement PCSI on top of cpu_ops and cpu_
Re: [PATCH v2] Documentation: gpiolib: document new interface
On 11/18/2013 06:34 PM, Linus Walleij wrote: On Sat, Nov 16, 2013 at 1:34 PM, Alexandre Courbot wrote: The first version received zero feedback, hopefully this one will get more attention. :) Not much changes, just some more proofreading and the fixes and improvements that came from it. It looks ok as far as I am concerned. Sorry I was swamped with other stuff... Linus, I hope this can be merged during the -rc cycle of 3.13, since the gpiod_ interface is going to be introduced there. It would not make much sense for it to come without its documentation. You're right of course. I'll read through it and apply fixes on top (or squash into your patch.) Great, thanks! Formal stuff: Don't we need an 00-INDEX file? (Maybe Rob can tell whether this is desirable.) Good idea. gpio.txt somehow fulfills that role, but it might be better if we split it. Would you like me to submit a new revision? I don't like double spaces after periods. I will just go and remove all of them from the new documentation unless you convince me otherwise. Reference: https://en.wikipedia.org/wiki/Sentence_spacing Please do - actually I started doing it (they come from the old documentation, not the bits I have written myself) but have obviously missed at least a few of them. :) Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Documentation: gpiolib: document new interface
On 11/18/2013 06:34 PM, Linus Walleij wrote: On Sat, Nov 16, 2013 at 1:34 PM, Alexandre Courbot acour...@nvidia.com wrote: The first version received zero feedback, hopefully this one will get more attention. :) Not much changes, just some more proofreading and the fixes and improvements that came from it. It looks ok as far as I am concerned. Sorry I was swamped with other stuff... Linus, I hope this can be merged during the -rc cycle of 3.13, since the gpiod_ interface is going to be introduced there. It would not make much sense for it to come without its documentation. You're right of course. I'll read through it and apply fixes on top (or squash into your patch.) Great, thanks! Formal stuff: Don't we need an 00-INDEX file? (Maybe Rob can tell whether this is desirable.) Good idea. gpio.txt somehow fulfills that role, but it might be better if we split it. Would you like me to submit a new revision? I don't like double spaces after periods. I will just go and remove all of them from the new documentation unless you convince me otherwise. Reference: https://en.wikipedia.org/wiki/Sentence_spacing Please do - actually I started doing it (they come from the old documentation, not the bits I have written myself) but have obviously missed at least a few of them. :) Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: move firmware_ops to drivers/firmware
On 11/18/2013 08:58 PM, Catalin Marinas wrote: On Mon, Nov 18, 2013 at 03:05:59AM +, Alex Courbot wrote: On 11/18/2013 12:59 AM, Catalin Marinas wrote: On 17 November 2013 08:49, Alexandre Courbot acour...@nvidia.com wrote: The ARM tree includes a firmware_ops interface that is designed to implement support for simple, TrustZone-based firmwares but could also cover other use-cases. It has been suggested that this interface might be useful to other architectures (e.g. arm64) and that it should be moved out of arch/arm. NAK. I'm for code sharing with arm via common locations but this API goes against the ARMv8 firmware standardisation efforts like PSCI, encouraging each platform to define there own non-standard interface. I have to say, I pretty much agree with your NAK. The reason for this patch is that the suggestion to move firmware_ops out of arch/arm is the last (I hope) thing that prevents my Trusted Foundation support series from being merged. Moving it into drivers shouldn't be a workaround. Nice try ;). Hehe. I thought that just sending a patch would settle the issue one way or the other and avoid a huge discussion. Woke up this morning to see how wrong I was. Now if we can all agree: * that ARMv8 will only use PSCI Or spin-table (which does not require secure calls). Otherwise, if secure firmware is present, SoCs should use PSCI (as the only firmware standard currently supported in the arm64 kernel). However, things evolve and we may have other needs in the future or PSCI may not be sufficient or we get newer PSCI revisions. This can be extended but my requirement is to decouple booting standard from SoC support (together with the aim of having no SoC-specific code under arch/arm64). I really don't see why SoCs can't agree on one (or very few) standard booting protocol (and legacy argument doesn't work since the ARMv8 firmware needs to be converted to AArch64 anyway). * that there is no use-case of this interface outside of arch/arm as of today (and none foreseen in the near future) The firmware_ops are only used under arch/arm so far, I don't see any drivers doing anything with it. Also, l2x0_init is ARMv7 only. On arm64, support for PSCI is handled via cpu_operations in the latest kernel. That's an arm64 abstraction and is extensible (but we want to keep tight control of this, hence no register_cpu_ops function). * that the firmware_ops interface is quite ARMv7-specific anyway, This was introduced to allow SoC code to enable hooks for SoC-specific firmware calls like cpu_idle, l2x0_init. By standardising the interface and decoupling it from SoC code on arm64, we don't need per-SoC firmware_ops. Of course, trusted foundations interface could be plugged into cpu_ops on arm64 but I will NAK it on the grounds of not using the PSCI API, nor the SMC calling convention (and it's easy to fix when porting to ARMv8). If a supported standard API is used, then there is no need for additional code in the kernel. BTW, is legacy code the reason for not converting the SMC # to PSCI? It's already supported on ARMv7, so you may not have much code left to merge in the kernel ;). The problem here is twofold: 1) we are just consumers of the TrustZone secure monitor who receive a binary and do not have any control over its calling conventions. I agree that it would be trivial to make it compatible with PSCI, but it's just not something we can make by ourselves (TF does not even follow the SMC calling convention). If this problem is to be addressed, it should be done by forcing the TrustZone secure monitors providers to follow PSCI. 2) devices have already shipped with this firmware. Are we going to just renounce supporting them, even though the necessary support is lightweight and fits within already existing interfaces? I certainly do hope that for ARMv8 things will be different and more standardized. But that's not something that can be guaranteed unless ARM strongly enforces it to firmware vendors. In case such a non-standard firmware gets used again, I *do* hope that using cpu_ops will be preferred over saying this device cannot be supported in mainline, ever. The kernel already supports non-standard hardware, BIOS, ACPI through hacks that are *way* more horrible than that. This should certainly not be encouraged, but that's not a valid reason to forbid otherwise perfectly fine devices to run mainline IMHO. * that should a need to move it (for whatever reason) occur later, it will be easy to do (as this patch hopefully demonstrates). I agree, it's not hard to unify this but so far I haven't seen a good reason. Same here. arm64 has its own cpu_operations. Other archs have different needs and if we move this to a common place it will just become a messy placeholder for function pointers from which each arch will only use a subset. Not to mention that if we follow the logic completely, we should then implement PCSI on top of cpu_ops
Re: [PATCH] ARM: tegra: properly use FUSE clock
On 11/18/2013 08:43 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Mon, Nov 18, 2013 at 07:40:47PM +0900, Alexandre Courbot wrote:
FUSE clock is enabled by most bootloaders, but we cannot expect it to be
on in all contexts (e.g. kexec).
This patch adds a FUSE clkdev to all Tegra platforms and makes sure
it is enabled before touching FUSE registers. tegra_init_fuse() is
invoked during very early boot and thus cannot rely on the clock
framework ; therefore the FUSE clock is forcibly enabled using a
register write in that function, and remains that way until the
clock framework can be used.
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
arch/arm/mach-tegra/fuse.c | 41 +++-
drivers/clk/tegra/clk-tegra114.c | 1 +
drivers/clk/tegra/clk-tegra124.c | 1 +
drivers/clk/tegra/clk-tegra20.c | 1 +
Isn't this missing the clock driver changes for Tegra30? Ah... Tegra30
already has this clock defined. I wonder why only Tegra30 has it. grep
says that fuse-tegra isn't used by any drivers, which also indicates
that perhaps we don't need the .dev_id in the first place. We should be
able to get by with just the .con_id = fuse.
Will fix that.
Also are there any reasons to keep this in one single patch? Since none
of the fuse clocks are used yet, I think the clock changes could be a
separate patch that can go in through the clock tree. And there isn't
even a hard runtime dependency, since if the Tegra changes were to go in
without the clock changes, then the fallback code in this patch should
still turn the clock on properly. It just might not be turned off again,
but isn't that something we can live with for a short period of time? I
think perhaps that could even be improved, see further below.
I've added Mike on Cc, he'll need to either take the patch in through
his tree or Ack this one, so he needs to see it eventually.
I will split the change into two patches - at first I thought it would
not be worth the trouble, but I overlooked the fact this needed to go
through the clock source tree.
4 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-tegra/fuse.c b/arch/arm/mach-tegra/fuse.c
index 9a4e910c3796..3b9191b930b5 100644
--- a/arch/arm/mach-tegra/fuse.c
+++ b/arch/arm/mach-tegra/fuse.c
@@ -22,6 +22,7 @@
#include linux/io.h
#include linux/export.h
#include linux/random.h
+#include linux/clk.h
#include linux/tegra-soc.h
#include fuse.h
@@ -54,6 +55,7 @@ int tegra_cpu_speedo_id; /* only exist in
Tegra30 and later */
int tegra_soc_speedo_id;
enum tegra_revision tegra_revision;
+static struct clk *fuse_clk;
static int tegra_fuse_spare_bit;
static void (*tegra_init_speedo_data)(void);
@@ -77,6 +79,22 @@ static const char *tegra_revision_name[TEGRA_REVISION_MAX] =
{
[TEGRA_REVISION_A04] = A04,
};
+static void tegra_fuse_enable_clk(void)
+{
+ if (IS_ERR(fuse_clk))
+ fuse_clk = clk_get_sys(fuse-tegra, fuse);
+ if (IS_ERR(fuse_clk))
+ return;
Perhaps instead of just returning here, this should actually be where
the code to enable the clock should go.
+ clk_prepare_enable(fuse_clk);
+}
+
+static void tegra_fuse_disable_clk(void)
+{
+ if (IS_ERR(fuse_clk))
+ return;
And this is where we could disable it again. That way we should get
equal functionality in both cases.
What Stephen said, basically - but let me address that in the other mail.
Thanks for the review!
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: properly use FUSE clock
On 11/19/2013 08:48 AM, Stephen Warren wrote:
On 11/18/2013 04:43 AM, Thierry Reding wrote:
On Mon, Nov 18, 2013 at 07:40:47PM +0900, Alexandre Courbot wrote:
FUSE clock is enabled by most bootloaders, but we cannot expect
it to be on in all contexts (e.g. kexec).
This patch adds a FUSE clkdev to all Tegra platforms and makes
sure it is enabled before touching FUSE registers.
tegra_init_fuse() is invoked during very early boot and thus
cannot rely on the clock framework ; therefore the FUSE clock is
forcibly enabled using a register write in that function, and
remains that way until the clock framework can be used.
Signed-off-by: Alexandre Courbot acour...@nvidia.com ---
arch/arm/mach-tegra/fuse.c | 41
+++-
drivers/clk/tegra/clk-tegra114.c | 1 +
drivers/clk/tegra/clk-tegra124.c | 1 +
drivers/clk/tegra/clk-tegra20.c | 1 +
Isn't this missing the clock driver changes for Tegra30? Ah...
Tegra30 already has this clock defined. I wonder why only Tegra30
has it. grep says that fuse-tegra isn't used by any drivers, which
also indicates that perhaps we don't need the .dev_id in the first
place. We should be able to get by with just the .con_id = fuse.
Also are there any reasons to keep this in one single patch? Since
none of the fuse clocks are used yet, I think the clock changes
could be a separate patch that can go in through the clock tree.
And there isn't even a hard runtime dependency, since if the Tegra
changes were to go in without the clock changes, then the fallback
code in this patch should still turn the clock on properly. It just
might not be turned off again, but isn't that something we can live
with for a short period of time? I think perhaps that could even be
improved, see further below.
I've added Mike on Cc, he'll need to either take the patch in
through his tree or Ack this one, so he needs to see it
eventually.
4 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-tegra/fuse.c
b/arch/arm/mach-tegra/fuse.c index 9a4e910c3796..3b9191b930b5
100644 --- a/arch/arm/mach-tegra/fuse.c +++
b/arch/arm/mach-tegra/fuse.c @@ -22,6 +22,7 @@ #include
linux/io.h #include linux/export.h #include linux/random.h
+#include linux/clk.h #include linux/tegra-soc.h
#include fuse.h @@ -54,6 +55,7 @@ int tegra_cpu_speedo_id; /*
only exist in Tegra30 and later */ int tegra_soc_speedo_id; enum
tegra_revision tegra_revision;
+static struct clk *fuse_clk; static int tegra_fuse_spare_bit;
static void (*tegra_init_speedo_data)(void);
@@ -77,6 +79,22 @@ static const char
*tegra_revision_name[TEGRA_REVISION_MAX] = { [TEGRA_REVISION_A04]
= A04, };
+static void tegra_fuse_enable_clk(void) +{ + if
(IS_ERR(fuse_clk)) +fuse_clk = clk_get_sys(fuse-tegra,
fuse); +if (IS_ERR(fuse_clk)) + return;
Perhaps instead of just returning here, this should actually be
where the code to enable the clock should go.
+ clk_prepare_enable(fuse_clk); +} + +static void
tegra_fuse_disable_clk(void) +{ + if (IS_ERR(fuse_clk)) +
return;
And this is where we could disable it again. That way we should
get equal functionality in both cases.
That would need a shared lock with the clock code; at some point, the
clock will be registered, and the clock subsystem in control of the
enable bit. I think having a very early tegra_init_fuse() come along
and force the clock on, and then having the rest of the fuse code use
the clock object as soon as it's available, is the safest approach.
Of course, I suppose there's still a window where the following might
happen:
cpu 0:
- tegra_fuse_enable_clk entered
- fails to clk_get
cpu 1
- tegra clk driver is registered
- clk subsystem initcall disables all
unused clocks
- access a fuse register
- badness
It seems to me that both solutions require a shared lock with the clock
code in order to be theoretically safe. The situation you described
requires a lock to be addressed ; and unless I missed something,
anything that could break with Thierry's proposal could also only do so
if we hold the clock when the tegra clk driver is registered.
However we can consider ourselves safe for both cases if we know for
sure that there is no fuse function in use when this happens. Since
of_clk_init() is called very early during boot with SMP and preemption
disabled, isn't that always the case?
I'm not sure how to protect against that, unless we simply assume that
all the fuse driver functions are guaranteed to happen early and
before the clk subsystem's initcall, so that can't happen?
That's probably the current behavior actually - the clk subsystem's
initcall disables the fuse clock, fuse functions are never used after
that, and we only notice when we try to kexec into another kernel that
the fuse clock is not enabled as expected.
With that in mind,
Re: [PATCH] ARM: move firmware_ops to drivers/firmware
On 11/18/2013 12:59 AM, Catalin Marinas wrote:
On 17 November 2013 08:49, Alexandre Courbot wrote:
The ARM tree includes a firmware_ops interface that is designed to
implement support for simple, TrustZone-based firmwares but could
also cover other use-cases. It has been suggested that this
interface might be useful to other architectures (e.g. arm64) and
that it should be moved out of arch/arm.
NAK. I'm for code sharing with arm via common locations but this API
goes against the ARMv8 firmware standardisation efforts like PSCI,
encouraging each platform to define there own non-standard interface.
I have to say, I pretty much agree with your NAK.
The reason for this patch is that the suggestion to move firmware_ops
out of arch/arm is the last (I hope) thing that prevents my Trusted
Foundation support series from being merged. Now if we can all agree:
* that ARMv8 will only use PSCI
* that there is no use-case of this interface outside of arch/arm as of
today (and none foreseen in the near future)
* that the firmware_ops interface is quite ARMv7-specific anyway,
* that should a need to move it (for whatever reason) occur later, it
will be easy to do (as this patch hopefully demonstrates).
... then this has indeed no reason to be. And maybe I can finally get
Russell's blessing on my series.
--- /dev/null
+++ b/include/linux/platform_firmware.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2012 Samsung Electronics.
+ * Kyungmin Park
+ * Tomasz Figa
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _PLATFORM_FIRMWARE_H
+#define _PLATFORM_FIRMWARE_H
+
+#include
+
+/*
+ * struct platform_firmware_ops
+ *
+ * A structure to specify available firmware operations.
+ *
+ * A filled up structure can be registered with
+ * register_platform_firmware_ops().
+ */
+struct platform_firmware_ops {
+ /*
+* Enters CPU idle mode
+*/
+ int (*do_idle)(void);
Covered by PSCI already.
+ /*
+* Sets boot address of specified physical CPU
+*/
+ int (*set_cpu_boot_addr)(int cpu, unsigned long boot_addr);
Covered either by PSCI or spin-table release method (PSCI if firmware
call is required).
+ /*
+* Boots specified physical CPU
+*/
+ int (*cpu_boot)(int cpu);
PSCI.
+ /*
+* Initializes L2 cache
+*/
+ int (*l2x0_init)(void);
No L2x0 (L210, L220, PL310) cache on ARMv8. And here I strongly
recommend the hardware people to make proper external caches which can
be flushed by standard CPU instructions, not MMIO. Any such caches
must be enabled by firmware before Linux starts.
The above firmware API is 32-bit ARM only. Form 64-bit ARM, you have
the choice of PSCI so far but as I said in a long thread to Nico, I'm
open to other standard interfaces if there are good reasons PSCI
cannot be used. Note the _standard_ part, I don't want every SoC with
their own firmware API for standard things like secondary CPU
booting/hotplug/idle.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: move firmware_ops to drivers/firmware
On 11/18/2013 12:59 AM, Catalin Marinas wrote:
On 17 November 2013 08:49, Alexandre Courbot acour...@nvidia.com wrote:
The ARM tree includes a firmware_ops interface that is designed to
implement support for simple, TrustZone-based firmwares but could
also cover other use-cases. It has been suggested that this
interface might be useful to other architectures (e.g. arm64) and
that it should be moved out of arch/arm.
NAK. I'm for code sharing with arm via common locations but this API
goes against the ARMv8 firmware standardisation efforts like PSCI,
encouraging each platform to define there own non-standard interface.
I have to say, I pretty much agree with your NAK.
The reason for this patch is that the suggestion to move firmware_ops
out of arch/arm is the last (I hope) thing that prevents my Trusted
Foundation support series from being merged. Now if we can all agree:
* that ARMv8 will only use PSCI
* that there is no use-case of this interface outside of arch/arm as of
today (and none foreseen in the near future)
* that the firmware_ops interface is quite ARMv7-specific anyway,
* that should a need to move it (for whatever reason) occur later, it
will be easy to do (as this patch hopefully demonstrates).
... then this has indeed no reason to be. And maybe I can finally get
Russell's blessing on my series.
--- /dev/null
+++ b/include/linux/platform_firmware.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2012 Samsung Electronics.
+ * Kyungmin Park kyungmin.p...@samsung.com
+ * Tomasz Figa t.f...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _PLATFORM_FIRMWARE_H
+#define _PLATFORM_FIRMWARE_H
+
+#include linux/bug.h
+
+/*
+ * struct platform_firmware_ops
+ *
+ * A structure to specify available firmware operations.
+ *
+ * A filled up structure can be registered with
+ * register_platform_firmware_ops().
+ */
+struct platform_firmware_ops {
+ /*
+* Enters CPU idle mode
+*/
+ int (*do_idle)(void);
Covered by PSCI already.
+ /*
+* Sets boot address of specified physical CPU
+*/
+ int (*set_cpu_boot_addr)(int cpu, unsigned long boot_addr);
Covered either by PSCI or spin-table release method (PSCI if firmware
call is required).
+ /*
+* Boots specified physical CPU
+*/
+ int (*cpu_boot)(int cpu);
PSCI.
+ /*
+* Initializes L2 cache
+*/
+ int (*l2x0_init)(void);
No L2x0 (L210, L220, PL310) cache on ARMv8. And here I strongly
recommend the hardware people to make proper external caches which can
be flushed by standard CPU instructions, not MMIO. Any such caches
must be enabled by firmware before Linux starts.
The above firmware API is 32-bit ARM only. Form 64-bit ARM, you have
the choice of PSCI so far but as I said in a long thread to Nico, I'm
open to other standard interfaces if there are good reasons PSCI
cannot be used. Note the _standard_ part, I don't want every SoC with
their own firmware API for standard things like secondary CPU
booting/hotplug/idle.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 0/7] ARM: support for Trusted Foundations secure monitor
On 11/14/2013 02:57 AM, Olof Johansson wrote: On Tue, Nov 12, 2013 at 6:14 PM, Alex Courbot wrote: On 11/13/2013 05:38 AM, Olof Johansson wrote: On Tue, Nov 12, 2013 at 12:26 PM, Stephen Warren wrote: On 11/07/2013 03:11 AM, Alexandre Courbot wrote: Just a set of small fixes to address the concerns expressed on v9 with the non-prefixed version DT properties. I hope there won't be a need for an eleventh (!) version. :P BTW, this version looks fine to me. On IRC, Olof said it looked OK to him. I'm just waiting to hear back from Olof/Russell whether I should merge this through the Tegra tree, or whether the first 1-3 patches should go through Russell's tree. I pinged Russell, and he brought up the fact that there were earlier requests to move it to drivers/firmware. It would make sense to try to get that done before merging, especially if you anticipate someone using TF on 64-bit platforms. IIRC when we discussed this point your last comment was as follows: Touche. :) Thanks for the reminder. On Tue, Oct 29, 2013 at 6:55 AM, Olof Johansson wrote: I think we can probably merge this under arch/arm now, and when we figure out what needs to be common with ARM64 we can move it out to a good location. It might be that mostly just a header file with ABI conventions needs to be shared, not actual implementation, for example. So I thought we agreed on that. If in the end we prefer to move the ARM firmware interface into drivers/firmware, I'm fine with that too (Tomasz also confirmed he would be ok with it) but I wonder if that would not be somehow premature. Another worry of mine is that this might delay this patchset some more. Support for TF is one of the last remaining step towards making NVIDIA branded Tegra retail devices (SHIELD and TegraNote at the moment) run upstream directly. I missed 3.13, I'd like to make sure I won't miss 3.14. Would it be acceptable if we move the ARM firmware interface to a common place after this patchset is merged? Well, as I already said I'm ok with things going into arch/arm to start with, as long as Russell is. Once we see 64-bit needs for the same we'll move it out -- it's not like it's a whole lot of code to start with. But Russell has veto on the topic. :-) Thanks Olof. Russell, are you ok with the patchset in its current form? I can start moving the firmware interface out of arch/arm if that's what you want (there is no user outside of ARM at the moment, but as Olof pointed out that's not too much code) but I'd really like to see this series secured for 3.14. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 0/7] ARM: support for Trusted Foundations secure monitor
On 11/14/2013 02:57 AM, Olof Johansson wrote: On Tue, Nov 12, 2013 at 6:14 PM, Alex Courbot acour...@nvidia.com wrote: On 11/13/2013 05:38 AM, Olof Johansson wrote: On Tue, Nov 12, 2013 at 12:26 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/07/2013 03:11 AM, Alexandre Courbot wrote: Just a set of small fixes to address the concerns expressed on v9 with the non-prefixed version DT properties. I hope there won't be a need for an eleventh (!) version. :P BTW, this version looks fine to me. On IRC, Olof said it looked OK to him. I'm just waiting to hear back from Olof/Russell whether I should merge this through the Tegra tree, or whether the first 1-3 patches should go through Russell's tree. I pinged Russell, and he brought up the fact that there were earlier requests to move it to drivers/firmware. It would make sense to try to get that done before merging, especially if you anticipate someone using TF on 64-bit platforms. IIRC when we discussed this point your last comment was as follows: Touche. :) Thanks for the reminder. On Tue, Oct 29, 2013 at 6:55 AM, Olof Johansson o...@lixom.net wrote: I think we can probably merge this under arch/arm now, and when we figure out what needs to be common with ARM64 we can move it out to a good location. It might be that mostly just a header file with ABI conventions needs to be shared, not actual implementation, for example. So I thought we agreed on that. If in the end we prefer to move the ARM firmware interface into drivers/firmware, I'm fine with that too (Tomasz also confirmed he would be ok with it) but I wonder if that would not be somehow premature. Another worry of mine is that this might delay this patchset some more. Support for TF is one of the last remaining step towards making NVIDIA branded Tegra retail devices (SHIELD and TegraNote at the moment) run upstream directly. I missed 3.13, I'd like to make sure I won't miss 3.14. Would it be acceptable if we move the ARM firmware interface to a common place after this patchset is merged? Well, as I already said I'm ok with things going into arch/arm to start with, as long as Russell is. Once we see 64-bit needs for the same we'll move it out -- it's not like it's a whole lot of code to start with. But Russell has veto on the topic. :-) Thanks Olof. Russell, are you ok with the patchset in its current form? I can start moving the firmware interface out of arch/arm if that's what you want (there is no user outside of ARM at the moment, but as Olof pointed out that's not too much code) but I'd really like to see this series secured for 3.14. Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 0/7] ARM: support for Trusted Foundations secure monitor
On 11/13/2013 05:38 AM, Olof Johansson wrote: On Tue, Nov 12, 2013 at 12:26 PM, Stephen Warren wrote: On 11/07/2013 03:11 AM, Alexandre Courbot wrote: Just a set of small fixes to address the concerns expressed on v9 with the non-prefixed version DT properties. I hope there won't be a need for an eleventh (!) version. :P BTW, this version looks fine to me. On IRC, Olof said it looked OK to him. I'm just waiting to hear back from Olof/Russell whether I should merge this through the Tegra tree, or whether the first 1-3 patches should go through Russell's tree. I pinged Russell, and he brought up the fact that there were earlier requests to move it to drivers/firmware. It would make sense to try to get that done before merging, especially if you anticipate someone using TF on 64-bit platforms. IIRC when we discussed this point your last comment was as follows: On Tue, Oct 29, 2013 at 6:55 AM, Olof Johansson wrote: > I think we can probably merge this under arch/arm now, and when we > figure out what needs to be common with ARM64 we can move it out to a > good location. It might be that mostly just a header file with ABI > conventions needs to be shared, not actual implementation, for > example. So I thought we agreed on that. If in the end we prefer to move the ARM firmware interface into drivers/firmware, I'm fine with that too (Tomasz also confirmed he would be ok with it) but I wonder if that would not be somehow premature. Another worry of mine is that this might delay this patchset some more. Support for TF is one of the last remaining step towards making NVIDIA branded Tegra retail devices (SHIELD and TegraNote at the moment) run upstream directly. I missed 3.13, I'd like to make sure I won't miss 3.14. Would it be acceptable if we move the ARM firmware interface to a common place after this patchset is merged? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 4/7] ARM: tegra: add support for Trusted Foundations
On 11/13/2013 05:23 AM, Stephen Warren wrote:
On 11/07/2013 03:11 AM, Alexandre Courbot wrote:
Register the firmware operations for Trusted Foundations if the device
tree indicates it is active on the device.
diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
void __init tegra_init_early(void)
{
+ of_register_trusted_foundations();
tegra_cpu_reset_handler_init();
tegra_apb_io_init();
tegra_init_fuse();
Your other bugfix patch for 3.13 moved tegra_cpu_reset_handler_init().
Should the call to of_register_trusted_foundations() move with it when
this is applied, or should it just stay right at the start of
tegra_init_early()? Either way is fine; just let me know which way to
fix up the conflict when this gets applied.
I rebased on -next and left it at the start of tegra_init_early(). Even
though at the moment the only requirement is that the call is made
before tegra_cpu_reset_handler_init(), it seems to make sense to get rid
of firmware-related initialization as early as possible.
Btw, this patchset is still based on the 3.12 code, before common.c got
renamed to tegra.c, so you will have a problem here as well (sorry about
that). The conflict is easy to resolve, but if you want me to send you a
properly rebased version, just let me know.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 4/7] ARM: tegra: add support for Trusted Foundations
On 11/13/2013 05:23 AM, Stephen Warren wrote:
On 11/07/2013 03:11 AM, Alexandre Courbot wrote:
Register the firmware operations for Trusted Foundations if the device
tree indicates it is active on the device.
diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
void __init tegra_init_early(void)
{
+ of_register_trusted_foundations();
tegra_cpu_reset_handler_init();
tegra_apb_io_init();
tegra_init_fuse();
Your other bugfix patch for 3.13 moved tegra_cpu_reset_handler_init().
Should the call to of_register_trusted_foundations() move with it when
this is applied, or should it just stay right at the start of
tegra_init_early()? Either way is fine; just let me know which way to
fix up the conflict when this gets applied.
I rebased on -next and left it at the start of tegra_init_early(). Even
though at the moment the only requirement is that the call is made
before tegra_cpu_reset_handler_init(), it seems to make sense to get rid
of firmware-related initialization as early as possible.
Btw, this patchset is still based on the 3.12 code, before common.c got
renamed to tegra.c, so you will have a problem here as well (sorry about
that). The conflict is easy to resolve, but if you want me to send you a
properly rebased version, just let me know.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v10 0/7] ARM: support for Trusted Foundations secure monitor
On 11/13/2013 05:38 AM, Olof Johansson wrote: On Tue, Nov 12, 2013 at 12:26 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 11/07/2013 03:11 AM, Alexandre Courbot wrote: Just a set of small fixes to address the concerns expressed on v9 with the non-prefixed version DT properties. I hope there won't be a need for an eleventh (!) version. :P BTW, this version looks fine to me. On IRC, Olof said it looked OK to him. I'm just waiting to hear back from Olof/Russell whether I should merge this through the Tegra tree, or whether the first 1-3 patches should go through Russell's tree. I pinged Russell, and he brought up the fact that there were earlier requests to move it to drivers/firmware. It would make sense to try to get that done before merging, especially if you anticipate someone using TF on 64-bit platforms. IIRC when we discussed this point your last comment was as follows: On Tue, Oct 29, 2013 at 6:55 AM, Olof Johansson o...@lixom.net wrote: I think we can probably merge this under arch/arm now, and when we figure out what needs to be common with ARM64 we can move it out to a good location. It might be that mostly just a header file with ABI conventions needs to be shared, not actual implementation, for example. So I thought we agreed on that. If in the end we prefer to move the ARM firmware interface into drivers/firmware, I'm fine with that too (Tomasz also confirmed he would be ok with it) but I wonder if that would not be somehow premature. Another worry of mine is that this might delay this patchset some more. Support for TF is one of the last remaining step towards making NVIDIA branded Tegra retail devices (SHIELD and TegraNote at the moment) run upstream directly. I missed 3.13, I'd like to make sure I won't miss 3.14. Would it be acceptable if we move the ARM firmware interface to a common place after this patchset is merged? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Any news on Runtime Interpreted Power Sequences
On 10/31/2013 01:59 PM, NeilBrown wrote: * PGP Signed by an unknown key On Tue, 29 Oct 2013 09:18:16 -0700 Mark Brown wrote: On Tue, Oct 29, 2013 at 11:10:37AM +1100, NeilBrown wrote: Yes, the device is soldered down and has a reset line that needs to be pulsed low at about the same time that the MMC port enables the regulator. How do you propose that I describe this? Which driver should know about the reset GPIO, how to I tell it about the GPIO, and which function should do the pulsing? I'd expect the driver for the device to know about this, obviously depending on what this actually does it might want to use this at runtime (for example, putting the device into reset to minimise power while it's idle). We really need a generic way for devices such as this on enumerable buses to run before the current probe() in order to allow them to manage their power up sequences in embedded systems, this is *far* from a unique situation. I agree. To me, this sounds a lot like saying "We need a way for enumerable buses to be given a power-on-sequence to power on the attached device". That is what I hopped RIPS would provide. There are a few ad-hoc solutions that provide such a mechanism using platform data. Take for instance include/linux/platform_data/brcmfmac-sdio.h. It allows you to register a platform device which sole purpose is to control the power sequence of a SDIO network device. When the platform device is registered, it powers the network device which can then be probed by the bus. The network driver also calls the platform power on/off functions when appropriate. This works quite well in the case of board files where you can write power sequencing code freely, but the question is how to translate it to device tree. You need to translate several, board-specific (and not device-specific) functions. Here I have to admit this seems like a good fit for in-DT power sequences. Maybe various devices could allow other devices to register for call-backs when the first device activates or deactivates a port (whether an MMC port or USB or Serial or whatever). Then a driver that needs to control the power-on sequence would register as a platform-device which registers a call-back with the appropriate parent and performs the required power-on/off. Does that sound like the right sort of thing? I think it does, but you are still left with the problem of how and where to define that board-specific power sequence. If things were always as simple as turning a regulator on, this would be easy, but apparently we also face more complex cases. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Any news on Runtime Interpreted Power Sequences
On 10/31/2013 01:59 PM, NeilBrown wrote: * PGP Signed by an unknown key On Tue, 29 Oct 2013 09:18:16 -0700 Mark Brown broo...@kernel.org wrote: On Tue, Oct 29, 2013 at 11:10:37AM +1100, NeilBrown wrote: Yes, the device is soldered down and has a reset line that needs to be pulsed low at about the same time that the MMC port enables the regulator. How do you propose that I describe this? Which driver should know about the reset GPIO, how to I tell it about the GPIO, and which function should do the pulsing? I'd expect the driver for the device to know about this, obviously depending on what this actually does it might want to use this at runtime (for example, putting the device into reset to minimise power while it's idle). We really need a generic way for devices such as this on enumerable buses to run before the current probe() in order to allow them to manage their power up sequences in embedded systems, this is *far* from a unique situation. I agree. To me, this sounds a lot like saying We need a way for enumerable buses to be given a power-on-sequence to power on the attached device. That is what I hopped RIPS would provide. There are a few ad-hoc solutions that provide such a mechanism using platform data. Take for instance include/linux/platform_data/brcmfmac-sdio.h. It allows you to register a platform device which sole purpose is to control the power sequence of a SDIO network device. When the platform device is registered, it powers the network device which can then be probed by the bus. The network driver also calls the platform power on/off functions when appropriate. This works quite well in the case of board files where you can write power sequencing code freely, but the question is how to translate it to device tree. You need to translate several, board-specific (and not device-specific) functions. Here I have to admit this seems like a good fit for in-DT power sequences. Maybe various devices could allow other devices to register for call-backs when the first device activates or deactivates a port (whether an MMC port or USB or Serial or whatever). Then a driver that needs to control the power-on sequence would register as a platform-device which registers a call-back with the appropriate parent and performs the required power-on/off. Does that sound like the right sort of thing? I think it does, but you are still left with the problem of how and where to define that board-specific power sequence. If things were always as simple as turning a regulator on, this would be easy, but apparently we also face more complex cases. Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 1/5] ARM: add basic support for Trusted Foundations
On 10/29/2013 05:12 PM, Kumar Gala wrote:
On Oct 28, 2013, at 7:25 PM, Mark Rutland wrote:
On Mon, Oct 28, 2013 at 11:31:36PM +, Tomasz Figa wrote:
On Monday 28 of October 2013 14:56:49 Olof Johansson wrote:
On Mon, Oct 28, 2013 at 05:57:04AM -0500, Kumar Gala wrote:
On Oct 28, 2013, at 5:28 AM, Alexandre Courbot wrote:
Trusted Foundations is a TrustZone-based secure monitor for ARM that
can be invoked using the same SMC-based API on all supported
platforms. This patch adds initial basic support for Trusted
Foundations using the ARM firmware API. Current features are limited
to the ability to boot secondary processors.
Note: The API followed by Trusted Foundations does *not* follow the
SMC
calling conventions. It has nothing to do with PSCI neither and is
only
relevant to devices that use Trusted Foundations (like most
Tegra-based
retail devices).
Signed-off-by: Alexandre Courbot
Reviewed-by: Tomasz Figa
Reviewed-by: Stephen Warren
---
.../arm/firmware/tl,trusted-foundations.txt| 20 ++
.../devicetree/bindings/vendor-prefixes.txt| 1 +
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/firmware/Kconfig | 28
arch/arm/firmware/Makefile | 1 +
arch/arm/firmware/trusted_foundations.c| 79
++ arch/arm/include/asm/trusted_foundations.h
| 67 ++ 8 files changed, 199 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundatio
ns.txt create mode 100644 arch/arm/firmware/Kconfig
create mode 100644 arch/arm/firmware/Makefile
create mode 100644 arch/arm/firmware/trusted_foundations.c
create mode 100644 arch/arm/include/asm/trusted_foundations.h
diff --git
a/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundat
ions.txt
b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundat
ions.txt new file mode 100644
index 000..2ec75c9
--- /dev/null
+++
b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundat
ions.txt @@ -0,0 +1,20 @@
+Trusted Foundations
+---
+
+Boards that use the Trusted Foundations secure monitor can signal
its
+presence by declaring a node compatible with
"tl,trusted-foundations"
+under the /firmware/ node
+
+Required properties:
+- compatible : "tl,trusted-foundations"
+- version-major : major version number of Trusted Foundations
firmware
+- version-minor: minor version number of Trusted Foundations
firmware
vendor prefix version.
Are you saying he should use tl,version-major tl,version-minor? For
bindings that are already vendor-specific we haven't (on ARM) asked for
vendor prefix on properties. It doesn't mean that we should keep going
down that route though, so I'm just asking for clarification for my own
edification. :)
This is a good question. We should decide what the right thing (TM) is and
write it down. I, on the contrary, was convinced that it's the way Kumar
says.
The impression I got was that properties should be prefixed when they're
extremely vendor-specific and could clash with a more generic property. I'm not
sure that firmware will ever have a generic binding given the variation even in
the set of implemented functionality.
I would imagine that there are many ways different firmwares might be
versioned, and I can't see version-major or version-minor clashing with a
generic property we might add later. However prefixing would not be harmful, so
I'm not opposed to it if others want that.
Another option would be to support a fallback compatible list (e.g.
"tl,trusted-foundations-${MAJOR}-${MINOR}", "tl,trusted-foundations"), and get
versioning information from there. Given that could be painful to handle I
don't want to force it if not required.
Thanks,
Mark.
I'm of the opinion that making all vendor specific properties vendor prefixed
is the easiest rule of thumb and leaves no gray area to have to argue about.
All good points, I will vendor-prefix these properties.
Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: build failure after merge of the final tree (gpio tree related)
On 10/29/2013 10:25 PM, Linus Walleij wrote: On Tue, Oct 29, 2013 at 10:10 AM, Stephen Rothwell wrote: I have applied the following patch for today (it should go into the gpio tree if it is considered correct): From: Stephen Rothwell Date: Tue, 29 Oct 2013 20:05:12 +1100 Subject: [PATCH] gpiolib: include gpio/consumer.h in of_gpio.h for desc_to_gpio() Fixes this build error on sparc: In file included from drivers/spi/spi.c:33:0: include/linux/of_gpio.h: In function 'of_get_named_gpio_flags': include/linux/of_gpio.h:93:3: error: implicit declaration of function 'desc_to_gpio' [-Werror=implicit-function-declaration] Signed-off-by: Stephen Rothwell Patch applied, unless Alexandre has any considerations. Nope, that include should definitely be there, actually I was convinced it was already... Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: linux-next: build failure after merge of the final tree (gpio tree related)
On 10/29/2013 10:25 PM, Linus Walleij wrote: On Tue, Oct 29, 2013 at 10:10 AM, Stephen Rothwell s...@canb.auug.org.au wrote: I have applied the following patch for today (it should go into the gpio tree if it is considered correct): From: Stephen Rothwell s...@canb.auug.org.au Date: Tue, 29 Oct 2013 20:05:12 +1100 Subject: [PATCH] gpiolib: include gpio/consumer.h in of_gpio.h for desc_to_gpio() Fixes this build error on sparc: In file included from drivers/spi/spi.c:33:0: include/linux/of_gpio.h: In function 'of_get_named_gpio_flags': include/linux/of_gpio.h:93:3: error: implicit declaration of function 'desc_to_gpio' [-Werror=implicit-function-declaration] Signed-off-by: Stephen Rothwell s...@canb.auug.org.au Patch applied, unless Alexandre has any considerations. Nope, that include should definitely be there, actually I was convinced it was already... Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 1/5] ARM: add basic support for Trusted Foundations
On 10/29/2013 05:12 PM, Kumar Gala wrote:
On Oct 28, 2013, at 7:25 PM, Mark Rutland wrote:
On Mon, Oct 28, 2013 at 11:31:36PM +, Tomasz Figa wrote:
On Monday 28 of October 2013 14:56:49 Olof Johansson wrote:
On Mon, Oct 28, 2013 at 05:57:04AM -0500, Kumar Gala wrote:
On Oct 28, 2013, at 5:28 AM, Alexandre Courbot wrote:
Trusted Foundations is a TrustZone-based secure monitor for ARM that
can be invoked using the same SMC-based API on all supported
platforms. This patch adds initial basic support for Trusted
Foundations using the ARM firmware API. Current features are limited
to the ability to boot secondary processors.
Note: The API followed by Trusted Foundations does *not* follow the
SMC
calling conventions. It has nothing to do with PSCI neither and is
only
relevant to devices that use Trusted Foundations (like most
Tegra-based
retail devices).
Signed-off-by: Alexandre Courbot acour...@nvidia.com
Reviewed-by: Tomasz Figa t.f...@samsung.com
Reviewed-by: Stephen Warren swar...@nvidia.com
---
.../arm/firmware/tl,trusted-foundations.txt| 20 ++
.../devicetree/bindings/vendor-prefixes.txt| 1 +
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/firmware/Kconfig | 28
arch/arm/firmware/Makefile | 1 +
arch/arm/firmware/trusted_foundations.c| 79
++ arch/arm/include/asm/trusted_foundations.h
| 67 ++ 8 files changed, 199 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundatio
ns.txt create mode 100644 arch/arm/firmware/Kconfig
create mode 100644 arch/arm/firmware/Makefile
create mode 100644 arch/arm/firmware/trusted_foundations.c
create mode 100644 arch/arm/include/asm/trusted_foundations.h
diff --git
a/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundat
ions.txt
b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundat
ions.txt new file mode 100644
index 000..2ec75c9
--- /dev/null
+++
b/Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundat
ions.txt @@ -0,0 +1,20 @@
+Trusted Foundations
+---
+
+Boards that use the Trusted Foundations secure monitor can signal
its
+presence by declaring a node compatible with
tl,trusted-foundations
+under the /firmware/ node
+
+Required properties:
+- compatible : tl,trusted-foundations
+- version-major : major version number of Trusted Foundations
firmware
+- version-minor: minor version number of Trusted Foundations
firmware
vendor prefix version.
Are you saying he should use tl,version-major tl,version-minor? For
bindings that are already vendor-specific we haven't (on ARM) asked for
vendor prefix on properties. It doesn't mean that we should keep going
down that route though, so I'm just asking for clarification for my own
edification. :)
This is a good question. We should decide what the right thing (TM) is and
write it down. I, on the contrary, was convinced that it's the way Kumar
says.
The impression I got was that properties should be prefixed when they're
extremely vendor-specific and could clash with a more generic property. I'm not
sure that firmware will ever have a generic binding given the variation even in
the set of implemented functionality.
I would imagine that there are many ways different firmwares might be
versioned, and I can't see version-major or version-minor clashing with a
generic property we might add later. However prefixing would not be harmful, so
I'm not opposed to it if others want that.
Another option would be to support a fallback compatible list (e.g.
tl,trusted-foundations-${MAJOR}-${MINOR}, tl,trusted-foundations), and get
versioning information from there. Given that could be painful to handle I
don't want to force it if not required.
Thanks,
Mark.
I'm of the opinion that making all vendor specific properties vendor prefixed
is the easiest rule of thumb and leaves no gray area to have to argue about.
All good points, I will vendor-prefix these properties.
Thanks,
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Any news on Runtime Interpreted Power Sequences
On 10/25/2013 04:33 PM, NeilBrown wrote:
* PGP Signed by an unknown key
On Fri, 25 Oct 2013 15:23:45 +0900 Alex Courbot wrote:
Hi Neil,
On 10/25/2013 09:22 AM, NeilBrown wrote:
I'm wondering if there was any news on the Runtime Interpreted Power
Sequences?
The most recent news I can find is
https://lkml.org/lkml/2013/4/27/73
where you say they might be ready for 3.11. Clearly that didn't work
(predictions being hard, especially about the future).
I'm really keen to see them turning into a reality and I gather others are
too. So ... can we hope?
A prerequisite of power sequences was to merge the gpiod interface, and
this is finally happening. It took much longer than I wanted, sorry
about that.
Logically speaking nothing should now stand in the way of a new version
of the power sequences. Expected maybe my own skepticism about them.
The first version of the power seqs is mainly the result of my
misunderstanding of the device tree. Reconsidering it now, if we strip
the DT support away power seqs would just become a simplified way to
describe how to power a device up and down. In other words, it would be
another way to express what can be expressed with C code and would not
bring any additional flexibility that DT-described power seqs would have
(and I say this totally convinced now that power sequences in the device
tree were a bad idea).
The advantage I could see is that using power sequences we could get rid
of the cumbersome and mistake-prone error checking code which is
basically the same for most devices. You would just need to describe
what you want to activate, and in which order, and the power seqs
framework would catch and report any error.
I'm not sure if this is a sufficient reason to introduce another
framework into the kernel, but if this is deemed a good reason by more
experienced people then I'm ok to give it a shot. If you have other
motivations for this, please also state them so I can get the whole
picture. Maybe I just need to be a little bit more motivated about this
idea myself. :)
Thanks,
Alex.
Hmmm... I'm not encouraged that you don't see them as belonging in
device-tree, as that is exactly where I want them.
Let me explain what I'm thinking.
I have two (or three) use-cases on my board ("GTA04" replacement motherboard
for Openmoko Phone).
Firstly the wifi chip is very fussy about being reset properly before being
accessed. However it shares the same power regulator as the bluetooth chip.
So if the bluetooth is in use when you "ifconfig down; ifconfig up" the
wifi, it won't get powered down, so it won't get reset, so it won't work.
What I need to do is to tell the 'mmc/sdio' driver that when it wants to
power-on the wifi, it must
pull a reset-gpio low
turn-on the regulator (which might already be on)
pull the reset-gpio high again
I currently have that hacked into the omap-hsmmc driver but I don't think the
code really belongs there.
I'd argue that it probably belongs here, on the contrary - unless this
power-on sequence breaks other use-cases (which I don't think it does),
it could simply be considered a safer way to power your device on.
Shared regulators are quite common in the kernel, so you cannot simply
rely on them solely to power a device down and expect it to be back to
its reset set the next time you switch the regulator on.
I would much rather that the omap-hsmmc could be given a 'RIPS' instead
of (or as well as) the regulator and be told to just turn that RIPS on or off.
The RIPS would do the appropriate timed fiddling with GPIO and Regulator.
The only place I can think of the describe the RIPS would be in device-tree.
The problem I have with this is that the power sequence of a device is
something that should be device-specific (as opposed to board-specific),
and thus should be handled by the device driver. If you need to come
with a board-specific way to power a device up, this most most certainly
means something is wrong with the device's powerup sequence to start
with. In the current case, it definitely seems to be the issue. Is there
any problem introduced by asserting reset on your device when you power
it up?
Secondly I have two devices that are behind serial ports - a GPS and the
bluetooth.
It makes perfect sense to tie the power-on/off of these to the serial-port
concept of "DTR". The omap-serial doesn't have a physical DTR, but is can be
configured with a GPIO to use as the DTR (it gets asserted on 'open' and
de-asserted on 'close').
I currently have a "gpio-to-regulator" converting driver plugged between the
serial port and the blue-tooth's regulator. When I open the serial port for
bluetooth it asserts the gpio line which is routed through gpiolib to my
driver which enables the appropriate regulator. I have something similar for
the GPS.
I think the gpio-to-regulator driver is a kludge. I would much rather tell
the omap-serial to activate/de-activ
Re: Any news on Runtime Interpreted Power Sequences
On 10/25/2013 04:33 PM, NeilBrown wrote: * PGP Signed by an unknown key On Fri, 25 Oct 2013 15:23:45 +0900 Alex Courbot acour...@nvidia.com wrote: Hi Neil, On 10/25/2013 09:22 AM, NeilBrown wrote: I'm wondering if there was any news on the Runtime Interpreted Power Sequences? The most recent news I can find is https://lkml.org/lkml/2013/4/27/73 where you say they might be ready for 3.11. Clearly that didn't work (predictions being hard, especially about the future). I'm really keen to see them turning into a reality and I gather others are too. So ... can we hope? A prerequisite of power sequences was to merge the gpiod interface, and this is finally happening. It took much longer than I wanted, sorry about that. Logically speaking nothing should now stand in the way of a new version of the power sequences. Expected maybe my own skepticism about them. The first version of the power seqs is mainly the result of my misunderstanding of the device tree. Reconsidering it now, if we strip the DT support away power seqs would just become a simplified way to describe how to power a device up and down. In other words, it would be another way to express what can be expressed with C code and would not bring any additional flexibility that DT-described power seqs would have (and I say this totally convinced now that power sequences in the device tree were a bad idea). The advantage I could see is that using power sequences we could get rid of the cumbersome and mistake-prone error checking code which is basically the same for most devices. You would just need to describe what you want to activate, and in which order, and the power seqs framework would catch and report any error. I'm not sure if this is a sufficient reason to introduce another framework into the kernel, but if this is deemed a good reason by more experienced people then I'm ok to give it a shot. If you have other motivations for this, please also state them so I can get the whole picture. Maybe I just need to be a little bit more motivated about this idea myself. :) Thanks, Alex. Hmmm... I'm not encouraged that you don't see them as belonging in device-tree, as that is exactly where I want them. Let me explain what I'm thinking. I have two (or three) use-cases on my board (GTA04 replacement motherboard for Openmoko Phone). Firstly the wifi chip is very fussy about being reset properly before being accessed. However it shares the same power regulator as the bluetooth chip. So if the bluetooth is in use when you ifconfig down; ifconfig up the wifi, it won't get powered down, so it won't get reset, so it won't work. What I need to do is to tell the 'mmc/sdio' driver that when it wants to power-on the wifi, it must pull a reset-gpio low turn-on the regulator (which might already be on) pull the reset-gpio high again I currently have that hacked into the omap-hsmmc driver but I don't think the code really belongs there. I'd argue that it probably belongs here, on the contrary - unless this power-on sequence breaks other use-cases (which I don't think it does), it could simply be considered a safer way to power your device on. Shared regulators are quite common in the kernel, so you cannot simply rely on them solely to power a device down and expect it to be back to its reset set the next time you switch the regulator on. I would much rather that the omap-hsmmc could be given a 'RIPS' instead of (or as well as) the regulator and be told to just turn that RIPS on or off. The RIPS would do the appropriate timed fiddling with GPIO and Regulator. The only place I can think of the describe the RIPS would be in device-tree. The problem I have with this is that the power sequence of a device is something that should be device-specific (as opposed to board-specific), and thus should be handled by the device driver. If you need to come with a board-specific way to power a device up, this most most certainly means something is wrong with the device's powerup sequence to start with. In the current case, it definitely seems to be the issue. Is there any problem introduced by asserting reset on your device when you power it up? Secondly I have two devices that are behind serial ports - a GPS and the bluetooth. It makes perfect sense to tie the power-on/off of these to the serial-port concept of DTR. The omap-serial doesn't have a physical DTR, but is can be configured with a GPIO to use as the DTR (it gets asserted on 'open' and de-asserted on 'close'). I currently have a gpio-to-regulator converting driver plugged between the serial port and the blue-tooth's regulator. When I open the serial port for bluetooth it asserts the gpio line which is routed through gpiolib to my driver which enables the appropriate regulator. I have something similar for the GPS. I think the gpio-to-regulator driver is a kludge. I would much rather tell the omap-serial to activate/de-activate a RIPS as the DTR action
Re: Any news on Runtime Interpreted Power Sequences
Hi Neil, On 10/25/2013 09:22 AM, NeilBrown wrote: I'm wondering if there was any news on the Runtime Interpreted Power Sequences? The most recent news I can find is https://lkml.org/lkml/2013/4/27/73 where you say they might be ready for 3.11. Clearly that didn't work (predictions being hard, especially about the future). I'm really keen to see them turning into a reality and I gather others are too. So ... can we hope? A prerequisite of power sequences was to merge the gpiod interface, and this is finally happening. It took much longer than I wanted, sorry about that. Logically speaking nothing should now stand in the way of a new version of the power sequences. Expected maybe my own skepticism about them. The first version of the power seqs is mainly the result of my misunderstanding of the device tree. Reconsidering it now, if we strip the DT support away power seqs would just become a simplified way to describe how to power a device up and down. In other words, it would be another way to express what can be expressed with C code and would not bring any additional flexibility that DT-described power seqs would have (and I say this totally convinced now that power sequences in the device tree were a bad idea). The advantage I could see is that using power sequences we could get rid of the cumbersome and mistake-prone error checking code which is basically the same for most devices. You would just need to describe what you want to activate, and in which order, and the power seqs framework would catch and report any error. I'm not sure if this is a sufficient reason to introduce another framework into the kernel, but if this is deemed a good reason by more experienced people then I'm ok to give it a shot. If you have other motivations for this, please also state them so I can get the whole picture. Maybe I just need to be a little bit more motivated about this idea myself. :) Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Any news on Runtime Interpreted Power Sequences
Hi Neil, On 10/25/2013 09:22 AM, NeilBrown wrote: I'm wondering if there was any news on the Runtime Interpreted Power Sequences? The most recent news I can find is https://lkml.org/lkml/2013/4/27/73 where you say they might be ready for 3.11. Clearly that didn't work (predictions being hard, especially about the future). I'm really keen to see them turning into a reality and I gather others are too. So ... can we hope? A prerequisite of power sequences was to merge the gpiod interface, and this is finally happening. It took much longer than I wanted, sorry about that. Logically speaking nothing should now stand in the way of a new version of the power sequences. Expected maybe my own skepticism about them. The first version of the power seqs is mainly the result of my misunderstanding of the device tree. Reconsidering it now, if we strip the DT support away power seqs would just become a simplified way to describe how to power a device up and down. In other words, it would be another way to express what can be expressed with C code and would not bring any additional flexibility that DT-described power seqs would have (and I say this totally convinced now that power sequences in the device tree were a bad idea). The advantage I could see is that using power sequences we could get rid of the cumbersome and mistake-prone error checking code which is basically the same for most devices. You would just need to describe what you want to activate, and in which order, and the power seqs framework would catch and report any error. I'm not sure if this is a sufficient reason to introduce another framework into the kernel, but if this is deemed a good reason by more experienced people then I'm ok to give it a shot. If you have other motivations for this, please also state them so I can get the whole picture. Maybe I just need to be a little bit more motivated about this idea myself. :) Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/3] gpiolib: add gpiod_get() and gpiod_put() functions
On 10/16/2013 04:37 AM, Linus Walleij wrote:
On Sat, Oct 12, 2013 at 12:31 AM, Alex Courbot wrote:
+#else
+static struct device_node *of_find_gpio(struct device *dev, const char *id
+ unsigned int idx, unsigned long flags)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
... and of course I forgot to fix the main compilation error. Linus,
would you mind squashing what follows into this patch? Apologies for
the inconvenience.
I guess you'll fix this if you respin it?
If it's too much trouble now we can just defer this for early in the v3.14
cycle.
No problem, I will try to rebase on top of your for-next, and squash
this today.
Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/3] gpiolib: add gpiod_get() and gpiod_put() functions
On 10/16/2013 04:37 AM, Linus Walleij wrote:
On Sat, Oct 12, 2013 at 12:31 AM, Alex Courbot acour...@nvidia.com wrote:
+#else
+static struct device_node *of_find_gpio(struct device *dev, const char *id
+ unsigned int idx, unsigned long flags)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
... and of course I forgot to fix the main compilation error. Linus,
would you mind squashing what follows into this patch? Apologies for
the inconvenience.
I guess you'll fix this if you respin it?
If it's too much trouble now we can just defer this for early in the v3.14
cycle.
No problem, I will try to rebase on top of your for-next, and squash
this today.
Thanks,
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] ARM: add basic support for Trusted Foundations
On 10/15/2013 04:07 AM, Russell King - ARM Linux wrote:
On Fri, Oct 11, 2013 at 02:45:34PM -0700, Alexandre Courbot wrote:
Trusted Foundations is a TrustZone-based secure monitor for ARM that
can be invoked using the same SMC-based API on all supported
platforms. This patch adds initial basic support for Trusted
Foundations using the ARM firmware API. Current features are limited
to the ability to boot secondary processors.
Note: The API followed by Trusted Foundations does *not* follow the SMC
calling conventions. It has nothing to do with PSCI neither and is only
relevant to devices that use Trusted Foundations (like most Tegra-based
retail devices).
Signed-off-by: Alexandre Courbot
Reviewed-by: Tomasz Figa
Reviewed-by: Stephen Warren
---
.../arm/firmware/tl,trusted-foundations.txt| 20 ++
.../devicetree/bindings/vendor-prefixes.txt| 1 +
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/firmware/Kconfig | 28
arch/arm/firmware/Makefile | 1 +
arch/arm/firmware/trusted_foundations.c| 79 ++
arch/arm/include/asm/trusted_foundations.h | 68 +++
8 files changed, 200 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt
create mode 100644 arch/arm/firmware/Kconfig
create mode 100644 arch/arm/firmware/Makefile
create mode 100644 arch/arm/firmware/trusted_foundations.c
create mode 100644 arch/arm/include/asm/trusted_foundations.h
Is having this under arch/arm appropriate? What happens if the API
gets re-used on ARM64 for example? Would drivers/firmware be a better
cross-arch location for this?
The reason why this has been put into arch/arm is that the firmware_ops
feature this patch depends also resides there
(arch/arm/include/asm/firmware.h).
On the other hand it might also make sense to move firmware_ops out of
ARM since I don't see anything ARM-specific with it. Tomasz, could we
have your thoughts on this?
diff --git a/arch/arm/include/asm/trusted_foundations.h
b/arch/arm/include/asm/trusted_foundations.h
new file mode 100644
index 000..c6f20bd
--- /dev/null
+++ b/arch/arm/include/asm/trusted_foundations.h
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+/*
+ * Support for the Trusted Foundations secure monitor.
+ *
+ * Trusted Foundation comes active on some ARM consumer devices (most
+ * Tegra-based devices sold on the market are concerned). Such devices can only
+ * perform some basic operations, like setting the CPU reset vector, through
+ * SMC calls to the secure monitor. The calls are completely specific to
+ * Trusted Foundations, and do *not* follow the SMC calling convention or the
+ * PSCI standard.
+ */
+
+#ifndef __ASM_ARM_TRUSTED_FOUNDATIONS_H
+#define __ASM_ARM_TRUSTED_FOUNDATIONS_H
+
+#include
+
+struct trusted_foundations_platform_data {
+ unsigned int version_major;
+ unsigned int version_minor;
+};
+
+#if IS_ENABLED(CONFIG_TRUSTED_FOUNDATIONS)
+
+void register_trusted_foundations(struct trusted_foundations_platform_data
*pd);
+void of_register_trusted_foundations(void);
+
+#else /* CONFIG_TRUSTED_FOUNDATIONS */
+
+#include
+#include
+#include
Please move these up along side the other #include - having includes depend
on config symbols is an additional unnecessary source of fragility.
Secondly, please use linux/*.h includes in preference to asm/*.h where the
linux/*.h include picks up the corresponding asm/*.h include. In this case,
that should be linux/bug.h, not asm/bug.h.
Will do, thanks!
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/5] ARM: add basic support for Trusted Foundations
On 10/15/2013 04:07 AM, Russell King - ARM Linux wrote:
On Fri, Oct 11, 2013 at 02:45:34PM -0700, Alexandre Courbot wrote:
Trusted Foundations is a TrustZone-based secure monitor for ARM that
can be invoked using the same SMC-based API on all supported
platforms. This patch adds initial basic support for Trusted
Foundations using the ARM firmware API. Current features are limited
to the ability to boot secondary processors.
Note: The API followed by Trusted Foundations does *not* follow the SMC
calling conventions. It has nothing to do with PSCI neither and is only
relevant to devices that use Trusted Foundations (like most Tegra-based
retail devices).
Signed-off-by: Alexandre Courbot acour...@nvidia.com
Reviewed-by: Tomasz Figa t.f...@samsung.com
Reviewed-by: Stephen Warren swar...@nvidia.com
---
.../arm/firmware/tl,trusted-foundations.txt| 20 ++
.../devicetree/bindings/vendor-prefixes.txt| 1 +
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/firmware/Kconfig | 28
arch/arm/firmware/Makefile | 1 +
arch/arm/firmware/trusted_foundations.c| 79 ++
arch/arm/include/asm/trusted_foundations.h | 68 +++
8 files changed, 200 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/firmware/tl,trusted-foundations.txt
create mode 100644 arch/arm/firmware/Kconfig
create mode 100644 arch/arm/firmware/Makefile
create mode 100644 arch/arm/firmware/trusted_foundations.c
create mode 100644 arch/arm/include/asm/trusted_foundations.h
Is having this under arch/arm appropriate? What happens if the API
gets re-used on ARM64 for example? Would drivers/firmware be a better
cross-arch location for this?
The reason why this has been put into arch/arm is that the firmware_ops
feature this patch depends also resides there
(arch/arm/include/asm/firmware.h).
On the other hand it might also make sense to move firmware_ops out of
ARM since I don't see anything ARM-specific with it. Tomasz, could we
have your thoughts on this?
diff --git a/arch/arm/include/asm/trusted_foundations.h
b/arch/arm/include/asm/trusted_foundations.h
new file mode 100644
index 000..c6f20bd
--- /dev/null
+++ b/arch/arm/include/asm/trusted_foundations.h
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+/*
+ * Support for the Trusted Foundations secure monitor.
+ *
+ * Trusted Foundation comes active on some ARM consumer devices (most
+ * Tegra-based devices sold on the market are concerned). Such devices can only
+ * perform some basic operations, like setting the CPU reset vector, through
+ * SMC calls to the secure monitor. The calls are completely specific to
+ * Trusted Foundations, and do *not* follow the SMC calling convention or the
+ * PSCI standard.
+ */
+
+#ifndef __ASM_ARM_TRUSTED_FOUNDATIONS_H
+#define __ASM_ARM_TRUSTED_FOUNDATIONS_H
+
+#include linux/kconfig.h
+
+struct trusted_foundations_platform_data {
+ unsigned int version_major;
+ unsigned int version_minor;
+};
+
+#if IS_ENABLED(CONFIG_TRUSTED_FOUNDATIONS)
+
+void register_trusted_foundations(struct trusted_foundations_platform_data
*pd);
+void of_register_trusted_foundations(void);
+
+#else /* CONFIG_TRUSTED_FOUNDATIONS */
+
+#include linux/printk.h
+#include linux/of.h
+#include asm/bug.h
Please move these up along side the other #include - having includes depend
on config symbols is an additional unnecessary source of fragility.
Secondly, please use linux/*.h includes in preference to asm/*.h where the
linux/*.h include picks up the corresponding asm/*.h include. In this case,
that should be linux/bug.h, not asm/bug.h.
Will do, thanks!
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/3] gpiolib: add gpiod_get() and gpiod_put() functions
> +#else
> +static struct device_node *of_find_gpio(struct device *dev, const char *id
> + unsigned int idx, unsigned long flags)
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +#endif
... and of course I forgot to fix the main compilation error. Linus,
would you mind squashing what follows into this patch? Apologies for
the inconvenience.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 05b9981..e1d1a15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2172,8 +2172,8 @@ static struct gpio_desc *of_find_gpio(struct device *dev,
const char *con_id,
return desc;
}
#else
-static struct device_node *of_find_gpio(struct device *dev, const char *id
- unsigned int idx, unsigned long flags)
+static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
+ unsigned int idx, unsigned long *flags)
{
return ERR_PTR(-ENODEV);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 3/3] gpiolib: add gpiod_get() and gpiod_put() functions
+#else
+static struct device_node *of_find_gpio(struct device *dev, const char *id
+ unsigned int idx, unsigned long flags)
+{
+ return ERR_PTR(-ENODEV);
+}
+#endif
... and of course I forgot to fix the main compilation error. Linus,
would you mind squashing what follows into this patch? Apologies for
the inconvenience.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 05b9981..e1d1a15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2172,8 +2172,8 @@ static struct gpio_desc *of_find_gpio(struct device *dev,
const char *con_id,
return desc;
}
#else
-static struct device_node *of_find_gpio(struct device *dev, const char *id
- unsigned int idx, unsigned long flags)
+static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
+ unsigned int idx, unsigned long *flags)
{
return ERR_PTR(-ENODEV);
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 0/5] ARM: support for Trusted Foundations secure monito
On 10/04/2013 09:37 AM, Alexandre Courbot wrote: Hopefully final version of this patchset. If I'm not mistaken the last thing that prevents Stephen from merging it is Russel's Ack. Russel, could you check it? Oops. s/Russel/Russell/g, with apologies for the continuous misspelling. m(__)m Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v7 0/5] ARM: support for Trusted Foundations secure monito
On 10/04/2013 09:37 AM, Alexandre Courbot wrote: Hopefully final version of this patchset. If I'm not mistaken the last thing that prevents Stephen from merging it is Russel's Ack. Russel, could you check it? Oops. s/Russel/Russell/g, with apologies for the continuous misspelling. m(__)m Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 0/5] ARM: tegra: support for Trusted Foundations
On 09/04/2013 03:42 AM, Stephen Warren wrote: On 08/29/2013 03:57 AM, Alexandre Courbot wrote: New version revised according to comments received for v3. Hopefully it will be good enough to be merged. Aside from the small issue in patch 1/5, the series, Reviewed-by: Stephen Warren Thanks! I have adressed the issues you mentioned and will submit v5 soon. Despite the fact this adds some non-Tegra stuff, will you be able to take it into your tree, or should I ask someone else? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 0/5] ARM: tegra: support for Trusted Foundations
On 09/04/2013 03:42 AM, Stephen Warren wrote: On 08/29/2013 03:57 AM, Alexandre Courbot wrote: New version revised according to comments received for v3. Hopefully it will be good enough to be merged. Aside from the small issue in patch 1/5, the series, Reviewed-by: Stephen Warren swar...@nvidia.com Thanks! I have adressed the issues you mentioned and will submit v5 soon. Despite the fact this adds some non-Tegra stuff, will you be able to take it into your tree, or should I ask someone else? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] decompressors: fix "no limit" output buffer length
On 07/23/2013 12:32 PM, Stephen Warren wrote: On 07/22/2013 07:15 PM, Alex Courbot wrote: ... Although the patch seems ok to me in its current form, there are two points for which I still have small doubts: 1) Whether size_t and pointers will have the same size on all platforms. ptrsize_t? Do you mean ptrdiff_t? (I cannot find ptrsize_t anywhere in the kernel) Looking further about the uses of size_t and ptrdiff_t, it seems like size_t is designed to store the maximum addressable member of an array, whereas ptrdiff_t is used to store a substraction of two pointers. In effect, they translate to the unsigned (size_t) and signed (ptrdiff_t) variants of the same type. But since here we know that the result of the substraction will always be positive and potentially big (for devices with memory in the lower half of the address space) using size_t sounds safer to avoid overflows and sign-conversion issues (strm->avail_out, where the value of out_len eventually ends, is an unsigned int). So point 1) at least seems to be handled correctly with size_t. Point 2) might still be of concern, but if your uncompressed kernel image ends up overflowing your addressable memory, I guess you have a bigger problem to start with. :) Andrew, do you think you can merge this as-is? Sorry if you are not the right person to ask, but there is no clear maintainer for this part of the code and you appear to have handled the latest patches that affect the same file. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] decompressors: fix "no limit" output buffer length
On 07/23/2013 03:08 AM, Jon Medhurst (Tixy) wrote:
On Mon, 2013-07-22 at 15:56 +0900, Alexandre Courbot wrote:
When decompressing into memory, the output buffer length is set to some
arbitrarily high value (0x7fff) to indicate the output is,
virtually, unlimited in size.
The problem with this is that some platforms have their physical memory
at high physical addresses (0x8000 or more), and that the output
buffer address and its "unlimited" length cannot be added without
overflowing. An example of this can be found in inflate_fast():
/* next_out is the output buffer address */
out = strm->next_out - OFF;
/* avail_out is the output buffer size. end will overflow if the output
* address is >= 0x8104 */
end = out + (strm->avail_out - 257);
This has huge consequences on the performance of kernel decompression,
since the following exit condition of inflate_fast() will be always
true:
} while (in < last && out < end);
Indeed, "end" has overflowed and is now always lower than "out". As a
result, inflate_fast() will return after processing one single byte of
input data, and will thus need to be called an unreasonably high number
of times. This probably went unnoticed because kernel decompression is
fast enough even with this issue.
Nonetheless, adjusting the output buffer length in such a way that the
above pointer arithmetic never overflows results in a kernel
decompression that is about 3 times faster on affected machines.
Signed-off-by: Alexandre Courbot
This speeds up booting of my Versatile Express TC2 by 15 seconds when
starting on the A7 cluster :-)
Tested-by: Jon Medhurst
Good to hear! Thanks for taking the time to test this.
Although the patch seems ok to me in its current form, there are two
points for which I still have small doubts:
1) Whether size_t and pointers will have the same size on all platforms.
It not we might end up with some funny behaviors. My limited research on
the topic did not end up with evidence that their size may differ, but I
don't have a definite case that they do neither.
2) Whether all platforms have their address space ending at (~0). I do
not have a concrete example in mind, but can imagine, say, a platform
which represents its addresses as 32-bit pointers but has a smaller
physical bus. In this case the current calculation could cause overflows
again.
If one (or both) of these points are to be concerned about, there may
exist macros I am not aware of that better represent the actual limits
of pointers in the kernel.
Thanks,
Alex.
---
lib/decompress_inflate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/decompress_inflate.c b/lib/decompress_inflate.c
index 19ff89e..d619b28 100644
--- a/lib/decompress_inflate.c
+++ b/lib/decompress_inflate.c
@@ -48,7 +48,7 @@ STATIC int INIT gunzip(unsigned char *buf, int len,
out_len = 0x8000; /* 32 K */
out_buf = malloc(out_len);
} else {
- out_len = 0x7fff; /* no limit */
+ out_len = ((size_t)~0) - (size_t)out_buf; /* no limit */
}
if (!out_buf) {
error("Out of memory while allocating output buffer");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] decompressors: fix no limit output buffer length
On 07/23/2013 03:08 AM, Jon Medhurst (Tixy) wrote:
On Mon, 2013-07-22 at 15:56 +0900, Alexandre Courbot wrote:
When decompressing into memory, the output buffer length is set to some
arbitrarily high value (0x7fff) to indicate the output is,
virtually, unlimited in size.
The problem with this is that some platforms have their physical memory
at high physical addresses (0x8000 or more), and that the output
buffer address and its unlimited length cannot be added without
overflowing. An example of this can be found in inflate_fast():
/* next_out is the output buffer address */
out = strm-next_out - OFF;
/* avail_out is the output buffer size. end will overflow if the output
* address is = 0x8104 */
end = out + (strm-avail_out - 257);
This has huge consequences on the performance of kernel decompression,
since the following exit condition of inflate_fast() will be always
true:
} while (in last out end);
Indeed, end has overflowed and is now always lower than out. As a
result, inflate_fast() will return after processing one single byte of
input data, and will thus need to be called an unreasonably high number
of times. This probably went unnoticed because kernel decompression is
fast enough even with this issue.
Nonetheless, adjusting the output buffer length in such a way that the
above pointer arithmetic never overflows results in a kernel
decompression that is about 3 times faster on affected machines.
Signed-off-by: Alexandre Courbot acour...@nvidia.com
This speeds up booting of my Versatile Express TC2 by 15 seconds when
starting on the A7 cluster :-)
Tested-by: Jon Medhurst t...@linaro.org
Good to hear! Thanks for taking the time to test this.
Although the patch seems ok to me in its current form, there are two
points for which I still have small doubts:
1) Whether size_t and pointers will have the same size on all platforms.
It not we might end up with some funny behaviors. My limited research on
the topic did not end up with evidence that their size may differ, but I
don't have a definite case that they do neither.
2) Whether all platforms have their address space ending at (~0). I do
not have a concrete example in mind, but can imagine, say, a platform
which represents its addresses as 32-bit pointers but has a smaller
physical bus. In this case the current calculation could cause overflows
again.
If one (or both) of these points are to be concerned about, there may
exist macros I am not aware of that better represent the actual limits
of pointers in the kernel.
Thanks,
Alex.
---
lib/decompress_inflate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/decompress_inflate.c b/lib/decompress_inflate.c
index 19ff89e..d619b28 100644
--- a/lib/decompress_inflate.c
+++ b/lib/decompress_inflate.c
@@ -48,7 +48,7 @@ STATIC int INIT gunzip(unsigned char *buf, int len,
out_len = 0x8000; /* 32 K */
out_buf = malloc(out_len);
} else {
- out_len = 0x7fff; /* no limit */
+ out_len = ((size_t)~0) - (size_t)out_buf; /* no limit */
}
if (!out_buf) {
error(Out of memory while allocating output buffer);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] decompressors: fix no limit output buffer length
On 07/23/2013 12:32 PM, Stephen Warren wrote: On 07/22/2013 07:15 PM, Alex Courbot wrote: ... Although the patch seems ok to me in its current form, there are two points for which I still have small doubts: 1) Whether size_t and pointers will have the same size on all platforms. ptrsize_t? Do you mean ptrdiff_t? (I cannot find ptrsize_t anywhere in the kernel) Looking further about the uses of size_t and ptrdiff_t, it seems like size_t is designed to store the maximum addressable member of an array, whereas ptrdiff_t is used to store a substraction of two pointers. In effect, they translate to the unsigned (size_t) and signed (ptrdiff_t) variants of the same type. But since here we know that the result of the substraction will always be positive and potentially big (for devices with memory in the lower half of the address space) using size_t sounds safer to avoid overflows and sign-conversion issues (strm-avail_out, where the value of out_len eventually ends, is an unsigned int). So point 1) at least seems to be handled correctly with size_t. Point 2) might still be of concern, but if your uncompressed kernel image ends up overflowing your addressable memory, I guess you have a bigger problem to start with. :) Andrew, do you think you can merge this as-is? Sorry if you are not the right person to ask, but there is no clear maintainer for this part of the code and you appear to have handled the latest patches that affect the same file. Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] simplefb: add support for a8b8g8r8 pixel format
On 06/07/2013 04:22 PM, Alexander van Heukelum wrote:
static struct simplefb_format simplefb_formats[] = {
{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+ { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
Hi,
31? I assume in practice this doesn't influence anything, but I think it should
have read 24.
Doh. Thanks for pointing it out.
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format
On 06/07/2013 01:32 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: We're talking about adding a bunch of code instead of one line in a table. Sorry, I NACK your NACK. If we get more entries we'll add the parser, but not just for this. as there is no NACK so far I do take as a NACK ... which means? Anyway, I have sent a new version of this patch that includes a summary motivating why we need it. Please explain clearly whether you accept it or not, and if not, what we need to do to add this mode in a satisfactory way. Thanks, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format
On 06/07/2013 01:32 AM, Jean-Christophe PLAGNIOL-VILLARD wrote: We're talking about adding a bunch of code instead of one line in a table. Sorry, I NACK your NACK. If we get more entries we'll add the parser, but not just for this. as there is no NACK so far I do take as a NACK ... which means? Anyway, I have sent a new version of this patch that includes a summary motivating why we need it. Please explain clearly whether you accept it or not, and if not, what we need to do to add this mode in a satisfactory way. Thanks, Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] simplefb: add support for a8b8g8r8 pixel format
On 06/07/2013 04:22 PM, Alexander van Heukelum wrote:
static struct simplefb_format simplefb_formats[] = {
{ r5g6b5, 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+ { a8b8g8r8, 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
Hi,
31? I assume in practice this doesn't influence anything, but I think it should
have read 24.
Doh. Thanks for pointing it out.
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: add basic SecureOS support
Hi Tomasz,
On 06/06/2013 07:17 PM, Tomasz Figa wrote:
+Global properties
+---
+
+The following properties can be specified into the "chosen" root
+node:
+
+ nvidia,secure-os: enable SecureOS.
Hmm, on Exynos we had something like
firmware@0203F000 {
compatible = "samsung,secure-firmware";
reg = <0x0203F000 0x1000>;
};
but your solution might be actually the proper one, since firmware is not
a hardware block. (The address in reg property is pointing to SYSRAM
memory, which is an additional communication channel with the firmware.)
Yes, I saw your implementation but decided to do it through the chosen
node anyway, since that's what it seems to be designed and we don't need
any reg parameter.
I think this patch could be split into several patches:
- add support for firmware
- split reset function
- add reset support using firmware.
Mmm possibly yes, but I wonder if that would not be too much splitting.
Stephen?
Hmm, I wonder if you need all this complexity here. Have a look at our
exynos_smc function
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606
Yes, I just embarrassed myself showing my ignorance of ARM assembler. ;)
The fix Russel proposed is pretty close to your version.
+static const struct firmware_ops tegra_firmware_ops = {
+ .set_cpu_boot_addr = tegra_set_cpu_boot_addr,
+};
It's good that this interface is finally getting some user besides Exynos.
I didn't know about it first but Joseph kindly pointed it out to me and
it indeed makes it easier to implement this.
Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: add basic SecureOS support
On 06/06/2013 06:35 PM, Russell King - ARM Linux wrote:
On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote:
+static int __attribute__((used)) __tegra_smc_stack[10];
+
+/*
+ * With EABI, subtype and arg already end up in r0, r1 and r2 as they are
+ * function arguments, but we prefer to play safe here and explicitly move
+ * these values into the expected registers anyway. mov instructions without
+ * any side-effect are turned into nops by the assembler, which limits
+ * overhead.
No they aren't. They will be preserved as:
mov r0, r0
mov r1, r1
mov r2, r2
I'm pretty sure I checked with objdump and saw these replaced by nops at
some point, but for some reason I cannot get that behavior again. So
simply put, my statement is wrong indeed.
Moreover, things will go wrong if the compiler decides for whatever reason
to move 'arg' into r0 before calling your code sequence. So really this
is quite fragile.
There's also no point in mentioning EABI in the above paragraph; all ARM
ABIs under Linux have always placed the arguments in r0..r3 and then on
the stack. You can assert that this is always true by using the __asmeq()
macro.
Good to know, thanks.
Also...
+ */
+static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
+{
+ asm volatile(
+ ".arch_extension sec\n\t"
+ "ldr r3, =__tegra_smc_stack\n\t"
+ "stmia r3, {r4-r12, lr}\n\t"
using a statically allocated area to save the registers in this way is
probably a bad move; although the hotplug code guarantees that there
will be no concurrency between CPU hotplug operations, this sets a
precident for it being used in places where no such protection exists.
Indeed. This function will be called from other places in the future,
and for these we cannot assume there will be no concurrency.
What is wrong with stacking r4-r12, lr onto the SVC stack?
Nothing, actually. /o\
You don't
save the SVC stack pointer, so one can only assume that your SMC
implmentation preserves this (if it doesn't, that's yet another bug
with this assembly code.)
SVC stack pointer is ok AFAICT.
Combining these two issues, you're probably far better off using an
__attribute__((naked)) function for this - which means you get to
write the entire function in assembly code without any compiler
interference:
>
static void __attribute__((naked)) tegra_generic_smc(u32 type, u32 subtype, u32
arg)
{
asm volatile(
".arch_extension sec\n\t"
"stmfd sp!, {r4 - r12, lr}\n\t"
__asmeq("%0", "r0")
__asmeq("%1", "r1")
__asmeq("%2", "r2")
"mov r3, #0\n\t"
"mov r4, #0\n\t"
"dsb\n\t"
"smc #0\n\t"
"ldmfd sp!, {r4 - r12, pc}"
:
: "r" (type), "r" (subtype), "r" (arg)
: "memory");
}
Well, that works beautifully indeed, and results in a much smaller
function that does nothing more beyond what's needed. On top of that, I
feel enlightened.
Thanks, I will resubmit a fixed version soon.
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format
On 06/06/2013 05:24 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
On Jun 6, 2013, at 10:12 AM, Alex Courbot wrote:
On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
On Jun 6, 2013, at 9:20 AM, Alexandre Courbot wrote:
Signed-off-by: Alexandre Courbot
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
drivers/video/simplefb.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..70c26f3 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -12,6 +12,7 @@ Required properties:
- stride: The number of bytes in each line of the framebuffer.
- format: The format of the framebuffer surface. Valid values are:
- r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+ - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
Example:
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index e2e9e3e..d7041aa 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -84,6 +84,7 @@ struct simplefb_format {
static struct simplefb_format simplefb_formats[] = {
{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+ { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
why don't you parse the string?
so you will a real generic bindings
Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
The list of modes of this driver should not grow too big. Even in terms of
footprint I'd say the list should remain smaller than the parsing code.
What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to
something more standard, say "rgba".
I'm going to be very honest I do not like the simplefb driver from the beginning
but I do found it useful. And as said in it's name it need to be *SIMPLE*
Then a huge list of compatible no way
otherwise we drop this from the simplefb and make it a generic helper
I do not want to see format parser in every drivers this need to handle at
video framework level
If I see that we start to increase again and again the simplefb I will not
accept
to merge the code as we must keep it simple
In that case it's probably better to maintain a "simple" list of
supported modes, which is what this patch does.
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format
On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
On Jun 6, 2013, at 9:20 AM, Alexandre Courbot wrote:
Signed-off-by: Alexandre Courbot
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
drivers/video/simplefb.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..70c26f3 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -12,6 +12,7 @@ Required properties:
- stride: The number of bytes in each line of the framebuffer.
- format: The format of the framebuffer surface. Valid values are:
- r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+ - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
Example:
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index e2e9e3e..d7041aa 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -84,6 +84,7 @@ struct simplefb_format {
static struct simplefb_format simplefb_formats[] = {
{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+ { "a8b8g8r8", 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
why don't you parse the string?
so you will a real generic bindings
Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
The list of modes of this driver should not grow too big. Even in terms
of footprint I'd say the list should remain smaller than the parsing code.
What we can discuss though is whether we want to keep this a8b8g8r8
syntax or switch to something more standard, say "rgba".
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format
On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
On Jun 6, 2013, at 9:20 AM, Alexandre Courbot acour...@nvidia.com wrote:
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
drivers/video/simplefb.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..70c26f3 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -12,6 +12,7 @@ Required properties:
- stride: The number of bytes in each line of the framebuffer.
- format: The format of the framebuffer surface. Valid values are:
- r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+ - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
Example:
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index e2e9e3e..d7041aa 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -84,6 +84,7 @@ struct simplefb_format {
static struct simplefb_format simplefb_formats[] = {
{ r5g6b5, 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+ { a8b8g8r8, 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
why don't you parse the string?
so you will a real generic bindings
Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
The list of modes of this driver should not grow too big. Even in terms
of footprint I'd say the list should remain smaller than the parsing code.
What we can discuss though is whether we want to keep this a8b8g8r8
syntax or switch to something more standard, say rgba.
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] simplefb: add support for a8b8g8r8 pixel format
On 06/06/2013 05:24 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
On Jun 6, 2013, at 10:12 AM, Alex Courbot acour...@nvidia.com wrote:
On 06/06/2013 04:59 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
On Jun 6, 2013, at 9:20 AM, Alexandre Courbot acour...@nvidia.com wrote:
Signed-off-by: Alexandre Courbot acour...@nvidia.com
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 1 +
drivers/video/simplefb.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 3ea4605..70c26f3 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -12,6 +12,7 @@ Required properties:
- stride: The number of bytes in each line of the framebuffer.
- format: The format of the framebuffer surface. Valid values are:
- r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+ - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
Example:
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index e2e9e3e..d7041aa 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -84,6 +84,7 @@ struct simplefb_format {
static struct simplefb_format simplefb_formats[] = {
{ r5g6b5, 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+ { a8b8g8r8, 32, {0, 8}, {8, 8}, {16, 8}, {31, 8} },
why don't you parse the string?
so you will a real generic bindings
Tried that already, got NACKed: https://lkml.org/lkml/2013/5/27/330
The list of modes of this driver should not grow too big. Even in terms of
footprint I'd say the list should remain smaller than the parsing code.
What we can discuss though is whether we want to keep this a8b8g8r8 syntax or switch to
something more standard, say rgba.
I'm going to be very honest I do not like the simplefb driver from the beginning
but I do found it useful. And as said in it's name it need to be *SIMPLE*
Then a huge list of compatible no way
otherwise we drop this from the simplefb and make it a generic helper
I do not want to see format parser in every drivers this need to handle at
video framework level
If I see that we start to increase again and again the simplefb I will not
accept
to merge the code as we must keep it simple
In that case it's probably better to maintain a simple list of
supported modes, which is what this patch does.
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: add basic SecureOS support
On 06/06/2013 06:35 PM, Russell King - ARM Linux wrote:
On Thu, Jun 06, 2013 at 04:28:07PM +0900, Alexandre Courbot wrote:
+static int __attribute__((used)) __tegra_smc_stack[10];
+
+/*
+ * With EABI, subtype and arg already end up in r0, r1 and r2 as they are
+ * function arguments, but we prefer to play safe here and explicitly move
+ * these values into the expected registers anyway. mov instructions without
+ * any side-effect are turned into nops by the assembler, which limits
+ * overhead.
No they aren't. They will be preserved as:
mov r0, r0
mov r1, r1
mov r2, r2
I'm pretty sure I checked with objdump and saw these replaced by nops at
some point, but for some reason I cannot get that behavior again. So
simply put, my statement is wrong indeed.
Moreover, things will go wrong if the compiler decides for whatever reason
to move 'arg' into r0 before calling your code sequence. So really this
is quite fragile.
There's also no point in mentioning EABI in the above paragraph; all ARM
ABIs under Linux have always placed the arguments in r0..r3 and then on
the stack. You can assert that this is always true by using the __asmeq()
macro.
Good to know, thanks.
Also...
+ */
+static void tegra_generic_smc(u32 type, u32 subtype, u32 arg)
+{
+ asm volatile(
+ .arch_extension sec\n\t
+ ldr r3, =__tegra_smc_stack\n\t
+ stmia r3, {r4-r12, lr}\n\t
using a statically allocated area to save the registers in this way is
probably a bad move; although the hotplug code guarantees that there
will be no concurrency between CPU hotplug operations, this sets a
precident for it being used in places where no such protection exists.
Indeed. This function will be called from other places in the future,
and for these we cannot assume there will be no concurrency.
What is wrong with stacking r4-r12, lr onto the SVC stack?
Nothing, actually. /o\
You don't
save the SVC stack pointer, so one can only assume that your SMC
implmentation preserves this (if it doesn't, that's yet another bug
with this assembly code.)
SVC stack pointer is ok AFAICT.
Combining these two issues, you're probably far better off using an
__attribute__((naked)) function for this - which means you get to
write the entire function in assembly code without any compiler
interference:
static void __attribute__((naked)) tegra_generic_smc(u32 type, u32 subtype, u32
arg)
{
asm volatile(
.arch_extension sec\n\t
stmfd sp!, {r4 - r12, lr}\n\t
__asmeq(%0, r0)
__asmeq(%1, r1)
__asmeq(%2, r2)
mov r3, #0\n\t
mov r4, #0\n\t
dsb\n\t
smc #0\n\t
ldmfd sp!, {r4 - r12, pc}
:
: r (type), r (subtype), r (arg)
: memory);
}
Well, that works beautifully indeed, and results in a much smaller
function that does nothing more beyond what's needed. On top of that, I
feel enlightened.
Thanks, I will resubmit a fixed version soon.
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: tegra: add basic SecureOS support
Hi Tomasz,
On 06/06/2013 07:17 PM, Tomasz Figa wrote:
+Global properties
+---
+
+The following properties can be specified into the chosen root
+node:
+
+ nvidia,secure-os: enable SecureOS.
Hmm, on Exynos we had something like
firmware@0203F000 {
compatible = samsung,secure-firmware;
reg = 0x0203F000 0x1000;
};
but your solution might be actually the proper one, since firmware is not
a hardware block. (The address in reg property is pointing to SYSRAM
memory, which is an additional communication channel with the firmware.)
Yes, I saw your implementation but decided to do it through the chosen
node anyway, since that's what it seems to be designed and we don't need
any reg parameter.
I think this patch could be split into several patches:
- add support for firmware
- split reset function
- add reset support using firmware.
Mmm possibly yes, but I wonder if that would not be too much splitting.
Stephen?
Hmm, I wonder if you need all this complexity here. Have a look at our
exynos_smc function
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-exynos/exynos-smc.S?id=refs/tags/next-20130606
Yes, I just embarrassed myself showing my ignorance of ARM assembler. ;)
The fix Russel proposed is pretty close to your version.
+static const struct firmware_ops tegra_firmware_ops = {
+ .set_cpu_boot_addr = tegra_set_cpu_boot_addr,
+};
It's good that this interface is finally getting some user besides Exynos.
I didn't know about it first but Joseph kindly pointed it out to me and
it indeed makes it easier to implement this.
Thanks,
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] Removal of GENERIC_GPIO
Hi Grant, Here is the pull request for the GENERIC_GPIO removal. It is almost certain that a few fixups will be necessary - while I don't have precise patches, the following steps should ensure the state of the code is clean: * "git grep CONFIG_GENERIC_GPIO" should return 0 hits. Matches should be replaced with CONFIG_GPIOLIB * "git grep '\bGENERIC_GPIO\b'" should return 1 hit in the Chinese documentation. Selectors of GENERIC_GPIO should be turned into selectors of GPIOLIB, while definitions of the option in architecture code should be deleted. Please let me know if there is something more I can do to ease your task. Thanks, Alex. The following changes since commit 10b38669d64c757cfd927e3820292c580ed70aae: Merge tag 'for-linus-v3.9-rc4' of git://oss.sgi.com/xfs/xfs (2013-03-19 15:17:40 -0700) are available in the git repository at: git://github.com/Gnurou/linux.git for_grant for you to fetch changes up to f4c54050640e7afa4749875cf9b900d42db361c0: gpio: update gpio Chinese documentation (2013-04-16 18:47:22 +0900) Alexandre Courbot (18): mips: remove redundant GENERIC_GPIO select mips: loongson: use GPIO driver on CONFIG_GPIOLIB mips: txx9: change GENERIC_GPIO to GPIOLIB mips: alchemy: require gpiolib arm: remove redundant GENERIC_GPIO selection arm: plat-orion: use GPIO driver on CONFIG_GPIOLIB unicore32: remove unneeded select GENERIC_GPIO unicore32: default GENERIC_GPIO to false powerpc: remove redundant GENERIC_GPIO selection sh: replace CONFIG_GENERIC_GPIO by CONFIG_GPIOLIB xtensa: remove explicit selection of GENERIC_GPIO avr32: default GENERIC_GPIO to false openrisc: default GENERIC_GPIO to false mips: pnx833x: remove requirement for GENERIC_GPIO m68k: coldfire: use gpiolib blackfin: force use of gpiolib Convert selectors of GENERIC_GPIO to GPIOLIB Remove GENERIC_GPIO config option Chen Baozi (1): gpio: update gpio Chinese documentation Documentation/gpio.txt | 10 +- Documentation/zh_CN/gpio.txt | 8 arch/alpha/Kconfig | 3 --- arch/arm/Kconfig | 3 --- arch/arm/plat-orion/Makefile | 2 +- arch/arm/plat-orion/gpio.c | 2 +- arch/arm64/Kconfig | 3 --- arch/avr32/Kconfig | 3 --- arch/blackfin/Kconfig| 5 + arch/hexagon/Kconfig | 3 --- arch/ia64/Kconfig| 3 --- arch/m68k/Kconfig| 3 --- arch/m68k/Kconfig.cpu| 3 +-- arch/metag/Kconfig | 3 --- arch/microblaze/Kconfig | 3 --- arch/mips/Kconfig| 10 +- arch/mips/loongson/common/Makefile | 2 +- arch/mips/txx9/generic/setup.c | 2 +- arch/openrisc/Kconfig| 3 --- arch/powerpc/Kconfig | 5 - arch/powerpc/platforms/40x/Kconfig | 1 - arch/powerpc/platforms/44x/Kconfig | 1 - arch/powerpc/platforms/85xx/Kconfig | 1 - arch/powerpc/platforms/86xx/Kconfig | 3 --- arch/powerpc/platforms/8xx/Kconfig | 1 - arch/powerpc/platforms/Kconfig | 4 arch/sh/Kconfig | 3 --- arch/sh/boards/mach-sdk7786/Makefile | 2 +- arch/sh/boards/mach-x3proto/Makefile | 2 +- arch/sh/kernel/cpu/sh2a/Makefile | 2 +- arch/sh/kernel/cpu/sh3/Makefile | 2 +- arch/sh/kernel/cpu/sh4a/Makefile | 2 +- arch/sparc/Kconfig | 5 - arch/unicore32/Kconfig | 6 +- arch/x86/Kconfig | 3 --- arch/xtensa/Kconfig | 3 --- arch/xtensa/configs/iss_defconfig| 1 - arch/xtensa/configs/s6105_defconfig | 1 - drivers/extcon/Kconfig | 2 +- drivers/gpio/Kconfig | 1 - drivers/gpio/gpio-lpc32xx.c | 2 +- drivers/i2c/busses/Kconfig | 4 ++-- drivers/i2c/muxes/Kconfig| 2 +- drivers/input/keyboard/Kconfig | 6 +++--- drivers/input/misc/Kconfig | 8 drivers/input/mouse/Kconfig | 2 +- drivers/leds/Kconfig | 6 +++--- drivers/mtd/maps/Kconfig | 2 +- drivers/mtd/nand/Kconfig | 2 +- drivers/net/phy/Kconfig | 2 +- drivers/pinctrl/sh-pfc/Kconfig | 26 +- drivers/regulator/Kconfig| 2 +- drivers/spi/Kconfig | 8 drivers/staging/android/Kconfig | 2 +- drivers/staging/iio/accel/Kconfig| 2 +- drivers/staging/iio/adc/Kconfig | 2 +- drivers/staging/iio/addac/Kconfig| 2 +- drivers/staging/iio/resolver/Kconfig | 4 ++-- drivers/staging/iio/trigger/Kconfig | 2 +- drivers/usb/otg/Kconfig | 2 +- drivers/video/Kconfig| 2 +-
[GIT PULL] Removal of GENERIC_GPIO
Hi Grant, Here is the pull request for the GENERIC_GPIO removal. It is almost certain that a few fixups will be necessary - while I don't have precise patches, the following steps should ensure the state of the code is clean: * git grep CONFIG_GENERIC_GPIO should return 0 hits. Matches should be replaced with CONFIG_GPIOLIB * git grep '\bGENERIC_GPIO\b' should return 1 hit in the Chinese documentation. Selectors of GENERIC_GPIO should be turned into selectors of GPIOLIB, while definitions of the option in architecture code should be deleted. Please let me know if there is something more I can do to ease your task. Thanks, Alex. The following changes since commit 10b38669d64c757cfd927e3820292c580ed70aae: Merge tag 'for-linus-v3.9-rc4' of git://oss.sgi.com/xfs/xfs (2013-03-19 15:17:40 -0700) are available in the git repository at: git://github.com/Gnurou/linux.git for_grant for you to fetch changes up to f4c54050640e7afa4749875cf9b900d42db361c0: gpio: update gpio Chinese documentation (2013-04-16 18:47:22 +0900) Alexandre Courbot (18): mips: remove redundant GENERIC_GPIO select mips: loongson: use GPIO driver on CONFIG_GPIOLIB mips: txx9: change GENERIC_GPIO to GPIOLIB mips: alchemy: require gpiolib arm: remove redundant GENERIC_GPIO selection arm: plat-orion: use GPIO driver on CONFIG_GPIOLIB unicore32: remove unneeded select GENERIC_GPIO unicore32: default GENERIC_GPIO to false powerpc: remove redundant GENERIC_GPIO selection sh: replace CONFIG_GENERIC_GPIO by CONFIG_GPIOLIB xtensa: remove explicit selection of GENERIC_GPIO avr32: default GENERIC_GPIO to false openrisc: default GENERIC_GPIO to false mips: pnx833x: remove requirement for GENERIC_GPIO m68k: coldfire: use gpiolib blackfin: force use of gpiolib Convert selectors of GENERIC_GPIO to GPIOLIB Remove GENERIC_GPIO config option Chen Baozi (1): gpio: update gpio Chinese documentation Documentation/gpio.txt | 10 +- Documentation/zh_CN/gpio.txt | 8 arch/alpha/Kconfig | 3 --- arch/arm/Kconfig | 3 --- arch/arm/plat-orion/Makefile | 2 +- arch/arm/plat-orion/gpio.c | 2 +- arch/arm64/Kconfig | 3 --- arch/avr32/Kconfig | 3 --- arch/blackfin/Kconfig| 5 + arch/hexagon/Kconfig | 3 --- arch/ia64/Kconfig| 3 --- arch/m68k/Kconfig| 3 --- arch/m68k/Kconfig.cpu| 3 +-- arch/metag/Kconfig | 3 --- arch/microblaze/Kconfig | 3 --- arch/mips/Kconfig| 10 +- arch/mips/loongson/common/Makefile | 2 +- arch/mips/txx9/generic/setup.c | 2 +- arch/openrisc/Kconfig| 3 --- arch/powerpc/Kconfig | 5 - arch/powerpc/platforms/40x/Kconfig | 1 - arch/powerpc/platforms/44x/Kconfig | 1 - arch/powerpc/platforms/85xx/Kconfig | 1 - arch/powerpc/platforms/86xx/Kconfig | 3 --- arch/powerpc/platforms/8xx/Kconfig | 1 - arch/powerpc/platforms/Kconfig | 4 arch/sh/Kconfig | 3 --- arch/sh/boards/mach-sdk7786/Makefile | 2 +- arch/sh/boards/mach-x3proto/Makefile | 2 +- arch/sh/kernel/cpu/sh2a/Makefile | 2 +- arch/sh/kernel/cpu/sh3/Makefile | 2 +- arch/sh/kernel/cpu/sh4a/Makefile | 2 +- arch/sparc/Kconfig | 5 - arch/unicore32/Kconfig | 6 +- arch/x86/Kconfig | 3 --- arch/xtensa/Kconfig | 3 --- arch/xtensa/configs/iss_defconfig| 1 - arch/xtensa/configs/s6105_defconfig | 1 - drivers/extcon/Kconfig | 2 +- drivers/gpio/Kconfig | 1 - drivers/gpio/gpio-lpc32xx.c | 2 +- drivers/i2c/busses/Kconfig | 4 ++-- drivers/i2c/muxes/Kconfig| 2 +- drivers/input/keyboard/Kconfig | 6 +++--- drivers/input/misc/Kconfig | 8 drivers/input/mouse/Kconfig | 2 +- drivers/leds/Kconfig | 6 +++--- drivers/mtd/maps/Kconfig | 2 +- drivers/mtd/nand/Kconfig | 2 +- drivers/net/phy/Kconfig | 2 +- drivers/pinctrl/sh-pfc/Kconfig | 26 +- drivers/regulator/Kconfig| 2 +- drivers/spi/Kconfig | 8 drivers/staging/android/Kconfig | 2 +- drivers/staging/iio/accel/Kconfig| 2 +- drivers/staging/iio/adc/Kconfig | 2 +- drivers/staging/iio/addac/Kconfig| 2 +- drivers/staging/iio/resolver/Kconfig | 4 ++-- drivers/staging/iio/trigger/Kconfig | 2 +- drivers/usb/otg/Kconfig | 2 +- drivers/video/Kconfig| 2 +-
Re: [PATCH 0/3] gpio: remove GENERIC_GPIO completely
On 03/29/2013 06:11 AM, Alexandre Courbot wrote: If I can get a few acks on these (or at least the first two ones) I'd like to include them into my next branch as soon as possible so points of breakage can be fixed. There are indeed a few new users of GENERIC_GPIO (CC Romain, I sent a warning but saw no action so far) in the next tree and compilation will break for them. Alternatively, Grant may want to pull all the patches I did so far - they are in next for more than 10 days now and no complain has been raised. Also it's probably better to keep all GPIO-related changes into the same tree. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/3] gpio: remove GENERIC_GPIO completely
On 03/29/2013 06:11 AM, Alexandre Courbot wrote: If I can get a few acks on these (or at least the first two ones) I'd like to include them into my next branch as soon as possible so points of breakage can be fixed. There are indeed a few new users of GENERIC_GPIO (CC Romain, I sent a warning but saw no action so far) in the next tree and compilation will break for them. Alternatively, Grant may want to pull all the patches I did so far - they are in next for more than 10 days now and no complain has been raised. Also it's probably better to keep all GPIO-related changes into the same tree. Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 09/17] sh: replace CONFIG_GENERIC_GPIO by CONFIG_GPIOLIB
On 03/12/2013 07:35 PM, Paul Mundt wrote: On Tue, Mar 12, 2013 at 07:12:22PM +0900, Alexandre Courbot wrote: SH GPIO drivers all use gpiolib and CONFIG_GENERIC_GPIO is only selected through CONFIG_GPIOLIB, yet some compilation units depended on CONFIG_GENERIC_GPIO. Make them depend on CONFIG_GPIOLIB instead since it is more accurate and prepares us for the future removal of CONFIG_GENERIC_GPIO. Signed-off-by: Alexandre Courbot Note that the bulk of the GENERIC_GPIO use for SH has shifted to drivers/pinctrl/sh-pfc/. If GPIOLIB is forced then a good chunk of the Kconfig/Makefile bits there ought to be refactored too. Yes, that is the case for many other drivers actually. This series just makes sure that GENERIC_GPIO is set through GPIOLIB *only*, thus making both options equivalent. If nobody shoots me for this, I will refactor individual drivers and ultimately remove GENERIC_GPIO completely. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 09/17] sh: replace CONFIG_GENERIC_GPIO by CONFIG_GPIOLIB
On 03/12/2013 07:35 PM, Paul Mundt wrote: On Tue, Mar 12, 2013 at 07:12:22PM +0900, Alexandre Courbot wrote: SH GPIO drivers all use gpiolib and CONFIG_GENERIC_GPIO is only selected through CONFIG_GPIOLIB, yet some compilation units depended on CONFIG_GENERIC_GPIO. Make them depend on CONFIG_GPIOLIB instead since it is more accurate and prepares us for the future removal of CONFIG_GENERIC_GPIO. Signed-off-by: Alexandre Courbot acour...@nvidia.com Note that the bulk of the GENERIC_GPIO use for SH has shifted to drivers/pinctrl/sh-pfc/. If GPIOLIB is forced then a good chunk of the Kconfig/Makefile bits there ought to be refactored too. Yes, that is the case for many other drivers actually. This series just makes sure that GENERIC_GPIO is set through GPIOLIB *only*, thus making both options equivalent. If nobody shoots me for this, I will refactor individual drivers and ultimately remove GENERIC_GPIO completely. Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 v5] pwm_bl: Add support for backlight enable regulator
On 03/08/2013 06:16 AM, Andrew Chew wrote:
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew
---
Renamed en_supply to enable_supply to match the corresponding device tree
property.
Removed unnecessary check for pb->enable_supply validity. This supply
is mandatory, and probe will fail if it is not provided.
Renamed the tracking bool from en_supply_enabled to enabled so that it's
more generic. Encapsulated pwm_enable() and pwm_disable() calls into the
enabled check so that we never unnecessarily turn on or off the pwm if
it's already been turned on or off.
.../bindings/video/backlight/pwm-backlight.txt | 14 +
drivers/video/backlight/pwm_bl.c | 56
2 files changed, 60 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the "brightness-levels" property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = "regulator-fixed";
+regulator-name = "bl-en-supply";
+regulator-boot-on;
+gpio = <_gpio>;
+enable-active-high;
+ };
+
backlight {
compatible = "pwm-backlight";
pwms = < 0 500>;
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+ enable-supply = <_en>;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c517d4a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include
#include
#include
+#include
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ boolenabled;
+ struct regulator*enable_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,38 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ /* Bail if we are already enabled. */
+ if (pb->enabled)
+ return;
+
+ pwm_enable(pb->pwm);
+
+ if (regulator_enable(pb->enable_supply) != 0)
+ dev_warn(>dev, "Failed to enable regulator");
+
+ pb->enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ /* Bail if we are already disabled. */
+ if (!pb->enabled)
+ return;
+
+ if (regulator_disable(pb->enable_supply) != 0)
+ dev_warn(>dev, "Failed to disable regulator");
+
+ pwm_disable(pb->pwm);
+
+ pb->enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(>dev);
@@ -52,7 +87,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +101,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb->lth_brightness +
(duty_cycle * (pb->period - pb->lth_brightness) / max);
pwm_config(pb->pwm, duty_cycle, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_enable(bl);
}
if (pb->notify_after)
@@ -145,12 +180,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
- /*
-* TODO: Most users of this driver use a number of GPIOs to control
-* backlight power.
Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
On 03/08/2013 06:07 AM, Andrew Chew wrote: From: Thierry Reding [mailto:thierry.red...@avionic-design.de] Sent: Thursday, March 07, 2013 3:27 AM To: Alex Courbot Cc: Andrew Chew; linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator * PGP Signed by an unknown key On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote: On 03/07/2013 04:11 PM, Thierry Reding wrote: + boolen_supply_enabled; This boolean can be dropped. As discussed in a previous thread, the pwm-backlight driver shouldn't need to know about any other uses of the regulator. Sorry for being obstinate - but I'm still not convinced we can get rid of it. I checked the regulator code, and as you mentioned in the previous version, calls to regulator_enable() and regulator_disable() *must* be balanced in this driver. Without this variable we would call regulator_enable() every time pwm_backlight_enable() is called (and same thing when disabling). Now imagine the driver is asked to set the following intensities: 5, 12, then 0. You would have two calls to regulator_enable() but only one to regulator_disable(), which would result in the enable GPIO remaining active even though it would be shut down. Or I missed something obvious. The regulator must be enabled/disabled on transitions from/to 0, and AFAICT there is no way for this driver to detect them. Yes, that's true, but I don't think it should be solved for just this one regulator. Instead if we need to track the enable state we might as well track it for *any* resource so that the PWM isn't enabled/disabled twice either. That makes sense, but I'm confused due to previous comments. The most obvious way to do this seems to be to have a bool track the enable state. Do you still want me to do away with this bool? I can satisfy your very last comment by keeping the bool (renaming it to something more generic) and encapsulating the pwm_enable()/pwm_disable() call within. I think that's what Thierry meant, yes. I expect that if the changes are split up then the board-setup code changes need to be done prior to the driver change. Using the lookup tables should make this easy because they aren't tied to the platform data and can be added independently. The patches should probably go through the same subsystem tree to take care of the dependencies. Keeping everything in one patch would work too, but it's certainly more chaotic. Am I supposed to handle those patches? I'm concerned that I don't have hardware to test properly, but I can give it a shot if it's my responsibility. Yes, if you introduce incompatibilities you have the burden of performing the transition without breaking things at any single point of the git history. Since this is just about adding a dummy regulator, it should go fine even without testing. And in the event it does not, that's what linux-next is for. Make sure you also update the dts of current device tree users, as they will break, too. What I don't know is if you should update all users in one big patch, or instead provide one patch per platform changed. Maybe Thierry can provide some guidance here. Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
On 03/07/2013 04:11 PM, Thierry Reding wrote:
+ boolen_supply_enabled;
This boolean can be dropped. As discussed in a previous thread, the
pwm-backlight driver shouldn't need to know about any other uses of the
regulator.
Sorry for being obstinate - but I'm still not convinced we can get rid
of it. I checked the regulator code, and as you mentioned in the
previous version, calls to regulator_enable() and regulator_disable()
*must* be balanced in this driver.
Without this variable we would call regulator_enable() every time
pwm_backlight_enable() is called (and same thing when disabling). Now
imagine the driver is asked to set the following intensities: 5, 12,
then 0. You would have two calls to regulator_enable() but only one to
regulator_disable(), which would result in the enable GPIO remaining
active even though it would be shut down. Or I missed something obvious.
The regulator must be enabled/disabled on transitions from/to 0, and
AFAICT there is no way for this driver to detect them.
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ pwm_enable(pb->pwm);
+
+ if (pb->en_supply && !pb->en_supply_enabled) {
+ if (regulator_enable(pb->en_supply) != 0)
+ dev_warn(>dev, "Failed to enable regulator");
+ else
+ pb->en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(>dev);
+
+ if (pb->en_supply && pb->en_supply_enabled) {
+ if (regulator_disable(pb->en_supply) != 0)
+ dev_warn(>dev, "Failed to disable regulator");
+ else
+ pb->en_supply_enabled = false;
+ }
+
+ pwm_disable(pb->pwm);
+}
Alex already brought this up: shouldn't the sequences be reversed. That
is, when enabling the backlight, turn on the regulator first, then
enable the PWM. When disabling, disable the PWM first, then turn off the
regulator?
Actually the current sequence seems to make sense - the PWM is always
active when the enable GPIO is switched. If we do the contrary, we might
have a short time where the backlight is enabled without receiving
anything from the PWM. Don't think that would be serious, but the
current behavior is similar to e.g. panels which we enable only after a
signal is available.
@@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
pb->exit = data->exit;
pb->dev = >dev;
+ pb->en_supply = devm_regulator_get(>dev, "enable");
+ if (IS_ERR(pb->en_supply)) {
+ ret = PTR_ERR(pb->en_supply);
+ pb->en_supply = NULL;
+ goto err_alloc;
+ }
This effectively makes the regulator mandatory, so the board files that
use pwm-backlight need to be updated or otherwise will break.
Yes. Btw, should such changes go into the same patch? This seems
difficult to split without breaking things at some point.
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
On 03/08/2013 06:07 AM, Andrew Chew wrote: From: Thierry Reding [mailto:thierry.red...@avionic-design.de] Sent: Thursday, March 07, 2013 3:27 AM To: Alex Courbot Cc: Andrew Chew; linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator * PGP Signed by an unknown key On Thu, Mar 07, 2013 at 07:11:25PM +0900, Alex Courbot wrote: On 03/07/2013 04:11 PM, Thierry Reding wrote: + boolen_supply_enabled; This boolean can be dropped. As discussed in a previous thread, the pwm-backlight driver shouldn't need to know about any other uses of the regulator. Sorry for being obstinate - but I'm still not convinced we can get rid of it. I checked the regulator code, and as you mentioned in the previous version, calls to regulator_enable() and regulator_disable() *must* be balanced in this driver. Without this variable we would call regulator_enable() every time pwm_backlight_enable() is called (and same thing when disabling). Now imagine the driver is asked to set the following intensities: 5, 12, then 0. You would have two calls to regulator_enable() but only one to regulator_disable(), which would result in the enable GPIO remaining active even though it would be shut down. Or I missed something obvious. The regulator must be enabled/disabled on transitions from/to 0, and AFAICT there is no way for this driver to detect them. Yes, that's true, but I don't think it should be solved for just this one regulator. Instead if we need to track the enable state we might as well track it for *any* resource so that the PWM isn't enabled/disabled twice either. That makes sense, but I'm confused due to previous comments. The most obvious way to do this seems to be to have a bool track the enable state. Do you still want me to do away with this bool? I can satisfy your very last comment by keeping the bool (renaming it to something more generic) and encapsulating the pwm_enable()/pwm_disable() call within. I think that's what Thierry meant, yes. I expect that if the changes are split up then the board-setup code changes need to be done prior to the driver change. Using the lookup tables should make this easy because they aren't tied to the platform data and can be added independently. The patches should probably go through the same subsystem tree to take care of the dependencies. Keeping everything in one patch would work too, but it's certainly more chaotic. Am I supposed to handle those patches? I'm concerned that I don't have hardware to test properly, but I can give it a shot if it's my responsibility. Yes, if you introduce incompatibilities you have the burden of performing the transition without breaking things at any single point of the git history. Since this is just about adding a dummy regulator, it should go fine even without testing. And in the event it does not, that's what linux-next is for. Make sure you also update the dts of current device tree users, as they will break, too. What I don't know is if you should update all users in one big patch, or instead provide one patch per platform changed. Maybe Thierry can provide some guidance here. Alex. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 v5] pwm_bl: Add support for backlight enable regulator
On 03/08/2013 06:16 AM, Andrew Chew wrote:
The backlight enable regulator is specified in the device tree node for
backlight.
Signed-off-by: Andrew Chew ac...@nvidia.com
---
Renamed en_supply to enable_supply to match the corresponding device tree
property.
Removed unnecessary check for pb-enable_supply validity. This supply
is mandatory, and probe will fail if it is not provided.
Renamed the tracking bool from en_supply_enabled to enabled so that it's
more generic. Encapsulated pwm_enable() and pwm_disable() calls into the
enabled check so that we never unnecessarily turn on or off the pwm if
it's already been turned on or off.
.../bindings/video/backlight/pwm-backlight.txt | 14 +
drivers/video/backlight/pwm_bl.c | 56
2 files changed, 60 insertions(+), 10 deletions(-)
diff --git
a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
index 1e4fc72..7e2e089 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
@@ -10,6 +10,11 @@ Required properties:
last value in the array represents a 100% duty cycle (brightest).
- default-brightness-level: the default brightness level (index into the
array defined by the brightness-levels property)
+ - enable-supply: A phandle to the regulator device tree node. This
+ regulator will be turned on and off as the pwm is enabled and disabled.
+ Many backlights are enabled via a GPIO. In this case, we instantiate
+ a fixed regulator and give that to enable-supply. If a regulator
+ is not needed, then provide a dummy fixed regulator.
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
@@ -19,10 +24,19 @@ Optional properties:
Example:
+ bl_en: fixed-regulator {
+compatible = regulator-fixed;
+regulator-name = bl-en-supply;
+regulator-boot-on;
+gpio = some_gpio;
+enable-active-high;
+ };
+
backlight {
compatible = pwm-backlight;
pwms = pwm 0 500;
brightness-levels = 0 4 8 16 32 64 128 255;
default-brightness-level = 6;
+ enable-supply = bl_en;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 069983c..c517d4a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,10 +20,13 @@
#include linux/pwm.h
#include linux/pwm_backlight.h
#include linux/slab.h
+#include linux/regulator/consumer.h
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ boolenabled;
+ struct regulator*enable_supply;
unsigned intperiod;
unsigned intlth_brightness;
unsigned int*levels;
@@ -35,6 +38,38 @@ struct pwm_bl_data {
void(*exit)(struct device *);
};
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ /* Bail if we are already enabled. */
+ if (pb-enabled)
+ return;
+
+ pwm_enable(pb-pwm);
+
+ if (regulator_enable(pb-enable_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+
+ pb-enabled = true;
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ /* Bail if we are already disabled. */
+ if (!pb-enabled)
+ return;
+
+ if (regulator_disable(pb-enable_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+
+ pwm_disable(pb-pwm);
+
+ pb-enabled = false;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
@@ -52,7 +87,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
if (brightness == 0) {
pwm_config(pb-pwm, 0, pb-period);
- pwm_disable(pb-pwm);
+ pwm_backlight_disable(bl);
} else {
int duty_cycle;
@@ -66,7 +101,7 @@ static int pwm_backlight_update_status(struct
backlight_device *bl)
duty_cycle = pb-lth_brightness +
(duty_cycle * (pb-period - pb-lth_brightness) / max);
pwm_config(pb-pwm, duty_cycle, pb-period);
- pwm_enable(pb-pwm);
+ pwm_backlight_enable(bl);
}
if (pb-notify_after)
@@ -145,12 +180,6 @@ static int pwm_backlight_parse_dt(struct device *dev,
data-max_brightness--;
}
- /*
-* TODO: Most users of this driver use
Re: [PATCH 1/1 v4] pwm_bl: Add support for backlight enable regulator
On 03/07/2013 04:11 PM, Thierry Reding wrote:
+ boolen_supply_enabled;
This boolean can be dropped. As discussed in a previous thread, the
pwm-backlight driver shouldn't need to know about any other uses of the
regulator.
Sorry for being obstinate - but I'm still not convinced we can get rid
of it. I checked the regulator code, and as you mentioned in the
previous version, calls to regulator_enable() and regulator_disable()
*must* be balanced in this driver.
Without this variable we would call regulator_enable() every time
pwm_backlight_enable() is called (and same thing when disabling). Now
imagine the driver is asked to set the following intensities: 5, 12,
then 0. You would have two calls to regulator_enable() but only one to
regulator_disable(), which would result in the enable GPIO remaining
active even though it would be shut down. Or I missed something obvious.
The regulator must be enabled/disabled on transitions from/to 0, and
AFAICT there is no way for this driver to detect them.
+static void pwm_backlight_enable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ pwm_enable(pb-pwm);
+
+ if (pb-en_supply !pb-en_supply_enabled) {
+ if (regulator_enable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to enable regulator);
+ else
+ pb-en_supply_enabled = true;
+ }
+}
+
+static void pwm_backlight_disable(struct backlight_device *bl)
+{
+ struct pwm_bl_data *pb = dev_get_drvdata(bl-dev);
+
+ if (pb-en_supply pb-en_supply_enabled) {
+ if (regulator_disable(pb-en_supply) != 0)
+ dev_warn(bl-dev, Failed to disable regulator);
+ else
+ pb-en_supply_enabled = false;
+ }
+
+ pwm_disable(pb-pwm);
+}
Alex already brought this up: shouldn't the sequences be reversed. That
is, when enabling the backlight, turn on the regulator first, then
enable the PWM. When disabling, disable the PWM first, then turn off the
regulator?
Actually the current sequence seems to make sense - the PWM is always
active when the enable GPIO is switched. If we do the contrary, we might
have a short time where the backlight is enabled without receiving
anything from the PWM. Don't think that would be serious, but the
current behavior is similar to e.g. panels which we enable only after a
signal is available.
@@ -213,6 +238,13 @@ static int pwm_backlight_probe(struct platform_device
*pdev)
pb-exit = data-exit;
pb-dev = pdev-dev;
+ pb-en_supply = devm_regulator_get(pdev-dev, enable);
+ if (IS_ERR(pb-en_supply)) {
+ ret = PTR_ERR(pb-en_supply);
+ pb-en_supply = NULL;
+ goto err_alloc;
+ }
This effectively makes the regulator mandatory, so the board files that
use pwm-backlight need to be updated or otherwise will break.
Yes. Btw, should such changes go into the same patch? This seems
difficult to split without breaking things at some point.
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
On 03/06/2013 04:00 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Wed, Mar 06, 2013 at 01:53:27PM +0900, Alex Courbot wrote:
On 03/06/2013 11:41 AM, Andrew Chew wrote:
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
Couldn't you use regulator_is_enabled() and get rid of en_supply_enabled?
It would also ensure the driver performs correctly no matter what the initial
state of the regulator is.
Are you sure this works? I'm concerned about the (bizarre and unlikely) case
where this supply is shared with another driver, so I use en_supply_enabled
to track the state of the supply such that I can ignore that case.
You're right, consumers can share regulators and the calls to
enable/disable need to be balanced. Also there is no way to check
the intensity of the backlight prior to the change to detect a
transition, so I guess your approach is indeed the most appropriate
here.
I think the right thing to do here is just enable the regulator when
the pwm-backlight driver needs it. If it is shared with other devices
they'll have to do the same and the reference counting should only
disable the regulator when there are no users.
>
Tracking this via platform data won't work because platform data is
statically defined at compile time. So if indeed there was another user
of the regulator it enable/disable the regulator at any time and your
en_supply_enabled would be wrong.
Oh wait. I thought regulator_enable/disable calls needed to be balanced,
is that not the case? So every consumer receives a different regulator
handle in case of a shared regulator, which becomes disabled if all
handles are disabled? In that case yes, we won't have to bother about a
status variable here and balancing calls. Sorry for the confusion.
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 v3] pwm_bl: Add support for backlight enable regulator
On 03/06/2013 04:00 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Wed, Mar 06, 2013 at 01:53:27PM +0900, Alex Courbot wrote:
On 03/06/2013 11:41 AM, Andrew Chew wrote:
struct pwm_bl_data {
struct pwm_device *pwm;
struct device *dev;
+ struct regulator*en_supply;
+ boolen_supply_enabled;
Couldn't you use regulator_is_enabled() and get rid of en_supply_enabled?
It would also ensure the driver performs correctly no matter what the initial
state of the regulator is.
Are you sure this works? I'm concerned about the (bizarre and unlikely) case
where this supply is shared with another driver, so I use en_supply_enabled
to track the state of the supply such that I can ignore that case.
You're right, consumers can share regulators and the calls to
enable/disable need to be balanced. Also there is no way to check
the intensity of the backlight prior to the change to detect a
transition, so I guess your approach is indeed the most appropriate
here.
I think the right thing to do here is just enable the regulator when
the pwm-backlight driver needs it. If it is shared with other devices
they'll have to do the same and the reference counting should only
disable the regulator when there are no users.
Tracking this via platform data won't work because platform data is
statically defined at compile time. So if indeed there was another user
of the regulator it enable/disable the regulator at any time and your
en_supply_enabled would be wrong.
Oh wait. I thought regulator_enable/disable calls needed to be balanced,
is that not the case? So every consumer receives a different regulator
handle in case of a shared regulator, which becomes disabled if all
handles are disabled? In that case yes, we won't have to bother about a
status variable here and balancing calls. Sorry for the confusion.
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
