Re: [edk2-devel] [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/27/19 3:59 PM, Pete Batard wrote:
> On 2019.11.27 14:47, Philippe Mathieu-Daudé wrote:
>> On 11/27/19 1:37 PM, Pete Batard wrote:
>>> Add missing RNG registers,
>>
>> This is one change.
>>
>>> prefer reusing shorter define's
>>> instead of PCDs and clean up spacing.
>>
>> This is another change.
> 
> All these changes belong to a "cleanup" category, which is what, 
> globally, this patch is all about. Of course, I expect people to argue 
> that adding trivial missing data does not count as cleaning up, which 
> I'd disagree with, but then again, it's not my repo.
> 
>> Why suddenly go back to fixed PERIPHERAL block base address?
> 
> Earlier version was:
> 
> #define BCM2836_WDOG_BASE_ADDRESS    0x3f100000
> 
> which we replaced with:
> 
> #define BCM2836_WDOG_BASE_ADDRESS   (FixedPcdGet64 
> (PcdBcm283xRegistersAddress) + BCM2836_WDOG_OFFSET)
> 
> But considering that we have "BCM2836_SOC_REGISTERS" that now equates 
>                              "(FixedPcdGet64 
> (PcdBcm283xRegistersAddress))" we can write the exact same thing as 
> above, only shorter, which we do.

I apologize I misread the code, you are right it is cleaner to use 
BCM2836_SOC_REGISTERS. Thanks for taking time to clarify.

> I personally find this more understandable for newcomers who'll be 
> looking at the code and easier to maintain. But I'm not the maintainer, 
> so I'll go with whatever suits you.

Neither am I, however I'll appreciate if you can keep patches simple, 
trying to do one atomic change per patch. So my suggestion to split this 
patch in 2 stands.

Anyhow Ard asked for a clone for the RNG part, so this patch need rework.

>>>
>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>> ---
>>>   Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h | 23 
>>> ++++++++++++--------
>>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git 
>>> a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h 
>>> b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
>>> index 72c8e9dc4b14..744c7ac3b9f4 100644
>>> --- a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
>>> +++ b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
>>> @@ -24,8 +24,7 @@
>>>   /* watchdog constants */
>>>   #define BCM2836_WDOG_OFFSET                                 0x00100000
>>> -#define BCM2836_WDOG_BASE_ADDRESS (FixedPcdGet64 
>>> (PcdBcm283xRegistersAddress) \
>>> -                                                            + 
>>> BCM2836_WDOG_OFFSET)
>>> +#define BCM2836_WDOG_BASE_ADDRESS (BCM2836_SOC_REGISTERS + 
>>> BCM2836_WDOG_OFFSET)
>>>   #define BCM2836_WDOG_PASSWORD                               0x5a000000
>>>   #define BCM2836_WDOG_RSTC_OFFSET                            0x0000001c
>>>   #define BCM2836_WDOG_WDOG_OFFSET                            0x00000024
>>> @@ -34,8 +33,7 @@
>>>   /* mailbox interface constants */
>>>   #define BCM2836_MBOX_OFFSET                                 0x0000b880
>>> -#define BCM2836_MBOX_BASE_ADDRESS (FixedPcdGet64 
>>> (PcdBcm283xRegistersAddress) \
>>> -                                                            + 
>>> BCM2836_MBOX_OFFSET)
>>> +#define BCM2836_MBOX_BASE_ADDRESS (BCM2836_SOC_REGISTERS + 
>>> BCM2836_MBOX_OFFSET)
>>>   #define BCM2836_MBOX_READ_OFFSET                            0x00000000
>>>   #define BCM2836_MBOX_STATUS_OFFSET                          0x00000018
>>>   #define BCM2836_MBOX_CONFIG_OFFSET                          0x0000001c
>>> @@ -51,12 +49,19 @@
>>>   #define BCM2836_INTC_TIMER_PENDING_OFFSET                   0x00000060
>>>   /* random number generator */
>>> -#define RNG_BASE_ADDRESS   (BCM2836_SOC_REGISTERS + 0x00104000)
>>> +#define BCM2836_RNG_OFFSET                                  0x00104000
>>> +#define RNG_BASE_ADDRESS (BCM2836_SOC_REGISTERS + BCM2836_RNG_OFFSET)
>>> -#define RNG_CTRL           (RNG_BASE_ADDRESS + 0x0)
>>> -#define RNG_STATUS         (RNG_BASE_ADDRESS + 0x4)
>>> -#define RNG_DATA           (RNG_BASE_ADDRESS + 0x8)
>>> +#define RNG_CTRL (RNG_BASE_ADDRESS + 0x0)
>>> +#define RNG_STATUS (RNG_BASE_ADDRESS + 0x4)
>>> +#define RNG_DATA (RNG_BASE_ADDRESS + 0x8)
>>> +#define RNG_BIT_COUNT (RNG_BASE_ADDRESS + 0xc)
>>> +#define RNG_BIT_COUNT_THRESHOLD (RNG_BASE_ADDRESS + 0x10)
>>> +#define RNG_INT_STATUS (RNG_BASE_ADDRESS + 0x18)
>>> +#define RNG_INT_ENABLE (RNG_BASE_ADDRESS + 0x1c)
>>> +#define RNG_FIFO_DATA (RNG_BASE_ADDRESS + 0x20)
>>> +#define RNG_FIFO_COUNT (RNG_BASE_ADDRESS + 0x24)
>>> -#define RNG_CTRL_ENABLE    0x1
>>> +#define RNG_CTRL_ENABLE                                     0x1
>>>   #endif /*__BCM2836_H__ */
>>>
>>
>>
>> 
>>
>