Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBUS source ......................................................................
soc/amd/picasso: Drop forked copy of SMBUS source
Change-Id: I48a2799a6160cb04fed7792e4010898ebd517c20 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/smbus.h D src/soc/amd/picasso/sm.c D src/soc/amd/picasso/smbus.c 4 files changed, 0 insertions(+), 307 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/38221/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index f1e10c1..680f0fa 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -41,7 +41,6 @@ romstage-y += romstage.c romstage-y += gpio.c romstage-y += pmutil.c -romstage-y += smbus.c romstage-y += memmap.c romstage-$(CONFIG_PICASSO_UART) += uart.c romstage-y += tsc_freq.c @@ -71,8 +70,6 @@ ramstage-y += pmutil.c ramstage-y += acp.c ramstage-y += sata.c -ramstage-y += sm.c -ramstage-y += smbus.c ramstage-y += memmap.c ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi.c ramstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c diff --git a/src/soc/amd/picasso/include/soc/smbus.h b/src/soc/amd/picasso/include/soc/smbus.h index c4bc28f..6066f10 100644 --- a/src/soc/amd/picasso/include/soc/smbus.h +++ b/src/soc/amd/picasso/include/soc/smbus.h @@ -21,15 +21,4 @@
#define SMB_SPEED_400KHZ (66000000 / (400000 * 4))
-/* - * Between 1-10 seconds, We should never timeout normally - * Longer than this is just painful when a timeout condition occurs. - */ -#define SMBUS_TIMEOUT (100 * 1000 * 10) - -int do_smbus_read_byte(u32 mmio, u8 device, u8 address); -int do_smbus_write_byte(u32 mmio, u8 device, u8 address, u8 val); -int do_smbus_recv_byte(u32 mmio, u8 device); -int do_smbus_send_byte(u32 mmio, u8 device, u8 val); - #endif /* __PICASSO_SMBUS_H__ */ diff --git a/src/soc/amd/picasso/sm.c b/src/soc/amd/picasso/sm.c deleted file mode 100644 index 438909d..0000000 --- a/src/soc/amd/picasso/sm.c +++ /dev/null @@ -1,103 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright (C) 2010 Advanced Micro Devices, Inc. - * - * 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. - */ - -#include <device/device.h> -#include <device/pci.h> -#include <device/pci_ids.h> -#include <device/pci_ops.h> -#include <device/smbus.h> -#include <cpu/x86/lapic.h> -#include <arch/ioapic.h> -#include <soc/southbridge.h> -#include <soc/smbus.h> - -/* -* The southbridge enables all USB controllers by default in SMBUS Control. -* The southbridge enables SATA by default in SMBUS Control. -*/ - -static void sm_init(struct device *dev) -{ - setup_ioapic(VIO_APIC_VADDR, CONFIG_MAX_CPUS); -} - -static u32 get_sm_mmio(struct device *dev) -{ - struct resource *res; - struct bus *pbus; - - pbus = get_pbus_smbus(dev); - res = find_resource(pbus->dev, 0x90); - if (res->base == SMB_BASE_ADDR) - return ACPIMMIO_SMBUS_BASE; - - return ACPIMMIO_ASF_BASE; -} - -static int lsmbus_recv_byte(struct device *dev) -{ - u8 device; - - device = dev->path.i2c.device; - return do_smbus_recv_byte(get_sm_mmio(dev), device); -} - -static int lsmbus_send_byte(struct device *dev, u8 val) -{ - u8 device; - - device = dev->path.i2c.device; - return do_smbus_send_byte(get_sm_mmio(dev), device, val); -} - -static int lsmbus_read_byte(struct device *dev, u8 address) -{ - u8 device; - - device = dev->path.i2c.device; - return do_smbus_read_byte(get_sm_mmio(dev), device, address); -} - -static int lsmbus_write_byte(struct device *dev, u8 address, u8 val) -{ - u8 device; - - device = dev->path.i2c.device; - return do_smbus_write_byte(get_sm_mmio(dev), device, address, val); -} -static struct smbus_bus_operations lops_smbus_bus = { - .recv_byte = lsmbus_recv_byte, - .send_byte = lsmbus_send_byte, - .read_byte = lsmbus_read_byte, - .write_byte = lsmbus_write_byte, -}; - -static struct pci_operations lops_pci = { - .set_subsystem = pci_dev_set_subsystem, -}; -static struct device_operations smbus_ops = { - .read_resources = DEVICE_NOOP, - .set_resources = DEVICE_NOOP, - .enable_resources = pci_dev_enable_resources, - .init = sm_init, - .scan_bus = scan_smbus, - .ops_pci = &lops_pci, - .ops_smbus_bus = &lops_smbus_bus, -}; -static const struct pci_driver smbus_driver __pci_driver = { - .ops = &smbus_ops, - .vendor = PCI_VENDOR_ID_AMD, - .device = PCI_DEVICE_ID_AMD_CZ_SMBUS, -}; diff --git a/src/soc/amd/picasso/smbus.c b/src/soc/amd/picasso/smbus.c deleted file mode 100644 index 79f09d6..0000000 --- a/src/soc/amd/picasso/smbus.c +++ /dev/null @@ -1,190 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * Copyright (C) 2010 Advanced Micro Devices, Inc. - * - * 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. - */ - -#include <stdint.h> -#include <console/console.h> -#include <amdblocks/acpimmio.h> -#include <soc/smbus.h> -#include <soc/southbridge.h> - -static u8 controller_read8(u32 base, u8 reg) -{ - switch (base) { - case ACPIMMIO_SMBUS_BASE: - return smbus_read8(reg); - case ACPIMMIO_ASF_BASE: - return asf_read8(reg); - default: - printk(BIOS_ERR, "Error attempting to read SMBus at address 0x%x\n", - base); - } - return 0xff; -} - -static void controller_write8(u32 base, u8 reg, u8 val) -{ - switch (base) { - case ACPIMMIO_SMBUS_BASE: - smbus_write8(reg, val); - break; - case ACPIMMIO_ASF_BASE: - asf_write8(reg, val); - break; - default: - printk(BIOS_ERR, "Error attempting to write SMBus at address 0x%x\n", - base); - } -} - -static int smbus_wait_until_ready(u32 mmio) -{ - u32 loops; - loops = SMBUS_TIMEOUT; - do { - u8 val; - val = controller_read8(mmio, SMBHSTSTAT); - val &= SMBHST_STAT_VAL_BITS; - if (val == 0) { /* ready now */ - return 0; - } - controller_write8(mmio, SMBHSTSTAT, val); - } while (--loops); - return -2; /* time out */ -} - -static int smbus_wait_until_done(u32 mmio) -{ - u32 loops; - loops = SMBUS_TIMEOUT; - do { - u8 val; - - val = controller_read8(mmio, SMBHSTSTAT); - val &= SMBHST_STAT_VAL_BITS; /* mask off reserved bits */ - if (val & SMBHST_STAT_ERROR_BITS) - return -5; /* error */ - if (val == SMBHST_STAT_NOERROR) { - controller_write8(mmio, SMBHSTSTAT, val); /* clr sts */ - return 0; - } - } while (--loops); - return -3; /* timeout */ -} - -int do_smbus_recv_byte(u32 mmio, u8 device) -{ - u8 byte; - - if (smbus_wait_until_ready(mmio) < 0) - return -2; /* not ready */ - - /* set the device I'm talking to */ - controller_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 1); - - byte = controller_read8(mmio, SMBHSTCTRL); - byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ - byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BTE_RW; /* set mode, start */ - controller_write8(mmio, SMBHSTCTRL, byte); - - /* poll for transaction completion */ - if (smbus_wait_until_done(mmio) < 0) - return -3; /* timeout or error */ - - /* read results of transaction */ - byte = controller_read8(mmio, SMBHSTDAT0); - - return byte; -} - -int do_smbus_send_byte(u32 mmio, u8 device, u8 val) -{ - u8 byte; - - if (smbus_wait_until_ready(mmio) < 0) - return -2; /* not ready */ - - /* set the command... */ - controller_write8(mmio, SMBHSTDAT0, val); - - /* set the device I'm talking to */ - controller_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 0); - - byte = controller_read8(mmio, SMBHSTCTRL); - byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ - byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BTE_RW; /* set mode, start */ - controller_write8(mmio, SMBHSTCTRL, byte); - - /* poll for transaction completion */ - if (smbus_wait_until_done(mmio) < 0) - return -3; /* timeout or error */ - - return 0; -} - -int do_smbus_read_byte(u32 mmio, u8 device, u8 address) -{ - u8 byte; - - if (smbus_wait_until_ready(mmio) < 0) - return -2; /* not ready */ - - /* set the command/address... */ - controller_write8(mmio, SMBHSTCMD, address & 0xff); - - /* set the device I'm talking to */ - controller_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 1); - - byte = controller_read8(mmio, SMBHSTCTRL); - byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ - byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BDT_RW; /* set mode, start */ - controller_write8(mmio, SMBHSTCTRL, byte); - - /* poll for transaction completion */ - if (smbus_wait_until_done(mmio) < 0) - return -3; /* timeout or error */ - - /* read results of transaction */ - byte = controller_read8(mmio, SMBHSTDAT0); - - return byte; -} - -int do_smbus_write_byte(u32 mmio, u8 device, u8 address, u8 val) -{ - u8 byte; - - if (smbus_wait_until_ready(mmio) < 0) - return -2; /* not ready */ - - /* set the command/address... */ - controller_write8(mmio, SMBHSTCMD, address & 0xff); - - /* set the device I'm talking to */ - controller_write8(mmio, SMBHSTADDR, ((device & 0x7f) << 1) | 0); - - /* output value */ - controller_write8(mmio, SMBHSTDAT0, val); - - byte = controller_read8(mmio, SMBHSTCTRL); - byte &= ~SMBHST_CTRL_MODE_BITS; /* Clear [4:2] */ - byte |= SMBHST_CTRL_STRT | SMBHST_CTRL_BDT_RW; /* set mode, start */ - controller_write8(mmio, SMBHSTCTRL, byte); - - /* poll for transaction completion */ - if (smbus_wait_until_done(mmio) < 0) - return -3; /* timeout or error */ - - return 0; -}
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBUS source ......................................................................
Patch Set 3: Code-Review+2
It's not used at all?
Hello Angel Pons, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38221
to look at the new patch set (#4).
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
soc/amd/picasso: Drop forked copy of SMBus source
Change-Id: I48a2799a6160cb04fed7792e4010898ebd517c20 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/picasso/Makefile.inc M src/soc/amd/picasso/include/soc/smbus.h D src/soc/amd/picasso/sm.c D src/soc/amd/picasso/smbus.c 4 files changed, 0 insertions(+), 307 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/38221/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 3: Code-Review+2
It's not used at all?
What am I missing and why is this being dropped? Is there a switch to common code somewhere that was omitted from the patch stack?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 3: Code-Review+2
It's not used at all?
What am I missing and why is this being dropped? Is there a switch to common code somewhere that was omitted from the patch stack?
That copy-paste should not have been approved in the first place. And no, I don't plan to fix it for you as none of soc/amd/picasso is build-tested on master.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4: Code-Review-2
Patch Set 4:
Patch Set 4:
Patch Set 3: Code-Review+2
It's not used at all?
What am I missing and why is this being dropped? Is there a switch to common code somewhere that was omitted from the patch stack?
That copy-paste should not have been approved in the first place.
Well, that's a fair enough argument, but assuming the preferred way to go is to use common code instead, then let's create the common code first before dropping PCO functionality.
And no, I don't plan to fix it for you as none of soc/amd/picasso is build-tested on master.
I'm not sure what you mean by "fix". If it's keeping PCO in sync with ST until we have common code, then I have no problem doing it myself.
As far as "none of soc/amd/picasso is build-tested on master", that's not a valid argument. By that same logic, you could justify removing the entire picasso directory right now. There can't/won't be a mainboard to test until the bootblock/romstage changes are updated and land. I presume you feel the Family 17h development is going too slow, but I'm sorry that you have very limited visibility into the other design, work, and negotiations that are actually happening.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4: Code-Review-2
Patch Set 4:
Patch Set 4:
Patch Set 3: Code-Review+2
It's not used at all?
What am I missing and why is this being dropped? Is there a switch to common code somewhere that was omitted from the patch stack?
That copy-paste should not have been approved in the first place.
Well, that's a fair enough argument, but assuming the preferred way to go is to use common code instead, then let's create the common code first before dropping PCO functionality.
With PSP doing raminit, SMBus on amd/picasso may remain unused for a long time. Not sure if your designs have some other SMBus components that use it, and even if they did, existing code only has reference to PCI_DEVICE_ID_AMD_CZ_SMBUS. Is that the correct PCI ID? If not, there is no functionality dropped with the source removal.
And no, I don't plan to fix it for you as none of soc/amd/picasso is build-tested on master.
I'm not sure what you mean by "fix". If it's keeping PCO in sync with ST until we have common code, then I have no problem doing it myself.
Fix, as in introduce new SOC_AMD_COMMON_SMBUS kconfig and add/replace necessary PCI IDs and move the ST files.
As far as "none of soc/amd/picasso is build-tested on master", that's not a valid argument. By that same logic, you could justify removing the entire picasso directory right now. There can't/won't be a mainboard to test until the bootblock/romstage changes are updated and land. I presume you feel the Family 17h development is going too slow, but I'm sorry that you have very limited visibility into the other design, work, and negotiations that are actually happening.
I would need to rebase the entire Mandolin CRB board and the dependencies to get a build-tested result for the common SMBUS approach. It's just less hassle for you to incorporate that on yout patcht-train once/if/where you want it.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
That copy-paste should not have been approved in the first place.
Well, that's a fair enough argument, but assuming the preferred way to go is to use common code instead, then let's create the common code first before dropping PCO functionality.
With PSP doing raminit, SMBus on amd/picasso may remain unused for a long time. Not sure if your designs have some other SMBus components that use it, and even if they did, existing code only has reference to PCI_DEVICE_ID_AMD_CZ_SMBUS. Is that the correct PCI ID? If not, there is no functionality dropped with the source removal.
I hope that once I have a chance to rip out the hybrid romstage changes, and make the stages look like the more traditional bootblock/<verstage>/romstage, the patches should be much more palatable and approvable quickly. It feels like Google engineers and I are >95% sure we know precisely how we want vboot to work on this system. Once that's settled, I'll be more apt to prioritize making the changes.
Yes, that's the correct device ID - 0x790b. On a separate note, I recall another gerrit entry pointed out that a device ID was added for a duplicate value although the ID was reused across products. (Notwithstanding it's easy to be confused by 0x790b "_CZ" used on PCO...) do you have opinions for how to best name/reuse device IDs? It seems like we typically maintain the ID's name as when it's first introduced.
And no, I don't plan to fix it for you as none of soc/amd/picasso is build-tested on master.
I'm not sure what you mean by "fix". If it's keeping PCO in sync with ST until we have common code, then I have no problem doing it myself.
Fix, as in introduce new SOC_AMD_COMMON_SMBUS kconfig and add/replace necessary PCI IDs and move the ST files.
OK, I'll add it to my list.
As far as "none of soc/amd/picasso is build-tested on master", that's not a valid argument. By that same logic, you could justify removing the entire picasso directory right now. There can't/won't be a mainboard to test until the bootblock/romstage changes are updated and land. I presume you feel the Family 17h development is going too slow, but I'm sorry that you have very limited visibility into the other design, work, and negotiations that are actually happening.
I would need to rebase the entire Mandolin CRB board and the dependencies to get a build-tested result for the common SMBUS approach. It's just less hassle for you to incorporate that on yout patcht-train once/if/where you want it.
I understand completely! I agree it's a real pain right now.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
That copy-paste should not have been approved in the first place.
Well, that's a fair enough argument, but assuming the preferred way to go is to use common code instead, then let's create the common code first before dropping PCO functionality.
With PSP doing raminit, SMBus on amd/picasso may remain unused for a long time. Not sure if your designs have some other SMBus components that use it, and even if they did, existing code only has reference to PCI_DEVICE_ID_AMD_CZ_SMBUS. Is that the correct PCI ID? If not, there is no functionality dropped with the source removal.
I hope that once I have a chance to rip out the hybrid romstage changes, and make the stages look like the more traditional bootblock/<verstage>/romstage, the patches should be much more palatable and approvable quickly. It feels like Google engineers and I are >95% sure we know precisely how we want vboot to work on this system. Once that's settled, I'll be more apt to prioritize making the changes.
Yes, that's the correct device ID - 0x790b. On a separate note, I recall another gerrit entry pointed out that a device ID was added for a duplicate value although the ID was reused across products. (Notwithstanding it's easy to be confused by 0x790b "_CZ" used on PCO...) do you have opinions for how to best name/reuse device IDs? It seems like we typically maintain the ID's name as when it's first introduced.
I wonder if AMD has documentation about the device IDs. I guess they haven't changed the ID because the functional block is the same for various chips. In any case, if SMBus is factored out, then the IDs can have comments on them that list the platforms that have a device with this ID.
And no, I don't plan to fix it for you as none of soc/amd/picasso is build-tested on master.
I'm not sure what you mean by "fix". If it's keeping PCO in sync with ST until we have common code, then I have no problem doing it myself.
Fix, as in introduce new SOC_AMD_COMMON_SMBUS kconfig and add/replace necessary PCI IDs and move the ST files.
OK, I'll add it to my list.
As far as "none of soc/amd/picasso is build-tested on master", that's not a valid argument. By that same logic, you could justify removing the entire picasso directory right now. There can't/won't be a mainboard to test until the bootblock/romstage changes are updated and land. I presume you feel the Family 17h development is going too slow, but I'm sorry that you have very limited visibility into the other design, work, and negotiations that are actually happening.
I would need to rebase the entire Mandolin CRB board and the dependencies to get a build-tested result for the common SMBUS approach. It's just less hassle for you to incorporate that on yout patcht-train once/if/where you want it.
I understand completely! I agree it's a real pain right now.
So, if the Picasso code is currently unused, and its SMBus code is copied from Stoneyridge, I would drop it now so that the other platforms can be refactored. Then, if Picasso would need SMBus support in the future, it can be easily added back through common code. Sounds pretty simple, am I missing something?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
I understand completely! I agree it's a real pain right now.
Marshall, if you do understand, why is there still a -2?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
I understand completely! I agree it's a real pain right now.
Marshall, if you do understand, why is there still a -2?
What's a pain (i.e. what I completely understand) is if someone wishes to build test changes in the picasso directory, they must checkout the entire work-in-progress stack up through the Mandolin mainboard. Then once it's tested, that change needs to be brought over to a stack to be pushed. The reason for the -2 is that I don't want functioning picasso code ripped out, i.e. assuming one has the mainboard code commit.
In my opinion, this patch should not preclude merging CB:38222. My recommended approach would be to submit that, abandon this one, then I or someone else can move the stoneyridge code to common.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
I understand completely! I agree it's a real pain right now.
Marshall, if you do understand, why is there still a -2?
What's a pain (i.e. what I completely understand) is if someone wishes to build test changes in the picasso directory, they must checkout the entire work-in-progress stack up through the Mandolin mainboard.
That's not something you can expect of other developers. The mess one creates when submitting code to coreboot's master branch that is not hooked up for build testing, is their own burden.
Then once it's tested, that change needs to be brought over to a stack to be pushed. The reason for the -2 is that I don't want functioning picasso code ripped out, i.e. assuming one has the mainboard code commit.
Functioning? Can you please point me to the commit that makes use of this copy?
In my opinion, this patch should not preclude merging CB:38222. My recommended approach would be to submit that, abandon this one, then I or someone else can move the stoneyridge code to common.
Well, that implies that no API changes are done, otherwise this copy would break anyway. I haven't looked to far into Kyösti's queue, though.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
What's a pain (i.e. what I completely understand) is if someone wishes to build test changes in the picasso directory, they must checkout the entire work-in-progress stack up through the Mandolin mainboard.
That's not something you can expect of other developers. The mess one creates when submitting code to coreboot's master branch that is not hooked up for build testing, is their own burden.
That's not something you can expect of other developers.
Agreed, and it was never my expectation to place that onus on other developers. It's an unfortunate result of how Picasso (and any Family 17h and later) is so unique in the x86 world; it has taken much longer to land the functionality than originally projected. (I have a "final" revision to the WIP in mind for when I get a moment to work on them.)
Then once it's tested, that change needs to be brought over to a stack to be pushed. The reason for the -2 is that I don't want functioning picasso code ripped out, i.e. assuming one has the mainboard code commit.
Functioning? Can you please point me to the commit that makes use of this copy?
Not one that uses the bus features. However, this is for the first function of the southbridge device, i.e. D14F0. We currently rely on this to run setup_ioapic(). I'm not arguing this is the right vs. wrong location, however it's in use. The WIP mainboard patch is CB:33772.
In my opinion, this patch should not preclude merging CB:38222. My recommended approach would be to submit that, abandon this one, then I or someone else can move the stoneyridge code to common.
Well, that implies that no API changes are done, otherwise this copy would break anyway. I haven't looked to far into Kyösti's queue, though.
CB:38222 only modified a handful of names static to one file. Landing that diverges from this, and leaving the soc//picasso/files as a worse fork -- I assume that's Kyösti's concern. I realize that's not optimal, but unforking this is much lower in priority than my other Picasso priorities at the moment. FWIW, (a) prior to beginning Picasso work, I moved quite a lot of functionality from amd/stoneyridge into amd/common to minimize forking. At this point, I don't recall why I skipped smbus; possibly because coreboot won't require reading SPDs and I didn't yet know what I wanted to do with it. (b) I carefully scrubbed all the remaining soc code to see where the functionality would diverge. (c) For the period of time when I still had many other changes in progress, as soc//stoneyridge was changing, I maintained those modifications in my work. All that to say that... it'll be worth my effort once Picasso lands to do another audit to ensure nothing else was missed.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
To coreboot leadership; I don't agree with the arguments Marshall gives for his -2.
Now when we consider amd/stoneyridge, where this code was forked from, it should have received some better review too initially. The concept of an I2C SMBus device side having to know the base address of the SMBus host controller is an invalid concept in its own. Yes, it was copied from Intel side and no, it is bad API and incompatible with SMBus/I2C bus multiplexing topologies.
CB:38222 discusses the stoneyridge code quality.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
To coreboot leadership; I don't agree with the arguments Marshall gives for his -2.
I also disagree with the -2 that is blocking this change. Especially if the blocker is CB:33772, a commit that has not been touched in over a month and has never managed to build successfully.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
[1]: https://doc.coreboot.org/getting_started/gerrit_guidelines.html
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Angel, thanks for your opinion. My reasoning is that the patch strips functionality from an soc where I'm actively trying to complete development, and it does not replace it with an alternative.
If you give a patch a -2, you are responsible for giving concrete recommendations for what could be changed to resolve the issue the patch addresses.
As I stated early, the right approach is to move the feature out of stoneyridge and into common so it's sharable between the stoneyridge and picasso. I'm even happy to do this work, however it's substantially lower in priority than my other areas of focus. If I do the work, then it must come at the cost of a delay. I'm fine, however, with my keeping the fork in sync with stoneyridge (i.e. the change in CB:38222) until the time comes that I can commonize the code.
But let's be clear, my -2 here is not holding up merging CB:38222. Those complaints don't hold up.
So again, thank you for sharing your opinion, but I'm not yet ready to agree to having a project defeatured like this while I'm trying to finish it. I'd like to hear others weigh in on why this should be removed, and tell me where I'm wrong.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Picking order or precedence in coreboot framework as I see it: - Individual boards don't get to block or dictate platform APIs. - Platform APIs don't get to block or dictate subsystem (PCI, ACPI, SMBus, etc) APIs. - Subsystems and arch/ need to evolve closely together for meaningful placement of header files to avoid guards around includes common code.
Blocking above can be understood as parallel works or duplicate implementations that cause naming collisions in a namespace, of which the subsystem should have complete ownership.
Unnecessary and untested features of a new platform like amd/picasso should not be committed to master until there is at least a build-tested change with the actual dependency. Did I miss the dependency? AFAIR Marshall already commented that he should have moved amd/stoneyridge SMBus code instead of forking another copy.
Now, if you ask me, in the cases when a commercial player blocks the work of a community task targeting a somewhat major cleanup on a subsystem, responsibility of said subsystem maintenance should be transferred to that vendor with an enforced deadline. I feel pretty disgusted of Martin and Marshall giving empty promises on behalf of AMD of supplying me with the CRB in question here since Sep 2019 and them having requestsed me personally on helping to clean topics around CAR since early Aug 2019. M & M: You are collecting enemies ánd losing competent reviewers fast.
Topic wipe-PRE_RAM reviews completed without much or any interaction from AMD in the period Aug 18-Nov 22. Largely thanks to Arthur, CAR_GLOBAL and ROMCC removals completed in December. As of Jan 25th 2020 even the NDA part for PCO documentation has not been completed or even communicated from AMD side.
This gets ugly, off-topic and non-techical fast -- further discussions by email with the leadership in Cc.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
You're right, this is getting off topic.
AFAIR Marshall already commented that he should have moved amd/stoneyridge SMBus code instead of forking another copy.
No, I said I started by listing all potential candidates for moving into common. Some were obvious moves, some were obvious keeps, however this is an example of one I wanted to wait until I had better information. I never made any type of admission of "should have". Items in the first category were moved.
I feel pretty disgusted of Martin and Marshall giving empty promises on behalf of AMD of supplying me with the CRB in question here since Sep 2019 and them having requestsed me personally on helping to clean topics around CAR since early Aug 2019.
Neither Martin nor I promised that. We urged AMD (I was not an employee) to try to make it happen. I agree its been a long time, and now I am more aware of the legal approvals it required as well as the competing demands on the attorneys' time. (You've expressed you feel I represent all of AMD however that's simply inaccurate; this was handled by an organization different than mine.) The status I heard this week is everything is complete internally and they are looking for a board for you. So when it shows up on your doorstep soon, the timing was due more to coincidence than your complaints here.
Look, I'm really tired of arguing about this and I see no point going back and forth here, except that it's hard to allow skewed history go uncorrected. If you feel a decision should be made by coreboot leadership, let's just get it in front of them and I'll state my case again.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Leadership was added to Cc reviewers Jan 13th 2019 but I expected to see a quick resolution here or progress on your amd/picasso patchset.
Since You chose to continue discussion here against my advice, so be it.
As for what I called a "promise" of a reference board, I noticed just now that You, Marshall, have not been a recipient in any of that e-mail exchange. You can make Your own judgement from the subject lines of that e-mail exchange, with selected personnel at Google, SilverBack and AMD between Aug and Nov 2019: - July 31st "Help removing CAR references from Picasso" - Sep 9th "Getting you a picasso board" - Sep 25th "Mandolin board for Kyösti Mälkki"
Or the following quotes: . J and W are working to get you a mandolin board. Can you send your shipping information? Obviously you'll also need to sign an NDA as well. - I wasn't sure if he needed access. If he does, I will need an NDA. I will send one via DocuSign. - It's up to you, but I'd be glad to help pay any import fees - I know that M and J at AMD have been working on it (board), so I don't have any further news right now.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Is this patch blocking anything important? I see a big patch chain of clean-up patches that have almost all been merged, so it seems progress is being made here. Why can't the common code be merged first with this as part of the chain?
Patch Set 4: Especially if the blocker is CB:33772, a commit that has not been touched in over a month and has never managed to build successfully.
That's misleading considering the time of year.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
What part of that are you citing? Marshall has outlined the plan for resolving this and +2'd CB:38222. In his -2 he made a reasonable suggestion to merge the common code first before dropping this code, as to avoid breaking platform development that is underway.
The guidelines also say "Don’t submit patches that you know will break other platforms", though it's murky in this case since Picasso appears to be broken in its current state. If it were orphaned or obsolete hardware then the answer could simply be to remove it entirely. But since it's being actively developed, that is another matter.
While we're citing guidelines, the ad hominem attacks used in some of the later comments definitely fails to meet the threshold for respectful conduct.
Cleaning up APIs is certainly necessary and Kyösti's efforts here are appreciated. AFAICT nobody is against this patch, the only resistance is due to timing of platform development currently underway.
Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Removed Code-Review+2 by Angel Pons th3fanbus@gmail.com
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Is this patch blocking anything important? I see a big patch chain of clean-up patches that have almost all been merged, so it seems progress is being made here. Why can't the common code be merged first with this as part of the chain?
I think this is more about a general problem. Should we have code on our master branch that is neither build tested nor reviewed, makes maintenance harder and let's the community pay for some nonsense? (refactored, put into common code or not, this code will likely never be useful for Picasso) It shouldn't have been merged (I think I even pointed that out while the review was skipped), now it drags coreboot down; and it's protected just to satisfy some ego (I doubt it can be anything else, because it's a five minute job to add it back in case it'd really be needed).
Patch Set 4: Especially if the blocker is CB:33772, a commit that has not been touched in over a month and has never managed to build successfully.
That's misleading considering the time of year.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
What part of that are you citing? Marshall has outlined the plan for resolving this and +2'd CB:38222. In his -2 he made a reasonable suggestion to merge the common code first before dropping this code, as to avoid breaking platform development that is underway.
I wouldn't call any suggestion reasonable that can't be done immediately. And nothing can be done immediately if the code isn't even hooked up for build testing. If there is nothing that can be done better, how can a -2 be warranted?
The guidelines also say "Don’t submit patches that you know will break other platforms", though it's murky in this case since Picasso appears to be broken in its current state. If it were orphaned or obsolete hardware then the answer could simply be to remove it entirely. But since it's being actively developed, that is another matter.
Well, I trust that it's actively developed. But for whom? AFAIK, it's still not clear if compatible binaries will ever be available for the coreboot community... [1] Same community that already pays for the maintenance.
While we're citing guidelines, the ad hominem attacks used in some of the later comments definitely fails to meet the threshold for respectful conduct.
Cleaning up APIs is certainly necessary and Kyösti's efforts here are appreciated. AFAICT nobody is against this patch, the only resistance is due to timing of platform development currently underway.
Timing is reversed here. The longer the code is hanging around untested on the master branch, the more problems will sneak in. It might even safe Marshall some time if we'd kill it earlier... (applies to the whole picasso/ code btw. IMHO, it should be removed and be added back once it can be build tested).
[1] https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/PHSD2...
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
Is this patch blocking anything important? I see a big patch chain of clean-up patches that have almost all been merged, so it seems progress is being made here. Why can't the common code be merged first with this as part of the chain?
I had to re-arrange to get them move forwards. I gave this a couple weeks of silence waiting for amd/picasso to be rebased so some of my local work is not avaiable in gerrit.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
What part of that are you citing? Marshall has outlined the plan for resolving this and +2'd CB:38222. In his -2 he made a reasonable suggestion to merge the common code first before dropping this code, as to avoid breaking platform development that is underway.
The dependency of amd/picasso on SMBus has not been pinpointed to me. Marshall argued it is needed for IOAPIC-enablement due the PCI dev.fn involved, but this on its own sounds wrong even for amd/stoneyridge where it was forked from. I have not investigated this argument any deeper.
The guidelines also say "Don’t submit patches that you know will break other platforms", though it's murky in this case since Picasso appears to be broken in its current state. If it were orphaned or obsolete hardware then the answer could simply be to remove it entirely. But since it's being actively developed, that is another matter.
I fail to see both the dependency on SMBus messages and any predictable timeline on amd/picasso code merges. What I clearly see, is there will be naming collision between source files that are currently not build-tested and newly introduced common headerfiles. Either my tree or Marshall's tree will get broken. If my tree lands on master first, and Marshall merges his changes without fresh rebase, master will be broken.
While we're citing guidelines, the ad hominem attacks used in some of the later comments definitely fails to meet the threshold for respectful conduct.
Well yes, my frustration came through in those comments, apologies for that. Maybe coreboot leadership instead can discuss with Martin and Marshall, if they have had any intentions of ever sending the board or if it was merely a scheme to get free work done from community side. Reminds me of past promises AMD made to Google about scrubbing amd/stoneyridge after first Chromebooks hit shelves that motivated me to contribute _a lot_ on amd/stoneyridge review. The fault is at AMD policies for sure, but I am pointing at the messengers and I feel very much justified in doing so.
Cleaning up APIs is certainly necessary and Kyösti's efforts here are appreciated. AFAICT nobody is against this patch, the only resistance is due to timing of platform development currently underway.
I gave this a couple weeks of silence waiting for amd/picasso rebase, and got response that the concept of amd/picasso bootblock-verstage-romstage transitions was going to be re-architected. I am actually pleased about this happening, my first impression from the brief comments I received is that after their December decisions they are going to take an approach I had suggested in my review comments earlier, maybe in June already, I did not track down the dates. I can only assess the new situation when their source changes land, so I may be mistaken about this.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Is this patch blocking anything important? I see a big patch chain of clean-up patches that have almost all been merged, so it seems progress is being made here. Why can't the common code be merged first with this as part of the chain?
I had to re-arrange to get them move forwards. I gave this a couple weeks of silence waiting for amd/picasso to be rebased so some of my local work is not avaiable in gerrit.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
What part of that are you citing? Marshall has outlined the plan for resolving this and +2'd CB:38222. In his -2 he made a reasonable suggestion to merge the common code first before dropping this code, as to avoid breaking platform development that is underway.
The dependency of amd/picasso on SMBus has not been pinpointed to me. Marshall argued it is needed for IOAPIC-enablement due the PCI dev.fn involved, but this on its own sounds wrong even for amd/stoneyridge where it was forked from. I have not investigated this argument any deeper.
The guidelines also say "Don’t submit patches that you know will break other platforms", though it's murky in this case since Picasso appears to be broken in its current state. If it were orphaned or obsolete hardware then the answer could simply be to remove it entirely. But since it's being actively developed, that is another matter.
I fail to see both the dependency on SMBus messages and any predictable timeline on amd/picasso code merges. What I clearly see, is there will be naming collision between source files that are currently not build-tested and newly introduced common headerfiles. Either my tree or Marshall's tree will get broken. If my tree lands on master first, and Marshall merges his changes without fresh rebase, master will be broken.
While we're citing guidelines, the ad hominem attacks used in some of the later comments definitely fails to meet the threshold for respectful conduct.
Well yes, my frustration came through in those comments, apologies for that. Maybe coreboot leadership instead can discuss with Martin and Marshall, if they have had any intentions of ever sending the board or if it was merely a scheme to get free work done from community side. Reminds me of past promises AMD made to Google about scrubbing amd/stoneyridge after first Chromebooks hit shelves that motivated me to contribute _a lot_ on amd/stoneyridge review. The fault is at AMD policies for sure, but I am pointing at the messengers and I feel very much justified in doing so.
Cleaning up APIs is certainly necessary and Kyösti's efforts here are appreciated. AFAICT nobody is against this patch, the only resistance is due to timing of platform development currently underway.
I gave this a couple weeks of silence waiting for amd/picasso rebase, and got response that the concept of amd/picasso bootblock-verstage-romstage transitions was going to be re-architected. I am actually pleased about this happening, my first impression from the brief comments I received is that after their December decisions they are going to take an approach I had suggested in my review comments earlier, maybe in June already, I did not track down the dates. I can only assess the new situation when their source changes land, so I may be mistaken about this.
Kyosti, you are stepping WAY over the line here. You need to follow the code of conduct and the guidelines for gerrit. If you need to vent, choose a more professional way to do it.
I never "PROMISED" you anything. I don't control AMD. Both Marshall and I are WORKING on getting you a CRB. It's gone through AMD legal and I believe it has been okayed, but with the attitude displayed here, I see no reason that ANYONE would want to work with you in any fashion.
That's all I'm saying here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Kyosti, you are stepping WAY over the line here. You need to follow the code of conduct and the guidelines for gerrit. If you need to vent, choose a more professional way to do it.
Surprisingly, I agree. And it is the leaderships' role to assess the situation as you Martin are also intimely involved in the review and merge-process of amd/picasso.
Aside the ad-hominem here, please pinpoint some commits, where you find I have not followed gerrit guidelines. In search of these, I found some comments where I specifically state that I shall not -2 amd/picasso work, nor do I feel I have enough authority to +2 any works that touch arch/x86. Being restricted to leaked and partially outdated documentation around AMD Secure Processor, it's actually impossible for most members of the community to do any purposeful review on amd/picasso. When possible, I have tried to forward the works I found confusing or overly complicated, to certain senior Google developers to have a look at instead. There is little I can do if they are too busy dealing with their other projects and Your patchtrain gets stalled due to that.
There's is really nothing I can do about the delays caused for the cases where You agree that my -1 review was actually spot-on and You decide to refactor things, and then someone from Google or community side comes up with different ideas. I acknowledge I don't possess the authority to block things on amd/picasso and I have many times adviced You to get second opinions should You find that my REQUESTS, not DEMANDS, cause overloads and disturbance on Your workflow. Sadly from Your perspective, seems like I am often able to find some senior developer to agree with my reasoning.
My request for coreboot leadership is to make a statement that src/arch/x86 integrity and future maintainability is guaranteed by (in-)formally requiring more authority to merge commits there.
I never "PROMISED" you anything. I don't control AMD. Both Marshall and I are WORKING on getting you a CRB. It's gone through AMD legal and I believe it has been okayed, but with the attitude displayed here, I see no reason that ANYONE would want to work with you in any fashion.
That's all I'm saying here.
90% of the confrontations I have had in the last 10 or so years are about stalled maintenance or review processes on AMD codebase in coreboot proper. You are entitled to Your opinion that my review work has no value. You can then revisit AGESA boards and realise that in terms of lines-of-code their footprint has been reduced by some 70% per board from the state AMD and SAGE originally shipped them in. It is largely due to AMD licensing that binaryPI is in the shape You see it.
Enforcing APIs will always be painful to vendors with deadlines. Yet that is exactly what is required to achieve any longterm design evolution like dropping LATE_CBMEM_INIT and CAR_GLOBAL and ROMCC like was done in the fall of 2019. Discussion on the mailing list hopefully gets us all back-on-the track wrt. technical details.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Could this patch and the following one be rebased? I can pick things up if it collectively helps people unblock things. Sounds like stoney's implementation should be ported to common so picasso can progress? If that's accurate, let me know, and let's rebase things.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
Could this patch and the following one be rebased?
Done. I will also push rest of my local and very much WIP commits on this topic later today.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 6:
Patch Set 4:
Could this patch and the following one be rebased? I can pick things up if it collectively helps people unblock things. Sounds like stoney's implementation should be ported to common so picasso can progress? If that's accurate, let me know, and let's rebase things.
I unified things starting from here: https://review.coreboot.org/c/coreboot/+/38611
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Abandoned
superseded by CB:38616
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
David Hendricks (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)