Attention is currently required from: Joe Tessler, Neill Corlett. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50101 )
Change subject: hatch: Modify genesis PcieClkSrc settings ......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/50101/comment/0da79e11_a7f85f5b PS2, Line 9: to work around issue with Longsys SSD Oops sorry missed your update.
This was required to support a specific SSD module on the M.2 slot that uses PCIe slots 11 & 12 as advertised for "NVMe hybrid storage devices".
My understanding of hybrid storage devices is that there are two devices seen on two separate PCIe RPs, but they share the same CLKSRC. Thus, while configuring these RPs in devicetree, you need to set both as enabled, but configure CLKSRC for only one of those. And then FSP ensures that both root ports are configured to use the same SRC. I think it is hacky on FSPs part and is just a limitation of the API that FSP exposes.
Anyways, in this case, I see that you dropped the CLKSRC part for RP11 and RP12 is marked as disabled. So, I am really not sure how RP11 clock is set up at all.
After chatting again with them, I confirmed this new SSD module is a raw PCIe drive (not SATA). Apparently the original settings only supported SATA drives.
I don't think that is accurate. SATA does not really require the CLKSRC/CLKREQ programming.
Do you think this is sufficient / should be added to the commit message?
Since this is limited to genesis, I am okay if you want to go ahead with this. But I don't think there is clear understanding of the problem or how the change is really helping. I would recommend ensuring the design is understood and then translating that into appropriate device tree configs.
-
Furquan Shaikh (Code Review)