[U-Boot] [PATCH 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420
This patchset adds support for S2MPS11 pmic on SMDK5420
This patchset has dependency on Rajeshwari's base patchset: [V4] EXYNOS5420: Add SMDK5420 board support http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/170582
Also, for testing we need Naveen's i2c patchset as well: i2c: improve s3c24x0 with High-speed and new SYS_I2C framework support http://www.mail-archive.com/u-boot@lists.denx.de/msg122679.html
Leela Krishna Amudala (6): exynos: Use common pmic_reg_update() definition power: Explicitly select pmic device's bus FDT: Exynos5420: Add compatible srings for PMIC SMDK5420: S2MPS11: Adds the register settings for S2MPS11 exynos: Add a common DT based PMIC driver initialization config: SMDK5420: Enable S2MPS11 pmic
board/samsung/common/board.c | 40 ++++++----- drivers/power/pmic/Makefile | 1 + drivers/power/pmic/pmic_common.c | 97 +++++++++++++++++++++++++ drivers/power/power_core.c | 14 ++++ drivers/power/power_i2c.c | 81 +++++++++++++++++++-- include/configs/smdk5420.h | 4 ++ include/fdtdec.h | 1 + include/power/pmic.h | 35 +++++++++ include/power/s2mps11_pmic.h | 144 ++++++++++++++++++++++++++++++++++++++ lib/fdtdec.c | 1 + 10 files changed, 394 insertions(+), 24 deletions(-) create mode 100644 drivers/power/pmic/pmic_common.c create mode 100644 include/power/s2mps11_pmic.h
This function is used by different Exynos platforms, put it in the common file.
Signed-off-by: Vadim Bendebury vbendeb@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com --- board/samsung/common/board.c | 19 ------------------- drivers/power/power_i2c.c | 19 +++++++++++++++++++ include/power/pmic.h | 1 + 3 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c index ce85ddb..87ca9de 100644 --- a/board/samsung/common/board.c +++ b/board/samsung/common/board.c @@ -152,25 +152,6 @@ static int board_init_cros_ec_devices(const void *blob)
#if defined(CONFIG_POWER) #ifdef CONFIG_POWER_MAX77686 -static int pmic_reg_update(struct pmic *p, int reg, uint regval) -{ - u32 val; - int ret = 0; - - ret = pmic_reg_read(p, reg, &val); - if (ret) { - debug("%s: PMIC %d register read failed\n", __func__, reg); - return -1; - } - val |= regval; - ret = pmic_reg_write(p, reg, val); - if (ret) { - debug("%s: PMIC %d register write failed\n", __func__, reg); - return -1; - } - return 0; -} - static int max77686_init(void) { struct pmic *p; diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index ac76870..47c606f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -96,6 +96,25 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val) return 0; }
+int pmic_reg_update(struct pmic *p, int reg, uint regval) +{ + u32 val; + int ret = 0; + + ret = pmic_reg_read(p, reg, &val); + if (ret) { + debug("%s: PMIC %d register read failed\n", __func__, reg); + return -1; + } + val |= regval; + ret = pmic_reg_write(p, reg, val); + if (ret) { + debug("%s: PMIC %d register write failed\n", __func__, reg); + return -1; + } + return 0; +} + int pmic_probe(struct pmic *p) { i2c_set_bus_num(p->bus); diff --git a/include/power/pmic.h b/include/power/pmic.h index 0e7aa31..d17dbdc 100644 --- a/include/power/pmic.h +++ b/include/power/pmic.h @@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p); int pmic_reg_read(struct pmic *p, u32 reg, u32 *val); int pmic_reg_write(struct pmic *p, u32 reg, u32 val); int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on); +int pmic_reg_update(struct pmic *p, int reg, uint regval);
#define pmic_i2c_addr (p->hw.i2c.addr) #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
Hi Leela,
This function is used by different Exynos platforms, put it in the common file.
Signed-off-by: Vadim Bendebury vbendeb@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com
board/samsung/common/board.c | 19 ------------------- drivers/power/power_i2c.c | 19 +++++++++++++++++++ include/power/pmic.h | 1 + 3 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c index ce85ddb..87ca9de 100644 --- a/board/samsung/common/board.c +++ b/board/samsung/common/board.c @@ -152,25 +152,6 @@ static int board_init_cros_ec_devices(const void *blob) #if defined(CONFIG_POWER) #ifdef CONFIG_POWER_MAX77686 -static int pmic_reg_update(struct pmic *p, int reg, uint regval) -{
- u32 val;
- int ret = 0;
- ret = pmic_reg_read(p, reg, &val);
- if (ret) {
debug("%s: PMIC %d register read failed\n",__func__, reg);
return -1;- }
- val |= regval;
- ret = pmic_reg_write(p, reg, val);
- if (ret) {
debug("%s: PMIC %d register write failed\n",__func__, reg);
return -1;- }
- return 0;
-}
static int max77686_init(void) { struct pmic *p; diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index ac76870..47c606f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -96,6 +96,25 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val) return 0; }
+int pmic_reg_update(struct pmic *p, int reg, uint regval)
It seems like this function shall be added to pmic_core.c, since it uses the pmic_reg_{read|write}, which are defined for both, spi and i2c.
+{
- u32 val;
- int ret = 0;
- ret = pmic_reg_read(p, reg, &val);
- if (ret) {
debug("%s: PMIC %d register read failed\n",__func__, reg);
return -1;- }
- val |= regval;
- ret = pmic_reg_write(p, reg, val);
- if (ret) {
debug("%s: PMIC %d register write failed\n",__func__, reg);
return -1;- }
- return 0;
+}
int pmic_probe(struct pmic *p) { i2c_set_bus_num(p->bus); diff --git a/include/power/pmic.h b/include/power/pmic.h index 0e7aa31..d17dbdc 100644 --- a/include/power/pmic.h +++ b/include/power/pmic.h @@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p); int pmic_reg_read(struct pmic *p, u32 reg, u32 *val); int pmic_reg_write(struct pmic *p, u32 reg, u32 val); int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on); +int pmic_reg_update(struct pmic *p, int reg, uint regval);
#define pmic_i2c_addr (p->hw.i2c.addr) #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbin adurbin@chromium.org Signed-off-by: Simon Glass sjg@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com --- drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -16,9 +16,45 @@ #include <i2c.h> #include <compiler.h>
+static int pmic_select(struct pmic *p) +{ + int ret, old_bus; + + old_bus = i2c_get_bus_num(); + if (old_bus != p->bus) { + debug("%s: Select bus %d\n", __func__, p->bus); + ret = i2c_set_bus_num(p->bus); + if (ret) { + debug("%s: Cannot select pmic %s, err %d\n", + __func__, p->name, ret); + return -1; + } + } + + return old_bus; +} + +static int pmic_deselect(int old_bus) +{ + int ret; + + if (old_bus != i2c_get_bus_num()) { + ret = i2c_set_bus_num(old_bus); + debug("%s: Select bus %d\n", __func__, old_bus); + if (ret) { + debug("%s: Cannot restore i2c bus, err %d\n", + __func__, ret); + return -1; + } + } + + return 0; +} + int pmic_reg_write(struct pmic *p, u32 reg, u32 val) { unsigned char buf[4] = { 0 }; + int ret, old_bus;
if (check_reg(p, reg)) return -1; @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val) return -1; }
- if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num)) + old_bus = pmic_select(p); + if (old_bus < 0) return -1;
- return 0; + ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num); + pmic_deselect(old_bus); + return ret; }
int pmic_reg_read(struct pmic *p, u32 reg, u32 *val) { unsigned char buf[4] = { 0 }; u32 ret_val = 0; + int ret, old_bus;
if (check_reg(p, reg)) return -1;
- if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num)) + old_bus = pmic_select(p); + if (old_bus < 0) return -1;
+ ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num); + pmic_deselect(old_bus); + if (ret) + return ret; + switch (pmic_i2c_tx_num) { case 3: if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int pmic_reg_update(struct pmic *p, int reg, uint regval)
int pmic_probe(struct pmic *p) { - i2c_set_bus_num(p->bus); + int ret, old_bus; + + old_bus = pmic_select(p); + if (old_bus < 0) + return -1; debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name); - if (i2c_probe(pmic_i2c_addr)) { + ret = i2c_probe(pmic_i2c_addr); + pmic_deselect(old_bus); + if (ret) { printf("Can't find PMIC:%s\n", p->name); return -1; }
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbin adurbin@chromium.org Signed-off-by: Simon Glass sjg@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -16,9 +16,45 @@ #include <i2c.h> #include <compiler.h>
+static int pmic_select(struct pmic *p) +{
- int ret, old_bus;
- old_bus = i2c_get_bus_num();
- if (old_bus != p->bus) {
debug("%s: Select bus %d\n", __func__, p->bus);ret = i2c_set_bus_num(p->bus);if (ret) {debug("%s: Cannot select pmic %s, err %d\n",__func__, p->name, ret);return -1;}- }
- return old_bus;
+}
+static int pmic_deselect(int old_bus) +{
- int ret;
- if (old_bus != i2c_get_bus_num()) {
ret = i2c_set_bus_num(old_bus);debug("%s: Select bus %d\n", __func__, old_bus);if (ret) {debug("%s: Cannot restore i2c bus, err %d\n",__func__, ret);return -1;}- }
- return 0;
+}
int pmic_reg_write(struct pmic *p, u32 reg, u32 val) { unsigned char buf[4] = { 0 };
int ret, old_bus;
if (check_reg(p, reg)) return -1;
@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val) return -1; }
- if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
- old_bus = pmic_select(p);
- if (old_bus < 0) return -1;
- return 0;
- ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
I'm wondering if setting the bus before each, single i2c write (when we usually perform several writes to one device) will not be an overkill (we search the ll_entry_count linker list each time to find max i2c adapter names) ?
The i2c_set_bus_num() is now done at pmic_probe(), but this also introduces overkill for "probing" each device when we want to read from it.
As a side note - I would appreciate if you would add Stefano Babic and me on the Cc (as we are listed at e.g. power_core.c).
- pmic_deselect(old_bus);
- return ret;
}
int pmic_reg_read(struct pmic *p, u32 reg, u32 *val) { unsigned char buf[4] = { 0 }; u32 ret_val = 0;
int ret, old_bus;
if (check_reg(p, reg)) return -1;
- if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
old_bus = pmic_select(p);
if (old_bus < 0) return -1;
ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
pmic_deselect(old_bus);
if (ret)
return ret;switch (pmic_i2c_tx_num) { case 3: if (p->sensor_byte_order ==
PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int pmic_reg_update(struct pmic *p, int reg, uint regval) int pmic_probe(struct pmic *p) {
- i2c_set_bus_num(p->bus);
- int ret, old_bus;
- old_bus = pmic_select(p);
- if (old_bus < 0)
return -1;
- if (i2c_probe(pmic_i2c_addr)) {
- ret = i2c_probe(pmic_i2c_addr);
- pmic_deselect(old_bus);
- if (ret) { printf("Can't find PMIC:%s\n", p->name); return -1; }
Hello Lukasz,
Am 02.10.2013 17:11, schrieb Lukasz Majewski:
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbinadurbin@chromium.org Signed-off-by: Simon Glasssjg@chromium.org Signed-off-by: Leela Krishna Amudalal.krishna@samsung.com Reviewed-by: Doug Andersondianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -16,9 +16,45 @@ #include<i2c.h> #include<compiler.h>
+static int pmic_select(struct pmic *p) +{
- int ret, old_bus;
- old_bus = i2c_get_bus_num();
- if (old_bus != p->bus) {
debug("%s: Select bus %d\n", __func__, p->bus);ret = i2c_set_bus_num(p->bus);if (ret) {debug("%s: Cannot select pmic %s, err %d\n",__func__, p->name, ret);return -1;}- }
- return old_bus;
+}
+static int pmic_deselect(int old_bus) +{
- int ret;
- if (old_bus != i2c_get_bus_num()) {
ret = i2c_set_bus_num(old_bus);debug("%s: Select bus %d\n", __func__, old_bus);if (ret) {debug("%s: Cannot restore i2c bus, err %d\n",__func__, ret);return -1;}- }
- return 0;
+}
int pmic_reg_write(struct pmic *p, u32 reg, u32 val) { unsigned char buf[4] = { 0 };
int ret, old_bus;
if (check_reg(p, reg)) return -1;
@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val) return -1; }
- if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
- old_bus = pmic_select(p);
- if (old_bus< 0) return -1;
- return 0;
- ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
I'm wondering if setting the bus before each, single i2c write (when we usually perform several writes to one device) will not be an overkill (we search the ll_entry_count linker list each time to find max i2c adapter names) ?
Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be enough to detect the max adapter once ... but it is not a "search"... ll_entry_count() calculates the number ...
Looking in i2c_set_bus_num(), I think it can be optimized ... lets speaking code:
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index d1072e8..170423a 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void) */ int i2c_set_bus_num(unsigned int bus) { - int max = ll_entry_count(struct i2c_adapter, i2c); + int max; + + if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0)) + return 0;
- if (I2C_ADAPTER(bus) >= max) { - printf("Error, wrong i2c adapter %d max %d possible\n", - I2C_ADAPTER(bus), max); - return -2; - } #ifndef CONFIG_SYS_I2C_DIRECT_BUS if (bus >= CONFIG_SYS_NUM_I2C_BUSES) return -1; #endif
- if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0)) - return 0; + max = ll_entry_count(struct i2c_adapter, i2c); + if (I2C_ADAPTER(bus) >= max) { + printf("Error, wrong i2c adapter %d max %d possible\n", + I2C_ADAPTER(bus), max); + return -2; + }
#ifndef CONFIG_SYS_I2C_DIRECT_BUS i2c_mux_disconnet_all();
So, first check, if we are on the correct bus, and return immediately! What do you think?
Beside of that, pmic_select() does the check, if we are on the correct bus too, and calls i2c_set_bus_num() only, if not ... so this is here no problem ... but exactly I want to get rid of this code as it is in pmic_select() someday, when all i2c drivers converted to the new i2c framework. i2c_set_bus_num() should go static then in drivers/i2c/i2c_core.c and i2c_read/write/... become a new "int bus" parameter ... but this will be a big api change ... but will prevent exactly such code all over the u-boot code ...
bye, Heiko
Hi Heiko,
Sorry for a late reply.
Hello Lukasz,
Am 02.10.2013 17:11, schrieb Lukasz Majewski:
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbinadurbin@chromium.org Signed-off-by: Simon Glasssjg@chromium.org Signed-off-by: Leela Krishna Amudalal.krishna@samsung.com Reviewed-by: Doug Andersondianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -16,9 +16,45 @@ #include<i2c.h> #include<compiler.h>
+static int pmic_select(struct pmic *p) +{
- int ret, old_bus;
- old_bus = i2c_get_bus_num();
- if (old_bus != p->bus) {
debug("%s: Select bus %d\n", __func__, p->bus);ret = i2c_set_bus_num(p->bus);if (ret) {debug("%s: Cannot select pmic %s, err%d\n",
__func__, p->name, ret);return -1;}- }
- return old_bus;
+}
+static int pmic_deselect(int old_bus) +{
- int ret;
- if (old_bus != i2c_get_bus_num()) {
ret = i2c_set_bus_num(old_bus);debug("%s: Select bus %d\n", __func__, old_bus);if (ret) {debug("%s: Cannot restore i2c bus, err%d\n",
__func__, ret);return -1;}- }
- return 0;
+}
int pmic_reg_write(struct pmic *p, u32 reg, u32 val) { unsigned char buf[4] = { 0 };
int ret, old_bus;
if (check_reg(p, reg)) return -1;
@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val) return -1; }
- if (i2c_write(pmic_i2c_addr, reg, 1, buf,
pmic_i2c_tx_num))
- old_bus = pmic_select(p);
- if (old_bus< 0) return -1;
- return 0;
- ret = i2c_write(pmic_i2c_addr, reg, 1, buf,
pmic_i2c_tx_num);
I'm wondering if setting the bus before each, single i2c write (when we usually perform several writes to one device) will not be an overkill (we search the ll_entry_count linker list each time to find max i2c adapter names) ?
Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be enough to detect the max adapter once ... but it is not a "search"... ll_entry_count() calculates the number ...
Yes, you are right. I've overlooked it.
With -Os compiler flag this compiles to a few ASM instructions. Obviously it is NOT a performance killer :-) (I made unnecessary fuzzz... sorry).
Looking in i2c_set_bus_num(), I think it can be optimized ... lets speaking code:
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index d1072e8..170423a 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void) */ int i2c_set_bus_num(unsigned int bus) {
int max = ll_entry_count(struct i2c_adapter, i2c);
int max;if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))return 0;
This looks nice.
if (I2C_ADAPTER(bus) >= max) {printf("Error, wrong i2c adapter %d max %dpossible\n",
I2C_ADAPTER(bus), max);return -2;}#ifndef CONFIG_SYS_I2C_DIRECT_BUS if (bus >= CONFIG_SYS_NUM_I2C_BUSES) return -1; #endif
if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))return 0;
max = ll_entry_count(struct i2c_adapter, i2c);if (I2C_ADAPTER(bus) >= max) {printf("Error, wrong i2c adapter %d max %dpossible\n",
I2C_ADAPTER(bus), max);return -2;}#ifndef CONFIG_SYS_I2C_DIRECT_BUS i2c_mux_disconnet_all();
So, first check, if we are on the correct bus, and return immediately! What do you think?
I think that it is acceptable.
Beside of that, pmic_select() does the check, if we are on the correct bus too, and calls i2c_set_bus_num() only, if not ... so this is here no problem ...
Yes, I see.
but exactly I want to get rid of this code as it is in pmic_select() someday, when all i2c drivers converted to the new i2c framework.
My 2 cents. I understand that pmic_select() preserves old i2c bus number, when PMIC performs transmission. This is probably done to not break the legacy code (where one driver assumed, that it is alone).
If this is necessary, then I'm OK with this. However I personally think, that drivers shall call API functions from i2c core (like i2c_bus_num()) only with bus number to switch and do not store and preserve the i2c value. This is my personal comment.
i2c_set_bus_num() should go static then in drivers/i2c/i2c_core.c and i2c_read/write/... become a new "int bus" parameter ... but this will be a big api change ... but will prevent exactly such code all over the u-boot code ...
bye, Heiko
Hello Lukasz,
Am 03.10.2013 18:15, schrieb Lukasz Majewski:
Hi Heiko,
Sorry for a late reply.
Hello Lukasz,
Am 02.10.2013 17:11, schrieb Lukasz Majewski:
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbinadurbin@chromium.org Signed-off-by: Simon Glasssjg@chromium.org Signed-off-by: Leela Krishna Amudalal.krishna@samsung.com Reviewed-by: Doug Andersondianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c
[...]
Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be enough to detect the max adapter once ... but it is not a "search"... ll_entry_count() calculates the number ...
Yes, you are right. I've overlooked it.
With -Os compiler flag this compiles to a few ASM instructions. Obviously it is NOT a performance killer :-) (I made unnecessary fuzzz... sorry).
No problem!
Looking in i2c_set_bus_num(), I think it can be optimized ... lets speaking code:
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index d1072e8..170423a 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void) */ int i2c_set_bus_num(unsigned int bus) {
int max = ll_entry_count(struct i2c_adapter, i2c);
int max;if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))return 0;This looks nice.
Ok! I post soon a patch for it ...
if (I2C_ADAPTER(bus)>= max) {printf("Error, wrong i2c adapter %d max %dpossible\n",
I2C_ADAPTER(bus), max);return -2;}#ifndef CONFIG_SYS_I2C_DIRECT_BUS if (bus>= CONFIG_SYS_NUM_I2C_BUSES) return -1; #endif
if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))return 0;
max = ll_entry_count(struct i2c_adapter, i2c);if (I2C_ADAPTER(bus)>= max) {printf("Error, wrong i2c adapter %d max %dpossible\n",
I2C_ADAPTER(bus), max);return -2;}#ifndef CONFIG_SYS_I2C_DIRECT_BUS i2c_mux_disconnet_all();
So, first check, if we are on the correct bus, and return immediately! What do you think?
I think that it is acceptable.
Good.
Beside of that, pmic_select() does the check, if we are on the correct bus too, and calls i2c_set_bus_num() only, if not ... so this is here no problem ...
Yes, I see.
but exactly I want to get rid of this code as it is in pmic_select() someday, when all i2c drivers converted to the new i2c framework.
My 2 cents. I understand that pmic_select() preserves old i2c bus number, when PMIC performs transmission. This is probably done to not break the legacy code (where one driver assumed, that it is alone).
If this is necessary, then I'm OK with this. However I personally think, that drivers shall call API functions from i2c core (like i2c_bus_num()) only with bus number to switch and do not store and preserve the i2c value. This is my personal comment.
Full Ack. I am just thinking, that we can get rid of such constructs, independent of the new i2c framework switch. We just need to introduce a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the current i2c bus where i2c commands are executed ... and all other subsystems, which use the i2c_api can call i2c_set_bus_num() without a previous "save old bus" and after the i2c bus usage a "restore i2c bus" ... I try to look into this, maybe we can do this before all i2c drivers are ported to the new framework ...
bye, Heiko
Hi Heiko,
Hello Lukasz,
Am 03.10.2013 18:15, schrieb Lukasz Majewski:
Hi Heiko,
Sorry for a late reply.
Hello Lukasz,
Am 02.10.2013 17:11, schrieb Lukasz Majewski:
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbinadurbin@chromium.org Signed-off-by: Simon Glasssjg@chromium.org Signed-off-by: Leela Krishna Amudalal.krishna@samsung.com Reviewed-by: Doug Andersondianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c
[...]
Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be enough to detect the max adapter once ... but it is not a "search"... ll_entry_count() calculates the number ...
Yes, you are right. I've overlooked it.
With -Os compiler flag this compiles to a few ASM instructions. Obviously it is NOT a performance killer :-) (I made unnecessary fuzzz... sorry).
No problem!
Looking in i2c_set_bus_num(), I think it can be optimized ... lets speaking code:
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index d1072e8..170423a 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void) */ int i2c_set_bus_num(unsigned int bus) {
int max = ll_entry_count(struct i2c_adapter, i2c);
int max;if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))return 0;This looks nice.
Ok! I post soon a patch for it ...
if (I2C_ADAPTER(bus)>= max) {printf("Error, wrong i2c adapter %d max %dpossible\n",
I2C_ADAPTER(bus), max);return -2;}#ifndef CONFIG_SYS_I2C_DIRECT_BUS if (bus>= CONFIG_SYS_NUM_I2C_BUSES) return -1; #endif
if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))return 0;
max = ll_entry_count(struct i2c_adapter, i2c);if (I2C_ADAPTER(bus)>= max) {printf("Error, wrong i2c adapter %d max %dpossible\n",
I2C_ADAPTER(bus), max);return -2;}#ifndef CONFIG_SYS_I2C_DIRECT_BUS i2c_mux_disconnet_all();
So, first check, if we are on the correct bus, and return immediately! What do you think?
I think that it is acceptable.
Good.
Beside of that, pmic_select() does the check, if we are on the correct bus too, and calls i2c_set_bus_num() only, if not ... so this is here no problem ...
Yes, I see.
but exactly I want to get rid of this code as it is in pmic_select() someday, when all i2c drivers converted to the new i2c framework.
My 2 cents. I understand that pmic_select() preserves old i2c bus number, when PMIC performs transmission. This is probably done to not break the legacy code (where one driver assumed, that it is alone).
If this is necessary, then I'm OK with this. However I personally think, that drivers shall call API functions from i2c core (like i2c_bus_num()) only with bus number to switch and do not store and preserve the i2c value. This is my personal comment.
Full Ack. I am just thinking, that we can get rid of such constructs, independent of the new i2c framework switch. We just need to introduce a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the current i2c bus where i2c commands are executed ...
I think that "last used bus" variable shall be stored/managed at i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with pmic and fuel gauge, which use different buses).
and all other subsystems, which use the i2c_api can call i2c_set_bus_num() without a previous "save old bus" and after the i2c bus usage a "restore i2c bus" ...
I try to look into this, maybe we can do this before all i2c drivers are ported to the new framework ...
Ok, lets wait for a patch.
bye, Heiko
Hello Lukasz,
Am 04.10.2013 10:58, schrieb Lukasz Majewski:
Hi Heiko,
Hello Lukasz,
Am 03.10.2013 18:15, schrieb Lukasz Majewski:
Hi Heiko,
Sorry for a late reply.
Hello Lukasz,
Am 02.10.2013 17:11, schrieb Lukasz Majewski:
Hi Leela,
[...]
but exactly I want to get rid of this code as it is in pmic_select() someday, when all i2c drivers converted to the new i2c framework.
My 2 cents. I understand that pmic_select() preserves old i2c bus number, when PMIC performs transmission. This is probably done to not break the legacy code (where one driver assumed, that it is alone).
If this is necessary, then I'm OK with this. However I personally think, that drivers shall call API functions from i2c core (like i2c_bus_num()) only with bus number to switch and do not store and preserve the i2c value. This is my personal comment.
Full Ack. I am just thinking, that we can get rid of such constructs, independent of the new i2c framework switch. We just need to introduce a "current_i2c_cmd_bus" in common/cmd_i2c.c. This var stores the current i2c bus where i2c commands are executed ...
I think that "last used bus" variable shall be stored/managed at i2c_core.c. I can use i2c without cmd_i2c.c compiled (as it is with
Thats the case for the new framework! It stores the current bus, and so i2c_set_bus_num() can decide, if it is necessary to switch to a new bus ...
Additionally to get rid of such "store oldbus, switch to new, restore old bus" constructs, the i2c commands needs to store somewhere, which i2c bus the i2c commands currently use, because you can do a "i2c md ..." then a "date" (maybe on antoher i2c bus) and then again a "i2c md ..." I think, thats the real historical reason for doing all over the code such "save oldbus" code ...
That var (saying i2c_cur_cmd_bus) can easily stored in common/cmd_i2c.c, as commands are only useable after relocation. So, we must just add a i2c_set_bus_num(i2c_cur_cmd_bus) call before a command is executed ... That can be done in common/cmd_i2c.c also ...
So, in the end, before every i2c access we call i2c_set_bus_num() and have no longer to store an "old bus" ... and if we have reached this point, we can make i2c_set_bus_num() static, and add to the i2c api a new first "int bus" parameter .... and can delete all i2c_set_bus_num() calls ... so every i2c device must know, on which i2c bus it works ... and as a bonus, we can get rid of all i2c_init() calls all over around the code, as i2c_set_bus_num() checks, if the drivers is initialized or not, if not initialize it, look if i2c muxes are involved on this bus, and so on ...
pmic and fuel gauge, which use different buses).
Yep, and its enough to call i2c_set_bus_num() before you want to access a device ... i2c_set_bus_num() do all necessary steps for you.
and all other subsystems, which use the i2c_api can call i2c_set_bus_num() without a previous "save old bus" and after the i2c bus usage a "restore i2c bus" ...
I try to look into this, maybe we can do this before all i2c drivers are ported to the new framework ...
Ok, lets wait for a patch.
I have such a patch in my queue, but it works only for drivers, which use the new framework I think ... so I must think about it, if this works for old style drivers too ...
bye, Heiko
Hello Lukasz,
Thanks for reviewing the patch.
On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbin adurbin@chromium.org Signed-off-by: Simon Glass sjg@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -16,9 +16,45 @@ #include <i2c.h> #include <compiler.h>
+static int pmic_select(struct pmic *p) +{
int ret, old_bus;old_bus = i2c_get_bus_num();if (old_bus != p->bus) {debug("%s: Select bus %d\n", __func__, p->bus);ret = i2c_set_bus_num(p->bus);if (ret) {debug("%s: Cannot select pmic %s, err %d\n",__func__, p->name, ret);return -1;}}return old_bus;+}
+static int pmic_deselect(int old_bus) +{
int ret;if (old_bus != i2c_get_bus_num()) {ret = i2c_set_bus_num(old_bus);debug("%s: Select bus %d\n", __func__, old_bus);if (ret) {debug("%s: Cannot restore i2c bus, err %d\n",__func__, ret);return -1;}}return 0;+}
int pmic_reg_write(struct pmic *p, u32 reg, u32 val) { unsigned char buf[4] = { 0 };
int ret, old_bus; if (check_reg(p, reg)) return -1;@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val) return -1; }
if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
old_bus = pmic_select(p);if (old_bus < 0) return -1;
return 0;
ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);I'm wondering if setting the bus before each, single i2c write (when we usually perform several writes to one device) will not be an overkill (we search the ll_entry_count linker list each time to find max i2c adapter names) ?
Here we are not setting the bus before each i2c write, If you look at the pmic_select() function code it shows like static int pmic_select(struct pmic *p) { /......./ old_bus = i2c_get_bus_num(); if (old_bus != p->bus) { /...../ } return old_bus; }
Here we are trying to get the bus number and if it is equal to the bus on which we are going to write we are returning immediately which is minimal.
The i2c_set_bus_num() is now done at pmic_probe(), but this also introduces overkill for "probing" each device when we want to read from it.
As a side note - I would appreciate if you would add Stefano Babic and me on the Cc (as we are listed at e.g. power_core.c).
Okay, will do it.
Best Wishes, Leela Krishna
pmic_deselect(old_bus);return ret;}
int pmic_reg_read(struct pmic *p, u32 reg, u32 *val) { unsigned char buf[4] = { 0 }; u32 ret_val = 0;
int ret, old_bus; if (check_reg(p, reg)) return -1;
if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
old_bus = pmic_select(p);if (old_bus < 0) return -1;ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);pmic_deselect(old_bus);if (ret)return ret;switch (pmic_i2c_tx_num) { case 3: if (p->sensor_byte_order ==PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int pmic_reg_update(struct pmic *p, int reg, uint regval) int pmic_probe(struct pmic *p) {
i2c_set_bus_num(p->bus);
int ret, old_bus;old_bus = pmic_select(p);if (old_bus < 0)return -1; debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
if (i2c_probe(pmic_i2c_addr)) {
ret = i2c_probe(pmic_i2c_addr);pmic_deselect(old_bus);if (ret) { printf("Can't find PMIC:%s\n", p->name); return -1; }-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Leela,
Hello Lukasz,
Thanks for reviewing the patch.
On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Leela,
The current pmic i2c code assumes the current i2c bus is the same as the pmic device's bus. There is nothing ensuring that to be true. Therefore, select the proper bus before performing a transaction.
Signed-off-by: Aaron Durbin adurbin@chromium.org Signed-off-by: Simon Glass sjg@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c index 47c606f..c22e01f 100644 --- a/drivers/power/power_i2c.c +++ b/drivers/power/power_i2c.c @@ -16,9 +16,45 @@ #include <i2c.h> #include <compiler.h>
+static int pmic_select(struct pmic *p) +{
int ret, old_bus;old_bus = i2c_get_bus_num();if (old_bus != p->bus) {debug("%s: Select bus %d\n", __func__, p->bus);ret = i2c_set_bus_num(p->bus);if (ret) {debug("%s: Cannot select pmic %s, err %d\n",__func__, p->name, ret);return -1;}}return old_bus;+}
+static int pmic_deselect(int old_bus) +{
int ret;if (old_bus != i2c_get_bus_num()) {ret = i2c_set_bus_num(old_bus);debug("%s: Select bus %d\n", __func__, old_bus);if (ret) {debug("%s: Cannot restore i2c bus, err %d\n",__func__, ret);return -1;}}return 0;+}
int pmic_reg_write(struct pmic *p, u32 reg, u32 val) { unsigned char buf[4] = { 0 };
int ret, old_bus; if (check_reg(p, reg)) return -1;@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val) return -1; }
if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
old_bus = pmic_select(p);if (old_bus < 0) return -1;
return 0;
ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);I'm wondering if setting the bus before each, single i2c write (when we usually perform several writes to one device) will not be an overkill (we search the ll_entry_count linker list each time to find max i2c adapter names) ?
Here we are not setting the bus before each i2c write, If you look at the pmic_select() function code it shows like static int pmic_select(struct pmic *p) { /......./ old_bus = i2c_get_bus_num(); if (old_bus != p->bus) { /...../ } return old_bus; }
Here we are trying to get the bus number and if it is equal to the bus on which we are going to write we are returning immediately which is minimal.
Thanks for explanation.
The i2c_set_bus_num() is now done at pmic_probe(), but this also introduces overkill for "probing" each device when we want to read from it.
As a side note - I would appreciate if you would add Stefano Babic and me on the Cc (as we are listed at e.g. power_core.c).
Okay, will do it.
Best Wishes, Leela Krishna
pmic_deselect(old_bus);return ret;}
int pmic_reg_read(struct pmic *p, u32 reg, u32 *val) { unsigned char buf[4] = { 0 }; u32 ret_val = 0;
int ret, old_bus; if (check_reg(p, reg)) return -1;
if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
old_bus = pmic_select(p);if (old_bus < 0) return -1;ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);pmic_deselect(old_bus);if (ret)return ret;switch (pmic_i2c_tx_num) { case 3: if (p->sensor_byte_order ==PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int pmic_reg_update(struct pmic *p, int reg, uint regval) int pmic_probe(struct pmic *p) {
i2c_set_bus_num(p->bus);
int ret, old_bus;old_bus = pmic_select(p);if (old_bus < 0)return -1; debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
if (i2c_probe(pmic_i2c_addr)) {
ret = i2c_probe(pmic_i2c_addr);pmic_deselect(old_bus);if (ret) { printf("Can't find PMIC:%s\n", p->name); return -1; }-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Add required compatible strings for PMIC S2MPS11
Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com --- include/fdtdec.h | 1 + lib/fdtdec.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/include/fdtdec.h b/include/fdtdec.h index 6bf83bf..6290078 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -85,6 +85,7 @@ enum fdt_compat_id { COMPAT_INFINEON_SLB9635_TPM, /* Infineon SLB9635 TPM */ COMPAT_INFINEON_SLB9645_TPM, /* Infineon SLB9645 TPM */ COMPAT_SAMSUNG_EXYNOS5_I2C, /* Exynos5 High Speed I2C Controller */ + COMPAT_SAMSUNG_S2MPS11_PMIC, /* S2MPS11 PMIC */
COMPAT_COUNT, }; diff --git a/lib/fdtdec.c b/lib/fdtdec.c index dc35856..0ea1c08 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -58,6 +58,7 @@ static const char * const compat_names[COMPAT_COUNT] = { COMPAT(INFINEON_SLB9635_TPM, "infineon,slb9635-tpm"), COMPAT(INFINEON_SLB9645_TPM, "infineon,slb9645-tpm"), COMPAT(SAMSUNG_EXYNOS5_I2C, "samsung,exynos5-hsi2c"), + COMPAT(SAMSUNG_S2MPS11_PMIC, "samsung,s2mps11-pmic"), };
const char *fdtdec_get_compatible(enum fdt_compat_id id)
Adds the register settings, addresses and voltages associated with S2MPS11
Signed-off-by: Alim Akhtar alim.akhtar@samsung.com Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Vadim Bendebury vbendeb@google.com --- include/power/s2mps11_pmic.h | 144 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 include/power/s2mps11_pmic.h
diff --git a/include/power/s2mps11_pmic.h b/include/power/s2mps11_pmic.h new file mode 100644 index 0000000..e4f21f9 --- /dev/null +++ b/include/power/s2mps11_pmic.h @@ -0,0 +1,144 @@ +/* + * s2mps11_pmic.h + * + * Copyright (c) 2012 Samsung Electronics Co., Ltd + * http://www.samsung.com + * + * 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. + * + */ +#ifndef __S2MPS11_H +#define __S2MPS11_H + +/* S2MPS11 registers */ +enum s2mps11_reg { + S2MPS11_REG_ID, + S2MPS11_REG_INT1, + S2MPS11_REG_INT2, + S2MPS11_REG_INT3, + S2MPS11_REG_INT1M, + S2MPS11_REG_INT2M, + S2MPS11_REG_INT3M, + S2MPS11_REG_ST1, + S2MPS11_REG_ST2, + S2MPS11_REG_OFFSRC, + S2MPS11_REG_PWRONSRC, + S2MPS11_REG_RTC_CTRL, + S2MPS11_REG_CTRL1, + S2MPS11_REG_ETC_TEST, + S2MPS11_REG_RSVD3, + S2MPS11_REG_BU_CHG, + S2MPS11_REG_RAMP, + S2MPS11_REG_RAMP_BUCK, + S2MPS11_REG_LDO1_8, + S2MPS11_REG_LDO9_16, + S2MPS11_REG_LDO17_24, + S2MPS11_REG_LDO25_32, + S2MPS11_REG_LDO33_38, + S2MPS11_REG_LDO1_8_1, + S2MPS11_REG_LDO9_16_1, + S2MPS11_REG_LDO17_24_1, + S2MPS11_REG_LDO25_32_1, + S2MPS11_REG_LDO33_38_1, + S2MPS11_REG_OTP_ADRL, + S2MPS11_REG_OTP_ADRH, + S2MPS11_REG_OTP_DATA, + S2MPS11_REG_MON1SEL, + S2MPS11_REG_MON2SEL, + S2MPS11_REG_LEE, + S2MPS11_REG_RSVD_NO, + S2MPS11_REG_UVLO, + S2MPS11_REG_LEE_NO, + S2MPS11_REG_B1CTRL1, + S2MPS11_REG_B1CTRL2, + S2MPS11_REG_B2CTRL1, + S2MPS11_REG_B2CTRL2, + S2MPS11_REG_B3CTRL1, + S2MPS11_REG_B3CTRL2, + S2MPS11_REG_B4CTRL1, + S2MPS11_REG_B4CTRL2, + S2MPS11_REG_B5CTRL1, + S2MPS11_REG_BUCK5_SW, + S2MPS11_REG_B5CTRL2, + S2MPS11_REG_B5CTRL3, + S2MPS11_REG_B5CTRL4, + S2MPS11_REG_B5CTRL5, + S2MPS11_REG_B6CTRL1, + S2MPS11_REG_B6CTRL2, + S2MPS11_REG_B7CTRL1, + S2MPS11_REG_B7CTRL2, + S2MPS11_REG_B8CTRL1, + S2MPS11_REG_B8CTRL2, + S2MPS11_REG_B9CTRL1, + S2MPS11_REG_B9CTRL2, + S2MPS11_REG_B10CTRL1, + S2MPS11_REG_B10CTRL2, + S2MPS11_REG_L1CTRL, + S2MPS11_REG_L2CTRL, + S2MPS11_REG_L3CTRL, + S2MPS11_REG_L4CTRL, + S2MPS11_REG_L5CTRL, + S2MPS11_REG_L6CTRL, + S2MPS11_REG_L7CTRL, + S2MPS11_REG_L8CTRL, + S2MPS11_REG_L9CTRL, + S2MPS11_REG_L10CTRL, + S2MPS11_REG_L11CTRL, + S2MPS11_REG_L12CTRL, + S2MPS11_REG_L13CTRL, + S2MPS11_REG_L14CTRL, + S2MPS11_REG_L15CTRL, + S2MPS11_REG_L16CTRL, + S2MPS11_REG_L17CTRL, + S2MPS11_REG_L18CTRL, + S2MPS11_REG_L19CTRL, + S2MPS11_REG_L20CTRL, + S2MPS11_REG_L21CTRL, + S2MPS11_REG_L22CTRL, + S2MPS11_REG_L23CTRL, + S2MPS11_REG_L24CTRL, + S2MPS11_REG_L25CTRL, + S2MPS11_REG_L26CTRL, + S2MPS11_REG_L27CTRL, + S2MPS11_REG_L28CTRL, + S2MPS11_REG_L29CTRL, + S2MPS11_REG_L30CTRL, + S2MPS11_REG_L31CTRL, + S2MPS11_REG_L32CTRL, + S2MPS11_REG_L33CTRL, + S2MPS11_REG_L34CTRL, + S2MPS11_REG_L35CTRL, + S2MPS11_REG_L36CTRL, + S2MPS11_REG_L37CTRL, + S2MPS11_REG_L38CTRL, + + S2MPS11_NUM_OF_REGS, +}; + +/* I2C device address for pmic S2MPS11 */ +#define S2MPS11_I2C_ADDR (0xCC >> 1) +#define S2MPS11_BUS_NUM 4 + +/* Value to set voltage as 1V */ +#define S2MPS11_BUCK_CTRL2_1V 0x40 +/* Value to set voltage as 1.2V */ +#define S2MPS11_BUCK_CTRL2_1_2V 0x60 +/* Value to set voltage as 1.2625V */ +#define S2MPS11_BUCK_CTRL2_1_2625V 0x6A + +/* Buck register addresses */ +#define S2MPS11_BUCK1_CTRL2 0x26 +#define S2MPS11_BUCK2_CTRL2 0x28 +#define S2MPS11_BUCK3_CTRL2 0x2a +#define S2MPS11_BUCK4_CTRL2 0x2c +#define S2MPS11_BUCK6_CTRL2 0x34 +#define S2MPS11_LDO22_CTRL 0x52 + +#define S2MPS11_DEVICE_NAME "S2MPS11_PMIC" + +#define S2MPS11_RTC_CTRL_32KHZ_CP_EN (1 << 1) +#define S2MPS11_RTC_CTRL_JIT (1 << 4) +#endif /* __LINUX_MFD_S2MPS11_H */
Hi Leela,
Adds the register settings, addresses and voltages associated with S2MPS11
Signed-off-by: Alim Akhtar alim.akhtar@samsung.com Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Vadim Bendebury vbendeb@google.com
include/power/s2mps11_pmic.h | 144 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 include/power/s2mps11_pmic.h
diff --git a/include/power/s2mps11_pmic.h b/include/power/s2mps11_pmic.h new file mode 100644 index 0000000..e4f21f9 --- /dev/null +++ b/include/power/s2mps11_pmic.h @@ -0,0 +1,144 @@ +/*
- s2mps11_pmic.h
- Copyright (c) 2012 Samsung Electronics Co., Ltd
http://www.samsung.com
- 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.
We have switched to SPDX license. Please write similar code to e.g. power_core.c
- */
+#ifndef __S2MPS11_H +#define __S2MPS11_H
+/* S2MPS11 registers */ +enum s2mps11_reg {
- S2MPS11_REG_ID,
- S2MPS11_REG_INT1,
- S2MPS11_REG_INT2,
- S2MPS11_REG_INT3,
- S2MPS11_REG_INT1M,
- S2MPS11_REG_INT2M,
- S2MPS11_REG_INT3M,
- S2MPS11_REG_ST1,
- S2MPS11_REG_ST2,
- S2MPS11_REG_OFFSRC,
- S2MPS11_REG_PWRONSRC,
- S2MPS11_REG_RTC_CTRL,
- S2MPS11_REG_CTRL1,
- S2MPS11_REG_ETC_TEST,
- S2MPS11_REG_RSVD3,
- S2MPS11_REG_BU_CHG,
- S2MPS11_REG_RAMP,
- S2MPS11_REG_RAMP_BUCK,
- S2MPS11_REG_LDO1_8,
- S2MPS11_REG_LDO9_16,
- S2MPS11_REG_LDO17_24,
- S2MPS11_REG_LDO25_32,
- S2MPS11_REG_LDO33_38,
- S2MPS11_REG_LDO1_8_1,
- S2MPS11_REG_LDO9_16_1,
- S2MPS11_REG_LDO17_24_1,
- S2MPS11_REG_LDO25_32_1,
- S2MPS11_REG_LDO33_38_1,
- S2MPS11_REG_OTP_ADRL,
- S2MPS11_REG_OTP_ADRH,
- S2MPS11_REG_OTP_DATA,
- S2MPS11_REG_MON1SEL,
- S2MPS11_REG_MON2SEL,
- S2MPS11_REG_LEE,
- S2MPS11_REG_RSVD_NO,
- S2MPS11_REG_UVLO,
- S2MPS11_REG_LEE_NO,
- S2MPS11_REG_B1CTRL1,
- S2MPS11_REG_B1CTRL2,
- S2MPS11_REG_B2CTRL1,
- S2MPS11_REG_B2CTRL2,
- S2MPS11_REG_B3CTRL1,
- S2MPS11_REG_B3CTRL2,
- S2MPS11_REG_B4CTRL1,
- S2MPS11_REG_B4CTRL2,
- S2MPS11_REG_B5CTRL1,
- S2MPS11_REG_BUCK5_SW,
- S2MPS11_REG_B5CTRL2,
- S2MPS11_REG_B5CTRL3,
- S2MPS11_REG_B5CTRL4,
- S2MPS11_REG_B5CTRL5,
- S2MPS11_REG_B6CTRL1,
- S2MPS11_REG_B6CTRL2,
- S2MPS11_REG_B7CTRL1,
- S2MPS11_REG_B7CTRL2,
- S2MPS11_REG_B8CTRL1,
- S2MPS11_REG_B8CTRL2,
- S2MPS11_REG_B9CTRL1,
- S2MPS11_REG_B9CTRL2,
- S2MPS11_REG_B10CTRL1,
- S2MPS11_REG_B10CTRL2,
- S2MPS11_REG_L1CTRL,
- S2MPS11_REG_L2CTRL,
- S2MPS11_REG_L3CTRL,
- S2MPS11_REG_L4CTRL,
- S2MPS11_REG_L5CTRL,
- S2MPS11_REG_L6CTRL,
- S2MPS11_REG_L7CTRL,
- S2MPS11_REG_L8CTRL,
- S2MPS11_REG_L9CTRL,
- S2MPS11_REG_L10CTRL,
- S2MPS11_REG_L11CTRL,
- S2MPS11_REG_L12CTRL,
- S2MPS11_REG_L13CTRL,
- S2MPS11_REG_L14CTRL,
- S2MPS11_REG_L15CTRL,
- S2MPS11_REG_L16CTRL,
- S2MPS11_REG_L17CTRL,
- S2MPS11_REG_L18CTRL,
- S2MPS11_REG_L19CTRL,
- S2MPS11_REG_L20CTRL,
- S2MPS11_REG_L21CTRL,
- S2MPS11_REG_L22CTRL,
- S2MPS11_REG_L23CTRL,
- S2MPS11_REG_L24CTRL,
- S2MPS11_REG_L25CTRL,
- S2MPS11_REG_L26CTRL,
- S2MPS11_REG_L27CTRL,
- S2MPS11_REG_L28CTRL,
- S2MPS11_REG_L29CTRL,
- S2MPS11_REG_L30CTRL,
- S2MPS11_REG_L31CTRL,
- S2MPS11_REG_L32CTRL,
- S2MPS11_REG_L33CTRL,
- S2MPS11_REG_L34CTRL,
- S2MPS11_REG_L35CTRL,
- S2MPS11_REG_L36CTRL,
- S2MPS11_REG_L37CTRL,
- S2MPS11_REG_L38CTRL,
- S2MPS11_NUM_OF_REGS,
+};
+/* I2C device address for pmic S2MPS11 */ +#define S2MPS11_I2C_ADDR (0xCC >> 1) +#define S2MPS11_BUS_NUM 4
+/* Value to set voltage as 1V */ +#define S2MPS11_BUCK_CTRL2_1V 0x40 +/* Value to set voltage as 1.2V */ +#define S2MPS11_BUCK_CTRL2_1_2V 0x60 +/* Value to set voltage as 1.2625V */ +#define S2MPS11_BUCK_CTRL2_1_2625V 0x6A
+/* Buck register addresses */ +#define S2MPS11_BUCK1_CTRL2 0x26 +#define S2MPS11_BUCK2_CTRL2 0x28 +#define S2MPS11_BUCK3_CTRL2 0x2a +#define S2MPS11_BUCK4_CTRL2 0x2c +#define S2MPS11_BUCK6_CTRL2 0x34 +#define S2MPS11_LDO22_CTRL 0x52
+#define S2MPS11_DEVICE_NAME "S2MPS11_PMIC"
+#define S2MPS11_RTC_CTRL_32KHZ_CP_EN (1 << 1) +#define S2MPS11_RTC_CTRL_JIT (1 << 4) +#endif /* __LINUX_MFD_S2MPS11_H */
Most of i2c PMIC drivers follow the same initialization sequence, let's generalize it in a common file.
The initialization function finds the PMIC in the device tree, and if found - registers it in the list of known PMICs and initializes it, iterating through the table of settings supplied by the caller.
Signed-off-by: Vadim Bendebury vbendeb@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com --- board/samsung/common/board.c | 23 ++++++++- drivers/power/pmic/Makefile | 1 + drivers/power/pmic/pmic_common.c | 97 ++++++++++++++++++++++++++++++++++++++ drivers/power/power_core.c | 14 ++++++ include/power/pmic.h | 34 +++++++++++++ 5 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 drivers/power/pmic/pmic_common.c
diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c index 87ca9de..561e270 100644 --- a/board/samsung/common/board.c +++ b/board/samsung/common/board.c @@ -19,6 +19,7 @@ #include <asm/arch/power.h> #include <power/pmic.h> #include <power/max77686_pmic.h> +#include <power/s2mps11_pmic.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -150,6 +151,25 @@ static int board_init_cros_ec_devices(const void *blob) } #endif
+#ifdef CONFIG_POWER_S2MPS11 +int board_init_s2mps11(void) +{ + const struct pmic_init_ops pmic_ops[] = { + {PMIC_REG_WRITE, S2MPS11_BUCK1_CTRL2, S2MPS11_BUCK_CTRL2_1V}, + {PMIC_REG_WRITE, S2MPS11_BUCK2_CTRL2, + S2MPS11_BUCK_CTRL2_1_2625V}, + {PMIC_REG_WRITE, S2MPS11_BUCK3_CTRL2, S2MPS11_BUCK_CTRL2_1V}, + {PMIC_REG_WRITE, S2MPS11_BUCK4_CTRL2, S2MPS11_BUCK_CTRL2_1V}, + {PMIC_REG_WRITE, S2MPS11_BUCK6_CTRL2, S2MPS11_BUCK_CTRL2_1V}, + {PMIC_REG_UPDATE, S2MPS11_REG_RTC_CTRL, + S2MPS11_RTC_CTRL_32KHZ_CP_EN | S2MPS11_RTC_CTRL_JIT}, + {PMIC_REG_BAIL} + }; + + return pmic_common_init(COMPAT_SAMSUNG_S2MPS11_PMIC, pmic_ops); +} +#endif + #if defined(CONFIG_POWER) #ifdef CONFIG_POWER_MAX77686 static int max77686_init(void) @@ -255,8 +275,9 @@ int power_init_board(void)
#ifdef CONFIG_POWER_MAX77686 ret = max77686_init(); +#elif defined(CONFIG_POWER_S2MPS11) + ret = board_init_s2mps11(); #endif - return ret; } #endif diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile index f054470..97b945c 100644 --- a/drivers/power/pmic/Makefile +++ b/drivers/power/pmic/Makefile @@ -13,6 +13,7 @@ COBJS-$(CONFIG_POWER_MAX8998) += pmic_max8998.o COBJS-$(CONFIG_POWER_MAX8997) += pmic_max8997.o COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o COBJS-$(CONFIG_POWER_MAX77686) += pmic_max77686.o +COBJS-$(CONFIG_POWER) += pmic_common.o
COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/power/pmic/pmic_common.c b/drivers/power/pmic/pmic_common.c new file mode 100644 index 0000000..772d1ee --- /dev/null +++ b/drivers/power/pmic/pmic_common.c @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2013 The Chromium OS Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ +#include <common.h> +#include <fdtdec.h> +#include <errno.h> +#include <power/pmic.h> +#include <power/s2mps11_pmic.h> +#include <power/max77686_pmic.h> + +DECLARE_GLOBAL_DATA_PTR; + +static unsigned pmic_number_of_regs(enum fdt_compat_id pmic_compat) +{ + switch (pmic_compat) { + case COMPAT_SAMSUNG_S2MPS11_PMIC: + return S2MPS11_NUM_OF_REGS; + default: + break; + } + return 0; +} + +int pmic_common_init(enum fdt_compat_id pmic_compat, + const struct pmic_init_ops *pmic_ops) +{ + const void *blob = gd->fdt_blob; + struct pmic *p; + int node, parent, ret; + unsigned number_of_regs = pmic_number_of_regs(pmic_compat); + const char *pmic_name, *comma; + + if (!number_of_regs) { + printf("%s: %s - not a supported PMIC\n", + __func__, fdtdec_get_compatible(pmic_compat)); + return -1; + } + + node = fdtdec_next_compatible(blob, 0, pmic_compat); + if (node < 0) { + debug("PMIC: Error %s. No node for %s in device tree\n", + fdt_strerror(node), fdtdec_get_compatible(pmic_compat)); + return node; + } + + pmic_name = fdtdec_get_compatible(pmic_compat); + comma = strchr(pmic_name, ','); + if (comma) + pmic_name = comma + 1; + + p = pmic_alloc(); + + if (!p) { + printf("%s: POWER allocation error!\n", __func__); + return -ENOMEM; + } + parent = fdt_parent_offset(blob, node); + if (parent < 0) { + debug("%s: Cannot find node parent\n", __func__); + return -1; + } + + p->bus = i2c_get_bus_num_fdt(parent); + if (p->bus < 0) { + debug("%s: Cannot find I2C bus\n", __func__); + return -1; + } + p->hw.i2c.addr = fdtdec_get_int(blob, node, "reg", 9); + + p->name = pmic_name; + p->interface = PMIC_I2C; + p->hw.i2c.tx_num = 1; + p->number_of_regs = number_of_regs; + p->compat_id = pmic_compat; + + ret = 0; + while ((pmic_ops->reg_op != PMIC_REG_BAIL) && !ret) { + if (pmic_ops->reg_op == PMIC_REG_WRITE) + ret = pmic_reg_write(p, + pmic_ops->reg_addr, + pmic_ops->reg_value); + else + ret = pmic_reg_update(p, + pmic_ops->reg_addr, + pmic_ops->reg_value); + pmic_ops++; + } + + if (ret) + printf("%s: Failed accessing reg 0x%x of %s\n", + __func__, pmic_ops[-1].reg_addr, p->name); + else + printf("PMIC %s initialized\n", p->name); + return ret; +} diff --git a/drivers/power/power_core.c b/drivers/power/power_core.c index d79971b..3e5eefa 100644 --- a/drivers/power/power_core.c +++ b/drivers/power/power_core.c @@ -205,6 +205,20 @@ int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_SUCCESS; }
+struct pmic *pmic_get_by_id(enum fdt_compat_id pmic_compat) +{ + struct pmic *p; + + list_for_each_entry(p, &pmic_list, list) { + if (p->compat_id == pmic_compat) { + debug("%s: pmic %s -> 0x%p\n", __func__, p->name, p); + return p; + } + } + + return NULL; +} + U_BOOT_CMD( pmic, CONFIG_SYS_MAXARGS, 1, do_pmic, "PMIC", diff --git a/include/power/pmic.h b/include/power/pmic.h index d17dbdc..f7d3cf4 100644 --- a/include/power/pmic.h +++ b/include/power/pmic.h @@ -12,6 +12,7 @@ #include <linux/list.h> #include <i2c.h> #include <power/power_chrg.h> +#include <fdtdec.h>
enum { PMIC_I2C, PMIC_SPI, PMIC_NONE}; enum { I2C_PMIC, I2C_NUM, }; @@ -72,6 +73,7 @@ struct pmic {
struct pmic *parent; struct list_head list; + enum fdt_compat_id compat_id; };
int pmic_init(unsigned char bus); @@ -84,6 +86,38 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val); int pmic_reg_write(struct pmic *p, u32 reg, u32 val); int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on); int pmic_reg_update(struct pmic *p, int reg, uint regval); +/* + * Find registered PMIC based on its compatibility ID. + * + * @param pmic_compat compatibility ID of the PMIC to search for. + * @return pointer to the relevant 'struct pmic' on success or NULL + */ +struct pmic *pmic_get_by_id(enum fdt_compat_id pmic_compat); + +enum pmic_reg_op { PMIC_REG_BAIL, PMIC_REG_WRITE, PMIC_REG_UPDATE }; +struct pmic_init_ops { + enum pmic_reg_op reg_op; + u8 reg_addr; + u8 reg_value; +}; + +/** + * Common function used to intialize an i2c based PMIC. + * + * This function finds the PMIC in the device tree based on its compatibility + * ID. If found, the struct pmic is allocated, initialized and registered. + * + * Then the table of initialization settings is scanned and the PMIC registers + * are set as dictated by the table contents, + * + * @param pmic_compat compatibility ID f the PMIC to be initialized. + * @param pmic_ops a pointer to the table containing PMIC initialization + * settings. The last entry contains reg_op + * of PMIC_REG_BAIL. + * @return zero on success, nonzero on failure + */ +int pmic_common_init(enum fdt_compat_id pmic_compat, + const struct pmic_init_ops *pmic_ops);
#define pmic_i2c_addr (p->hw.i2c.addr) #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
configure S2MPS11 pmic on SMDK5420
Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com --- include/configs/smdk5420.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/configs/smdk5420.h b/include/configs/smdk5420.h index 447f8e5..46aeec0 100644 --- a/include/configs/smdk5420.h +++ b/include/configs/smdk5420.h @@ -53,4 +53,8 @@
#define CONFIG_MAX_I2C_NUM 11
+#define CONFIG_POWER +#define CONFIG_POWER_I2C +#define CONFIG_POWER_S2MPS11 + #endif /* __CONFIG_5420_H */
Hello, I'll change the typo error in the patch header in next version.
/Leela krishna
On Oct 1, 2013 8:11 PM, "Leela Krishna Amudala" l.krishna@samsung.com wrote:
configure S2MPS11 pmic on SMDK5420
Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com
include/configs/smdk5420.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/configs/smdk5420.h b/include/configs/smdk5420.h index 447f8e5..46aeec0 100644 --- a/include/configs/smdk5420.h +++ b/include/configs/smdk5420.h @@ -53,4 +53,8 @@
#define CONFIG_MAX_I2C_NUM 11
+#define CONFIG_POWER +#define CONFIG_POWER_I2C +#define CONFIG_POWER_S2MPS11
#endif /* __CONFIG_5420_H */
1.7.10.4
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
participants (4)
-
Heiko Schocher -
Leela Krishna Amudala -
Leela Krishna Amudala -
Lukasz Majewski