* Re: [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed. [not found] <57eb87fe03d054ffd8078bd40652104c9bc5c76e.1632280373.git.xueshengfeng@byosoft.com.cn> @ 2021-09-22 5:45 ` Wu, Hao A 2021-09-22 5:47 ` [edk2-devel] " Wu, Hao A 2021-09-22 6:41 ` Ni, Ray 1 sibling, 1 reply; 3+ messages in thread From: Wu, Hao A @ 2021-09-22 5:45 UTC (permalink / raw) To: Xue, Shengfeng, devel@edk2.groups.io, gaoliming@byosoft.com.cn, Ni, Ray Cc: Xue, ShengfengX, Liang, PanlingX Two inline comments below: > -----Original Message----- > From: xueshengfeng <xueshengfeng@byosoft.com.cn> > Sent: Wednesday, September 22, 2021 11:18 AM > To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Wu, Hao A > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com> > Cc: Xue, ShengfengX <shengfengx.xue@intel.com>; Liang, PanlingX > <panlingx.liang@intel.com> > Subject: [PATCH] On branch PCIBus dulePkg/PciBusDxe: > PciTestSupportedAttribute logic should be changed. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3635 > > In file Edk2\MdeModulePkg\Bus\Pci\PciBusDxe\PciEnumeratorSupport.c > Function PciTestSupportedAttribute, This function is called when doing the PCI > enumerate, it tries to test whether the device can support given attributes by > follow steps. > 1), read the original register value > 2), set to the input register value > 3), read back the register value, return this value as output 4), restore the > original value This will cause the enabled bits in this > register be disabled during this sequence. > > Below are the new suggested flow: > 1) , read the original register value. > 2), set to input register value OR(|) the original register value. > 3), read back the register value, return the value AND(&) the input > command value as output. > 4), restore the original value > > Above sequence will not change the enabled bits in the register, > and can the new supported attributes by the device. > > Signed-off-by: shengfengx.xue@intel.com > Reviewed-by: gaoliming@byosoft.com.cn Please fix the inconsistent space indent within the commit log message. > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > index db1b35f8ef..2462f58833 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > @@ -933,6 +933,7 @@ PciTestSupportedAttribute ( > ) > { > EFI_TPL OldTpl; > + UINT16 CommandTemp = 0; Please separate the local variable definition and its initial value assignment. With the above 2 comments handled, Reviewed-by: Hao A Wu <hao.a.wu@intel.com> Best Regards, Hao Wu > > // > // Preserve the original value > @@ -944,9 +945,10 @@ PciTestSupportedAttribute ( > // > OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); > > - PCI_SET_COMMAND_REGISTER (PciIoDevice, *Command); > - PCI_READ_COMMAND_REGISTER (PciIoDevice, Command); > + PCI_SET_COMMAND_REGISTER (PciIoDevice, (*Command | *OldCommand)); > + PCI_READ_COMMAND_REGISTER (PciIoDevice, &CommandTemp); > > + *Command = (*Command) & CommandTemp; > // > // Write back the original value > // > -- > 2.31.1.windows.1 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed. 2021-09-22 5:45 ` [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed Wu, Hao A @ 2021-09-22 5:47 ` Wu, Hao A 0 siblings, 0 replies; 3+ messages in thread From: Wu, Hao A @ 2021-09-22 5:47 UTC (permalink / raw) To: devel@edk2.groups.io, Wu, Hao A, Xue, Shengfeng, gaoliming@byosoft.com.cn, Ni, Ray Cc: Xue, ShengfengX, Liang, PanlingX Sorry for missing one comment. Please help to update the subject of the patch to: MdeModulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A > Sent: Wednesday, September 22, 2021 1:45 PM > To: Xue, Shengfeng <xueshengfeng@byosoft.com.cn>; devel@edk2.groups.io; > gaoliming@byosoft.com.cn; Ni, Ray <ray.ni@intel.com> > Cc: Xue, ShengfengX <shengfengx.xue@intel.com>; Liang, PanlingX > <panlingx.liang@intel.com> > Subject: Re: [edk2-devel] [PATCH] On branch PCIBus dulePkg/PciBusDxe: > PciTestSupportedAttribute logic should be changed. > > Two inline comments below: > > > > -----Original Message----- > > From: xueshengfeng <xueshengfeng@byosoft.com.cn> > > Sent: Wednesday, September 22, 2021 11:18 AM > > To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Wu, Hao A > > <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com> > > Cc: Xue, ShengfengX <shengfengx.xue@intel.com>; Liang, PanlingX > > <panlingx.liang@intel.com> > > Subject: [PATCH] On branch PCIBus dulePkg/PciBusDxe: > > PciTestSupportedAttribute logic should be changed. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3635 > > > > In file Edk2\MdeModulePkg\Bus\Pci\PciBusDxe\PciEnumeratorSupport.c > > Function PciTestSupportedAttribute, This function is called when > > doing the PCI enumerate, it tries to test whether the device can > > support given attributes by follow steps. > > 1), read the original register value > > 2), set to the input register value > > 3), read back the register value, return this value as output 4), > > restore the original value This will cause the enabled bits in this > > register be disabled during this sequence. > > > > Below are the new suggested flow: > > 1) , read the original register value. > > 2), set to input register value OR(|) the original register value. > > 3), read back the register value, return the value AND(&) the input > > command value as output. > > 4), restore the original value > > > > Above sequence will not change the enabled bits in the register, > > and can the new supported attributes by the device. > > > > Signed-off-by: shengfengx.xue@intel.com > > Reviewed-by: gaoliming@byosoft.com.cn > > > Please fix the inconsistent space indent within the commit log message. > > > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > index db1b35f8ef..2462f58833 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > > @@ -933,6 +933,7 @@ PciTestSupportedAttribute ( > > ) > > { > > EFI_TPL OldTpl; > > + UINT16 CommandTemp = 0; > > > Please separate the local variable definition and its initial value assignment. > > With the above 2 comments handled, > Reviewed-by: Hao A Wu <hao.a.wu@intel.com> > > Best Regards, > Hao Wu > > > > > > // > > // Preserve the original value > > @@ -944,9 +945,10 @@ PciTestSupportedAttribute ( > > // > > OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); > > > > - PCI_SET_COMMAND_REGISTER (PciIoDevice, *Command); > > - PCI_READ_COMMAND_REGISTER (PciIoDevice, Command); > > + PCI_SET_COMMAND_REGISTER (PciIoDevice, (*Command | > *OldCommand)); > > + PCI_READ_COMMAND_REGISTER (PciIoDevice, &CommandTemp); > > > > + *Command = (*Command) & CommandTemp; > > // > > // Write back the original value > > // > > -- > > 2.31.1.windows.1 > > > > > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed. [not found] <57eb87fe03d054ffd8078bd40652104c9bc5c76e.1632280373.git.xueshengfeng@byosoft.com.cn> 2021-09-22 5:45 ` [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed Wu, Hao A @ 2021-09-22 6:41 ` Ni, Ray 1 sibling, 0 replies; 3+ messages in thread From: Ni, Ray @ 2021-09-22 6:41 UTC (permalink / raw) To: Xue, Shengfeng, devel@edk2.groups.io, gaoliming@byosoft.com.cn, Wu, Hao A Cc: Xue, ShengfengX, Liang, PanlingX Shengfeng, I guess your patch is to avoid some other bits (other than EFI_PCI_COMMAND_IO_SPACE, EFI_PCI_COMMAND_MEMORY_SPACE, EFI_PCI_COMMAND_BUS_MASTER, EFI_PCI_COMMAND_VGA_PALETTE_SNOOP) be cleared by the first PCI_SET_COMMAND_REGISTER(). Can you refine your commit message to describe it clearly? The current commit message looks very confusing to me. And don't do inline assignment please. The flow could be: UINT16 CommandValue; PCI_READ_COMMAND_REGISTER (PciIoDevice, OldCommand); // // Raise TPL to high level to disable timer interrupt while the BAR is probed // OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); CommandValue = *Command | OldCommand; PCI_SET_COMMAND_REGISTER (PciIoDevice, CommandValue); PCI_READ_COMMAND_REGISTER (PciIoDevice, &CommandValue); *Command = *Command & CommandValue; // // Write back the original value // PCI_SET_COMMAND_REGISTER (PciIoDevice, *OldCommand); -----Original Message----- From: xueshengfeng <xueshengfeng@byosoft.com.cn> Sent: Wednesday, September 22, 2021 11:18 AM To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com> Cc: Xue, ShengfengX <shengfengx.xue@intel.com>; Liang, PanlingX <panlingx.liang@intel.com> Subject: [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3635 In file Edk2\MdeModulePkg\Bus\Pci\PciBusDxe\PciEnumeratorSupport.c Function PciTestSupportedAttribute, This function is called when doing the PCI enumerate, it tries to test whether the device can support given attributes by follow steps. 1), read the original register value 2), set to the input register value 3), read back the register value, return this value as output 4), restore the original value This will cause the enabled bits in this register be disabled during this sequence. Below are the new suggested flow: 1) , read the original register value. 2), set to input register value OR(|) the original register value. 3), read back the register value, return the value AND(&) the input command value as output. 4), restore the original value Above sequence will not change the enabled bits in the register, and can the new supported attributes by the device. Signed-off-by: shengfengx.xue@intel.com Reviewed-by: gaoliming@byosoft.com.cn --- MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c index db1b35f8ef..2462f58833 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c @@ -933,6 +933,7 @@ PciTestSupportedAttribute ( ) { EFI_TPL OldTpl; + UINT16 CommandTemp = 0; // // Preserve the original value @@ -944,9 +945,10 @@ PciTestSupportedAttribute ( // OldTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL); - PCI_SET_COMMAND_REGISTER (PciIoDevice, *Command); - PCI_READ_COMMAND_REGISTER (PciIoDevice, Command); + PCI_SET_COMMAND_REGISTER (PciIoDevice, (*Command | *OldCommand)); + PCI_READ_COMMAND_REGISTER (PciIoDevice, &CommandTemp); + *Command = (*Command) & CommandTemp; // // Write back the original value // -- 2.31.1.windows.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-22 6:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <57eb87fe03d054ffd8078bd40652104c9bc5c76e.1632280373.git.xueshengfeng@byosoft.com.cn>
2021-09-22 5:45 ` [PATCH] On branch PCIBus dulePkg/PciBusDxe: PciTestSupportedAttribute logic should be changed Wu, Hao A
2021-09-22 5:47 ` [edk2-devel] " Wu, Hao A
2021-09-22 6:41 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox