Change in ...coreboot[master]: soc/intel/braswell/smbus.c: Add support for i2c block write
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33225
Change subject: soc/intel/braswell/smbus.c: Add support for i2c block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c block write
Intel Braswell supports i2c block writes using SMBus controller.
i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc M src/soc/intel/braswell/smbus.c 2 files changed, 34 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/1
diff --git a/src/soc/intel/braswell/Makefile.inc b/src/soc/intel/braswell/Makefile.inc index e479a3c..a7d6297 100644 --- a/src/soc/intel/braswell/Makefile.inc +++ b/src/soc/intel/braswell/Makefile.inc @@ -30,6 +30,7 @@ ramstage-y += emmc.c ramstage-y += gpio.c ramstage-y += gfx.c +ramstage-y += smbus.c
ramstage-y += gpio_support.c ramstage-y += iosf.c diff --git a/src/soc/intel/braswell/smbus.c b/src/soc/intel/braswell/smbus.c index 7e1b0df..f849926 100644 --- a/src/soc/intel/braswell/smbus.c +++ b/src/soc/intel/braswell/smbus.c @@ -3,6 +3,7 @@ * * Copyright (C) 2017 Intel Corporation. * Copyright (C) 2019 3mdeb + * Copyright (C) 2019 Eltan B.V. * * 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 @@ -16,6 +17,11 @@
#include <device/early_smbus.h> #include <soc/iomap.h> +#include <soc/pci_devs.h> +#include <device/pci_def.h> +#include <device/pci_type.h> +#include <device/pci_ops.h> +#include <soc/smbus.h> #include <southbridge/intel/common/smbus.h>
u8 smbus_read_byte(u32 smbus_dev, u8 addr, u8 offset) @@ -27,3 +33,30 @@ { return do_smbus_write_byte(SMBUS_BASE_ADDRESS, addr, offset, value); } + +int i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, + u32 bytes, const u8 *buf) +{ +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev = PCI_DEV(0, SMBUS_DEV, SMBUS_FUNC); +#else + struct device *dev = pcidev_on_root(SMBUS_DEV, SMBUS_FUNC); +#endif + u32 reg; + u32 smb_ctlr_reg; + int status; + + /* SMBus I/O BAR */ + reg = pci_read_config32(dev, PCI_BASE_ADDRESS_4) & 0xFFFFFFFE; + + /* Enable I2C_EN bit in HOSTC register */ + smb_ctlr_reg = pci_read_config32(dev, HOSTC); + pci_write_config32(dev, HOSTC, smb_ctlr_reg | HOSTC_I2C_EN); + + status = do_i2c_block_write(reg, device, cmd, bytes, buf); + + /* Restore I2C_EN bit */ + pci_write_config32(dev, HOSTC, smb_ctlr_reg); + + return status; +}
Hello Patrick Rudolph, Arthur Heymans, David Hendricks, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#2).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c block write
Intel Braswell supports i2c block writes using SMBus controller.
i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 61 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/2
Hello Patrick Rudolph, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#4).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_smbus_block_write(). i2c clients expect first byte after i2c slave address is register address[16:8]. The SMBus controlller put the slave address on the bus followed by the value in SMBHSTDAT1. smbus_i2c_block_write() will fill the SMBHSTDAT1 register before executing do_smbus_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/33225/4/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/33225/4/src/soc/intel/braswell/include/soc/s... PS4, Line 25: int smbus_i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, line over 80 characters
https://review.coreboot.org/#/c/33225/4/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/4/src/soc/intel/braswell/smbus.c@37 PS4, Line 37: int smbus_i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, line over 80 characters
Hello Patrick Rudolph, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#5).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_smbus_block_write(). i2c clients expect first byte after i2c slave address is register address[16:8]. The SMBus controlller put the slave address on the bus followed by the value in SMBHSTDAT1. smbus_i2c_block_write() will fill the SMBHSTDAT1 register before executing do_smbus_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 2 files changed, 61 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/5
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33225/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33225/5//COMMIT_MSG@14 PS5, Line 14: controlller controller
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#6).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_smbus_block_write(). i2c clients expect first byte after i2c slave address is register address[16:8]. The SMBus controller put the slave address on the bus followed by the value in SMBHSTDAT1. smbus_i2c_block_write() will fill the SMBHSTDAT1 register before executing do_smbus_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 2 files changed, 61 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/6
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/33225/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33225/5//COMMIT_MSG@14 PS5, Line 14: controlller
controller
Done
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#7).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_smbus_block_write(). i2c clients expect first byte is i2c slave address followed by data bytes. The SMBus controller puts the slave address on the bus followed by the value in SMBHSTDAT1. smbus_i2c_block_write() will fill the SMBHSTDAT1 register before executing do_smbus_block_write(). On the SMBus:: <addr> <SMBHSTDAT1> <data0> [<data1> ... <dataN>]
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP and verify that LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 2 files changed, 61 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/7
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#8).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_smbus_block_write(). i2c clients expect first byte is i2c slave address followed by data bytes. The SMBus controller puts the slave address on the bus followed by the value in SMBHSTDAT1. smbus_i2c_block_write() will fill the SMBHSTDAT1 register before executing do_smbus_block_write(). On the SMBus:: <addr> <SMBHSTDAT1> <data0> [<data1> ... <dataN>]
Add smbus.c to ramstage.
BUG=N/A TEST=Config eDP and verify that LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/8
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#9).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in I2C mode before calling do_smbus_block_write(). i2c clients expect first byte is i2c slave address followed by data bytes. The SMBus controller puts the slave address on the bus followed by the value in SMBHSTDAT1. smbus_i2c_block_write() will fill the SMBHSTDAT1 register before executing do_smbus_block_write(). On the SMBus:: <addr> <SMBHSTDAT1> <data0> [<data1> ... <dataN>]
Add smbus.c to ramstage.
BUG=N/A TEST=Verify LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/9
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
https://review.coreboot.org/c/coreboot/+/33433 is using this patchset
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
This smbus i2c block write is used by https://review.coreboot.org/c/coreboot/+/33433
The code configured the eDP bridge correctly using the SMBus controller in i2c mode.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(3 comments)
The code itself looks good to me; I'd prefer having a less convoluted API though that abstracts a bit more the weirdness of the I2C mode of the SMBUS controller. See my comments in the code for that.
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/include/soc/s... File src/soc/intel/braswell/include/soc/smbus.h:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/include/soc/s... PS9, Line 25: int smbus_i2c_block_write(u8 addr, u8 offset, const unsigned int bytes, : const u8 *buf); see my other comment on this API
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@37 PS9, Line 37: smbus_i2c_block_write(u8 addr, u8 offset, const unsigned int bytes, : const u8 *buf) I think "int smbus_i2c_block_write(u8 addr, const unsigned int bytes, const u8 *buf)" would be a nicer, more generic and possibly easier to use API. So basically also put the "offset" byte in *buf, use buf[0] instead of offset and &buf[1] instead of buf and bytes - 1 instead of bytes (and of course do some error checking for the case that bytes is less than 1). The I2C protocol doesn't define what the payload bytes (everything after the 7 or when using the address extension 14 bit I2C device address) and different devices use different amount of bytes to select an internal register address; I've also seen an I2C device that just had one 8 bit register and didn't use any internal register addressing.
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf); It would probably be useful to add a bit of documentation here, since the code isn't that obvious. From what I understood, in I2C mode the first I2C payload byte is put in the SMBUS command byte, the I2C write transfer payload length excluding the first byte is put in the SMBUS length register, that doesn't get transmitted when the SMBUS controller is in I2C mode, and the remaining bytes of the I2C write transfer payload gets sent like the SMBUS payload. This is a rather convoluted way of doing an I2C write transfer, but we can't change the hardware...
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@37 PS9, Line 37: smbus_i2c_block_write(u8 addr, u8 offset, const unsigned int bytes, : const u8 *buf)
I think "int smbus_i2c_block_write(u8 addr, const unsigned int bytes, const u8 *buf)" would be a nic […]
I agree with with having the API clean.
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
It would probably be useful to add a bit of documentation here, since the code isn't that obvious. […]
The command byte is not forward in the i2c payload bytes. It is used to 'satisfy' the SMBus controller. For Braswell the i2c stream is <SMBXNITADD> <SMBHSTDAT1> <SMBBLKDAT> ... <SMBBLKDAT>
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
The command byte is not forward in the i2c payload bytes. […]
ah, ok; didn't look too closely into that. It would still be good to have a short description of how that stuff works
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
The command byte is not forward in the i2c payload bytes. […]
I will modify the arguments removing offset as input parameter.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
I will modify the arguments removing offset as input parameter.
You really should not touch any of the SMBUS IO-mapped registers before checking that the host is not busy (setup_command() in common/smbus.c). Same goes for the PCI configuration registers. Admittedly, we are not that likely to multi-task here but still... Datasheets specificly say to not touch IO register bank while transaction is in progress. FYI: Back in 2005 or so, bad SMBUS state machines bricked a lot of Thinkpads, although the error was on the EEPROM device back then.
I think it's better to fork do_smbus_block_write() to do_i2c_block_write(), and replace SMBHSTCMD with SMBHSTDAT1 there __if__ that is really needed. Use of SMBHSTDAT1 differs from any Intel document/datasheet I have looked at (most suck equally for SMBus host) so let's try to document this carefully.
https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/sm...
That is the best doc what I have found so far about block formats wih I2C_EN but dates to 2009. I am looking at Figure 14 page 11.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
You really should not touch any of the SMBUS IO-mapped registers before checking that the host is no […]
I agree that that is not much information Braswell SMBus registers. (The IO mapped registers are not decribed in Intel EDS #547871, only the memory mapped!). The Fig 14 page 11 is the correct the SMBus controller format. However the SMBus controller uses the registers <SMBXNITADD> <SMBHSTDAT1> <SMBBLKDAT> ... <SMBBLKDAT> to compose this format.
Will verify if mod of PCI registers after check not busy will work.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
I agree that that is not much information Braswell SMBus registers. […]
Fork in Southbridge Intel?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
Fork in Southbridge Intel?
Well I have no access to #547871.
In the doc I referred to, see Figure 16, this is our do_i2c_eeprom_read() implementation. There the diagram clearly says "Send DATA1 register" to refer to SMBHSTDAT1. From what I can tell, "Command Code" in that paper always refers to SMBHSTCMD.
The weirdness here is code currently fills both SMBHSTCMD and SMBHSTDAT1 registers with the same value. I guess I am not happy about this until I hear it works by writing SMBHSTDAT1 only (and not SMBHSTCMD) and after this possible discrepancy is documented within the code and not only the commit message.
Looking at i2c-i801 kernel module, SMBHSTDAT1 is not involved for I2C block writes there either.
Yes, I'd fork it inside common/smbus.c. There's probably going to be some conditionals around CMD/DAT1 use if we expand this to older silicons.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
(1 comment)
my goal is having SMBus i2c support merged in coreboot, but I get comments which points to different ways.
To avoid accessing smbus registers before 'checking ready' I suggest creating do_i2c_block_write() in SB/intel/common.
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/9/src/soc/intel/braswell/smbus.c@57 PS9, Line 57: status = do_smbus_block_write(smbase, addr, offset, bytes, buf);
Well I have no access to #547871. […]
When the SMBHSTDAT1 register is not written a value of 0x00 will appear on the SMBus after the register offset. So in this case the sequence is: <SMBXNITADD> 0x00. When the SMBHSTCMD register is not written the value of SMHSTDAT1 does not appear on the SMBus.
Both writes are required.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
Patch Set 9:
(1 comment)
my goal is having SMBus i2c support merged in coreboot, but I get comments which points to different ways.
To avoid accessing smbus registers before 'checking ready' I suggest creating do_i2c_block_write() in SB/intel/common.
I agree, you can still pass just the array of bytes to send (with count) as arguments, drop the separate offset, thus making a clean API. You can then write both CMD and DAT1 in the new implementation with sufficient commentary.
BTW: Was it actually documented for Braswell like that, such that both registers have to be written, or was this trial-and-error? Really sounds like errata to me.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
(1 comment)
my goal is having SMBus i2c support merged in coreboot, but I get comments which points to different ways.
To avoid accessing smbus registers before 'checking ready' I suggest creating do_i2c_block_write() in SB/intel/common.
I agree, you can still pass just the array of bytes to send (with count) as arguments, drop the separate offset, thus making a clean API. You can then write both CMD and DAT1 in the new implementation with sufficient commentary.
BTW: Was it actually documented for Braswell like that, such that both registers have to be written, or was this trial-and-error? Really sounds like errata to me.
I could not find it in the implementation. Our SMBus analyzer showed a value of '0x0' in the sequence. During analyze it turned out that this was the SMBHSTDAT1 value. The i2c support is not (well) documented.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
(1 comment)
my goal is having SMBus i2c support merged in coreboot, but I get comments which points to different ways.
To avoid accessing smbus registers before 'checking ready' I suggest creating do_i2c_block_write() in SB/intel/common.
I agree, you can still pass just the array of bytes to send (with count) as arguments, drop the separate offset, thus making a clean API. You can then write both CMD and DAT1 in the new implementation with sufficient commentary.
BTW: Was it actually documented for Braswell like that, such that both registers have to be written, or was this trial-and-error? Really sounds like errata to me.
I could not find it in the implementation. Our SMBus analyzer showed a value of '0x0' in the sequence. During analyze it turned out that this was the SMBHSTDAT1 value. The i2c support is not (well) documented.
Feel free to update Documentation/ with everything you find helpful or think that others might find helpful to understand how hardware or software works.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
There is also the Linux smbus driver which doesn't know any quirks, it seems. It uses SMBHSTCMD for the first byte for I2C block writes with I2C_EN, and SMBHSTDAT1 for the I2C_ BLOCK_READ command without I2C_EN. So basically just as documented. But it's hard to tell if that code is tested (on all platforms?).
I have APL/SKL/CFL boards here with an 24LC256, so I should be able to write some test patterns. Then we could also say if these platforms behave as documented.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 9:
I have APL/SKL/CFL boards here with an 24LC256, so I should be able to write some test patterns. Then we could also say if these platforms behave as documented.
Scratch that, the EEPROM is attached to another I2C controller. But one could do the same with SPDs, in theory ;)
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#10).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in i2c mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Verify LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 63 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/10
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#11).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in i2c mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Verify LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/11
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#12).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in i2c mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Verify LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc M src/soc/intel/braswell/smbus.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/12
Hello Patrick Rudolph, Felix Held, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33225
to look at the new patch set (#13).
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in i2c mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Verify LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33225/13
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 14: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c@49 PS14, Line 49: smbase = pci_read_config32(dev, PCI_BASE_ADDRESS_4) & 0xFFFFFFFE; There is asymmetry in that the functions above use constant SMBUS_BASE_ADRESS and take (unused) smbus_dev argument. The big picture is, smbus prototypes are a mess in our tree...
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c@53 PS14, Line 53: pci_write_config32(dev, HOSTC, smb_ctrl_reg | HOSTC_I2C_EN); This should only be done after we know SMBUS host controller is not busy. I can live this, but this is really common to intel since ICH5 from what I know, and eventually needs refactorin and moving outside Braswell.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c@53 PS14, Line 53: pci_write_config32(dev, HOSTC, smb_ctrl_reg | HOSTC_I2C_EN);
This should only be done after we know SMBUS host controller is not busy. […]
On CB:3800 patchset #13 the comment was that only smbus register must be accessed in smbus.c
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
Patch Set 14: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c File src/soc/intel/braswell/smbus.c:
https://review.coreboot.org/#/c/33225/14/src/soc/intel/braswell/smbus.c@53 PS14, Line 53: pci_write_config32(dev, HOSTC, smb_ctrl_reg | HOSTC_I2C_EN);
On CB:3800 patchset #13 the comment was that only smbus register must be accessed in smbus. […]
You probably meant CB:30800 # 13 line 415? There was only a question about symmetry on parameter passing there. A clean approach would be to not do the HOSTC backup/restore here, but to always set/clear I2C_EN bit correctly in between the setup_command() and execute_command() calls.
In general, I am not so keen on leaving TODOs in, but I understand it's a lot wider refactor work and I don't want to keep thisS SMBUS part blocking the board port from getting submitted.
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33225 )
Change subject: soc/intel/braswell/smbus.c: Add support for i2c mode block write ......................................................................
soc/intel/braswell/smbus.c: Add support for i2c mode block write
Intel Braswell supports i2c block write using SMBus controller.
smbus_i2c_block_write() is added to configure SMBus controller in i2c mode before calling do_i2c_block_write().
Add smbus.c to ramstage.
BUG=N/A TEST=Verify LCD display is working on Facebook FBG-1701
Change-Id: I50c1a03f624b3ab3b987d4f3b1d15dac4374e48a Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33225 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/braswell/Makefile.inc A src/soc/intel/braswell/include/soc/smbus.h M src/soc/intel/braswell/smbus.c 3 files changed, 59 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/soc/intel/braswell/Makefile.inc b/src/soc/intel/braswell/Makefile.inc index 1017d80..cc111da 100644 --- a/src/soc/intel/braswell/Makefile.inc +++ b/src/soc/intel/braswell/Makefile.inc @@ -35,6 +35,7 @@ ramstage-y += emmc.c ramstage-y += gpio.c ramstage-y += gfx.c +ramstage-y += smbus.c
ramstage-y += gpio_support.c ramstage-y += iosf.c diff --git a/src/soc/intel/braswell/include/soc/smbus.h b/src/soc/intel/braswell/include/soc/smbus.h new file mode 100644 index 0000000..8bc62f7 --- /dev/null +++ b/src/soc/intel/braswell/include/soc/smbus.h @@ -0,0 +1,26 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2013 Google Inc. + * Copyright (C) 2015 Intel Corp. + * Copyright (C) 2019 Eltan B.V. + * + * 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; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _SOC_SMBUS_H_ +#define _SOC_SMBUS_H_ + +/* PCI Configuration Space SMBus */ +#define HOSTC 0x40 +#define HOSTC_I2C_EN (1 << 2) + +int smbus_i2c_block_write(u8 addr, u8 bytes, u8 *buf); +#endif /* _SOC_SMBUS_H_ */ diff --git a/src/soc/intel/braswell/smbus.c b/src/soc/intel/braswell/smbus.c index 7e1b0df..1dfd4c7 100644 --- a/src/soc/intel/braswell/smbus.c +++ b/src/soc/intel/braswell/smbus.c @@ -3,6 +3,7 @@ * * Copyright (C) 2017 Intel Corporation. * Copyright (C) 2019 3mdeb + * Copyright (C) 2019 Eltan B.V. * * 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 @@ -16,6 +17,11 @@
#include <device/early_smbus.h> #include <soc/iomap.h> +#include <soc/pci_devs.h> +#include <device/pci_def.h> +#include <device/pci_type.h> +#include <device/pci_ops.h> +#include <soc/smbus.h> #include <southbridge/intel/common/smbus.h>
u8 smbus_read_byte(u32 smbus_dev, u8 addr, u8 offset) @@ -27,3 +33,29 @@ { return do_smbus_write_byte(SMBUS_BASE_ADDRESS, addr, offset, value); } + +int smbus_i2c_block_write(u8 addr, u8 bytes, u8 *buf) +{ +#ifdef __SIMPLE_DEVICE__ + pci_devfn_t dev = PCI_DEV(0, SMBUS_DEV, SMBUS_FUNC); +#else + struct device *dev = pcidev_on_root(SMBUS_DEV, SMBUS_FUNC); +#endif + u32 smbase; + u32 smb_ctrl_reg; + int status; + + /* SMBus I/O BAR */ + smbase = pci_read_config32(dev, PCI_BASE_ADDRESS_4) & 0xFFFFFFFE; + + /* Enable I2C_EN bit in HOSTC register */ + smb_ctrl_reg = pci_read_config32(dev, HOSTC); + pci_write_config32(dev, HOSTC, smb_ctrl_reg | HOSTC_I2C_EN); + + status = do_i2c_block_write(smbase, addr, bytes, buf); + + /* Restore I2C_EN bit */ + pci_write_config32(dev, HOSTC, smb_ctrl_reg); + + return status; +}
-
build bot (Jenkins) (Code Review) -
Felix Held (Code Review)
-
Frans Hendriks (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)