[Linaro-uefi,v2,08/24] Hisilicon/PciHostBridgeDxe: add support for Hi1616

Message ID 1476796207-94336-9-git-send-email-heyi.guo@linaro.org
State New
Headers show

Commit Message

gary guo Oct. 18, 2016, 1:09 p.m.
PCIe root port controllers on Hi1616 are almost the same with those on
Hi1610, but there are some system integration differences. So we set
SOC type to a new value but in this driver they will enter the same
branch.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Leif Lindholm Oct. 27, 2016, 2:09 p.m. | #1
On Tue, Oct 18, 2016 at 09:09:52PM +0800, Heyi Guo wrote:
> PCIe root port controllers on Hi1616 are almost the same with those on
> Hi1610, but there are some system integration differences. So we set
> SOC type to a new value but in this driver they will enter the same
> branch.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> index 03edcf1..91775a9 100644
> --- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -741,7 +741,7 @@ BOOLEAN PcieIsLinkUp (UINT32 SocType, UINTN RbPciBar, UINTN Port)
>  {
>      UINT32                     Value = 0;
>  
> -    if (0x1610 == SocType)
> +    if ((0x1610 == SocType) || (0x1616 == SocType))

I would still like to see the comparison in logical order.
Or convert it to a switch statement.

>      {

And '{' on same line as 'if'.

>          Value = MmioRead32(RbPciBar + 0x131C);
>          if ((Value & 0x3F) == 0x11)
> -- 
> 1.9.1
>
gary guo Nov. 2, 2016, 2:31 a.m. | #2
As we are going to use the same SOC type as D03 for D05, this patch will 
be dropped.


Heyi


在 10/27/2016 10:09 PM, Leif Lindholm 写道:
> On Tue, Oct 18, 2016 at 09:09:52PM +0800, Heyi Guo wrote:
>> PCIe root port controllers on Hi1616 are almost the same with those on
>> Hi1610, but there are some system integration differences. So we set
>> SOC type to a new value but in this driver they will enter the same
>> branch.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> ---
>>   Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> index 03edcf1..91775a9 100644
>> --- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -741,7 +741,7 @@ BOOLEAN PcieIsLinkUp (UINT32 SocType, UINTN RbPciBar, UINTN Port)
>>   {
>>       UINT32                     Value = 0;
>>   
>> -    if (0x1610 == SocType)
>> +    if ((0x1610 == SocType) || (0x1616 == SocType))
> I would still like to see the comparison in logical order.
> Or convert it to a switch statement.
>
>>       {
> And '{' on same line as 'if'.
>
>>           Value = MmioRead32(RbPciBar + 0x131C);
>>           if ((Value & 0x3F) == 0x11)
>> -- 
>> 1.9.1
>>
Leif Lindholm Nov. 2, 2016, 5:20 p.m. | #3
On Wed, Nov 02, 2016 at 10:31:27AM +0800, Heyi Guo wrote:
> As we are going to use the same SOC type as D03 for D05, this patch will be
> dropped.

That will lead to confusion in modifying and debugging this code.
A preferable solution would be to set up a shared macro replacing all
of these direct hexadecimal comparisons.

> 
> Heyi
> 
> 
> 在 10/27/2016 10:09 PM, Leif Lindholm 写道:
> >On Tue, Oct 18, 2016 at 09:09:52PM +0800, Heyi Guo wrote:
> >>PCIe root port controllers on Hi1616 are almost the same with those on
> >>Hi1610, but there are some system integration differences. So we set
> >>SOC type to a new value but in this driver they will enter the same
> >>branch.
> >>
> >>Contributed-under: TianoCore Contribution Agreement 1.0
> >>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>---
> >>  Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> >>index 03edcf1..91775a9 100644
> >>--- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> >>+++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
> >>@@ -741,7 +741,7 @@ BOOLEAN PcieIsLinkUp (UINT32 SocType, UINTN RbPciBar, UINTN Port)
> >>  {
> >>      UINT32                     Value = 0;
> >>-    if (0x1610 == SocType)
> >>+    if ((0x1610 == SocType) || (0x1616 == SocType))
> >I would still like to see the comparison in logical order.
> >Or convert it to a switch statement.
> >
> >>      {
> >And '{' on same line as 'if'.
> >
> >>          Value = MmioRead32(RbPciBar + 0x131C);
> >>          if ((Value & 0x3F) == 0x11)
> >>-- 
> >>1.9.1
> >>
>

Patch

diff --git a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
index 03edcf1..91775a9 100644
--- a/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/Chips/Hisilicon/Drivers/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -741,7 +741,7 @@  BOOLEAN PcieIsLinkUp (UINT32 SocType, UINTN RbPciBar, UINTN Port)
 {
     UINT32                     Value = 0;
 
-    if (0x1610 == SocType)
+    if ((0x1610 == SocType) || (0x1616 == SocType))
     {
         Value = MmioRead32(RbPciBar + 0x131C);
         if ((Value & 0x3F) == 0x11)