Re: Hardcoded IDE Controller B/D/F (0/1/1) in BdsPlatform.c?

[Ken Taylor <Ken_Taylor@phoenix.com>]

Hi Bryan,

If you were talking about PC architecture, I would suggest that there should be a hierarchy of configuration that needs to occur.  Specifically, there should be silicon / board / platform specific configuration that is architecturally independent of the SATA driver.

Normally, there would be early initialization to write to silicon specific registers.  Later, the PCI bus driver should expose PCI device interfaces.  That driver would normally be responsible for writing to the PCI configuration header for the device, as well as for configuring any PCI bridges between the host bus and the device.

The SATA driver would then connect to the PCI device interface exposed by the PCI bus driver.

However, I think you’re using an ARM based system.  The architecture is different from what I am used to.  Is what you have is a SATA driver talking directly to the PCI device interface?  This skips several levels of the architecture that would be responsible for maintaining platform independence.

If it were me, I would only put generic SATA code in the SATA driver.  There wouldn’t be anything silicon or platform specific at all.

If we were talking about the old IDE interfaces (because CF maybe), there used to be stuff for configuring 80 conductor vs 40 conductor cables that was handled by legacy BIOSes, and configuring high speed DMA and PIO transfer modes was silicon specific.  In the legacy architectures I worked on, there was usually an interface for silicon specific configuration that was architecturally independent of the IDE driver itself.

If you are facing the same sort of thing, then I would define a protocol for features that may require silicon specific register writes, and make my SATA code call that protocol.  This allows you to keep all your silicon/board/platform specific code in one location (perhaps one per design), and you don’t have to touch as many files every time your hardware design changes.  Ideally, you should have all the things that need to change boiled down to one particular piece of code, so you can focus on what you need to do to make new hardware work when you get new hardware and don’t have to discover all sorts of files that need editing piecemeal as you discover non-functional hardware features.

Regards,
-Ken.

From: Bryan Rosario [mailto:bcr@google.com]
Sent: Wednesday, November 21, 2018 5:29 PM
To: Ken Taylor
Cc: Alain Gefflaut; edk2-devel@lists.01.org
Subject: Re: [edk2] Hardcoded IDE Controller B/D/F (0/1/1) in BdsPlatform.c?

Hi Ken,

That's what I had guessed -- thanks for confirming that it's platform specific.

In my use case, I actually want the code to work on multiple hardware configurations, so I can't just replace the hardcoded B/D/F with the one used by a particular platform. Does the change to SataController.c that I mentioned in the original email make sense? I've tested it and confirmed that it works on my platform, but I want to get a sanity check that that is an appropriate place to put this code.

Thanks,
Bryan

On Wed, Nov 21, 2018 at 4:48 PM Ken Taylor <Ken_Taylor@phoenix.com<mailto:Ken_Taylor@phoenix.com>> wrote:
Hi Bryan,

That appears to be silicon or platform specific code.  It's intended for one specific hardware configuration.  Unless you have an identical hardware configuration, those particular register writes are actually nonsensical.

I think what you need to do is consult the silicon specific documentation for your particular hardware configuration, and determine what silicon specific registers need to be configured to what specific values to support your specific hardware design.

Regards,
-Ken.

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org<mailto:edk2-devel-bounces@lists.01.org>] On Behalf Of Bryan Rosario via edk2-devel
Sent: Wednesday, November 21, 2018 4:07 PM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Alain Gefflaut
Subject: [edk2] Hardcoded IDE Controller B/D/F (0/1/1) in BdsPlatform.c?

Hi all,

I noticed that there is apparently a hardcoded B/D/F for an IDE Controller in BdsPlatform.c, when setting bit 15 of a particular register:
https://github.com/tianocore/edk2/blob/14923c1a6bf9940b48feeaf47cb5d6c662b6528c/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c#L1612-L1617
.

Why is this hardcoded? Is it just old code that has hung around? I noticed that this code is from this commit, and the surrounding code at the time had lots of hardcoded B/D/Fs:
https://github.com/tianocore/edk2/commit/40f2c454343be84ab3bacf9955cc8d7842c70b5c
.

The context of this question is that I'm trying to make this work in a configuration with an IDE Controller located at a different B/D/F, and so the hardcoded value of 0/1/1 doesn't work for me.

If I want to change this so that it's not hardcoded, what is a good approach? I've added some code locally to the IdeInitSetTiming function in SataController.c to set the bit using the PciIo structure which is opened specifically on the controller in question -- does that sound like the right approach?
Link to the function I'm referring to:
https://github.com/tianocore/edk2/blob/f6b0258d25a63ae3d3bc6430abe30fb625abc52a/OvmfPkg/SataControllerDxe/SataController.c#L1091-L1099

Thanks,
Bryan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel