Re: [PATCH edk2-platforms v4 2/9] Silicon/Broadcom/BcmGenetDxe: add support for broadcast filtering

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 5/11/20 10:34 PM, Jeremy Linton wrote:
> Hi,
> 
> On 5/11/20 9:55 AM, Ard Biesheuvel wrote:
>> Add a helper to configure the first MDF filter for filtering the
>> broadcast Ethernet address.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h |  7 +++++
>>   Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c | 33 
>> ++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h 
>> b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h
>> index b21a284b6221..b491ea4665b0 100644
>> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h
>> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.h
>> @@ -107,6 +107,7 @@
>>   #define GENET_UMAC_MDF_CTRL                     0xe50
>>   #define GENET_UMAC_MDF_ADDR0(n)                 (0xe54 + (n) * 0x8)
>>   #define GENET_UMAC_MDF_ADDR1(n)                 (0xe58 + (n) * 0x8)
>> +#define GENET_MAX_MDF_FILTER                    17
>>   #define GENET_DMA_DESC_COUNT                    256
>>   #define GENET_DMA_DESC_SIZE                     12
>> @@ -300,6 +301,12 @@ GenetSetPromisc (
>>     IN BOOLEAN            Enable
>>     );
>> +VOID
>> +GenetEnableBroadcastFilter (
>> +  IN GENET_PRIVATE_DATA   *Genet,
>> +  IN BOOLEAN              Enable
>> +  );
>> +
>>   VOID
>>   GenetDmaInitRings (
>>     IN GENET_PRIVATE_DATA *Genet
>> diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c 
>> b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c
>> index 7ae9acec4c78..71c659e7f882 100644
>> --- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c
>> +++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/GenetUtil.c
>> @@ -492,6 +492,39 @@ GenetSetPromisc (
>>     GenetMmioWrite (Genet, GENET_UMAC_CMD, Value);
>>   }
>> +/**
>> +  Enable the MAC filter for the Ethernet broadcast address
>> +
>> +  @param  Genet[in]   Pointer to GENET_PRIVATE_DATA.
>> +  @param  Enable[in]  Promiscuous mode state.
>> +
>> +**/
>> +VOID
>> +GenetEnableBroadcastFilter (
>> +  IN GENET_PRIVATE_DATA   *Genet,
>> +  IN BOOLEAN              Enable
>> +  )
>> +{
>> +  CONST EFI_MAC_ADDRESS   *MacAddr = &Genet->SnpMode.CurrentAddress;
>> +  UINT32                  Value;
>> +
>> +  if (Enable) {
>> +    GenetMmioWrite (Genet, GENET_UMAC_MDF_ADDR0 (0),
>> +      MacAddr->Addr[1] | MacAddr->Addr[0] << 8);
>> +    GenetMmioWrite (Genet, GENET_UMAC_MDF_ADDR1 (0),
>> +      MacAddr->Addr[5] | MacAddr->Addr[4] << 8 |
>> +      MacAddr->Addr[3] << 16 | MacAddr->Addr[2] << 24);
>> +
>> +    GenetMmioWrite (Genet, GENET_UMAC_MDF_ADDR0 (1), 0xffff);
>> +    GenetMmioWrite (Genet, GENET_UMAC_MDF_ADDR1 (1), 0xffffffff);
>> +
>> +    Value = (1U << GENET_MAX_MDF_FILTER) & ~(BIT1 | BIT0);
> Is this right? Looking at the linux code, it looks like the a filter is 
> enabled in a big endian field starting at bit 16 and working its way down.
> 
> AFAIK, you want bit 16, and bit 15 set for two filters?
> 

Yeah, you're right. I was reading both the BSD code and Linux code 
wrong. That solves the mystery why value = 0x0 works as expected as well.

So I will change this to

Value = BIT16 | BIT15;

instead.

Thanks,