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

[Bryan Rosario <bcr@google.com>]

This is for a virtual x86 platform using OVMF. I think I have enough info
at this point to get things set up. Thanks for the help.

Bryan

On Wed, Nov 21, 2018 at 6:06 PM Ken Taylor <Ken_Taylor@phoenix.com> wrote:

> 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> 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] On Behalf Of
> Bryan Rosario via edk2-devel
> Sent: Wednesday, November 21, 2018 4:07 PM
> To: 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
> https://lists.01.org/mailman/listinfo/edk2-devel
>
>