Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
ec/kontron: add option to configure I2C frequency
Allows to change the frequency for the I2C bus by overriding i2c_freq_khz option from the devicetree of the board.
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [1,2].
[1] https: //review.coreboot.org/c/coreboot/+/39133 [2] https: //review.coreboot.org/c/coreboot/+/39846
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 22 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/1
diff --git a/src/ec/kontron/kempld/chip.h b/src/ec/kontron/kempld/chip.h index 8ca8d24..2ac0949 100644 --- a/src/ec/kontron/kempld/chip.h +++ b/src/ec/kontron/kempld/chip.h @@ -19,6 +19,7 @@
struct ec_kontron_kempld_config { struct kempld_uart uart[KEMPLD_NUM_UARTS]; + unsigned short i2c_freq_khz; };
#endif /* EC_KONTRON_KEMPLD_CHIP_H */ diff --git a/src/ec/kontron/kempld/kempld_i2c.c b/src/ec/kontron/kempld/kempld_i2c.c index 296cf76..9ba2f71 100644 --- a/src/ec/kontron/kempld/kempld_i2c.c +++ b/src/ec/kontron/kempld/kempld_i2c.c @@ -13,6 +13,7 @@ #include <timer.h> #include <delay.h>
+#include "chip.h" #include "kempld.h" #include "kempld_internal.h"
@@ -40,8 +41,8 @@ #define I2C_CMD_READ_NACK 0x29 #define I2C_CMD_IACK 0x01
-#define KEMPLD_I2C_FREQ_MAX 2700 /* 2.7 mHz */ -#define KEMPLD_I2C_FREQ_STD 100 /* 100 kHz */ +#define KEMPLD_I2C_FREQ_MAX 2700 /* 2.7 mHz */ +#define KEMPLD_I2C_FREQ_STD_DEFAULT 100 /* 100 kHz */
#define EIO 5 #define ENXIO 6 @@ -230,12 +231,17 @@
void kempld_i2c_device_init(struct device *const dev) { + const struct ec_kontron_kempld_config *const config = dev->chip_info; + unsigned short frequency; u16 prescale_corr; long prescale; u8 ctrl; u8 stat; u8 cfg;
+ if (!config) + return; + if (kempld_get_mutex(100) < 0) return;
@@ -244,11 +250,22 @@ ctrl &= ~(I2C_CTRL_EN | I2C_CTRL_IEN); kempld_write8(KEMPLD_I2C_CTRL, ctrl);
+ frequency = KEMPLD_I2C_FREQ_STD_DEFAULT; + if (config->i2c_freq_khz) { + if (config->i2c_freq_khz <= KEMPLD_I2C_FREQ_MAX) + frequency = config->i2c_freq_khz; + else + printk(BIOS_WARNING, + "kempld_i2c: %d kHz is an invalid frequency value\n", + config->i2c_freq_khz); + } + printk(BIOS_INFO, "kempld_i2c: Use frequency %d\n", frequency); + const u8 spec_major = KEMPLD_SPEC_GET_MAJOR(kempld_read8(KEMPLD_SPEC)); if (spec_major == 1) - prescale = KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 5) - 1000; + prescale = KEMPLD_CLK / (frequency * 5) - 1000; else - prescale = KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 4) - 3000; + prescale = KEMPLD_CLK / (frequency * 4) - 3000;
if (prescale < 0) prescale = 0;
Maxim Polyakov has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
ec/kontron: add option to configure I2C frequency
Allows to change the frequency for the I2C bus by overriding i2c_freq_khz option from the devicetree of the board.
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [1,2].
[1] https: //review.coreboot.org/c/coreboot/+/39133 [2] https: //review.coreboot.org/c/coreboot/+/39846
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 22 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#5).
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
ec/kontron: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_freq_khz option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 27 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/5/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/5/src/ec/kontron/kempld/kempl... PS5, Line 51: I2C_FREQ_STANDART_MODE = 100, /* 100 kHz */ 'STANDART' may be misspelled - perhaps 'STANDARD'?
https://review.coreboot.org/c/coreboot/+/44476/5/src/ec/kontron/kempld/kempl... PS5, Line 257: frequency = I2C_FREQ_STANDART_MODE; 'STANDART' may be misspelled - perhaps 'STANDARD'?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#6).
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
ec/kontron: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_freq_khz option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 27 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/6
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
Patch Set 9: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... PS9, Line 53: mHz MHz as 1000 kHz and not 0.001 Hz.
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... PS9, Line 54: mHz same here
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... PS9, Line 239: unsigned short Use u16 here to stay consistent with the rest of the code?
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Werner Zeh, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#10).
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
ec/kontron: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_freq_khz option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/10
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
Patch Set 10:
(3 comments)
Thanks for the review
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... PS9, Line 53: mHz
MHz as 1000 kHz and not 0.001 Hz.
copy-paste from previous version Fixed
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... PS9, Line 54: mHz
same here
Done
https://review.coreboot.org/c/coreboot/+/44476/9/src/ec/kontron/kempld/kempl... PS9, Line 239: unsigned short
Use u16 here to stay consistent with the rest of the code?
Done
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
Patch Set 10: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron: add option to configure I2C frequency ......................................................................
Patch Set 10: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44476/10//COMMIT_MSG@7 PS10, Line 7: ec/kontron nit: ec/kontron/kempld
https://review.coreboot.org/c/coreboot/+/44476/10/src/ec/kontron/kempld/chip... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/44476/10/src/ec/kontron/kempld/chip... PS10, Line 22: what's with these spaces?
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Werner Zeh, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#11).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_freq_khz option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/11
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Werner Zeh, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#12).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_freq_khz option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/12
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44476/10//COMMIT_MSG@7 PS10, Line 7: ec/kontron
nit: ec/kontron/kempld
Done
https://review.coreboot.org/c/coreboot/+/44476/10/src/ec/kontron/kempld/chip... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/44476/10/src/ec/kontron/kempld/chip... PS10, Line 22:
what's with these spaces?
Done
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 12: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 12: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 50: stadart ?
Plus, if you indicate these are in kHz, the comments can go away
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 262: is an invalid frequency value "is too high"?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 52: I2C_FREQ_FAST_MODE = 400, /* 400 kHz */ : I2C_FREQ_FAST_PLUS_MODE = 1000, /* 1 MHz */ These two seem unused. Why not make them available through `chip.h`? Then one can use them in the devicetree. Adding the frequency would make it more readable, e.g.
I2C_FREQ_FAST_MODE_400KHZ = 400,
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Andrey Petrov, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#13).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_freq_khz option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/13
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 13: Code-Review+2
(7 comments)
https://review.coreboot.org/c/coreboot/+/44476/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44476/13//COMMIT_MSG@9 PS13, Line 9: _khz Needs slight update.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/chip... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/chip... PS13, Line 19: I2C_FREQ_MAX = 2700, Nit, technically these should have a KEMPLD_ prefix. All the `chip.h` are included in the `build/mainboard*/*/static.c`, hence conflicts are possible and hard to control. But it's nothing that can't be fixed later if it collides eventually.
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 50: stadart
? […]
Done
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 52: I2C_FREQ_FAST_MODE = 400, /* 400 kHz */ : I2C_FREQ_FAST_PLUS_MODE = 1000, /* 1 MHz */
These two seem unused. Why not make them available through `chip.h`? Then […]
Done
https://review.coreboot.org/c/coreboot/+/44476/12/src/ec/kontron/kempld/kemp... PS12, Line 262: is an invalid frequency value
"is too high"?
Done
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 254: BIOS_WARNING Warning is a bit too high, from `commonlib/loglevel.h`:
Log level for when the system has noticed an issue that most likely will not preclude a successful boot.
I don't think this could make a boot fail.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 256: config->i2c_frequency); Nit, while not mandated, it would be nice to have braces around the branches because they are (visibly) multi-lined.
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Andrey Petrov, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#14).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_frequency option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/14
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 14:
(4 comments)
Thanks for the review!
https://review.coreboot.org/c/coreboot/+/44476/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44476/13//COMMIT_MSG@9 PS13, Line 9: _khz
Needs slight update.
Done
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/chip... File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/chip... PS13, Line 19: I2C_FREQ_MAX = 2700,
Nit, technically these should have a KEMPLD_ prefix. All the `chip.h` are […]
Done
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 254: BIOS_WARNING
Warning is a bit too high, from `commonlib/loglevel.h`: […]
In that case, is BIOS_INFO okay?
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 256: config->i2c_frequency);
Nit, while not mandated, it would be nice to have braces around the branches […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 254: BIOS_WARNING
In that case, is BIOS_INFO okay?
The description says INFO is for expected things, I'd use NOTICE.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 256: config->i2c_frequency);
Done
Sorry, maybe I should have mentioned. If there are braces on one path, all paths should get them, c.f. https://doc.coreboot.org/coding_style.html#placing-braces-and-spaces
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Andrey Petrov, Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#15).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_frequency option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/15
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 254: BIOS_WARNING
The description says INFO is for expected things, I'd use NOTICE.
Ok, done. Thx
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 256: config->i2c_frequency);
Sorry, maybe I should have mentioned. If there are braces on one path, all paths should […]
Done
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info; Would it make sense to use config_of(dev) here?
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 238: if (!config) This could be dropped in case of config_of(dev)-usage because this API introduces a devtree_die() in the case where the config could not be obtained.
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Werner Zeh, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#16).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_frequency option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 25 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/16
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16: Code-Review+2
Please be aware that your logic for "config not found" will now lead to a die() instead of continue to boot while not setting up the device at all.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16:
(2 comments)
Patch Set 16: Code-Review+2
Please be aware that your logic for "config not found" will now lead to a die() instead of continue to boot while not setting up the device at all.
Yes, according to the document,
static inline DEVTREE_CONST void *config_of(const struct device *dev) { if (dev && dev->chip_info) return dev->chip_info;
devtree_die(); }
this is true
Thank you for review
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info;
Would it make sense to use config_of(dev) here?
Yes you are right. It would be better if I use config_of () here. Thanks!
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 238: if (!config)
This could be dropped in case of config_of(dev)-usage because this API introduces a devtree_die() in […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16: Code-Review-2
(2 comments)
Regressed since the last time I looked at it.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info;
Yes you are right. It would be better if I use config_of () here. […]
config_of() is a shorthand to avoid an explicit null-check which is not necessary here.
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 238: if (!config)
Done
That's not true. It dies iff `dev` in NULL.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 238: if (!config)
That's not true. It dies iff `dev` in NULL.
Ah, sorry, misread the code for a second. But a die() is wrong anyway as long as we know a way to go on.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16: Code-Review-1
Sorry, -2 was too harsh. But I'd really prefer the readable check over a hidden die().
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info;
config_of() is a shorthand to avoid an explicit null-check which is not […]
For me config_of provides a common way to get the config struct pointer of a given device. As an add-on, it makes sure, that we do have access to the struct at all. If everything is well configured, config _is_ accessible and the die() will never be taken. On the other hand, if the pointer to the config-struct could not be obtained, there might be an error in the configuration of the build (not enabled the device while the driver stays enabled). In this case I would prefer to be informed as soon as possible to have a chance to correct it. Nevertheless, I have no strong opinion in favor of any way. So if you feel to change it back, I can deal with it a s well.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/16/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/16/src/ec/kontron/kempld/kemp... PS16, Line 247: if (config->i2c_frequency) { Since the only thing you're using `config` for is to get the I2C frequency to use, `config_of` provides no benefit over a null-check here:
const struct ec_kontron_kempld_config *const config = dev->chip_info;
/* ... */
if (config && config->i2c_frequency) { /* ... */
If `config` happens to be null, the default I2C frequency will be used. This is much better than the deliberate brick caused by `die()` in the `config_of()` function.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info;
For me config_of provides a common way to get the config struct pointer of a given device. […]
It's a nice, common way, I agree. But it also adds an unnecessary die(). Not every developer has ISP comfortably hooked up, so such a die() can be a real PITA. And not everybody is a developer at all. We already have assertions not trigger by default because people used them wrong. If we now use die() as a development tool, we'll soon have a NON_FATAL_DIE Kconfig.
IIRC, the reason that the die() was added was that people were throwing NULL pointers around in soc/intel/. They needed to be told the hard way because the problem became uncontrollable. Generally, we don't have such NULL pointer problems in coreboot. The `dev`, for instance, that is passed to `.init` hooks is guaranteed to be valid. I see now that it was short- sighted to add config_of() as is. :-/
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info;
It's a nice, common way, I agree. But it also adds an unnecessary die(). Not […]
Yes, your point is valid Nico. As already mentioned I can totally live with the old code.
Hello Felix Singer, Lance Zhao, build bot (Jenkins), Nico Huber, Andrey Petrov, Angel Pons, Werner Zeh, Andrey Petrov,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44476
to look at the new patch set (#17).
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_frequency option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 25 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/44476/17
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 17: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44476/16/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/16/src/ec/kontron/kempld/kemp... PS16, Line 247: if (config->i2c_frequency) {
Since the only thing you're using `config` for is to get the I2C frequency to use, `config_of` provi […]
Done
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 17:
(2 comments)
Thanks for the review!
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/13/src/ec/kontron/kempld/kemp... PS13, Line 231: dev->chip_info;
Yes, your point is valid Nico. […]
Ok, let's abandon the changes regarding config_of ().
https://review.coreboot.org/c/coreboot/+/44476/16/src/ec/kontron/kempld/kemp... File src/ec/kontron/kempld/kempld_i2c.c:
https://review.coreboot.org/c/coreboot/+/44476/16/src/ec/kontron/kempld/kemp... PS16, Line 247: if (config->i2c_frequency) {
Done
Good idea, thanks!
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 17: Code-Review+2
Thanks for the patience :)
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
Patch Set 17:
Patch Set 17: Code-Review+2
Thanks for the patience :)
No problems :)
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44476 )
Change subject: ec/kontron/kempld: add option to configure I2C frequency ......................................................................
ec/kontron/kempld: add option to configure I2C frequency
Allows to change the I2C bus frequency by overriding i2c_frequency option from the board devicetree. Thus, the I2C controller can use Fast-mode (Fm), with a bit rate up to 400 kbit/s and Fast-mode Plus (Fm+), with a bit rate up to 1 Mbit/s [1].
Tested on Kontron mAL10 COMe module with T10-TNI carrierboard [2].
[1] I2C-bus specification and user manual, doc #UM10204, Rev. 6, 4 April 2014. [2] https://review.coreboot.org/c/coreboot/+/39133
Change-Id: If0eb477af10d00eb4f17f9c01209f170b746ad3d Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44476 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/ec/kontron/kempld/chip.h M src/ec/kontron/kempld/kempld_i2c.c 2 files changed, 25 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/ec/kontron/kempld/chip.h b/src/ec/kontron/kempld/chip.h index 8ca8d24..30f40fe 100644 --- a/src/ec/kontron/kempld/chip.h +++ b/src/ec/kontron/kempld/chip.h @@ -12,6 +12,13 @@ KEMPLD_UART_2E8 = 3, };
+enum kempld_i2c_frequency { + KEMPLD_I2C_FREQ_STANDARD_MODE_100KHZ = 100, + KEMPLD_I2C_FREQ_FAST_MODE_400KHZ = 400, + KEMPLD_I2C_FREQ_FAST_PLUS_MODE_1MHZ = 1000, + KEMPLD_I2C_FREQ_MAX = 2700, +}; + struct kempld_uart { enum kempld_uart_io io; unsigned int irq; @@ -19,6 +26,7 @@
struct ec_kontron_kempld_config { struct kempld_uart uart[KEMPLD_NUM_UARTS]; + unsigned short i2c_frequency; };
#endif /* EC_KONTRON_KEMPLD_CHIP_H */ diff --git a/src/ec/kontron/kempld/kempld_i2c.c b/src/ec/kontron/kempld/kempld_i2c.c index 296cf76..8abbb4b 100644 --- a/src/ec/kontron/kempld/kempld_i2c.c +++ b/src/ec/kontron/kempld/kempld_i2c.c @@ -13,6 +13,7 @@ #include <timer.h> #include <delay.h>
+#include "chip.h" #include "kempld.h" #include "kempld_internal.h"
@@ -40,9 +41,6 @@ #define I2C_CMD_READ_NACK 0x29 #define I2C_CMD_IACK 0x01
-#define KEMPLD_I2C_FREQ_MAX 2700 /* 2.7 mHz */ -#define KEMPLD_I2C_FREQ_STD 100 /* 100 kHz */ - #define EIO 5 #define ENXIO 6 #define EAGAIN 11 @@ -230,7 +228,8 @@
void kempld_i2c_device_init(struct device *const dev) { - u16 prescale_corr; + const struct ec_kontron_kempld_config *const config = dev->chip_info; + u16 prescale_corr, frequency; long prescale; u8 ctrl; u8 stat; @@ -244,11 +243,23 @@ ctrl &= ~(I2C_CTRL_EN | I2C_CTRL_IEN); kempld_write8(KEMPLD_I2C_CTRL, ctrl);
+ frequency = KEMPLD_I2C_FREQ_STANDARD_MODE_100KHZ; + if (config && config->i2c_frequency) { + if (config->i2c_frequency <= KEMPLD_I2C_FREQ_MAX) { + frequency = config->i2c_frequency; + } else { + printk(BIOS_NOTICE, + "kempld_i2c: %d kHz is too high!\n", + config->i2c_frequency); + } + } + printk(BIOS_INFO, "kempld_i2c: Use frequency %d\n", frequency); + const u8 spec_major = KEMPLD_SPEC_GET_MAJOR(kempld_read8(KEMPLD_SPEC)); if (spec_major == 1) - prescale = KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 5) - 1000; + prescale = KEMPLD_CLK / (frequency * 5) - 1000; else - prescale = KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 4) - 3000; + prescale = KEMPLD_CLK / (frequency * 4) - 3000;
if (prescale < 0) prescale = 0;
-
Andrey Petrov (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Maxim Polyakov (Code Review)
-
Nico Huber (Code Review)
-
Werner Zeh (Code Review)