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

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

[Bryan Rosario]

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

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

[Bryan Rosario]

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
>
>

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

[Ken Taylor]

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

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

[Bryan Rosario]

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
>
>

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

[Laszlo Ersek]

On 11/22/18 01:06, Bryan Rosario via edk2-devel wrote:
> 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?

It's ancient code that should not exist, arguably. I've always been
afraid to rip it out, because I've never known what would break.

In general, BDS has zero business writing register of various PCI
devices. Even if it does, due to various platform glitches or hacks, BDS
should preferably not use PciLib APIs for that, but the UEFI PciIo
protocol. (I totally know that OVMF's Platform BDS does not live up to
this.)

> 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?

Remove it altogether from BDS (or else move it to the specific UEFI
driver) and see if things still work. :)

> 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?

Definitely. It certainly helps if you know what those register writes
actually mean. (I don't.)

Thanks,
Laszlo

> Link to the function I'm referring to:
> https://github.com/tianocore/edk2/blob/f6b0258d25a63ae3d3bc6430abe30fb625abc52a/OvmfPkg/SataControllerDxe/SataController.c#L1091-L1099