Change in ...coreboot[master]: southbridge/intel/common/smbus.{c,h}: Add support for I2C commands
Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30800
Change subject: southbridge/intel/common/smbus.{c,h}: Add support for I2C commands ......................................................................
southbridge/intel/common/smbus.{c,h}: Add support for I2C commands
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Added i2c_block_read() and i2c_block_write() which configures the SMBus controller into I2C mode before sending the commands. Added do_i2c_block_write() for sending block writes in I2C mode.
BUG=N/A TEST=Facebook FBG-1701 booting Embedded Linux
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 124 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/1
diff --git a/src/southbridge/intel/common/smbus.c b/src/southbridge/intel/common/smbus.c index de4ff91..3b50e6b 100644 --- a/src/southbridge/intel/common/smbus.c +++ b/src/southbridge/intel/common/smbus.c @@ -4,6 +4,7 @@ * Copyright (C) 2005 Yinghai Lu yinghailu@gmail.com * Copyright (C) 2009 coresystems GmbH * Copyright (C) 2013 Vladimir Serbinenko + * Copyright (C) 2018 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,7 @@ */
#include <arch/io.h> +#include <device/pci.h> #include <console/console.h> #include <device/smbus_def.h> #include <stdlib.h> @@ -96,9 +98,8 @@ unsigned char val; smbus_delay(); val = inb(smbus_base + SMBHSTSTAT); - if ((val & SMBHSTSTS_HOST_BUSY)) { + if ((val & SMBHSTSTS_HOST_BUSY)) break; - } } while (--loops); return loops ? 0 : -1; } @@ -208,6 +209,7 @@ int slave_bytes; int bytes_read = 0; unsigned int loops = SMBUS_TIMEOUT; + if (smbus_wait_until_ready(smbus_base) < 0) return SMBUS_WAIT_UNTIL_READY_TIMEOUT;
@@ -220,7 +222,7 @@ /* Set the device I'm talking to */ outb(((device & 0x7f) << 1) | 1, smbus_base + SMBXMITADD); /* Set the command/address... */ - outb(cmd & 0xff, smbus_base + SMBHSTCMD); + outb(cmd, smbus_base + SMBHSTCMD); /* Set up for a block data read */ outb((inb(smbus_base + SMBHSTCTL) & 0xc3) | I801_BLOCK_DATA, (smbus_base + SMBHSTCTL)); @@ -350,6 +352,19 @@ }
/* Only since ICH5 */ +int i2c_block_read(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, + u32 bytes, u8 *buf) +{ + struct device *smbus_dev; + u32 reg; + + /* SMBus I/O BAR */ + smbus_dev = dev_find_slot(0, PCI_DEVFN(smbus_devnr, smbus_func)); + reg = pci_read_config32(smbus_dev, PCI_BASE_ADDRESS_4) & 0xFFFFFFFE; + + return do_i2c_block_read((unsigned int)reg, device, cmd, bytes, buf); +} + int do_i2c_block_read(unsigned int smbus_base, u8 device, unsigned int offset, const unsigned int bytes, u8 *buf) { @@ -418,3 +433,102 @@
return bytes_read; } + +int i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, + u32 bytes, u8 *buf) +{ + struct device *smbus_dev; + u32 reg; + u32 smb_ctlr_reg; + int status; + + /* SMBus I/O BAR */ + smbus_dev = dev_find_slot(0, PCI_DEVFN(smbus_devnr, smbus_func)); + reg = pci_read_config32(smbus_dev, PCI_BASE_ADDRESS_4) & 0xFFFFFFFE; + + smb_ctlr_reg = pci_read_config32(smbus_dev, 0x40); + + /* Enable I2C_EN bit */ + pci_write_config32(smbus_dev, 0x40, smb_ctlr_reg | 4); + + status = do_i2c_block_write((unsigned int)reg, device, cmd, bytes, + buf); + + /* Restore I2C_EN bit */ + pci_write_config32(smbus_dev, 0x40, smb_ctlr_reg); + + return status; +} + +int do_i2c_block_write(unsigned int smbus_base, u8 device, u8 cmd, + const unsigned int bytes, u8 *buf) +{ + u8 status; + int bytes_write; + unsigned int loops = SMBUS_TIMEOUT; + + if (smbus_wait_until_ready(smbus_base) < 0) + return SMBUS_WAIT_UNTIL_READY_TIMEOUT; + + /* Set up transaction */ + /* Disable interrupts */ + outb(inb(smbus_base + SMBHSTCTL) & ~SMBHSTCNT_INTREN, + smbus_base + SMBHSTCTL); + + /* Set the slave address */ + outb(((device & 0x7f) << 1), smbus_base + SMBXMITADD); + + outb(cmd, smbus_base + SMBHSTCMD); + outb(cmd, smbus_base + SMBHSTDAT1); + + /* Number of bytes to write */ + outb(bytes, smbus_base + SMBHSTDAT0); + + /* Set up for a smbus block data write */ + outb((inb(smbus_base + SMBHSTCTL) & 0xc3) | I801_BLOCK_DATA, + (smbus_base + SMBHSTCTL)); + + /* Clear any lingering errors, so the transaction will run */ + outb(inb(smbus_base + SMBHSTSTAT), smbus_base + SMBHSTSTAT); + + /* Send first byte from buffer */ + outb(*buf++, smbus_base + SMBBLKDAT); + + for (bytes_write = 0; bytes_write < bytes; bytes_write++) { + /* Start the command */ + outb((inb(smbus_base + SMBHSTCTL) | SMBHSTCNT_START), + smbus_base + SMBHSTCTL); + + /* poll for it to start */ + if (smbus_wait_until_active(smbus_base) < 0) + return SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT; + + loops = SMBUS_TIMEOUT; + do { + loops--; + status = inb(smbus_base + SMBHSTSTAT); + if (status & (SMBHSTSTS_FAILED | /* FAILED */ + SMBHSTSTS_BUS_ERR | /* BUS ERR */ + SMBHSTSTS_DEV_ERR)) /* DEV ERR */ + return SMBUS_ERROR; + } while (!(status & SMBHSTSTS_BYTE_DONE) && loops); + + if (status & SMBHSTSTS_BYTE_DONE) { + if (bytes_write+1 < bytes) + outb(*buf++, smbus_base + SMBBLKDAT); + if ((bytes_write + 1 >= bytes) && (bytes > 1)) { + /* indicate that next byte is the last one */ + outb(inb(smbus_base + SMBHSTCTL) | + SMBHSTCNT_LAST_BYTE, + smbus_base + SMBHSTCTL); + } + + outb(status & (SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR), + smbus_base + SMBHSTSTAT); + } else { + return SMBUS_ERROR; + } + } + return (bytes_write+1); +} + diff --git a/src/southbridge/intel/common/smbus.h b/src/southbridge/intel/common/smbus.h index be1aa76..f272165 100644 --- a/src/southbridge/intel/common/smbus.h +++ b/src/southbridge/intel/common/smbus.h @@ -4,6 +4,7 @@ * Copyright (C) 2005 Yinghai Lu yinghailu@gmail.com * Copyright (C) 2009 coresystems GmbH * Copyright (C) 2013 Vladimir Serbinenko + * Copyright (C) 2018 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 @@ -43,4 +44,10 @@ /* Only since ICH5 */ int do_i2c_block_read(unsigned int smbus_base, u8 device, unsigned int offset, unsigned int bytes, u8 *buf); +int do_i2c_block_write(unsigned int smbus_base, u8 device, u8 cmd, + const unsigned int bytes, u8 *buf); +int i2c_block_read(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, + u32 bytes, u8 *buf); +int i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, + u32 bytes, u8 *buf); #endif
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
(4 comments)
Sorry, this will not be usable after the refactoring on smbus commands. I'll implement that i2c block write command you need, and make it available on all recent intels.
Frans, can you point me to a commit where you use i2c block write? Also no need to respond to the review comments here.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 482: outb(cmd, smbus_base + SMBHSTDAT1); This does not look right, putting cmd in to two registers.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 485: outb(bytes, smbus_base + SMBHSTDAT0); Why? Isn't the block length determined by how long we run the loop and when last_byte bit is set.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 498: /* Start the command */ For i2c block command there should be a single start and single stop visible on the i2c lines. The comment suggest there is a start for each byte?
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 503: if (smbus_wait_until_active(smbus_base) < 0) This was buggy; see https://review.coreboot.org/c/coreboot/+/21119
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
Patch Set 3:
(4 comments)
Sorry, this will not be usable after the refactoring on smbus commands. I'll implement that i2c block write command you need, and make it available on all recent intels.
Frans, can you point me to a commit where you use i2c block write? Also no need to respond to the review comments here.
I did not upload the commit where i2c block write is used yet. This function is used in an update of commit https://review.coreboot.org/c/coreboot/+/30414. Uploading without availability of the function will result into a gerrit build error.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 482: outb(cmd, smbus_base + SMBHSTDAT1);
This does not look right, putting cmd in to two registers.
When the controller is placed into I2C mode the output stream differs from SMBus. The I2C slave expects the next sequence: START [i2c_address] ACK [reg_address_15_8] ACK [reg_address_7_0] ACK [data7_0] ACK [data15_8] ACK [data23_16] ACK [data31_24] ACK STOP. The SMBHSTCMD byte is not send to the I2C device, but used to start SMBus with a command value. SMBHSTDAT1 is first byte send to I2C containing the reg_address_15_8
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 485: outb(bytes, smbus_base + SMBHSTDAT0);
Why? Isn't the block length determined by how long we run the loop and when last_byte bit is set.
The block length is not send to the slave address, but used for the SMBus controller only. The I2C
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 498: /* Start the command */
For i2c block command there should be a single start and single stop visible on the i2c lines. […]
For the used I2C slave device it's working. I'll check if one command is working also
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 503: if (smbus_wait_until_active(smbus_base) < 0)
This was buggy; see https://review.coreboot. […]
The new patch was created after this patch. Need to sync.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
Ok. I just wanted to be sure about the command format the slave side expects to see.
Unless you really want to, you don't need to rebase your changes. I think it's more valuable if you are available for testing the new code I push.
The trouble here is the lack of symmetry on i2c_block_read() and i2c_block_write(), as they use different command code fields on the host controller.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
(2 comments)
Patch Set 3:
Ok. I just wanted to be sure about the command format the slave side expects to see.
Unless you really want to, you don't need to rebase your changes. I think it's more valuable if you are available for testing the new code I push.
The trouble here is the lack of symmetry on i2c_block_read() and i2c_block_write(), as they use different command code fields on the host controller.
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 482: outb(cmd, smbus_base + SMBHSTDAT1);
When the controller is placed into I2C mode the output stream differs from SMBus. […]
Done
https://review.coreboot.org/#/c/30800/3/src/southbridge/intel/common/smbus.c... PS3, Line 485: outb(bytes, smbus_base + SMBHSTDAT0);
The block length is not send to the slave address, but used for the SMBus controller only. […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 3:
Patch Set 3:
(2 comments)
Patch Set 3:
Ok. I just wanted to be sure about the command format the slave side expects to see.
Unless you really want to, you don't need to rebase your changes. I think it's more valuable if you are available for testing the new code I push.
The trouble here is the lack of symmetry on i2c_block_read() and i2c_block_write(), as they use different command code fields on the host controller.
For sure I'm available for testing code.
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#4).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C commands
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Added i2c_block_read() and i2c_block_write() which configures the SMBus controller into I2C mode before sending the commands. Added do_i2c_block_write() for sending block writes in I2C mode.
BUG=N/A TEST=Facebook FBG-1701 booting Embedded Linux
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 76 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/30800/4/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/4/src/southbridge/intel/common/smbus.c... PS4, Line 474: return (bytes); return is not a function, parentheses are not required
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#5).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C commands ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C commands
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Added i2c_block_read() and i2c_block_write() which configures the SMBus controller into I2C mode before sending the commands. Added do_i2c_block_write() for sending block writes in I2C mode.
BUG=N/A TEST=Facebook FBG-1701 booting Embedded Linux
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 76 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/5
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#6).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Added i2c_block_write() which configures the SMBus controller into I2C mode before sending the commands. Added do_i2c_block_write() for sending block writes in I2C mode.
BUG=N/A TEST=Facebook FBG-1701 booting Embedded Linux
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 63 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/6
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#7).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 63 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/7
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#8).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 63 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/8
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, build bot (Jenkins), Patrick Georgi, Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#9).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports communication with I2C device using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 63 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/9
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 9:
Patch Set 1: Code-Review-2
Thanks, I'll pick the block write command from here.
The i2C block write is still not implemented in southbridge\intel\common. Should latest patch be integrated?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 9:
Kyösti do You have Your patch ready with generic i2c block write? Could You please link the patch here for review and testing?
If not, maybe You could unblock Frans and point him how You wanted to implement that? Thanks in advance.
Kyösti Mälkki has removed a vote on this change.
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Removed Code-Review-2 by Kyösti Mälkki kyosti.malkki@gmail.com
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 9:
(5 comments)
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h File src/southbridge/intel/common/smbus.h:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h... PS9, Line 44: /* Only since ICH5 */ Did this I2C_EN bit in PCI config space appear with ICH5 as well?
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h... PS9, Line 49: int i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd, I don't see why we need SMBUS PCI dev.fn passed here?
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 426: smb_ctlr_reg = pci_read_config32(smbus_dev, 0x40); Invent a name for register 0x40.
Now it would be cleaner to explicitly clear or set this bit inside setup_command(), but I quess we can leave that as followup work, as this particular commit is blocking some platform/board port, apparently.
Also I would have liked to see the symmetric i2c_block_read() implemented and tested here already.
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 443: if (!has_i2c_read_command()) Seems unrelated. Please check when I2C_EN was implemented in hardware and add a similar test.
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1); It's weird that you would need to write 'cmd' in two different host controller registers and without comments this looks like an error to me.
Intel datasheets for block commands are a mess, I imagine you really only need to write one of them.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 9:
(4 comments)
Will implement the comment in a new patchset.
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h File src/southbridge/intel/common/smbus.h:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h... PS9, Line 44: /* Only since ICH5 */
Did this I2C_EN bit in PCI config space appear with ICH5 as well?
Yes, Intel ICH5 datasheet contains documentation about this bit in HOSTC
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.h... PS9, Line 49: int i2c_block_write(u8 smbus_devnr, u8 smbus_func, unsigned int device, u8 cmd,
I don't see why we need SMBUS PCI dev. […]
Southbridge code is responsible of configuration of the SMBus controller. To enable the I2C mode the southbridge need to know on which PCI device the I2C_EN must be set.
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 443: if (!has_i2c_read_command())
Seems unrelated. Please check when I2C_EN was implemented in hardware and add a similar test.
Since ICH5. Will use same function for i2c_block_write()
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1);
It's weird that you would need to write 'cmd' in two different host controller registers and without […]
I agree that the Intel datasheet is not clear about this, but we did SMBus analysis on the system also. The SMBHSTCMD register is used by the SMBus controller itself. The first data that is sent on the SMBus is the value in the SMBHSTDAT1 register. This byte is required by the I2C slave to compose a 16-byte address
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, build bot (Jenkins), Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#10).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports I2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 71 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/10
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, build bot (Jenkins), Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#11).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports I2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 71 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/30800/12/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.h:
https://review.coreboot.org/#/c/30800/12/src/southbridge/intel/common/smbus.... PS12, Line 24: #define HOSTC_I2C_EN (1 << 2) please, no space before tabs
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, build bot (Jenkins), Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#13).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports I2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 71 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/13
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... PS13, Line 415: all other functions in this file expect "smbus_base" as argument. Should the HOSTC I2C_EN moved out if this function to have a similar function prototype?
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... PS13, Line 437: status = do_i2c_block_write((unsigned int)reg, device, cmd, bytes, cast not needed
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... PS13, Line 437: status = do_i2c_block_write((unsigned int)reg, device, cmd, bytes,
cast not needed
Will update in new patchset
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, build bot (Jenkins), Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#14).
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
southbridge/intel/common/smbus.c: Add support for I2C block write
Intel Braswell supports I2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in I2C mode.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/14
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for I2C block write ......................................................................
Patch Set 14:
(1 comment)
Patch Set 13:
(2 comments)
Any new comment?
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/13/src/southbridge/intel/common/smbus.... PS13, Line 415:
all other functions in this file expect "smbus_base" as argument. […]
How to proceed with this patch set and handle your comment? The I2C_EN is required to run int I2C mode. An alternative can be moving i2c_block_write() to SoC Braswell and have this support for Braswell only.
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#15).
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
southbridge/intel/common/smbus.c: Add support for i2c block write
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() is added to Braswell SoC to configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in i2c mode.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 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 M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 5 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/15
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#16).
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
southbridge/intel/common/smbus.c: Add support for i2c block write
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - i2c_block_write() - do_i2c_block_write()
i2c_block_write() is added to Braswell SoC to configures SMBus controller in I2C mode before calling do_i2c_block_write(). do_i2c_block_write() sends the block writes in i2c mode.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 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 M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 5 files changed, 93 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/16
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Arthur Heymans, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#17).
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
southbridge/intel/common/smbus.c: Add support for i2c block write
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add: - do_i2c_block_write()
do_i2c_block_write() sends the block writes using I2C protocol.
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/17
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17: Code-Review+1
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1);
I agree that the Intel datasheet is not clear about this, but we did SMBus analysis on the system al […]
I too see the need for a comment. Also, what is `cmd`? is it the offset?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1);
I too see the need for a comment. […]
'cmd' is the 'offset'. For the i2c this is the higher past of the 16-byte address.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17: Code-Review-1
(1 comment)
Sorry, I don't understand this. Is the datasheet of the tested I2C slave public? AFAICT, the current code would issue an SMBUS (not I2C) transaction.
https://review.coreboot.org/#/c/30800/17/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/17/src/southbridge/intel/common/smbus.... PS17, Line 424: I801_BLOCK_DATA This is not the I2C command. I wonder now what is the difference here to do_smbus_block_write()?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1);
'cmd' is the 'offset'. For the i2c this is the higher past of the 16-byte address.
You mean a 16-bit address? but where is the lower part?
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/9/src/southbridge/intel/common/smbus.c... PS9, Line 453: outb(cmd, smbus_base + SMBHSTDAT1);
You mean a 16-bit address? but where is the lower part?
First byte of the block_cmd_loop() is the lower part. In the I2C mode the controller sends out the value in SMBHSTDAT1, when a command is init.
https://review.coreboot.org/#/c/30800/17/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/17/src/southbridge/intel/common/smbus.... PS17, Line 424: I801_BLOCK_DATA
This is not the I2C command. I wonder now what is the difference […]
The do_smbus_block_write() does not check for i2c support and does not write the cmd to SMBHSTDAT1 register.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/30800/17/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/17/src/southbridge/intel/common/smbus.... PS17, Line 424: I801_BLOCK_DATA
The do_smbus_block_write() does not check for i2c support and does not write the cmd to SMBHSTDAT1 r […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
this patch is superseded by 33225, right?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);
i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }
AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);
For your chip, this would be called with `offset_bytes == 2`.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
if (count) { cmd = buf[0]; buf++; }
I forgot `count--;` ._.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);For your chip, this would be called with `offset_bytes == 2`.
FYI: I have tested (building/booting/working) all patches on facebook fbg1701.
You're right that this patch is superseded by CB:33225. Using this patchset it does not influence southbridge support. For Braswell I'm 100% sure that it's working on a system, can't test other chipsets. Working combinations are: CB:33225 patchset #3 rebased on CB:30800 patchset #17 CB:33225 patchest #5
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
Patch Set 17:
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);For your chip, this would be called with `offset_bytes == 2`.
FYI: I have tested (building/booting/working) all patches on facebook fbg1701.
You're right that this patch is superseded by CB:33225. Using this patchset it does not influence southbridge support. For Braswell I'm 100% sure that it's working on a system, can't test other chipsets. Working combinations are: CB:33225 patchset #3 rebased on CB:30800 patchset #17 CB:33225 patchest #5
I suggest using CB:33225. The function smbus_i2c_block_write() will input parameters address, offset, bytes and *buf. I'll upload the mainboard functions where this support is used later today.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
Patch Set 17:
Patch Set 17:
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);For your chip, this would be called with `offset_bytes == 2`.
FYI: I have tested (building/booting/working) all patches on facebook fbg1701.
You're right that this patch is superseded by CB:33225. Using this patchset it does not influence southbridge support. For Braswell I'm 100% sure that it's working on a system, can't test other chipsets. Working combinations are: CB:33225 patchset #3 rebased on CB:30800 patchset #17 CB:33225 patchest #5
I suggest using CB:33225. The function smbus_i2c_block_write() will input parameters address, offset, bytes and *buf. I'll upload the mainboard functions where this support is used later today.
https://review.coreboot.org/c/coreboot/+/33433 contains the calling function of the smbus_i2c_block_write().
I prefer to focus and merge SoC Braswell patchset CB:33225. Then we might consider to abondon this patchset (at the moment?) Or have both patchset?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);For your chip, this would be called with `offset_bytes == 2`.
FYI: I have tested (building/booting/working) all patches on facebook fbg1701.
You're right that this patch is superseded by CB:33225. Using this patchset it does not influence southbridge support. For Braswell I'm 100% sure that it's working on a system, can't test other chipsets. Working combinations are: CB:33225 patchset #3 rebased on CB:30800 patchset #17 CB:33225 patchest #5
I suggest using CB:33225. The function smbus_i2c_block_write() will input parameters address, offset, bytes and *buf. I'll upload the mainboard functions where this support is used later today.
https://review.coreboot.org/c/coreboot/+/33433 contains the calling function of the smbus_i2c_block_write().
I prefer to focus and merge SoC Braswell patchset CB:33225. Then we might consider to abondon this patchset (at the moment?) Or have both patchset?
Ack, if it's working without this, we should abandon it.
Btw. this also shows that SMBHSTDAT1 is not involved but SMBHSTCMD is used instead to transfer the first byte.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
Btw. this also shows that SMBHSTDAT1 is not involved but SMBHSTCMD is used instead to transfer the first byte.
Sorry, this was a wrong assumption. I didn't realize that you moved the write to SMBHSTDAT1 into the other change. Can you please also test it without any write to SMBHSTDAT1? The datasheet doesn't mention the use of SMBHSTDAT1, if it is really used, we should add a comment that the documen- tation is incomplete.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
Patch Set 17:
this patch is superseded by 33225, right?
What Felix noticed is that in CB:33225, since patch set 4, you are not calling the newly added function here, but the original do_smbus_block_write(). Did you have time yet to test this version?
We discussed this further and seemed to understand things better with the exception of the second address byte. Do you pass that as part of the `buf` argument? Can you please point us to / push the calling code to Gerrit?
To make the interface clearer, the function that sets I2C_EN and calls do_*_block_write() should have a signature like this:
int i2c_block_write(device, unsigned int slave_device, unsigned int count, const uint8_t *buf);i.e. without any address or cmd parameter because these are either SMBus or slave-chip specific.
This function would then have to split the first byte out and pass it as `cmd` because of the weird way Intel handles I2C block transfers. e.g.
if (count) { cmd = buf[0]; buf++; }AFAICT :) hopefully Felix agrees.
On top of this, one could add a convinience function with an additional `offset` parameter and the number of bytes that the chip expects for the offset. e.g.
int i2c_block_write_at(device, unsigned int slave_device, unsigned int offset, unsigned int offset_bytes, unsigned int count, const uint8_t *buf);For your chip, this would be called with `offset_bytes == 2`.
FYI: I have tested (building/booting/working) all patches on facebook fbg1701.
You're right that this patch is superseded by CB:33225. Using this patchset it does not influence southbridge support. For Braswell I'm 100% sure that it's working on a system, can't test other chipsets. Working combinations are: CB:33225 patchset #3 rebased on CB:30800 patchset #17 CB:33225 patchest #5
I suggest using CB:33225. The function smbus_i2c_block_write() will input parameters address, offset, bytes and *buf. I'll upload the mainboard functions where this support is used later today.
https://review.coreboot.org/c/coreboot/+/33433 contains the calling function of the smbus_i2c_block_write().
I prefer to focus and merge SoC Braswell patchset CB:33225. Then we might consider to abondon this patchset (at the moment?) Or have both patchset?
Ack, if it's working without this, we should abandon it.
Btw. this also shows that SMBHSTDAT1 is not involved but SMBHSTCMD is used instead to transfer the first byte.
The value of SMBHSTDAT1 is send on the SMBus after the SMBHSTCMD byte is written. The value in SMBHSTCMD is used by the SMBus controller, but not forward to the host bus.
I will abandon this patch when the Braswell patch is reviewed and merged.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus.c: Add support for i2c block write ......................................................................
Patch Set 17:
I prefer to focus and merge SoC Braswell patchset CB:33225.
Getting CB:33225 into shape is probably much easier and since the code using the I2C write via SMBUS controller is only for the Braswell platform at the moment, working on this patch isn't necessary to get the prerequisites for the mainboard code in the tree.
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#18).
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
southbridge/intel/common/smbus: Add do_i2c_block_write()
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add do_i2c_block_write() which is a based on do_smbus_block_write() but also write first byte to SMBHSTDAT1 he caller needs to configure the SMBus controller in i2c mode.
In i2c mode SMBus controller will send the next sequence: SMBXINTADD, SMBHSTDAT1, SMBBLKDAT .. SMBBLKDAT
To ensure the the command is send over the bus the SMBHSTCMD register must be written also
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/18
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/#/c/30800/18/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/18/src/southbridge/intel/common/smbus.... PS18, Line 424: if (!bytes || (bytes > SMBUS_BLOCK_MAXLEN) ) space prohibited before that close parenthesis ')'
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#19).
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
southbridge/intel/common/smbus: Add do_i2c_block_write()
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add do_i2c_block_write() which is a based on do_smbus_block_write() but also write first byte to SMBHSTDAT1 he caller needs to configure the SMBus controller in i2c mode.
In i2c mode SMBus controller will send the next sequence: SMBXINTADD, SMBHSTDAT1, SMBBLKDAT .. SMBBLKDAT
To ensure the the command is send over the bus the SMBHSTCMD register must be written also
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/19
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
Patch Set 20: Code-Review+1
(5 comments)
https://review.coreboot.org/#/c/30800/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30800/20//COMMIT_MSG@13 PS20, Line 13: also write first byte to SMBHSTDAT1 End full sentences with period.
https://review.coreboot.org/#/c/30800/20//COMMIT_MSG@14 PS20, Line 14: he caller needs to configure the SMBus controller in i2c mode. The
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... PS20, Line 415: int do_i2c_block_write(unsigned int smbus_base, u8 device, Please add a comment the caller is currently (and sort of incorrectly) responsible of setting HOSTC I2C_EN bit prior to making this call.
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... PS20, Line 436: * will generate the i2c sequence. Please file a bug thru the official channels to Intel on this.
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... PS20, Line 441: outb(cmd, smbus_base + SMBHSTDAT1); I notice the order of CMD and DAT1 access changed. Hopefully the hardware is equally stable both ways.
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#21).
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
southbridge/intel/common/smbus: Add do_i2c_block_write()
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add do_i2c_block_write() which is a based on do_smbus_block_write() but also write first byte to SMBHSTDAT1. The caller needs to configure the SMBus controller in i2c mode.
In i2c mode SMBus controller will send the next sequence: SMBXINTADD, SMBHSTDAT1, SMBBLKDAT .. SMBBLKDAT
To ensure the the command is send over the bus the SMBHSTCMD register must be written also
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/21
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
Patch Set 21:
(5 comments)
https://review.coreboot.org/#/c/30800/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30800/20//COMMIT_MSG@13 PS20, Line 13: also write first byte to SMBHSTDAT1
End full sentences with period.
Done
https://review.coreboot.org/#/c/30800/20//COMMIT_MSG@14 PS20, Line 14: he caller needs to configure the SMBus controller in i2c mode.
The
Done
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... File src/southbridge/intel/common/smbus.c:
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... PS20, Line 415: int do_i2c_block_write(unsigned int smbus_base, u8 device,
Please add a comment the caller is currently (and sort of incorrectly) responsible of setting HOSTC […]
Done
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... PS20, Line 436: * will generate the i2c sequence.
Please file a bug thru the official channels to Intel on this.
Done
https://review.coreboot.org/#/c/30800/20/src/southbridge/intel/common/smbus.... PS20, Line 441: outb(cmd, smbus_base + SMBHSTDAT1);
I notice the order of CMD and DAT1 access changed. […]
Both sequence work. These registers need to be configured before execute_commond().
Hello Kyösti Mälkki, Patrick Rudolph, Piotr Król, Arthur Heymans, David Hendricks, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Michał Żygowski, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30800
to look at the new patch set (#22).
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
southbridge/intel/common/smbus: Add do_i2c_block_write()
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add do_i2c_block_write() which is a based on do_smbus_block_write() but also write first byte to SMBHSTDAT1. The caller needs to configure the SMBus controller in i2c mode.
In i2c mode SMBus controller will send the next sequence: SMBXINTADD, SMBHSTDAT1, SMBBLKDAT .. SMBBLKDAT
To ensure the the command is send over the bus the SMBHSTCMD register must be written also
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/30800/22
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
Patch Set 22: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30800 )
Change subject: southbridge/intel/common/smbus: Add do_i2c_block_write() ......................................................................
southbridge/intel/common/smbus: Add do_i2c_block_write()
Intel Braswell supports i2c block writes using SMBus controller. This support is missing in actual smbus routines.
Add do_i2c_block_write() which is a based on do_smbus_block_write() but also write first byte to SMBHSTDAT1. The caller needs to configure the SMBus controller in i2c mode.
In i2c mode SMBus controller will send the next sequence: SMBXINTADD, SMBHSTDAT1, SMBBLKDAT .. SMBBLKDAT
To ensure the the command is send over the bus the SMBHSTCMD register must be written also
BUG=N/A TEST=Config eDP for LCD display on Facebook FBG-1701
Change-Id: I40f8c0f5257a62398189f36892b8159052481693 Signed-off-by: Frans Hendriks fhendriks@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/30800 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/intel/common/smbus.c M src/southbridge/intel/common/smbus.h 2 files changed, 47 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/southbridge/intel/common/smbus.c b/src/southbridge/intel/common/smbus.c index af1eb60..e575abc 100644 --- a/src/southbridge/intel/common/smbus.c +++ b/src/southbridge/intel/common/smbus.c @@ -4,6 +4,7 @@ * Copyright (C) 2005 Yinghai Lu yinghailu@gmail.com * Copyright (C) 2009 coresystems GmbH * Copyright (C) 2013 Vladimir Serbinenko + * Copyright (C) 2018-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 @@ -410,3 +411,47 @@
return ret; } + +/* + * The caller is responsible of settings HOSTC I2C_EN bit prior to making this + * call! + */ +int do_i2c_block_write(unsigned int smbus_base, u8 device, + unsigned int bytes, u8 *buf) +{ + u8 cmd; + int ret; + + if (!CONFIG(SOC_INTEL_BRASWELL)) + return SMBUS_ERROR; + + if (!bytes || (bytes > SMBUS_BLOCK_MAXLEN)) + return SMBUS_ERROR; + + /* Set up for a block data write. */ + ret = setup_command(smbus_base, I801_BLOCK_DATA, XMIT_WRITE(device)); + if (ret < 0) + return ret; + + /* + * In i2c mode SMBus controller sequence on bus will be: + * <SMBXINTADD> <SMBHSTDAT1> <SMBBLKDAT> .. <SMBBLKDAT> + * The SMBHSTCMD must be written also to ensure the SMBUs controller + * will generate the i2c sequence. + */ + cmd = *buf++; + bytes--; + outb(cmd, smbus_base + SMBHSTCMD); + outb(cmd, smbus_base + SMBHSTDAT1); + + /* Execute block transaction. */ + ret = block_cmd_loop(smbus_base, buf, bytes, BLOCK_WRITE); + if (ret < 0) + return ret; + + if (ret < bytes) + return SMBUS_ERROR; + + ret++; /* 1st byte has been written using SMBHSTDAT1 */ + return ret; +} diff --git a/src/southbridge/intel/common/smbus.h b/src/southbridge/intel/common/smbus.h index ded31d0..4875581 100644 --- a/src/southbridge/intel/common/smbus.h +++ b/src/southbridge/intel/common/smbus.h @@ -43,4 +43,6 @@ /* Only since ICH5 */ int do_i2c_eeprom_read(unsigned int smbus_base, u8 device, unsigned int offset, unsigned int bytes, u8 *buf); +int do_i2c_block_write(unsigned int smbus_base, u8 device, + unsigned int bytes, u8 *buf); #endif
-
build bot (Jenkins) (Code Review) -
David Hendricks (Code Review)
-
Felix Held (Code Review)
-
Frans Hendriks (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)