Change in coreboot[master]: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7
This patch fixes SATA device detection issue connected at Port > 2 on Intel H-PCH.
TEST=Able to detect SATA device connect at Port 4 on CML-H.
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/sata/sata.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/1
diff --git a/src/soc/intel/common/block/sata/sata.c b/src/soc/intel/common/block/sata/sata.c index d3f82ed..3e475d1 100644 --- a/src/soc/intel/common/block/sata/sata.c +++ b/src/soc/intel/common/block/sata/sata.c @@ -41,7 +41,7 @@ if (CONFIG(SOC_AHCI_PORT_IMPLEMENTED_INVERT)) port_impl = ~port_impl;
- port_impl &= 0x07; /* bit 0-2 */ + port_impl &= 0xFF; /* bit 0-7 */
/* Port enable */ temp = pci_read_config8(dev, SATA_PCI_CFG_PORT_CTL_STS);
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H? I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1: Code-Review+1
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1:
Patch Set 1:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H?
Yes, SHL/KBL H also has total 8 SATA port. The only difference is SPD bit definitions there. thats we have already handled like this
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Difference between H and LP would be for SATA, LP has 0-2 implemented and rest 3-7 reserved.
I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
May your SATA direct connect Port is between 0-2, else you would also see this issue for sure.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1:
(1 comment)
The next commit should probably squashed into this one, as now all eight bits are written.
https://review.coreboot.org/c/coreboot/+/41674/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41674/1//COMMIT_MSG@10 PS1, Line 10: on Intel H-PCH. Is there a bug report for that? Please elaborate why the range is increased.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/1/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/1/src/soc/intel/common/block/... PS1, Line 44: port_impl &= 0xFF; /* bit 0-7 */ I think we can remove this line now. port_impl(u8) would be 8 bit read from config space.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41674/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41674/1//COMMIT_MSG@10 PS1, Line 10: on Intel H-PCH.
Is there a bug report for that? Please elaborate why the range is increased.
Ack
https://review.coreboot.org/c/coreboot/+/41674/1/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/1/src/soc/intel/common/block/... PS1, Line 44: port_impl &= 0xFF; /* bit 0-7 */
I think we can remove this line now. port_impl(u8) would be 8 bit read from config space.
Ack
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41674
to look at the new patch set (#2).
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7
This patch ensures SATA PCI config space offset 0x92 (SPD) is written correctly based on MMIO offset 0x0c (GHC_PI)
PCH-H platform has more than 3 SATA port hence observed this issue when SATA device is connected at Port > 3.
Without this CL : GHC_PI = 0x0F (Port 0-3 enable) Value written into SPD = 0xFF (Assume SPD read returns 0xFF) Results = All SATA Ports are disable
With this CL : GHC_PI = 0x0F (Port 0-3 enable) Value written into SPD = 0xF0 Results = Port 0-3 are enable and 4-7 are disable.
TEST=Able to detect SATA device connect at Port 4 on CML-H.
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/sata/sata.c 1 file changed, 2 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/2
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41674
to look at the new patch set (#3).
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7
This patch ensures SATA PCI config space offset 0x92 (SPD) is written correctly based on MMIO offset 0x0c (GHC_PI)
PCH-H platform has more than 3 SATA port hence observed this issue when SATA device is connected at Port > 3.
Without this CL : GHC_PI = 0x0F (Port 0-3 enable) Value written into SPD = 0xFF (Assume SPD read returns 0xFF) Results = All SATA Ports are disabled
With this CL : GHC_PI = 0x0F (Port 0-3 enable) Value written into SPD = 0xF0 Results = Port 0-3 are enable and 4-7 are disabled.
TEST=Able to detect SATA device connect at Port 4 on CML-H.
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/sata/sata.c 1 file changed, 2 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/3
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Aamir Bohra, Michael Niewöhner, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41674
to look at the new patch set (#4).
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7
This patch ensures SATA PCI config space offset 0x92 (SPD) is written correctly based on MMIO offset 0x0c (GHC_PI)
PCH-H platform has more than 3 SATA port hence observed this issue when SATA device is connected at Port > 3.
Without this CL : GHC_PI = 0x0F (Port 0-3 enable) Value written into SPD = 0xFF (Assume SPD read returns 0xFF) Results = All SATA Ports are disabled
With this CL : GHC_PI = 0x0F (Port 0-3 enable) Value written into SPD = 0xF0 Results = Port 0-3 are enable and 4-7 are disabled.
TEST=Able to detect SATA device connect at Port 4 on CML-H.
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/sata/sata.c 1 file changed, 2 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/4
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92 This changed to 0x94 with Cannon Point PCH.
SOC_AHCI_PORT_IMPLEMENTED_INVERTED covers this up. Pre CNP, the code configured Port Control and Status (PCS), but for later generations it writes into Port Mapping (MAP) by accident. MAP contains disable bits, hence the INVERTED option was necessary to not break things, but PCS is still not written.
A hunch tells me that the whole driver here is not necessary for platforms where FSP already sets PCS. We keep patching this code, but only so that it doesn't break things. Does it do anything useful, though? We could dump the PCS state to figure out.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92
This changed to 0x94 with Cannon Point PCH.
[Subrata] All chipset still has 2 register no, its always 2 registers 0x90: MAP 0x94: PCS
SOC_AHCI_PORT_IMPLEMENTED_INVERTED covers this up. Pre CNP, the code configured Port Control and Status (PCS), but for later generations it writes into Port Mapping (MAP) by accident. MAP contains disable bits, hence the INVERTED option was necessary to not break things, but PCS is still not written.
A hunch tells me that the whole driver here is not necessary for platforms where FSP already sets PCS. We keep patching this code, but only so that it doesn't break things. Does it do anything useful, though? We could dump the PCS state to figure out.
[Subrata] SPD and PCS registers are interrelated when we are trying to disable a port. Write of 1 to PCS.PxE has no effect when the corresponding SPD[x] bit is 1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 1:
Patch Set 1:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H?
Yes, SHL/KBL H also has total 8 SATA port. The only difference is SPD bit definitions there. thats we have already handled like this
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Difference between H and LP would be for SATA, LP has 0-2 implemented and rest 3-7 reserved.
I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
May your SATA direct connect Port is between 0-2, else you would also see this issue for sure.
I have 8 SATA Ports and use 2-6 and there are no problems
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 1:
Patch Set 1:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H?
Yes, SHL/KBL H also has total 8 SATA port. The only difference is SPD bit definitions there. thats we have already handled like this
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Difference between H and LP would be for SATA, LP has 0-2 implemented and rest 3-7 reserved.
I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
May your SATA direct connect Port is between 0-2, else you would also see this issue for sure.
I have 8 SATA Ports and use 2-6 and there are no problems
Hmm, I guess FSP already handles it right, as Nico suggested
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92
This changed to 0x94 with Cannon Point PCH. […]
coreboot never set SPD bits on purpose for these FSP platforms. Please confirm if it's coreboot's or FSP's responsibility to set those. If you are unsure, just dump the register state after FSP-S run and check if it already handles things. If it does, we can disable this driver for all platforms that we tested positively without it.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92
coreboot never set SPD bits on purpose for these FSP platforms. Please […]
@Nico see my comment, I guess this is what I'm experiencing (that it's not needd)
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92
@Nico see my comment, I guess this is what I'm experiencing (that it's not needd)
Before jumping to conclusions: Are you sure the code is run on your device? i.e. is the driver selected and your SATA PCI ID in the list below?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 1:
Patch Set 1:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H?
Yes, SHL/KBL H also has total 8 SATA port. The only difference is SPD bit definitions there. thats we have already handled like this
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Difference between H and LP would be for SATA, LP has 0-2 implemented and rest 3-7 reserved.
I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
May your SATA direct connect Port is between 0-2, else you would also see this issue for sure.
I have 8 SATA Ports and use 2-6 and there are no problems
You seems lucky that in ur case FSP might have already written to SPD register and as this SPD register is RW/O hence coreboot next writes has no effect. But if FSP skips settings those SPD and coreboot only know abut (ports 0-2) and disables other ports it would have result the same what i'm seeing now.
So now question is do you like to over commit that FSP will always things proper and coreboot might not need to program SPD bits at finalize stage ? This code might be like double insurance in coreboot to handle such misses from FSP.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92
Before jumping to conclusions: Are you sure the code is run on your device? […]
yes and yes
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H?
Yes, SHL/KBL H also has total 8 SATA port. The only difference is SPD bit definitions there. thats we have already handled like this
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Difference between H and LP would be for SATA, LP has 0-2 implemented and rest 3-7 reserved.
I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
May your SATA direct connect Port is between 0-2, else you would also see this issue for sure.
I have 8 SATA Ports and use 2-6 and there are no problems
You seems lucky that in ur case FSP might have already written to SPD register and as this SPD register is RW/O hence coreboot next writes has no effect. But if FSP skips settings those SPD and coreboot only know abut (ports 0-2) and disables other ports it would have result the same what i'm seeing now.
So now question is do you like to over commit that FSP will always things proper and coreboot might not need to program SPD bits at finalize stage ? This code might be like double insurance in coreboot to handle such misses from FSP.
I would prefer to get rid of this driver. We could do it step by step, e.g. for every platform where it causes trouble, test (from the OS, .final might be too early) if FSP configured things, and if so, remove the PCI IDs for that platform from this driver. Leave a comment to make sure they are not added back by accident. If we end up with an empty PCI ID list eventually, we can drop the driver.
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
Did you compare testing results between CML-H and other platforms like SKL/KBL-H?
Yes, SHL/KBL H also has total 8 SATA port. The only difference is SPD bit definitions there. thats we have already handled like this
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Difference between H and LP would be for SATA, LP has 0-2 implemented and rest 3-7 reserved.
I wonder why I had no problems with this on X11SSM-F (100series PCH-H) so far
May your SATA direct connect Port is between 0-2, else you would also see this issue for sure.
I have 8 SATA Ports and use 2-6 and there are no problems
You seems lucky that in ur case FSP might have already written to SPD register and as this SPD register is RW/O hence coreboot next writes has no effect. But if FSP skips settings those SPD and coreboot only know abut (ports 0-2) and disables other ports it would have result the same what i'm seeing now.
So now question is do you like to over commit that FSP will always things proper and coreboot might not need to program SPD bits at finalize stage ? This code might be like double insurance in coreboot to handle such misses from FSP.
I would prefer to get rid of this driver. We could do it step by step, e.g. for every platform where it causes trouble, test (from the OS, .final might be too early) if FSP configured things, and if so, remove the PCI IDs for that platform from this driver. Leave a comment to make sure they are not added back by accident. If we end up with an empty PCI ID list eventually, we can drop the driver.
SG
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
tbh I'm not sure if there is any sense in keeping a driver... FSP already does everything. when you need hot-plug aka detection after bootup, then enable hotplug in the devicetree/upds
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
In my tests (I needed to add the PCH-H device id) FSP writes and locks MAP. I could not write it anymore in sata_final.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4: -Code-Review
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
If different FSP binaries do different things with these registers and then we have to do different things in coreboot, maybe we should make this platform-specific.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4: -Code-Review
Patch Set 4:
If you want to keep and fix the driver, I suggest to first test at what stage (silicon init / notification phase?) FSP configures things. coreboot's .final seems to run after the PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
If different FSP binaries do different things with these registers and then we have to do different things in coreboot, maybe we should make this platform-specific.
It might be a bug inside FSP which will fix that not the concern here, but my concern is if FSP misses such that doesn't mean coreboot will perform wrong programming hence the invention was to make common code right to handle PCH-H SATA controller as well. This driver is doing thing right as per register it just because those registers are RW/O hence 2nd time write doesn't take any effect.
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4: -Code-Review
Patch Set 4:
> If you want to keep and fix the driver, I suggest to first > test at what stage (silicon init / notification phase?) FSP > configures things. coreboot's .final seems to run after the > PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
If different FSP binaries do different things with these registers and then we have to do different things in coreboot, maybe we should make this platform-specific.
AFAICT cnl and kbl don't differ in this regard and they both do the "right thing" except that disabling ports that are non-hot-plug and have no device present is not very nice behaviour...
It might be a bug inside FSP which will fix that not the concern here, but my concern is if FSP misses such that doesn't mean coreboot will perform wrong programming hence the invention was to make common code right to handle PCH-H SATA controller as well. This driver is doing thing right as per register it just because those registers are RW/O hence 2nd time write doesn't take any effect.
I don't see a real bug in FSP - this seems to be wanted behaviour from the devs...
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
I guess we can't do anything about that bad FSP behaviour. A workaround would be setting hotplug for all ports that have no device present - then FSP won't disabled them - but I don't really like that tbh :S
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
I would just remove the whole driver. Looking at its history: * It was added in a broken state. The issue it reportedly fixed can only mean that FSP left the AHCI Ports Implemented register in an inconsistent state (or maybe just unitialized, but this driver didn't initialize it either). * It was most likely added to help with a broken FSP1.1. Which we don't use anymore. * It was initially written for SKL and KBL U/Y. Still other, incompatible PCI IDs were added which resulted in wild patching. And it still writes a different register for all other platforms than originally intended. * It needed a lot of maintenance, probably sucked thousands of dollars out of the coreboot community and still continues to. But it most likely doesn't provide any value since FSP2.0. * Doing things in FSP and coreboot redundantly can increase complexity and costs by a power of 2.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
I would just remove the whole driver. Looking at its history:
- It was added in a broken state. The issue it reportedly fixed can only mean that FSP left the AHCI Ports Implemented register in an inconsistent state (or maybe just unitialized, but this driver didn't initialize it either).
[Subrata] Without this driver, patching such FSP issue would have impossible and we need to wait for FSP fix which might be timely affair in some case when it need the most.
- It was most likely added to help with a broken FSP1.1. Which we don't use anymore.
[Subrata] Earlier FSP was not programming those chipset register during POST hence we were seeing those issue. original CL https://review.coreboot.org/c/coreboot/+/17229/ Lately FSP looks like has started taking care of SATA register hence this driver lost its purpose due to RW/O enforcement.
- It was initially written for SKL and KBL U/Y. Still other, incompatible PCI IDs were added which resulted in wild patching. And it still writes a different register for all other platforms than originally intended.
[Subrata] Not sure why you have repeatedly telling about incompatible PCI ID. Since gen5 all SATA chipset register are same except SPD register value been inverted which already taken care.
- It needed a lot of maintenance, probably sucked thousands of dollars out of the coreboot community and still continues to. But it most likely doesn't provide any value since FSP2.0.
- Doing things in FSP and coreboot redundantly can increase complexity and costs by a power of 2.
[Subrata] Agree, but there is no such good document to really callout what is redundant between those 2
Can we take a step approach here. Any suggestion here ?
1. Let this CL fix the SATA driver issue and unblock booting from SATA when FSP missed to program those register and its remain at default state. 2. We shall remove latest SOC SATA ID (TGL and JSL) from this driver to ensure latest IA doesn't use this as FSP already do the needful. I don't want to disturb other SOC by removing entire driver at first place 3. From 1 generation from now (from coreboot next version release) we again evaluate the need of this driver and remove completely?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
- It was most likely added to help with a broken FSP1.1. Which we don't use anymore.
[Subrata] Earlier FSP was not programming those chipset register during POST hence we were seeing those issue. original CL https://review.coreboot.org/c/coreboot/+/17229/ Lately FSP looks like has started taking care of SATA register hence this driver lost its purpose due to RW/O enforcement.
So... which platform is left then where the driver does anything?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
I would just remove the whole driver. Looking at its history:
- It was added in a broken state. The issue it reportedly fixed can only mean that FSP left the AHCI Ports Implemented register in an inconsistent state (or maybe just unitialized, but this driver didn't initialize it either).
[Subrata] Without this driver, patching such FSP issue would have impossible and we need to wait for FSP fix which might be timely affair in some case when it need the most.
Yes, no need to argue here. The original driver solved a problem. But it was only intended for SKL/KBL U/Y. And all later patches were wrong.
- It was most likely added to help with a broken FSP1.1. Which we don't use anymore.
[Subrata] Earlier FSP was not programming those chipset register during POST hence we were seeing those issue. original CL https://review.coreboot.org/c/coreboot/+/17229/ Lately FSP looks like has started taking care of SATA register hence this driver lost its purpose due to RW/O enforcement.
You are mixing things up. The register bits written in CB:17229 still exist and are *not* R/WO. Only their location changed.
- It was initially written for SKL and KBL U/Y. Still other, incompatible PCI IDs were added which resulted in wild patching. And it still writes a different register for all other platforms than originally intended.
[Subrata] Not sure why you have repeatedly telling about incompatible PCI ID.
Because you are wrong. Have you looked into all datasheets for the given IDs? I have.
Since gen5 all SATA chipset register are same except SPD register value been inverted which already taken care.
That's not true. SPD bits always acted the way they do on current chips. The D is for disable btw. and these bits were introduced with ICH9. Again only the register layout changed. Which made this driver write SPD bits *by accident*.
- It needed a lot of maintenance, probably sucked thousands of dollars out of the coreboot community and still continues to. But it most likely doesn't provide any value since FSP2.0.
- Doing things in FSP and coreboot redundantly can increase complexity and costs by a power of 2.
[Subrata] Agree, but there is no such good document to really callout what is redundant between those 2
Well, I have approached Intel about this 4 years ago. It doesn't help.
Can we take a step approach here. Any suggestion here ?
- Let this CL fix the SATA driver issue and unblock booting from SATA when FSP missed to program those register and its remain at default state.
- We shall remove latest SOC SATA ID (TGL and JSL) from this driver to ensure latest IA doesn't use this as FSP already do the needful. I don't want to disturb other SOC by removing entire driver at first place
- From 1 generation from now (from coreboot next version release) we again evaluate the need of this driver and remove completely?
I don't think it's worth to make any plans as long as the developer behind this driver doesn't read the datasheets correctly.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
I would just remove the whole driver. Looking at its history:
- It was added in a broken state. The issue it reportedly fixed can only mean that FSP left the AHCI Ports Implemented register in an inconsistent state (or maybe just unitialized, but this driver didn't initialize it either).
[Subrata] Without this driver, patching such FSP issue would have impossible and we need to wait for FSP fix which might be timely affair in some case when it need the most.
Yes, no need to argue here. The original driver solved a problem. But it was only intended for SKL/KBL U/Y. And all later patches were wrong.
- It was most likely added to help with a broken FSP1.1. Which we don't use anymore.
[Subrata] Earlier FSP was not programming those chipset register during POST hence we were seeing those issue. original CL https://review.coreboot.org/c/coreboot/+/17229/ Lately FSP looks like has started taking care of SATA register hence this driver lost its purpose due to RW/O enforcement.
You are mixing things up. The register bits written in CB:17229 still exist and are *not* R/WO. Only their location changed.
- It was initially written for SKL and KBL U/Y. Still other, incompatible PCI IDs were added which resulted in wild patching. And it still writes a different register for all other platforms than originally intended.
[Subrata] Not sure why you have repeatedly telling about incompatible PCI ID.
Because you are wrong. Have you looked into all datasheets for the given IDs? I have.
Since gen5 all SATA chipset register are same except SPD register value been inverted which already taken care.
That's not true. SPD bits always acted the way they do on current chips. The D is for disable btw. and these bits were introduced with ICH9. Again only the register layout changed. Which made this driver write SPD bits *by accident*.
- It needed a lot of maintenance, probably sucked thousands of dollars out of the coreboot community and still continues to. But it most likely doesn't provide any value since FSP2.0.
- Doing things in FSP and coreboot redundantly can increase complexity and costs by a power of 2.
[Subrata] Agree, but there is no such good document to really callout what is redundant between those 2
Well, I have approached Intel about this 4 years ago. It doesn't help.
Can we take a step approach here. Any suggestion here ?
- Let this CL fix the SATA driver issue and unblock booting from SATA when FSP missed to program those register and its remain at default state.
- We shall remove latest SOC SATA ID (TGL and JSL) from this driver to ensure latest IA doesn't use this as FSP already do the needful. I don't want to disturb other SOC by removing entire driver at first place
- From 1 generation from now (from coreboot next version release) we again evaluate the need of this driver and remove completely?
I don't think it's worth to make any plans as long as the developer behind this driver doesn't read the datasheets correctly.
So, lets get this driver drop now ? we do have any objection from SKL/KBL users still ?
Hello build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Angel Pons, Michael Niewöhner, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41674
to look at the new patch set (#5).
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
soc/intel/common/{pch,sata}: Remove SATA common code driver
Right now all FSP2.0 based IA platform doesn't need this driver anymore hence removing to avoid debug and maintenance effort.
TEST=Verified booting from SATA on SPT/CNP/ICP/TGP PCH platforms.
<TEST in progress will get back with results>
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- D src/soc/intel/common/block/sata/Kconfig D src/soc/intel/common/block/sata/Makefile.inc D src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/pch/Kconfig 4 files changed, 0 insertions(+), 118 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/5
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, Wonkyu Kim, Angel Pons, Michael Niewöhner, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41674
to look at the new patch set (#6).
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
soc/intel/common/{pch,sata}: Remove SATA common code driver
Right now all FSP2.0 based IA platform doesn't need this driver anymore hence removing to avoid debug and maintenance effort.
TEST=Verified booting from SATA on SPT/CNP/ICP/TGP PCH platforms.
<TEST in progress will get back with results>
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig D src/soc/intel/common/block/sata/Kconfig D src/soc/intel/common/block/sata/Makefile.inc D src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/pch/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/tigerlake/Kconfig 8 files changed, 0 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Matt, can you give this a shot on one of the SKL Librems? Would be good to know that SATA still works and an `lspci -xxx` for the SATA controller would be nice.
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
So, lets get this driver drop now ? we do have any objection from SKL/KBL users still ?
I would like to clarify this with the guys from facebook. @Andrew, @David, @Jonathan, please tell us, can SATA in the Lewisburg C62x chipset work without this driver? Can we abandon CB:39918?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
So, lets get this driver drop now ? we do have any objection from SKL/KBL users still ?
I would like to clarify this with the guys from facebook. @Andrew, @David, @Jonathan, please tell us, can SATA in the Lewisburg C62x chipset work without this driver? Can we abandon CB:39918?
Valid ask, i was thinking if its still must for SPT PCH, we shall only select the required SoC Kconfig and not from common PCH Kconfig.
Will wait for complete test report from Intel/Matt/FB folks. Thanks for taking this.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
I'll test this on the librem_skl boards tomorrow
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
I'll test this on the librem_skl boards tomorrow
Thanks Matt
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
I'll test this on the librem_skl boards tomorrow
Thanks Matt
no issues on a Librem 13v2 (SKL) w/2.5" SATA drive
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
I'll test this on the librem_skl boards tomorrow
Thanks Matt
no issues on a Librem 13v2 (SKL) w/2.5" SATA drive
Thanks Matt.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
no issues on a Librem 13v2 (SKL) w/2.5" SATA drive
Thanks Matt.
lspci output (same w/ and w/o patch):
00:17.0 SATA controller: Intel Corporation Sunrise Point-LP SATA Controller [AHCI mode] (rev 21) 00: 86 80 03 9d 07 04 b0 02 21 01 06 01 00 00 00 00 10: 00 00 43 d1 00 60 43 d1 61 1c 00 00 69 1c 00 00 20: 41 1c 00 00 00 50 43 d1 00 00 00 00 86 80 70 72 30: 00 00 00 00 80 00 00 00 00 00 00 00 0b 01 00 00
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
lspci output (same w/ and w/o patch):
this time with root:
00:17.0 SATA controller: Intel Corporation Sunrise Point-LP SATA Controller [AHCI mode] (rev 21) 00: 86 80 03 9d 07 04 b0 02 21 01 06 01 00 00 00 00 10: 00 00 23 d1 00 60 23 d1 61 1c 00 00 69 1c 00 00 20: 41 1c 00 00 00 50 23 d1 00 00 00 00 86 80 70 72 30: 00 00 00 00 80 00 00 00 00 00 00 00 0b 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70: 01 a8 03 40 08 00 00 00 00 00 00 00 00 00 00 00 80: 05 70 01 00 38 02 e0 fe 00 00 00 00 00 00 00 00 90: 00 06 01 81 83 01 00 00 24 02 dc 20 30 00 00 80 a0: a4 00 00 00 00 00 00 00 12 00 10 00 48 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 11 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 b3 0f 40 08 00 00 00 00
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6: Code-Review+1
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
hey on OCP hw (tiogapass and sonorapass) this was needed last time I checked ;)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
hey on OCP hw (tiogapass and sonorapass) this was needed last time I checked ;)
@Andrey, can you please help me to share the underlying SOCs for those boards. I suspect FSP is not programming those registers hence CB programming are needed to override chipset defaults.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6: Code-Review+1
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
@Andrey, can you please help me to share the underlying SOCs for those boards.
these are not SoCs, but for "cooperlake-sp" and "skylake-sp" this driver was useful last time I checked. Without it linux kernel would not touch ports since they were disabled (seabios and tianocore did not care if ports were enabled and just used them anyway) Jonathan should be more up-to-date than I am, because I am no longer involved with OCP work
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
@Andrey, can you please help me to share the underlying SOCs for those boards.
these are not SoCs, but for "cooperlake-sp" and "skylake-sp" this driver was useful last time I checked. Without it linux kernel would not touch ports since they were disabled (seabios and tianocore did not care if ports were enabled and just used them anyway) Jonathan should be more up-to-date than I am, because I am no longer involved with OCP work
i mean to say if i got to know a SoC/Processor name or ID it would be easy to implement W/A. i had the same issue what u are facing inside Depthcharge AHCI controller code
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
@Andrey, can you please help me to share the underlying SOCs for those boards.
these are not SoCs, but for "cooperlake-sp" and "skylake-sp" this driver was useful last time I checked. Without it linux kernel would not touch ports since they were disabled (seabios and tianocore did not care if ports were enabled and just used them anyway) Jonathan should be more up-to-date than I am, because I am no longer involved with OCP work
Do they use Lewisburg PCH? This driver only writes disable bits for LWB, AFAICS, and wouldn't enable anything.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
@Andrey, can you please help me to share the underlying SOCs for those boards.
these are not SoCs, but for "cooperlake-sp" and "skylake-sp" this driver was useful last time I checked. Without it linux kernel would not touch ports since they were disabled (seabios and tianocore did not care if ports were enabled and just used them anyway) Jonathan should be more up-to-date than I am, because I am no longer involved with OCP work
i mean to say if i got to know a SoC/Processor name or ID it would be easy to implement W/A. i had the same issue what u are facing inside Depthcharge AHCI controller code
The IDs can be found in src/soc/intel/xeon_sp/skx/include/soc/cpu.h or src/soc/intel/xeon_sp/cpx/include/soc/cpu.h files. Refer to https://review.coreboot.org/c/coreboot/+/39918 for some contexts. Also: * On my TiogaPass (SKX-SP based), there is SATA drive attached to PCH, it works fine without coreboot SATA driver. * SonoraPass (CPX-SP based) server program was cancelled. My deltalake (CPX-SP based) server does not have SATA drive, so I am not able to verify this.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
@Andrey, can you please help me to share the underlying SOCs for those boards.
these are not SoCs, but for "cooperlake-sp" and "skylake-sp" this driver was useful last time I checked. Without it linux kernel would not touch ports since they were disabled (seabios and tianocore did not care if ports were enabled and just used them anyway) Jonathan should be more up-to-date than I am, because I am no longer involved with OCP work
i mean to say if i got to know a SoC/Processor name or ID it would be easy to implement W/A. i had the same issue what u are facing inside Depthcharge AHCI controller code
The IDs can be found in src/soc/intel/xeon_sp/skx/include/soc/cpu.h or src/soc/intel/xeon_sp/cpx/include/soc/cpu.h files. Refer to https://review.coreboot.org/c/coreboot/+/39918 for some contexts. Also:
- On my TiogaPass (SKX-SP based), there is SATA drive attached to PCH, it works fine without coreboot SATA driver.
Thanks for confirming this.
- SonoraPass (CPX-SP based) server program was cancelled. My deltalake (CPX-SP based) server does not have SATA drive, so I am not able to verify this.
So, looks like we are good with this CL and remove SATA common driver support.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 6: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/41674/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41674/6//COMMIT_MSG@14 PS6, Line 14: <TEST in progress will get back with results> Please update.
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/41674/4/src/soc/intel/common/block/... PS4, Line 12: #define SATA_PCI_CFG_PORT_CTL_STS 0x92
yes and yes
Done
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Furquan Shaikh, Wonkyu Kim, Matt DeVillier, Angel Pons, Michael Niewöhner, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41674
to look at the new patch set (#7).
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
soc/intel/common/{pch,sata}: Remove SATA common code driver
Right now all FSP2.0 based IA platform doesn't need this driver anymore hence removing to avoid debug and maintenance effort.
TEST=Verified booting from SATA on SPT/CNP/ICP/TGP PCH platforms.
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig D src/soc/intel/common/block/sata/Kconfig D src/soc/intel/common/block/sata/Makefile.inc D src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/pch/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/tigerlake/Kconfig 8 files changed, 0 insertions(+), 122 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/41674/7
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41674/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41674/6//COMMIT_MSG@14 PS6, Line 14: <TEST in progress will get back with results>
Please update.
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 7: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 7: Code-Review+2
Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
soc/intel/common/{pch,sata}: Remove SATA common code driver
Right now all FSP2.0 based IA platform doesn't need this driver anymore hence removing to avoid debug and maintenance effort.
TEST=Verified booting from SATA on SPT/CNP/ICP/TGP PCH platforms.
Change-Id: Ied3832b26ba1fdd4c30fafe8149689a01d302c3e Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41674 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Michael Niewöhner Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/Kconfig D src/soc/intel/common/block/sata/Kconfig D src/soc/intel/common/block/sata/Makefile.inc D src/soc/intel/common/block/sata/sata.c M src/soc/intel/common/pch/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/jasperlake/Kconfig M src/soc/intel/tigerlake/Kconfig 8 files changed, 0 insertions(+), 122 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index f06d84b..dfc90cd 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -84,7 +84,6 @@ select PLATFORM_USES_FSP2_0 select REG_SCRIPT select SMP - select SOC_AHCI_PORT_IMPLEMENTED_INVERT select PMC_GLOBAL_RESET_ENABLE_LOCK select SOC_INTEL_COMMON select SOC_INTEL_COMMON_ACPI_WAKE_SOURCE diff --git a/src/soc/intel/common/block/sata/Kconfig b/src/soc/intel/common/block/sata/Kconfig deleted file mode 100644 index 98ff696..0000000 --- a/src/soc/intel/common/block/sata/Kconfig +++ /dev/null @@ -1,14 +0,0 @@ -config SOC_INTEL_COMMON_BLOCK_SATA - bool - help - Intel Processor common SATA support - -config SOC_AHCI_PORT_IMPLEMENTED_INVERT - depends on SOC_INTEL_COMMON_BLOCK_SATA - bool - help - SATA PCI configuration space offset 0x92 Port - implement register bit 0-2 represents respective - SATA port enable status as in 0 = Disable; 1 = Enable. - If this option is selected then port enable status will be - inverted as in 0 = Enable; 1 = Disable. diff --git a/src/soc/intel/common/block/sata/Makefile.inc b/src/soc/intel/common/block/sata/Makefile.inc deleted file mode 100644 index 623d151..0000000 --- a/src/soc/intel/common/block/sata/Makefile.inc +++ /dev/null @@ -1 +0,0 @@ -ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_SATA) += sata.c diff --git a/src/soc/intel/common/block/sata/sata.c b/src/soc/intel/common/block/sata/sata.c deleted file mode 100644 index d3f82ed..0000000 --- a/src/soc/intel/common/block/sata/sata.c +++ /dev/null @@ -1,102 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ - -#include <device/mmio.h> -#include <device/device.h> -#include <device/pci.h> -#include <device/pci_ops.h> -#include <device/pci_def.h> -#include <device/pci_ids.h> -#include <soc/pci_devs.h> - -#define SATA_ABAR_PORT_IMPLEMENTED 0x0c -#define SATA_PCI_CFG_PORT_CTL_STS 0x92 - -static void *sata_get_ahci_bar(struct device *dev) -{ - uintptr_t bar; - - bar = pci_read_config32(dev, PCI_BASE_ADDRESS_5); - return (void *)(bar & ~PCI_BASE_ADDRESS_MEM_ATTR_MASK); -} - -/* - * SATA Port control and Status. By default, the SATA ports are set (by HW) - * to the disabled state (e.g. bits[3:0] == '0') as a result of an initial - * power on reset. When enabled by software as per SATA port mapping, - * the ports can transition between the on, partial and slumber states - * and can detect devices. When disabled, the port is in the off state and - * can't detect any devices. - */ -static void sata_final(struct device *dev) -{ - void *ahcibar = sata_get_ahci_bar(dev); - u8 port_impl, temp; - - /* Set Bus Master */ - pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); - - /* Read Ports Implemented (GHC_PI) */ - port_impl = read8(ahcibar + SATA_ABAR_PORT_IMPLEMENTED); - - if (CONFIG(SOC_AHCI_PORT_IMPLEMENTED_INVERT)) - port_impl = ~port_impl; - - port_impl &= 0x07; /* bit 0-2 */ - - /* Port enable */ - temp = pci_read_config8(dev, SATA_PCI_CFG_PORT_CTL_STS); - temp |= port_impl; - pci_write_config8(dev, SATA_PCI_CFG_PORT_CTL_STS, temp); -} - -static struct device_operations sata_ops = { - .read_resources = pci_dev_read_resources, - .set_resources = pci_dev_set_resources, - .enable_resources = pci_dev_enable_resources, - .final = sata_final, - .ops_pci = &pci_dev_ops_pci, -}; - -static const unsigned short pci_device_ids[] = { - PCI_DEVICE_ID_INTEL_SPT_U_SATA, - PCI_DEVICE_ID_INTEL_SPT_U_Y_PREMIUM_SATA, - PCI_DEVICE_ID_INTEL_SPT_KBL_SATA, - PCI_DEVICE_ID_INTEL_LWB_SATA_AHCI, - PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI, - PCI_DEVICE_ID_INTEL_LWB_SATA_RAID, - PCI_DEVICE_ID_INTEL_LWB_SSATA_RAID, - PCI_DEVICE_ID_INTEL_LWB_SATA_AHCI_SUPER, - PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI_SUPER, - PCI_DEVICE_ID_INTEL_LWB_SATA_RAID_SUPER, - PCI_DEVICE_ID_INTEL_LWB_SSATA_RAID_SUPER, - PCI_DEVICE_ID_INTEL_LWB_SATA_ALT, - PCI_DEVICE_ID_INTEL_LWB_SATA_ALT_RST, - PCI_DEVICE_ID_INTEL_LWB_SSATA_ALT, - PCI_DEVICE_ID_INTEL_LWB_SSATA_ALT_RST, - PCI_DEVICE_ID_INTEL_CNL_SATA, - PCI_DEVICE_ID_INTEL_CNL_PREMIUM_SATA, - PCI_DEVICE_ID_INTEL_CNP_CMP_COMPAT_SATA, - PCI_DEVICE_ID_INTEL_CNP_H_SATA, - PCI_DEVICE_ID_INTEL_CNP_LP_SATA, - PCI_DEVICE_ID_INTEL_ICP_U_SATA, - PCI_DEVICE_ID_INTEL_CMP_SATA, - PCI_DEVICE_ID_INTEL_CMP_PREMIUM_SATA, - PCI_DEVICE_ID_INTEL_CMP_LP_SATA, - PCI_DEVICE_ID_INTEL_CMP_H_SATA, - PCI_DEVICE_ID_INTEL_CMP_H_HALO_SATA, - PCI_DEVICE_ID_INTEL_CMP_H_PREMIUM_SATA, - PCI_DEVICE_ID_INTEL_TGP_LP_SATA, - PCI_DEVICE_ID_INTEL_TGP_SATA, - PCI_DEVICE_ID_INTEL_TGP_PREMIUM_SATA, - PCI_DEVICE_ID_INTEL_TGP_COMPAT_SATA, - PCI_DEVICE_ID_INTEL_MCC_AHCI_SATA, - PCI_DEVICE_ID_INTEL_JSP_SATA_1, - PCI_DEVICE_ID_INTEL_JSP_SATA_2, - 0 -}; - -static const struct pci_driver pch_sata __pci_driver = { - .ops = &sata_ops, - .vendor = PCI_VENDOR_ID_INTEL, - .devices = pci_device_ids, -}; diff --git a/src/soc/intel/common/pch/Kconfig b/src/soc/intel/common/pch/Kconfig index cca65d6..6e7f2f6 100644 --- a/src/soc/intel/common/pch/Kconfig +++ b/src/soc/intel/common/pch/Kconfig @@ -32,7 +32,6 @@ select SOC_INTEL_COMMON_BLOCK_PCR select SOC_INTEL_COMMON_BLOCK_PMC select SOC_INTEL_COMMON_BLOCK_RTC - select SOC_INTEL_COMMON_BLOCK_SATA select SOC_INTEL_COMMON_BLOCK_SMBUS select SOC_INTEL_COMMON_BLOCK_SPI select SOC_INTEL_COMMON_BLOCK_TCO diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 2a5156b..52092ba 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -34,7 +34,6 @@ select PLATFORM_USES_FSP2_1 select REG_SCRIPT select SMP - select SOC_AHCI_PORT_IMPLEMENTED_INVERT select PMC_GLOBAL_RESET_ENABLE_LOCK select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON diff --git a/src/soc/intel/jasperlake/Kconfig b/src/soc/intel/jasperlake/Kconfig index bfefbf2..21dba0e 100644 --- a/src/soc/intel/jasperlake/Kconfig +++ b/src/soc/intel/jasperlake/Kconfig @@ -35,7 +35,6 @@ select PLATFORM_USES_FSP2_1 select REG_SCRIPT select SMP - select SOC_AHCI_PORT_IMPLEMENTED_INVERT select PMC_GLOBAL_RESET_ENABLE_LOCK select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index e0d29fb..fbf56b4 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -34,7 +34,6 @@ select PLATFORM_USES_FSP2_1 select REG_SCRIPT select SMP - select SOC_AHCI_PORT_IMPLEMENTED_INVERT select PMC_GLOBAL_RESET_ENABLE_LOCK select CPU_INTEL_COMMON_SMM select SOC_INTEL_COMMON
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
Patch Set 8:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4714 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4713 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4712 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4711
Please note: This test is under development and might not be accurate at all!
Andrew McRae has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/{pch,sata}: Remove SATA common code driver ......................................................................
-
9elements QA (Code Review) -
Aamir Bohra (Code Review)
-
Andrew McRae (Code Review)
-
Andrey Petrov (Code Review)
-
Angel Pons (Code Review)
-
Jonathan Zhang (Code Review)
-
Matt DeVillier (Code Review)
-
Maxim Polyakov (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)