<p>Richard Spiegel has uploaded this change for <strong>review</strong>.</p><p><a href="https://review.coreboot.org/21887">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">soc/amd/stoneyridge: SMBUS code simplify<br><br>Solve issues left from review 19722. Unify code: smbus.c to have the actual<br>execution code, sm.c and smbus_spd.c call functions within smbus.c. Include<br>file smbus.h should only be used by sm.c, smbus.c and smbus_spd.c.<br><br>BUG=b:62200225<br><br>Change-Id: Ibd55560c95b6752652a4f255b04198e7a4e77d05<br>Signed-off-by: Richard Spiegel <richard.spiegel@silverbackltd.com><br>---<br>M src/soc/amd/stoneyridge/include/soc/smbus.h<br>M src/soc/amd/stoneyridge/sm.c<br>M src/soc/amd/stoneyridge/smbus.c<br>M src/soc/amd/stoneyridge/smbus_spd.c<br>M src/soc/amd/stoneyridge/southbridge.c<br>5 files changed, 92 insertions(+), 153 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/21887/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/src/soc/amd/stoneyridge/include/soc/smbus.h b/src/soc/amd/stoneyridge/include/soc/smbus.h<br>index 727f72e..e1b4c5f 100644<br>--- a/src/soc/amd/stoneyridge/include/soc/smbus.h<br>+++ b/src/soc/amd/stoneyridge/include/soc/smbus.h<br>@@ -18,57 +18,60 @@<br> <br> #include <stdint.h><br> <br>-#define SMBHSTSTAT 0x0<br>-#define SMBHST_STAT_FAILED 0x10<br>-#define SMBHST_STAT_COLLISION 0x08<br>+#define SMBHSTSTAT 0x0<br>+#define SMBHST_STAT_FAILED 0x10<br>+#define SMBHST_STAT_COLLISION 0x08<br> #define SMBHST_STAT_ERROR 0x04<br>-#define SMBHST_STAT_INTERRUPT 0x02<br>+#define SMBHST_STAT_INTERRUPT 0x02<br> #define SMBHST_STAT_BUSY 0x01<br> #define SMBHST_STAT_CLEAR 0xff<br>-#define SMBHST_STAT_NOERROR 0x02<br>+#define SMBHST_STAT_NOERROR 0x02<br>+#define SMBHST_STAT_VAL_BITS 0x1f<br>+#define SMBHST_STAT_ERROR_BITS 0x1c<br> <br>-#define SMBSLVSTAT 0x1<br>+#define SMBSLVSTAT 0x1<br> #define SMBSLV_STAT_ALERT 0x20<br>-#define SMBSLV_STAT_SHADOW2 0x10<br>-#define SMBSLV_STAT_SHADOW1 0x08<br>-#define SMBSLV_STAT_SLV_STS 0x04<br>-#define SMBSLV_STAT_SLV_INIT 0x02<br>-#define SMBSLV_STAT_SLV_BUSY 0x01<br>+#define SMBSLV_STAT_SHADOW2 0x10<br>+#define SMBSLV_STAT_SHADOW1 0x08<br>+#define SMBSLV_STAT_SLV_STS 0x04<br>+#define SMBSLV_STAT_SLV_INIT 0x02<br>+#define SMBSLV_STAT_SLV_BUSY 0x01<br> #define SMBSLV_STAT_CLEAR 0x1f<br> <br>-#define SMBHSTCTRL 0x2<br>+#define SMBHSTCTRL 0x2<br> #define SMBHST_CTRL_RST 0x80<br> #define SMBHST_CTRL_STRT 0x40<br>-#define SMBHST_CTRL_QCK_RW 0x00<br>-#define SMBHST_CTRL_BTE_RW 0x04<br>-#define SMBHST_CTRL_BDT_RW 0x08<br>-#define SMBHST_CTRL_WDT_RW 0x0c<br>-#define SMBHST_CTRL_BLK_RW 0x14<br>+#define SMBHST_CTRL_QCK_RW 0x00<br>+#define SMBHST_CTRL_BTE_RW 0x04<br>+#define SMBHST_CTRL_BDT_RW 0x08<br>+#define SMBHST_CTRL_WDT_RW 0x0c<br>+#define SMBHST_CTRL_BLK_RW 0x14<br> #define SMBHST_CTRL_KILL 0x02<br> #define SMBHST_CTRL_IEN 0x01<br>+#define SMBHST_CTRL_MODE_BITS 0x1c<br> <br>-#define SMBHSTCMD 0x3<br>-#define SMBHSTADDR 0x4<br>-#define SMBHSTDAT0 0x5<br>-#define SMBHSTDAT1 0x6<br>-#define SMBHSTBLKDAT 0x7<br>-#define SMBSLVCTRL 0x8<br>-#define SMBSLVCMD_SHADOW 0x9<br>-#define SMBSLVEVT 0xa<br>-#define SMBSLVDAT 0xc<br>-#define SMBTIMING 0xe<br>+#define SMBHSTCMD 0x3<br>+#define SMBHSTADDR 0x4<br>+#define SMBHSTDAT0 0x5<br>+#define SMBHSTDAT1 0x6<br>+#define SMBHSTBLKDAT 0x7<br>+#define SMBSLVCTRL 0x8<br>+#define SMBSLVCMD_SHADOW 0x9<br>+#define SMBSLVEVT 0xa<br>+#define SMBSLVDAT 0xc<br>+#define SMBTIMING 0xe<br> <br>-#define SMB_BASE_ADDR 0xb00<br>+#define SMB_BASE_ADDR 0xb00<br> <br>-#define AX_INDXC 0<br>-#define AX_INDXP 2<br>-#define AXCFG 4<br>-#define ABCFG 6<br>-#define RC_INDXC 1<br>-#define RC_INDXP 3<br>+#define AX_INDXC 0<br>+#define AX_INDXP 2<br>+#define AXCFG 4<br>+#define ABCFG 6<br>+#define RC_INDXC 1<br>+#define RC_INDXP 3<br> <br>-#define AB_INDX 0xcd8<br>-#define AB_DATA (AB_INDX+4)<br>+#define AB_INDX 0xcd8<br>+#define AB_DATA (AB_INDX+4)<br> <br> /*<br> * Between 1-10 seconds, We should never timeout normally<br>diff --git a/src/soc/amd/stoneyridge/sm.c b/src/soc/amd/stoneyridge/sm.c<br>index 0c31a3e..3898a1f 100644<br>--- a/src/soc/amd/stoneyridge/sm.c<br>+++ b/src/soc/amd/stoneyridge/sm.c<br>@@ -108,20 +108,12 @@<br> .write_byte = lsmbus_write_byte,<br> };<br> <br>-static void sm_read_resources(device_t dev)<br>-{<br>-}<br>-<br>-static void sm_set_resources(struct device *dev)<br>-{<br>-}<br>-<br> static struct pci_operations lops_pci = {<br> .set_subsystem = pci_dev_set_subsystem,<br> };<br> static struct device_operations smbus_ops = {<br>- .read_resources = sm_read_resources,<br>- .set_resources = sm_set_resources,<br>+ .read_resources = DEVICE_NOOP,<br>+ .set_resources = DEVICE_NOOP,<br> .enable_resources = pci_dev_enable_resources,<br> .init = sm_init,<br> .scan_bus = scan_smbus,<br>diff --git a/src/soc/amd/stoneyridge/smbus.c b/src/soc/amd/stoneyridge/smbus.c<br>index b216f1e..855f86c 100644<br>--- a/src/soc/amd/stoneyridge/smbus.c<br>+++ b/src/soc/amd/stoneyridge/smbus.c<br>@@ -24,7 +24,7 @@<br> do {<br> u8 val;<br> val = inb(smbus_io_base + SMBHSTSTAT);<br>- val &= 0x1f;<br>+ val &= SMBHST_STAT_VAL_BITS;<br> if (val == 0) { /* ready now */<br> return 0;<br> }<br>@@ -41,10 +41,10 @@<br> u8 val;<br> <br> val = inb(smbus_io_base + SMBHSTSTAT);<br>- val &= 0x1f; /* mask off reserved bits */<br>- if (val & 0x1c)<br>+ val &= SMBHST_STAT_VAL_BITS; /* mask off reserved bits */<br>+ if (val & SMBHST_STAT_ERROR_BITS)<br> return -5; /* error */<br>- if (val == 0x02) {<br>+ if (val == SMBHST_STAT_NOERROR) {<br> outb(val, smbus_io_base + SMBHSTSTAT); /* clear sts */<br> return 0;<br> }<br>@@ -63,8 +63,8 @@<br> outb(((device & 0x7f) << 1) | 1, smbus_io_base + SMBHSTADDR);<br> <br> byte = inb(smbus_io_base + SMBHSTCTRL);<br>- byte &= 0xe3; /* Clear [4:2] */<br>- byte |= (1 << 2) | (1 << 6); /* Byte data R/W cmd, start the command */<br>+ byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */<br>+ byte |= SMBHST_CTRL_STRT + SMBHST_CTRL_BTE_RW; /* set mode, start */<br> outb(byte, smbus_io_base + SMBHSTCTRL);<br> <br> /* poll for transaction completion */<br>@@ -72,7 +72,7 @@<br> return -3; /* timeout or error */<br> <br> /* read results of transaction */<br>- byte = inb(smbus_io_base + SMBHSTCMD);<br>+ byte = inb(smbus_io_base + SMBHSTDAT0);<br> <br> return byte;<br> }<br>@@ -91,8 +91,8 @@<br> outb(((device & 0x7f) << 1) | 0, smbus_io_base + SMBHSTADDR);<br> <br> byte = inb(smbus_io_base + SMBHSTCTRL);<br>- byte &= 0xe3; /* Clear [4:2] */<br>- byte |= (1 << 2) | (1 << 6); /* Byte data R/W cmd, start command */<br>+ byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */<br>+ byte |= SMBHST_CTRL_STRT + SMBHST_CTRL_BTE_RW; /* set mode, start */<br> outb(byte, smbus_io_base + SMBHSTCTRL);<br> <br> /* poll for transaction completion */<br>@@ -102,8 +102,7 @@<br> return 0;<br> }<br> <br>-int do_smbus_read_byte(u32 smbus_io_base, u32 device,<br>- u32 address)<br>+int do_smbus_read_byte(u32 smbus_io_base, u32 device, u32 address)<br> {<br> u8 byte;<br> <br>@@ -117,8 +116,8 @@<br> outb(((device & 0x7f) << 1) | 1, smbus_io_base + SMBHSTADDR);<br> <br> byte = inb(smbus_io_base + SMBHSTCTRL);<br>- byte &= 0xe3; /* Clear [4:2] */<br>- byte |= (1 << 3) | (1 << 6); /* Byte data R/W cmd, start command */<br>+ byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */<br>+ byte |= SMBHST_CTRL_STRT + SMBHST_CTRL_BDT_RW; /* set mode, start */<br> outb(byte, smbus_io_base + SMBHSTCTRL);<br> <br> /* poll for transaction completion */<br>@@ -131,8 +130,7 @@<br> return byte;<br> }<br> <br>-int do_smbus_write_byte(u32 smbus_io_base, u32 device,<br>- u32 address, u8 val)<br>+int do_smbus_write_byte(u32 smbus_io_base, u32 device, u32 address, u8 val)<br> {<br> u8 byte;<br> <br>@@ -149,8 +147,8 @@<br> outb(val, smbus_io_base + SMBHSTDAT0);<br> <br> byte = inb(smbus_io_base + SMBHSTCTRL);<br>- byte &= 0xe3; /* Clear [4:2] */<br>- byte |= (1 << 3) | (1 << 6); /* Byte data R/W cmd, start command */<br>+ byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */<br>+ byte |= SMBHST_CTRL_STRT + SMBHST_CTRL_BDT_RW; /* set mode, start */<br> outb(byte, smbus_io_base + SMBHSTCTRL);<br> <br> /* poll for transaction completion */<br>@@ -160,8 +158,7 @@<br> return 0;<br> }<br> <br>-void alink_ab_indx(u32 reg_space, u32 reg_addr,<br>- u32 mask, u32 val)<br>+void alink_ab_indx(u32 reg_space, u32 reg_addr, u32 mask, u32 val)<br> {<br> u32 tmp;<br> <br>@@ -185,8 +182,7 @@<br> outl(0, AB_INDX);<br> }<br> <br>-void alink_rc_indx(u32 reg_space, u32 reg_addr, u32 port,<br>- u32 mask, u32 val)<br>+void alink_rc_indx(u32 reg_space, u32 reg_addr, u32 port, u32 mask, u32 val)<br> {<br> u32 tmp;<br> <br>@@ -210,7 +206,8 @@<br> outl(0, AB_INDX);<br> }<br> <br>-/* space = 0: AX_INDXC, AX_DATAC<br>+/*<br>+ * space = 0: AX_INDXC, AX_DATAC<br> * space = 1: AX_INDXP, AX_DATAP<br> */<br> void alink_ax_indx(u32 space /*c or p? */, u32 axindc, u32 mask, u32 val)<br>diff --git a/src/soc/amd/stoneyridge/smbus_spd.c b/src/soc/amd/stoneyridge/smbus_spd.c<br>index 167929c..eb04a3e 100644<br>--- a/src/soc/amd/stoneyridge/smbus_spd.c<br>+++ b/src/soc/amd/stoneyridge/smbus_spd.c<br>@@ -2,6 +2,7 @@<br> * This file is part of the coreboot project.<br> *<br> * Copyright (C) 2012 Advanced Micro Devices, Inc.<br>+ * Copyright (C) 2017 Silverback, Ltd.<br> *<br> * This program is free software; you can redistribute it and/or modify<br> * it under the terms of the GNU General Public License as published by<br>@@ -13,82 +14,16 @@<br> * GNU General Public License for more details.<br> */<br> <br>+#include <io.h><br> #include <device/pci_def.h><br> #include <device/device.h><br> <br>-/* warning: Porting.h includes an open #pragma pack(1) */<br> #include <Porting.h><br> #include <AGESA.h><br> #include <amdlib.h><br> #include <soc/southbridge.h><br> #include <soc/smbus.h><br> #include <dimmSpd.h><br>-<br>-/*<br>- * readSmbusByteData - read a single SPD byte from any offset<br>- */<br>-static int readSmbusByteData(int iobase, int address, char *buffer, int offset)<br>-{<br>- unsigned int status;<br>- UINT64 limit;<br>-<br>- address |= 1; // set read bit<br>-<br>- __outbyte(iobase + SMBHSTSTAT, SMBHST_STAT_CLEAR);<br>- __outbyte(iobase + SMBSLVSTAT, SMBSLV_STAT_CLEAR);<br>- __outbyte(iobase + SMBHSTCMD, offset); // offset in eeprom<br>- __outbyte(iobase + SMBHSTADDR, address); // slave addr & read bit<br>- __outbyte(iobase + SMBHSTCTRL, SMBHST_CTRL_STRT + SMBHST_CTRL_BDT_RW);<br>-<br>- // time limit to avoid hanging for unexpected error status<br>- limit = __rdtsc() + 2000000000 / 10;<br>- for (;;) {<br>- status = __inbyte(iobase + SMBHSTSTAT);<br>- if (__rdtsc() > limit)<br>- break;<br>- if ((status & SMBHST_STAT_INTERRUPT) == 0)<br>- continue; // SMBusInterrupt not set, keep waiting<br>- if ((status & SMBHST_STAT_BUSY) == SMBHST_STAT_BUSY)<br>- continue; // HostBusy set, keep waiting<br>- break;<br>- }<br>-<br>- buffer[0] = __inbyte(iobase + SMBHSTDAT0);<br>- if (status == SMBHST_STAT_NOERROR)<br>- status = 0; // done with no errors<br>- return status;<br>-}<br>-<br>-/*<br>- * readSmbusByte - read a single SPD byte from the default offset<br>- * this function is faster function readSmbusByteData<br>- */<br>-static int readSmbusByte(int iobase, int address, char *buffer)<br>-{<br>- unsigned int status;<br>- UINT64 limit;<br>-<br>- __outbyte(iobase + SMBHSTSTAT, SMBHST_STAT_CLEAR);<br>- __outbyte(iobase + SMBHSTCTRL, SMBHST_CTRL_STRT + SMBHST_CTRL_BTE_RW);<br>-<br>- // time limit to avoid hanging for unexpected error status<br>- limit = __rdtsc() + 2000000000 / 10;<br>- for (;;) {<br>- status = __inbyte(iobase + SMBHSTSTAT);<br>- if (__rdtsc() > limit)<br>- break;<br>- if ((status & SMBHST_STAT_INTERRUPT) == 0)<br>- continue; // SMBusInterrupt not set, keep waiting<br>- if ((status & SMBHST_STAT_BUSY) == SMBHST_STAT_BUSY)<br>- continue; // HostBusy set, keep waiting<br>- break;<br>- }<br>-<br>- buffer[0] = __inbyte(iobase + SMBHSTDAT0);<br>- if (status == SMBHST_STAT_NOERROR)<br>- status = 0; // done with no errors<br>- return status;<br>-}<br> <br> /*<br> * readspd - Read one or more SPD bytes from a DIMM.<br>@@ -99,27 +34,40 @@<br> */<br> static int readspd(int iobase, int SmbusSlaveAddress, char *buffer, int count)<br> {<br>+ u32 smbus_base, dev_addr;<br> int index, error;<br>+ char *pbuf = buffer;<br> <br> printk(BIOS_SPEW, "-------------READING SPD-----------\n");<br> printk(BIOS_SPEW, "iobase: 0x%08X, SmbusSlave: 0x%08X, count: %d\n",<br> iobase, SmbusSlaveAddress, count);<br> <br>- /* read the first byte using offset zero */<br>- error = readSmbusByteData(iobase, SmbusSlaveAddress, buffer, 0);<br>+ /*<br>+ * Convert received parameters to the format accepted by<br>+ * do_smbus_read_byte and do_smbus_recv_byte.<br>+ */<br>+ dev_addr = (u32)((SmbusSlaveAddress & 0xfe) >> 1);<br>+ smbus_base = (u32) iobase;<br> <br>- if (error) {<br>+ /* Read the first SPD byte */<br>+ error = do_smbus_read_byte(smbus_base, dev_addr, 0);<br>+ if (error < 0) {<br> printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n");<br> return error;<br>+ } else {<br>+ *pbuf = (char) (error & 0xff);<br>+ pbuf++;<br> }<br> <br>- /* read the remaining bytes using auto-increment for speed */<br>+ /* Read the remaining SPD bytes using do_smbus_recv_byte for speed */<br> for (index = 1 ; index < count ; index++) {<br>- error = readSmbusByte(iobase, SmbusSlaveAddress,<br>- &buffer[index]);<br>- if (error) {<br>+ error = do_smbus_recv_byte(smbus_base, dev_addr);<br>+ if (error < 0) {<br> printk(BIOS_ERR, "-------------SPD READ ERROR-----------\n");<br> return error;<br>+ } else {<br>+ *pbuf = (char) (error & 0xff);<br>+ pbuf++;<br> }<br> }<br> printk(BIOS_SPEW, "\n");<br>@@ -130,8 +78,8 @@<br> <br> static void writePmReg(int reg, int data)<br> {<br>- __outbyte(PM_INDEX, reg);<br>- __outbyte(PM_DATA, data);<br>+ outb(reg, PM_INDEX);<br>+ outb(data, PM_DATA);<br> }<br> <br> static void setupFch(int ioBase)<br>@@ -141,6 +89,9 @@<br> writePmReg(0x2c, ioBase | 1);<br> /* set SMBus clock to 400 KHz */<br> __outbyte(ioBase + SMBTIMING, 66000000 / 400000 / 4);<br>+ /* Clear all SMBUS status bits */<br>+ outb(SMBHST_STAT_CLEAR, ioBase + SMBHSTSTAT);<br>+ outb(SMBSLV_STAT_CLEAR, ioBase + SMBSLVSTAT);<br> }<br> <br> int sb_readSpd(int spdAddress, char *buf, size_t len)<br>diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c<br>index 5e36100..895e45a 100644<br>--- a/src/soc/amd/stoneyridge/southbridge.c<br>+++ b/src/soc/amd/stoneyridge/southbridge.c<br>@@ -26,11 +26,8 @@<br> #include <cbmem.h><br> #include <amd_pci_util.h><br> #include <soc/southbridge.h><br>-#include <soc/smbus.h><br> #include <soc/smi.h><br>-#if IS_ENABLED(CONFIG_STONEYRIDGE_IMC_FWM)<br> #include <fchec.h><br>-#endif<br> <br> <br> int acpi_get_sleep_type(void)<br>@@ -98,12 +95,11 @@<br> <br> void southbridge_final(void *chip_info)<br> {<br>-#if IS_ENABLED(CONFIG_STONEYRIDGE_IMC_FWM)<br>- agesawrapper_fchecfancontrolservice();<br>-#if !IS_ENABLED(CONFIG_ACPI_ENABLE_THERMAL_ZONE)<br>- enable_imc_thermal_zone();<br>-#endif<br>-#endif<br>+ if (IS_ENABLED(CONFIG_STONEYRIDGE_IMC_FWM)) {<br>+ agesawrapper_fchecfancontrolservice();<br>+ if (IS_ENABLED(CONFIG_ACPI_ENABLE_THERMAL_ZONE))<br>+ enable_imc_thermal_zone();<br>+ }<br> }<br> <br> /*<br></pre><p>To view, visit <a href="https://review.coreboot.org/21887">change 21887</a>. To unsubscribe, visit <a href="https://review.coreboot.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://review.coreboot.org/21887"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: coreboot </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ibd55560c95b6752652a4f255b04198e7a4e77d05 </div>
<div style="display:none"> Gerrit-Change-Number: 21887 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Spiegel <richard.spiegel@silverbackltd.com> </div>