* [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 12:37 [edk2-platforms][PATCH 0/5] Further RPi4 support groundwork Pete Batard
@ 2019-11-27 12:37 ` Pete Batard
2019-11-27 12:48 ` Ard Biesheuvel
2019-11-27 14:47 ` Philippe Mathieu-Daudé
2019-11-27 12:37 ` [edk2-platforms][PATCH 2/5] Silicon/Bcm283x: Add FIFO mode for RNG Pete Batard
` (3 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:37 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif.lindholm, philmd, samer.el-haj-mahmoud,
andrey.warkentin
Add missing RNG registers, prefer reusing shorter define's
instead of PCDs and clean up spacing.
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__ */
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 12:37 ` [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header Pete Batard
@ 2019-11-27 12:48 ` Ard Biesheuvel
2019-11-27 12:56 ` Pete Batard
2019-11-27 14:47 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2019-11-27 12:48 UTC (permalink / raw)
To: Pete Batard
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>
> Add missing RNG registers, prefer reusing shorter define's
> instead of PCDs and clean up spacing.
>
Is there a source for these register definitions? It seems the Linux
driver deviates from the below (and the warmup count thing uses the
status register as well), so it would be helpful to quote the
authoritative reference here.
> 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__ */
> --
> 2.21.0.windows.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 12:48 ` Ard Biesheuvel
@ 2019-11-27 12:56 ` Pete Batard
2019-11-27 13:00 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:56 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On 2019.11.27 12:48, Ard Biesheuvel wrote:
> On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>>
>> Add missing RNG registers, prefer reusing shorter define's
>> instead of PCDs and clean up spacing.
>>
>
> Is there a source for these register definitions?
I used the most recent Linux driver I could find (but I guess I need to
point out that it was used for reference with regards to the registers.
Especially, no code was copied from that source).
> It seems the Linux
> driver deviates from the below (and the warmup count thing uses the
> status register as well), so it would be helpful to quote the
> authoritative reference here.
https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/iproc-rng200.c#L223-L224
seems to indicate that RNG_WARMUP_COUNT (0x40000) should go into
RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET (them) / RNG_BIT_COUNT_THRESHOLD
(us) and not the status register.
Now, of course, since we don't have a public datasheet, it's hard to
have absolute certainty on what's the proper register to write, but I
guess the most recent code with a Broadcom Corporation copyright has to
our next best thing when it comes to authoritative answer...
Regards,
/Pete
>
>
>> 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__ */
>> --
>> 2.21.0.windows.1
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 12:56 ` Pete Batard
@ 2019-11-27 13:00 ` Ard Biesheuvel
2019-11-27 13:09 ` Pete Batard
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2019-11-27 13:00 UTC (permalink / raw)
To: Pete Batard
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On Wed, 27 Nov 2019 at 13:56, Pete Batard <pete@akeo.ie> wrote:
>
> On 2019.11.27 12:48, Ard Biesheuvel wrote:
> > On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
> >>
> >> Add missing RNG registers, prefer reusing shorter define's
> >> instead of PCDs and clean up spacing.
> >>
> >
> > Is there a source for these register definitions?
>
> I used the most recent Linux driver I could find (but I guess I need to
> point out that it was used for reference with regards to the registers.
> Especially, no code was copied from that source).
>
> > It seems the Linux
> > driver deviates from the below (and the warmup count thing uses the
> > status register as well), so it would be helpful to quote the
> > authoritative reference here.
>
> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/iproc-rng200.c#L223-L224
> seems to indicate that RNG_WARMUP_COUNT (0x40000) should go into
> RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET (them) / RNG_BIT_COUNT_THRESHOLD
> (us) and not the status register.
>
> Now, of course, since we don't have a public datasheet, it's hard to
> have absolute certainty on what's the proper register to write, but I
> guess the most recent code with a Broadcom Corporation copyright has to
> our next best thing when it comes to authoritative answer...
>
Well, there is this one too
https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/bcm2835-rng.c
which is what I used as the basis for the existing version.
> >
> >
> >> 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__ */
> >> --
> >> 2.21.0.windows.1
> >>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 13:00 ` Ard Biesheuvel
@ 2019-11-27 13:09 ` Pete Batard
2019-11-27 13:17 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Pete Batard @ 2019-11-27 13:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On 2019.11.27 13:00, Ard Biesheuvel wrote:
> On Wed, 27 Nov 2019 at 13:56, Pete Batard <pete@akeo.ie> wrote:
>>
>> On 2019.11.27 12:48, Ard Biesheuvel wrote:
>>> On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>>>>
>>>> Add missing RNG registers, prefer reusing shorter define's
>>>> instead of PCDs and clean up spacing.
>>>>
>>>
>>> Is there a source for these register definitions?
>>
>> I used the most recent Linux driver I could find (but I guess I need to
>> point out that it was used for reference with regards to the registers.
>> Especially, no code was copied from that source).
>>
>>> It seems the Linux
>>> driver deviates from the below (and the warmup count thing uses the
>>> status register as well), so it would be helpful to quote the
>>> authoritative reference here.
>>
>> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/iproc-rng200.c#L223-L224
>> seems to indicate that RNG_WARMUP_COUNT (0x40000) should go into
>> RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET (them) / RNG_BIT_COUNT_THRESHOLD
>> (us) and not the status register.
>>
>> Now, of course, since we don't have a public datasheet, it's hard to
>> have absolute certainty on what's the proper register to write, but I
>> guess the most recent code with a Broadcom Corporation copyright has to
>> our next best thing when it comes to authoritative answer...
>>
>
> Well, there is this one too
>
> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/bcm2835-rng.c
>
> which is what I used as the basis for the existing version.
Okay. Looks like 2835 (which has to be the basis for the Bcm2837 used on
the Pi 3) and 2838 (basis for Bcm2711) have at least one difference with
regards to where the warmup count should go.
I guess an easy fix, if we don't want to have to spin 2 separate
drivers, is to write to both RNG_STATUS and RNG_BIT_COUNT_THRESHOLD
during init.
I have tested on Pi 3 and not seen any ill effect to writing to
RNG_BIT_COUNT_THRESHOLD.
Would something like this work for you in a v2?
Alternatively, I could change the new PcdBcm283xRngUseFifo PCD to
PcdBcm283xRng2838Compatible and use that for the conditional code.
Regards,
/Pete
>
>
>
>
>
>>>
>>>
>>>> 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__ */
>>>> --
>>>> 2.21.0.windows.1
>>>>
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 13:09 ` Pete Batard
@ 2019-11-27 13:17 ` Ard Biesheuvel
2019-11-27 13:21 ` Pete Batard
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2019-11-27 13:17 UTC (permalink / raw)
To: Pete Batard
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On Wed, 27 Nov 2019 at 14:09, Pete Batard <pete@akeo.ie> wrote:
>
> On 2019.11.27 13:00, Ard Biesheuvel wrote:
> > On Wed, 27 Nov 2019 at 13:56, Pete Batard <pete@akeo.ie> wrote:
> >>
> >> On 2019.11.27 12:48, Ard Biesheuvel wrote:
> >>> On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
> >>>>
> >>>> Add missing RNG registers, prefer reusing shorter define's
> >>>> instead of PCDs and clean up spacing.
> >>>>
> >>>
> >>> Is there a source for these register definitions?
> >>
> >> I used the most recent Linux driver I could find (but I guess I need to
> >> point out that it was used for reference with regards to the registers.
> >> Especially, no code was copied from that source).
> >>
> >>> It seems the Linux
> >>> driver deviates from the below (and the warmup count thing uses the
> >>> status register as well), so it would be helpful to quote the
> >>> authoritative reference here.
> >>
> >> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/iproc-rng200.c#L223-L224
> >> seems to indicate that RNG_WARMUP_COUNT (0x40000) should go into
> >> RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET (them) / RNG_BIT_COUNT_THRESHOLD
> >> (us) and not the status register.
> >>
> >> Now, of course, since we don't have a public datasheet, it's hard to
> >> have absolute certainty on what's the proper register to write, but I
> >> guess the most recent code with a Broadcom Corporation copyright has to
> >> our next best thing when it comes to authoritative answer...
> >>
> >
> > Well, there is this one too
> >
> > https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/bcm2835-rng.c
> >
> > which is what I used as the basis for the existing version.
>
> Okay. Looks like 2835 (which has to be the basis for the Bcm2837 used on
> the Pi 3) and 2838 (basis for Bcm2711) have at least one difference with
> regards to where the warmup count should go.
>
> I guess an easy fix, if we don't want to have to spin 2 separate
> drivers, is to write to both RNG_STATUS and RNG_BIT_COUNT_THRESHOLD
> during init.
>
> I have tested on Pi 3 and not seen any ill effect to writing to
> RNG_BIT_COUNT_THRESHOLD.
>
I don't like that at all tbh. A misbehaving RNG is virtually
undetectable (unless it starts returning zeroes), so knowingly
deviating from the initialization routines like that should really be
avoided.
> Would something like this work for you in a v2?
>
> Alternatively, I could change the new PcdBcm283xRngUseFifo PCD to
> PcdBcm283xRng2838Compatible and use that for the conditional code.
>
These are two different IP blocks, with different register layouts
etc, so I don't really see the point in parameterizing the existing
driver like that. I'd prefer it if we could just clone the existing
one and make the changes unconditionally.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 13:17 ` Ard Biesheuvel
@ 2019-11-27 13:21 ` Pete Batard
0 siblings, 0 replies; 21+ messages in thread
From: Pete Batard @ 2019-11-27 13:21 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On 2019.11.27 13:17, Ard Biesheuvel wrote:
> On Wed, 27 Nov 2019 at 14:09, Pete Batard <pete@akeo.ie> wrote:
>>
>> On 2019.11.27 13:00, Ard Biesheuvel wrote:
>>> On Wed, 27 Nov 2019 at 13:56, Pete Batard <pete@akeo.ie> wrote:
>>>>
>>>> On 2019.11.27 12:48, Ard Biesheuvel wrote:
>>>>> On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>>>>>>
>>>>>> Add missing RNG registers, prefer reusing shorter define's
>>>>>> instead of PCDs and clean up spacing.
>>>>>>
>>>>>
>>>>> Is there a source for these register definitions?
>>>>
>>>> I used the most recent Linux driver I could find (but I guess I need to
>>>> point out that it was used for reference with regards to the registers.
>>>> Especially, no code was copied from that source).
>>>>
>>>>> It seems the Linux
>>>>> driver deviates from the below (and the warmup count thing uses the
>>>>> status register as well), so it would be helpful to quote the
>>>>> authoritative reference here.
>>>>
>>>> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/iproc-rng200.c#L223-L224
>>>> seems to indicate that RNG_WARMUP_COUNT (0x40000) should go into
>>>> RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET (them) / RNG_BIT_COUNT_THRESHOLD
>>>> (us) and not the status register.
>>>>
>>>> Now, of course, since we don't have a public datasheet, it's hard to
>>>> have absolute certainty on what's the proper register to write, but I
>>>> guess the most recent code with a Broadcom Corporation copyright has to
>>>> our next best thing when it comes to authoritative answer...
>>>>
>>>
>>> Well, there is this one too
>>>
>>> https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/char/hw_random/bcm2835-rng.c
>>>
>>> which is what I used as the basis for the existing version.
>>
>> Okay. Looks like 2835 (which has to be the basis for the Bcm2837 used on
>> the Pi 3) and 2838 (basis for Bcm2711) have at least one difference with
>> regards to where the warmup count should go.
>>
>> I guess an easy fix, if we don't want to have to spin 2 separate
>> drivers, is to write to both RNG_STATUS and RNG_BIT_COUNT_THRESHOLD
>> during init.
>>
>> I have tested on Pi 3 and not seen any ill effect to writing to
>> RNG_BIT_COUNT_THRESHOLD.
>>
>
> I don't like that at all tbh. A misbehaving RNG is virtually
> undetectable (unless it starts returning zeroes), so knowingly
> deviating from the initialization routines like that should really be
> avoided.
>
>> Would something like this work for you in a v2?
>>
>> Alternatively, I could change the new PcdBcm283xRngUseFifo PCD to
>> PcdBcm283xRng2838Compatible and use that for the conditional code.
>>
>
> These are two different IP blocks, with different register layouts
> etc, so I don't really see the point in parameterizing the existing
> driver like that. I'd prefer it if we could just clone the existing
> one and make the changes unconditionally.
Okay. I was pretty much expecting that answer...
Regards,
/Pete
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 12:37 ` [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header Pete Batard
2019-11-27 12:48 ` Ard Biesheuvel
@ 2019-11-27 14:47 ` Philippe Mathieu-Daudé
2019-11-27 14:59 ` [edk2-devel] " Pete Batard
1 sibling, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-27 14:47 UTC (permalink / raw)
To: Pete Batard, devel
Cc: ard.biesheuvel, leif.lindholm, samer.el-haj-mahmoud,
andrey.warkentin
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.
Why suddenly go back to fixed PERIPHERAL block base address?
>
> 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__ */
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 14:47 ` Philippe Mathieu-Daudé
@ 2019-11-27 14:59 ` Pete Batard
2019-11-27 15:18 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Pete Batard @ 2019-11-27 14:59 UTC (permalink / raw)
To: devel, philmd
Cc: ard.biesheuvel, leif.lindholm, samer.el-haj-mahmoud,
andrey.warkentin
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 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.
Regards,
/Pete
>
>>
>> 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__ */
>>
>
>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-devel] [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header
2019-11-27 14:59 ` [edk2-devel] " Pete Batard
@ 2019-11-27 15:18 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-27 15:18 UTC (permalink / raw)
To: Pete Batard, devel
Cc: ard.biesheuvel, leif.lindholm, samer.el-haj-mahmoud,
andrey.warkentin
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__ */
>>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [edk2-platforms][PATCH 2/5] Silicon/Bcm283x: Add FIFO mode for RNG
2019-11-27 12:37 [edk2-platforms][PATCH 0/5] Further RPi4 support groundwork Pete Batard
2019-11-27 12:37 ` [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header Pete Batard
@ 2019-11-27 12:37 ` Pete Batard
2019-11-27 12:44 ` Ard Biesheuvel
2019-11-27 12:37 ` [edk2-platforms][PATCH 3/5] Platform/RPi/MmcDxe: Factorize SCR call and clean up MMC init Pete Batard
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:37 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif.lindholm, philmd, samer.el-haj-mahmoud,
andrey.warkentin
The Bcm283x Random Number Generator does not work in regular mode
on the Bcm2711 powered Raspberry Pi 4. It does however work when
using FIFO mode. So we add this new mode, which is governed by the
use of the new PcdBcm283xRngUseFifo boolean PCD.
Note that just as a Pi 4 doesn't seem to support regular mode, a
Pi 3 doesn't seem to support FIFO mode, which is why we can't
switch to simply using FIFO mode for both platforms.
We also fix the register to which RNG_WARMUP_COUNT is written,
which was incorrect.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Silicon/Broadcom/Bcm283x/Bcm283x.dec | 1 +
Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c | 96 +++++++++++++++-----
Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf | 2 +
3 files changed, 74 insertions(+), 25 deletions(-)
diff --git a/Silicon/Broadcom/Bcm283x/Bcm283x.dec b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
index 5b839b00d286..4a9be7b18c97 100644
--- a/Silicon/Broadcom/Bcm283x/Bcm283x.dec
+++ b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
@@ -21,3 +21,4 @@ [Guids]
[PcdsFixedAtBuild.common]
gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0x0|UINT32|0x00000001
+ gBcm283xTokenSpaceGuid.PcdBcm283xRngUseFifo|0x0|BOOLEAN|0x00000002
diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
index 722815d32f06..462a21a6f3c3 100644
--- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
+++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
@@ -2,6 +2,7 @@
This driver produces an EFI_RNG_PROTOCOL instance for the Broadcom 2836 RNG
+ Copyright (C) 2019, Pete Batard <pete@akeo.ie>
Copyright (C) 2019, Linaro Ltd. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -12,6 +13,8 @@
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/TimerLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <IndustryStandard/Bcm2836.h>
@@ -20,6 +23,8 @@
#define RNG_WARMUP_COUNT 0x40000
#define RNG_MAX_RETRIES 0x100 // arbitrary upper bound
+#define RNG_FIFO_NUMVAL_MASK 0xff
+#define RNG_STATUS_NUMVAL_SHIFT 24
/**
Returns information about the random number generation implementation.
@@ -84,6 +89,54 @@ Bcm2836RngGetInfo (
return EFI_SUCCESS;
}
+/**
+ Read a single random value, in either FIFO or regular mode.
+
+ @param[in] Val A pointer to the 32-bit word that is to
+ be filled with a random value.
+
+ @retval EFI_SUCCESS A random value was successfully read.
+ @retval EFI_NOT_READY The number of retries elapsed before a
+ random value was generated.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+Bcm2836RngReadValue (
+ IN OUT UINT32 *Val
+)
+{
+ UINT32 Avail;
+ UINT32 i;
+ BOOLEAN UseFifo = FixedPcdGetBool (PcdBcm283xRngUseFifo);
+
+ ASSERT (Val != NULL);
+
+ Avail = UseFifo ?
+ (MmioRead32 (RNG_FIFO_COUNT) & RNG_FIFO_NUMVAL_MASK) :
+ (MmioRead32 (RNG_STATUS) >> RNG_STATUS_NUMVAL_SHIFT);
+
+ //
+ // If we don't have a value ready, wait 1 us and retry.
+ //
+ for (i = 0; Avail < 1 && i < RNG_MAX_RETRIES; i++) {
+ MicroSecondDelay (1);
+ Avail = UseFifo ?
+ (MmioRead32 (RNG_FIFO_COUNT) & RNG_FIFO_NUMVAL_MASK) :
+ (MmioRead32 (RNG_STATUS) >> RNG_STATUS_NUMVAL_SHIFT);
+ }
+ if (Avail < 1) {
+ return EFI_NOT_READY;
+ }
+
+ *Val = UseFifo ?
+ MmioRead32 (RNG_FIFO_DATA):
+ MmioRead32 (RNG_DATA);
+
+ return EFI_SUCCESS;
+}
+
/**
Produces and returns an RNG value using either the default or specified RNG
algorithm.
@@ -123,9 +176,8 @@ Bcm2836RngGetRNG (
OUT UINT8 *RNGValue
)
{
- UINT32 Val;
- UINT32 Num;
- UINT32 Retries;
+ EFI_STATUS Status;
+ UINT32 Val;
if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
return EFI_INVALID_PARAMETER;
@@ -139,30 +191,24 @@ Bcm2836RngGetRNG (
return EFI_UNSUPPORTED;
}
- while (RNGValueLength > 0) {
- Retries = RNG_MAX_RETRIES;
- do {
- Num = MmioRead32 (RNG_STATUS) >> 24;
- MemoryFence ();
- } while (!Num && Retries-- > 0);
-
- if (!Num) {
- return EFI_DEVICE_ERROR;
+ while (RNGValueLength >= sizeof (UINT32)) {
+ Status = Bcm2836RngReadValue (&Val);
+ if (EFI_ERROR (Status)) {
+ return Status;
}
+ WriteUnaligned32 ((VOID *)RNGValue, Val);
+ RNGValue += sizeof (UINT32);
+ RNGValueLength -= sizeof (UINT32);
+ }
- while (RNGValueLength >= sizeof (UINT32) && Num > 0) {
- WriteUnaligned32 ((VOID *)RNGValue, MmioRead32 (RNG_DATA));
- RNGValue += sizeof (UINT32);
- RNGValueLength -= sizeof (UINT32);
- Num--;
+ if (RNGValueLength > 0) {
+ Status = Bcm2836RngReadValue (&Val);
+ if (EFI_ERROR (Status)) {
+ return Status;
}
-
- if (RNGValueLength > 0 && Num > 0) {
- Val = MmioRead32 (RNG_DATA);
- while (RNGValueLength--) {
- *RNGValue++ = (UINT8)Val;
- Val >>= 8;
- }
+ while (RNGValueLength--) {
+ *RNGValue++ = (UINT8)Val;
+ Val >>= 8;
}
}
return EFI_SUCCESS;
@@ -190,7 +236,7 @@ Bcm2836RngEntryPoint (
NULL);
ASSERT_EFI_ERROR (Status);
- MmioWrite32 (RNG_STATUS, RNG_WARMUP_COUNT);
+ MmioWrite32 (RNG_BIT_COUNT_THRESHOLD, RNG_WARMUP_COUNT);
MmioWrite32 (RNG_CTRL, RNG_CTRL_ENABLE);
return EFI_SUCCESS;
diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
index 8eb90de85cfd..31415231ae0b 100644
--- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
+++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
@@ -28,6 +28,7 @@ [LibraryClasses]
DebugLib
IoLib
PcdLib
+ TimerLib
UefiBootServicesTableLib
UefiDriverEntryPoint
@@ -39,6 +40,7 @@ [Guids]
[FixedPcd]
gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
+ gBcm283xTokenSpaceGuid.PcdBcm283xRngUseFifo
[Depex]
TRUE
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 2/5] Silicon/Bcm283x: Add FIFO mode for RNG
2019-11-27 12:37 ` [edk2-platforms][PATCH 2/5] Silicon/Bcm283x: Add FIFO mode for RNG Pete Batard
@ 2019-11-27 12:44 ` Ard Biesheuvel
2019-11-27 12:47 ` Pete Batard
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2019-11-27 12:44 UTC (permalink / raw)
To: Pete Batard
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>
> The Bcm283x Random Number Generator does not work in regular mode
> on the Bcm2711 powered Raspberry Pi 4. It does however work when
> using FIFO mode. So we add this new mode, which is governed by the
> use of the new PcdBcm283xRngUseFifo boolean PCD.
>
> Note that just as a Pi 4 doesn't seem to support regular mode, a
> Pi 3 doesn't seem to support FIFO mode, which is why we can't
> switch to simply using FIFO mode for both platforms.
>
> We also fix the register to which RNG_WARMUP_COUNT is written,
> which was incorrect.
>
At the risk of triggering another endless debate, could we /please/
not mix in trivial bugfixes with more complicated refactoring like
this?
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
> Silicon/Broadcom/Bcm283x/Bcm283x.dec | 1 +
> Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c | 96 +++++++++++++++-----
> Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf | 2 +
> 3 files changed, 74 insertions(+), 25 deletions(-)
>
> diff --git a/Silicon/Broadcom/Bcm283x/Bcm283x.dec b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> index 5b839b00d286..4a9be7b18c97 100644
> --- a/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> +++ b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> @@ -21,3 +21,4 @@ [Guids]
>
> [PcdsFixedAtBuild.common]
> gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0x0|UINT32|0x00000001
> + gBcm283xTokenSpaceGuid.PcdBcm283xRngUseFifo|0x0|BOOLEAN|0x00000002
> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
> index 722815d32f06..462a21a6f3c3 100644
> --- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
> +++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
> @@ -2,6 +2,7 @@
>
> This driver produces an EFI_RNG_PROTOCOL instance for the Broadcom 2836 RNG
>
> + Copyright (C) 2019, Pete Batard <pete@akeo.ie>
> Copyright (C) 2019, Linaro Ltd. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -12,6 +13,8 @@
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/TimerLib.h>
> #include <Library/UefiBootServicesTableLib.h>
>
> #include <IndustryStandard/Bcm2836.h>
> @@ -20,6 +23,8 @@
>
> #define RNG_WARMUP_COUNT 0x40000
> #define RNG_MAX_RETRIES 0x100 // arbitrary upper bound
> +#define RNG_FIFO_NUMVAL_MASK 0xff
> +#define RNG_STATUS_NUMVAL_SHIFT 24
>
> /**
> Returns information about the random number generation implementation.
> @@ -84,6 +89,54 @@ Bcm2836RngGetInfo (
> return EFI_SUCCESS;
> }
>
> +/**
> + Read a single random value, in either FIFO or regular mode.
> +
> + @param[in] Val A pointer to the 32-bit word that is to
> + be filled with a random value.
> +
> + @retval EFI_SUCCESS A random value was successfully read.
> + @retval EFI_NOT_READY The number of retries elapsed before a
> + random value was generated.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Bcm2836RngReadValue (
> + IN OUT UINT32 *Val
> +)
> +{
> + UINT32 Avail;
> + UINT32 i;
> + BOOLEAN UseFifo = FixedPcdGetBool (PcdBcm283xRngUseFifo);
> +
> + ASSERT (Val != NULL);
> +
> + Avail = UseFifo ?
> + (MmioRead32 (RNG_FIFO_COUNT) & RNG_FIFO_NUMVAL_MASK) :
> + (MmioRead32 (RNG_STATUS) >> RNG_STATUS_NUMVAL_SHIFT);
> +
> + //
> + // If we don't have a value ready, wait 1 us and retry.
> + //
> + for (i = 0; Avail < 1 && i < RNG_MAX_RETRIES; i++) {
> + MicroSecondDelay (1);
> + Avail = UseFifo ?
> + (MmioRead32 (RNG_FIFO_COUNT) & RNG_FIFO_NUMVAL_MASK) :
> + (MmioRead32 (RNG_STATUS) >> RNG_STATUS_NUMVAL_SHIFT);
> + }
> + if (Avail < 1) {
> + return EFI_NOT_READY;
> + }
> +
> + *Val = UseFifo ?
> + MmioRead32 (RNG_FIFO_DATA):
> + MmioRead32 (RNG_DATA);
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Produces and returns an RNG value using either the default or specified RNG
> algorithm.
> @@ -123,9 +176,8 @@ Bcm2836RngGetRNG (
> OUT UINT8 *RNGValue
> )
> {
> - UINT32 Val;
> - UINT32 Num;
> - UINT32 Retries;
> + EFI_STATUS Status;
> + UINT32 Val;
>
> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
> return EFI_INVALID_PARAMETER;
> @@ -139,30 +191,24 @@ Bcm2836RngGetRNG (
> return EFI_UNSUPPORTED;
> }
>
> - while (RNGValueLength > 0) {
> - Retries = RNG_MAX_RETRIES;
> - do {
> - Num = MmioRead32 (RNG_STATUS) >> 24;
> - MemoryFence ();
> - } while (!Num && Retries-- > 0);
> -
> - if (!Num) {
> - return EFI_DEVICE_ERROR;
> + while (RNGValueLength >= sizeof (UINT32)) {
> + Status = Bcm2836RngReadValue (&Val);
> + if (EFI_ERROR (Status)) {
> + return Status;
> }
> + WriteUnaligned32 ((VOID *)RNGValue, Val);
> + RNGValue += sizeof (UINT32);
> + RNGValueLength -= sizeof (UINT32);
> + }
>
> - while (RNGValueLength >= sizeof (UINT32) && Num > 0) {
> - WriteUnaligned32 ((VOID *)RNGValue, MmioRead32 (RNG_DATA));
> - RNGValue += sizeof (UINT32);
> - RNGValueLength -= sizeof (UINT32);
> - Num--;
> + if (RNGValueLength > 0) {
> + Status = Bcm2836RngReadValue (&Val);
> + if (EFI_ERROR (Status)) {
> + return Status;
> }
> -
> - if (RNGValueLength > 0 && Num > 0) {
> - Val = MmioRead32 (RNG_DATA);
> - while (RNGValueLength--) {
> - *RNGValue++ = (UINT8)Val;
> - Val >>= 8;
> - }
> + while (RNGValueLength--) {
> + *RNGValue++ = (UINT8)Val;
> + Val >>= 8;
> }
> }
> return EFI_SUCCESS;
> @@ -190,7 +236,7 @@ Bcm2836RngEntryPoint (
> NULL);
> ASSERT_EFI_ERROR (Status);
>
> - MmioWrite32 (RNG_STATUS, RNG_WARMUP_COUNT);
> + MmioWrite32 (RNG_BIT_COUNT_THRESHOLD, RNG_WARMUP_COUNT);
> MmioWrite32 (RNG_CTRL, RNG_CTRL_ENABLE);
>
> return EFI_SUCCESS;
> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
> index 8eb90de85cfd..31415231ae0b 100644
> --- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
> +++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
> @@ -28,6 +28,7 @@ [LibraryClasses]
> DebugLib
> IoLib
> PcdLib
> + TimerLib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
>
> @@ -39,6 +40,7 @@ [Guids]
>
> [FixedPcd]
> gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> + gBcm283xTokenSpaceGuid.PcdBcm283xRngUseFifo
>
> [Depex]
> TRUE
> --
> 2.21.0.windows.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 2/5] Silicon/Bcm283x: Add FIFO mode for RNG
2019-11-27 12:44 ` Ard Biesheuvel
@ 2019-11-27 12:47 ` Pete Batard
0 siblings, 0 replies; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:47 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-groups-io, Leif Lindholm, Philippe Mathieu-Daudé,
samer.el-haj-mahmoud, Andrei E. Warkentin
On 2019.11.27 12:44, Ard Biesheuvel wrote:
> On Wed, 27 Nov 2019 at 13:37, Pete Batard <pete@akeo.ie> wrote:
>>
>> The Bcm283x Random Number Generator does not work in regular mode
>> on the Bcm2711 powered Raspberry Pi 4. It does however work when
>> using FIFO mode. So we add this new mode, which is governed by the
>> use of the new PcdBcm283xRngUseFifo boolean PCD.
>>
>> Note that just as a Pi 4 doesn't seem to support regular mode, a
>> Pi 3 doesn't seem to support FIFO mode, which is why we can't
>> switch to simply using FIFO mode for both platforms.
>>
>> We also fix the register to which RNG_WARMUP_COUNT is written,
>> which was incorrect.
>>
>
> At the risk of triggering another endless debate, could we /please/
> not mix in trivial bugfixes with more complicated refactoring like
> this?
Well, the "trivial" is the precise reason it is mixed in, but sure, I
can split that in v2.
Regards,
/Pete
>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>> Silicon/Broadcom/Bcm283x/Bcm283x.dec | 1 +
>> Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c | 96 +++++++++++++++-----
>> Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf | 2 +
>> 3 files changed, 74 insertions(+), 25 deletions(-)
>>
>> diff --git a/Silicon/Broadcom/Bcm283x/Bcm283x.dec b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
>> index 5b839b00d286..4a9be7b18c97 100644
>> --- a/Silicon/Broadcom/Bcm283x/Bcm283x.dec
>> +++ b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
>> @@ -21,3 +21,4 @@ [Guids]
>>
>> [PcdsFixedAtBuild.common]
>> gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0x0|UINT32|0x00000001
>> + gBcm283xTokenSpaceGuid.PcdBcm283xRngUseFifo|0x0|BOOLEAN|0x00000002
>> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
>> index 722815d32f06..462a21a6f3c3 100644
>> --- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
>> +++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.c
>> @@ -2,6 +2,7 @@
>>
>> This driver produces an EFI_RNG_PROTOCOL instance for the Broadcom 2836 RNG
>>
>> + Copyright (C) 2019, Pete Batard <pete@akeo.ie>
>> Copyright (C) 2019, Linaro Ltd. All rights reserved.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -12,6 +13,8 @@
>> #include <Library/BaseMemoryLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/IoLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/TimerLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>>
>> #include <IndustryStandard/Bcm2836.h>
>> @@ -20,6 +23,8 @@
>>
>> #define RNG_WARMUP_COUNT 0x40000
>> #define RNG_MAX_RETRIES 0x100 // arbitrary upper bound
>> +#define RNG_FIFO_NUMVAL_MASK 0xff
>> +#define RNG_STATUS_NUMVAL_SHIFT 24
>>
>> /**
>> Returns information about the random number generation implementation.
>> @@ -84,6 +89,54 @@ Bcm2836RngGetInfo (
>> return EFI_SUCCESS;
>> }
>>
>> +/**
>> + Read a single random value, in either FIFO or regular mode.
>> +
>> + @param[in] Val A pointer to the 32-bit word that is to
>> + be filled with a random value.
>> +
>> + @retval EFI_SUCCESS A random value was successfully read.
>> + @retval EFI_NOT_READY The number of retries elapsed before a
>> + random value was generated.
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
>> +Bcm2836RngReadValue (
>> + IN OUT UINT32 *Val
>> +)
>> +{
>> + UINT32 Avail;
>> + UINT32 i;
>> + BOOLEAN UseFifo = FixedPcdGetBool (PcdBcm283xRngUseFifo);
>> +
>> + ASSERT (Val != NULL);
>> +
>> + Avail = UseFifo ?
>> + (MmioRead32 (RNG_FIFO_COUNT) & RNG_FIFO_NUMVAL_MASK) :
>> + (MmioRead32 (RNG_STATUS) >> RNG_STATUS_NUMVAL_SHIFT);
>> +
>> + //
>> + // If we don't have a value ready, wait 1 us and retry.
>> + //
>> + for (i = 0; Avail < 1 && i < RNG_MAX_RETRIES; i++) {
>> + MicroSecondDelay (1);
>> + Avail = UseFifo ?
>> + (MmioRead32 (RNG_FIFO_COUNT) & RNG_FIFO_NUMVAL_MASK) :
>> + (MmioRead32 (RNG_STATUS) >> RNG_STATUS_NUMVAL_SHIFT);
>> + }
>> + if (Avail < 1) {
>> + return EFI_NOT_READY;
>> + }
>> +
>> + *Val = UseFifo ?
>> + MmioRead32 (RNG_FIFO_DATA):
>> + MmioRead32 (RNG_DATA);
>> +
>> + return EFI_SUCCESS;
>> +}
>> +
>> /**
>> Produces and returns an RNG value using either the default or specified RNG
>> algorithm.
>> @@ -123,9 +176,8 @@ Bcm2836RngGetRNG (
>> OUT UINT8 *RNGValue
>> )
>> {
>> - UINT32 Val;
>> - UINT32 Num;
>> - UINT32 Retries;
>> + EFI_STATUS Status;
>> + UINT32 Val;
>>
>> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
>> return EFI_INVALID_PARAMETER;
>> @@ -139,30 +191,24 @@ Bcm2836RngGetRNG (
>> return EFI_UNSUPPORTED;
>> }
>>
>> - while (RNGValueLength > 0) {
>> - Retries = RNG_MAX_RETRIES;
>> - do {
>> - Num = MmioRead32 (RNG_STATUS) >> 24;
>> - MemoryFence ();
>> - } while (!Num && Retries-- > 0);
>> -
>> - if (!Num) {
>> - return EFI_DEVICE_ERROR;
>> + while (RNGValueLength >= sizeof (UINT32)) {
>> + Status = Bcm2836RngReadValue (&Val);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> }
>> + WriteUnaligned32 ((VOID *)RNGValue, Val);
>> + RNGValue += sizeof (UINT32);
>> + RNGValueLength -= sizeof (UINT32);
>> + }
>>
>> - while (RNGValueLength >= sizeof (UINT32) && Num > 0) {
>> - WriteUnaligned32 ((VOID *)RNGValue, MmioRead32 (RNG_DATA));
>> - RNGValue += sizeof (UINT32);
>> - RNGValueLength -= sizeof (UINT32);
>> - Num--;
>> + if (RNGValueLength > 0) {
>> + Status = Bcm2836RngReadValue (&Val);
>> + if (EFI_ERROR (Status)) {
>> + return Status;
>> }
>> -
>> - if (RNGValueLength > 0 && Num > 0) {
>> - Val = MmioRead32 (RNG_DATA);
>> - while (RNGValueLength--) {
>> - *RNGValue++ = (UINT8)Val;
>> - Val >>= 8;
>> - }
>> + while (RNGValueLength--) {
>> + *RNGValue++ = (UINT8)Val;
>> + Val >>= 8;
>> }
>> }
>> return EFI_SUCCESS;
>> @@ -190,7 +236,7 @@ Bcm2836RngEntryPoint (
>> NULL);
>> ASSERT_EFI_ERROR (Status);
>>
>> - MmioWrite32 (RNG_STATUS, RNG_WARMUP_COUNT);
>> + MmioWrite32 (RNG_BIT_COUNT_THRESHOLD, RNG_WARMUP_COUNT);
>> MmioWrite32 (RNG_CTRL, RNG_CTRL_ENABLE);
>>
>> return EFI_SUCCESS;
>> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
>> index 8eb90de85cfd..31415231ae0b 100644
>> --- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
>> +++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
>> @@ -28,6 +28,7 @@ [LibraryClasses]
>> DebugLib
>> IoLib
>> PcdLib
>> + TimerLib
>> UefiBootServicesTableLib
>> UefiDriverEntryPoint
>>
>> @@ -39,6 +40,7 @@ [Guids]
>>
>> [FixedPcd]
>> gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
>> + gBcm283xTokenSpaceGuid.PcdBcm283xRngUseFifo
>>
>> [Depex]
>> TRUE
>> --
>> 2.21.0.windows.1
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [edk2-platforms][PATCH 3/5] Platform/RPi/MmcDxe: Factorize SCR call and clean up MMC init
2019-11-27 12:37 [edk2-platforms][PATCH 0/5] Further RPi4 support groundwork Pete Batard
2019-11-27 12:37 ` [edk2-platforms][PATCH 1/5] Silicon/Bcm283x: Clean up Bcm2836.h header Pete Batard
2019-11-27 12:37 ` [edk2-platforms][PATCH 2/5] Silicon/Bcm283x: Add FIFO mode for RNG Pete Batard
@ 2019-11-27 12:37 ` Pete Batard
2019-11-27 12:37 ` [edk2-platforms][PATCH 4/5] Platform/RPi/MmcDxe: Improve MMC driver stability Pete Batard
2019-11-27 12:37 ` [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model Pete Batard
4 siblings, 0 replies; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:37 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif.lindholm, philmd, samer.el-haj-mahmoud,
andrey.warkentin
From: Andrei Warkentin <andrey.warkentin@gmail.com>
This is mostly a maintainability improvement for the
InitializeSdMmcDevice () call achieved by factorizing the
code related to SCR execution into a new SdExecuteScr () call.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Drivers/MmcDxe/MmcIdentification.c | 105 ++++++++++++--------
1 file changed, 62 insertions(+), 43 deletions(-)
diff --git a/Platform/RaspberryPi/Drivers/MmcDxe/MmcIdentification.c b/Platform/RaspberryPi/Drivers/MmcDxe/MmcIdentification.c
index 4ee5c5ca6fb2..34a97e954220 100644
--- a/Platform/RaspberryPi/Drivers/MmcDxe/MmcIdentification.c
+++ b/Platform/RaspberryPi/Drivers/MmcDxe/MmcIdentification.c
@@ -1,5 +1,6 @@
/** @file
*
+ * Copyright (c) 2019, Andrei Warkentin <andrey.warkentin@gmail.com>
* Copyright (c) 2011-2015, ARM Limited. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -567,6 +568,48 @@ SdSetSpeed (
return EFI_SUCCESS;
}
+STATIC
+EFI_STATUS
+SdExecuteScr (
+ IN MMC_HOST_INSTANCE *MmcHostInstance,
+ OUT SCR *Scr
+ )
+{
+ EFI_STATUS Status;
+ UINT32 Response[4];
+ EFI_MMC_HOST_PROTOCOL *MmcHost = MmcHostInstance->MmcHost;
+
+ Status = MmcHost->SendCommand (MmcHost, MMC_CMD55,
+ MmcHostInstance->CardInfo.RCA << 16);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a (MMC_CMD55): Error and Status = %r\n", __FUNCTION__, Status));
+ return Status;
+ }
+ Status = MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a (MMC_CMD55): Error and Status = %r\n", __FUNCTION__, Status));
+ return Status;
+ }
+ if ((Response[0] & MMC_STATUS_APP_CMD) == 0) {
+ return EFI_SUCCESS;
+ }
+
+ /* SCR */
+ Status = MmcHost->SendCommand (MmcHost, MMC_ACMD51, 0);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a(MMC_ACMD51): Error and Status = %r\n", __func__, Status));
+ return Status;
+ }
+
+ Status = MmcHost->ReadBlockData (MmcHost, 0, 8, (VOID *) Scr);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a(MMC_ACMD51): ReadBlockData Error and Status = %r\n", __func__, Status));
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
+
STATIC
EFI_STATUS
InitializeSdMmcDevice (
@@ -574,7 +617,6 @@ InitializeSdMmcDevice (
)
{
UINT32 Response[4];
- UINT32 Buffer[128];
UINTN BlockSize;
UINTN CardSize;
UINTN NumBlocks;
@@ -621,58 +663,35 @@ InitializeSdMmcDevice (
return Status;
}
- Status = MmcHost->SendCommand (MmcHost, MMC_CMD55,
- MmcHostInstance->CardInfo.RCA << 16);
+ Status = SdExecuteScr (MmcHostInstance, &Scr);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a (MMC_CMD55): Error and Status = %r\n", __FUNCTION__, Status));
- return Status;
- }
- Status = MmcHost->ReceiveResponse (MmcHost, MMC_RESPONSE_TYPE_R1, Response);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a (MMC_CMD55): Error and Status = %r\n", __FUNCTION__, Status));
- return Status;
- }
- if ((Response[0] & MMC_STATUS_APP_CMD) == 0) {
- return EFI_SUCCESS;
+ return Status;
}
- /* SCR */
- Status = MmcHost->SendCommand (MmcHost, MMC_ACMD51, 0);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a(MMC_ACMD51): Error and Status = %r\n", __func__, Status));
- return Status;
- } else {
- Status = MmcHost->ReadBlockData (MmcHost, 0, 8, Buffer);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a(MMC_ACMD51): ReadBlockData Error and Status = %r\n", __func__, Status));
- return Status;
- }
- CopyMem (&Scr, Buffer, 8);
- if (Scr.SD_SPEC == 2) {
- if (Scr.SD_SPEC3 == 1) {
- if (Scr.SD_SPEC4 == 1) {
- DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 4.xx\n"));
- } else {
- DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 3.0x\n"));
- }
+ if (Scr.SD_SPEC == 2) {
+ if (Scr.SD_SPEC3 == 1) {
+ if (Scr.SD_SPEC4 == 1) {
+ DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 4.xx\n"));
} else {
- if (Scr.SD_SPEC4 == 0) {
- DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 2.0\n"));
- } else {
- DEBUG ((DEBUG_ERROR, "Found invalid SD Card\n"));
- }
+ DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 3.0x\n"));
}
} else {
- if ((Scr.SD_SPEC3 == 0) && (Scr.SD_SPEC4 == 0)) {
- if (Scr.SD_SPEC == 1) {
- DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 1.10\n"));
- } else {
- DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 1.0\n"));
- }
+ if (Scr.SD_SPEC4 == 0) {
+ DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 2.0\n"));
} else {
DEBUG ((DEBUG_ERROR, "Found invalid SD Card\n"));
}
}
+ } else {
+ if ((Scr.SD_SPEC3 == 0) && (Scr.SD_SPEC4 == 0)) {
+ if (Scr.SD_SPEC == 1) {
+ DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 1.10\n"));
+ } else {
+ DEBUG ((DEBUG_INFO, "Found SD Card for Spec Version 1.0\n"));
+ }
+ } else {
+ DEBUG ((DEBUG_ERROR, "Found invalid SD Card\n"));
+ }
}
Status = SdSetSpeed (MmcHostInstance, CccSwitch);
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-platforms][PATCH 4/5] Platform/RPi/MmcDxe: Improve MMC driver stability
2019-11-27 12:37 [edk2-platforms][PATCH 0/5] Further RPi4 support groundwork Pete Batard
` (2 preceding siblings ...)
2019-11-27 12:37 ` [edk2-platforms][PATCH 3/5] Platform/RPi/MmcDxe: Factorize SCR call and clean up MMC init Pete Batard
@ 2019-11-27 12:37 ` Pete Batard
2019-11-27 12:37 ` [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model Pete Batard
4 siblings, 0 replies; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:37 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif.lindholm, philmd, samer.el-haj-mahmoud,
andrey.warkentin
From: Andrei Warkentin <andrey.warkentin@gmail.com>
For reasons that aren't entirely understood, SCR reads can randomly
fail on the Raspberry Pi 4. It can be on the first boot or during
subsequent reboots.
To alleviate this, and improve the overall behavior of the code,
we modify CheckCardsCallback () to retry InitializeMmcDevice ()
in case of failure.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Drivers/MmcDxe/Mmc.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Platform/RaspberryPi/Drivers/MmcDxe/Mmc.c b/Platform/RaspberryPi/Drivers/MmcDxe/Mmc.c
index c3c7279e4707..f6c4cc7bc3a3 100644
--- a/Platform/RaspberryPi/Drivers/MmcDxe/Mmc.c
+++ b/Platform/RaspberryPi/Drivers/MmcDxe/Mmc.c
@@ -2,6 +2,7 @@
*
* Main file of the MMC Dxe driver. The driver entrypoint is defined into this file.
*
+ * Copyright (c) 2019, Andrei Warkentin <andrey.warkentin@gmail.com>
* Copyright (c) 2011-2013, ARM Limited. All rights reserved.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -367,7 +368,11 @@ CheckCardsCallback (
MmcHostInstance->Initialized = !MmcHostInstance->Initialized;
if (MmcHostInstance->BlockIo.Media->MediaPresent) {
- InitializeMmcDevice (MmcHostInstance);
+ Status = InitializeMmcDevice (MmcHostInstance);
+ if (EFI_ERROR (Status)) {
+ MmcHostInstance->Initialized = !MmcHostInstance->Initialized;
+ continue;
+ }
}
Status = gBS->ReinstallProtocolInterface (
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model
2019-11-27 12:37 [edk2-platforms][PATCH 0/5] Further RPi4 support groundwork Pete Batard
` (3 preceding siblings ...)
2019-11-27 12:37 ` [edk2-platforms][PATCH 4/5] Platform/RPi/MmcDxe: Improve MMC driver stability Pete Batard
@ 2019-11-27 12:37 ` Pete Batard
2019-11-27 15:24 ` Philippe Mathieu-Daudé
4 siblings, 1 reply; 21+ messages in thread
From: Pete Batard @ 2019-11-27 12:37 UTC (permalink / raw)
To: devel
Cc: ard.biesheuvel, leif.lindholm, philmd, samer.el-haj-mahmoud,
andrey.warkentin
From: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
The Raspberry Pi 4 has a new SD controller. As a result we must
handle SD routing according to the model, which we perform in
the Config driver by using the GetModelFamily () call that was
recently introduced.
Signed-off-by: Pete Batard <pete@akeo.ie>
---
Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 137 ++++++++++++++------
1 file changed, 96 insertions(+), 41 deletions(-)
diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
index 98e58a560ed4..26bc92f28185 100644
--- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
+++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
@@ -1,6 +1,7 @@
/** @file
*
- * Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
+ * Copyright (c) 2019, ARM Limited. All rights reserved.
+ * Copyright (c) 2018 - 2019, Andrei Warkentin <andrey.warkentin@gmail.com>
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*
@@ -9,10 +10,12 @@
#include <Uefi.h>
#include <Library/HiiLib.h>
#include <Library/DebugLib.h>
+#include <Library/IoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>
#include <Library/DevicePathLib.h>
#include <IndustryStandard/RpiMbox.h>
+#include <IndustryStandard/Bcm2836.h>
#include <IndustryStandard/Bcm2836Gpio.h>
#include <Library/GpioLib.h>
#include <Protocol/RpiFirmware.h>
@@ -212,6 +215,7 @@ ApplyVariables (
UINT32 CpuClock = PcdGet32 (PcdCpuClock);
UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock);
UINT32 Rate = 0;
+ UINT32 ModelFamily = 0;
if (CpuClock != 0) {
if (CpuClock == 2) {
@@ -245,51 +249,102 @@ ApplyVariables (
DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
}
- /*
- * Switching two groups around, so disable both first.
- *
- * No, I've not seen a problem, but having a group be
- * routed to two sets of pins seems like asking for trouble.
- */
- GpioPinFuncSet (34, GPIO_FSEL_INPUT);
- GpioPinFuncSet (35, GPIO_FSEL_INPUT);
- GpioPinFuncSet (36, GPIO_FSEL_INPUT);
- GpioPinFuncSet (37, GPIO_FSEL_INPUT);
- GpioPinFuncSet (38, GPIO_FSEL_INPUT);
- GpioPinFuncSet (39, GPIO_FSEL_INPUT);
- GpioPinFuncSet (48, GPIO_FSEL_INPUT);
- GpioPinFuncSet (49, GPIO_FSEL_INPUT);
- GpioPinFuncSet (50, GPIO_FSEL_INPUT);
- GpioPinFuncSet (51, GPIO_FSEL_INPUT);
- GpioPinFuncSet (52, GPIO_FSEL_INPUT);
- GpioPinFuncSet (53, GPIO_FSEL_INPUT);
- if (PcdGet32 (PcdSdIsArasan)) {
- DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
- Gpio48Group = GPIO_FSEL_ALT3;
+ Status = mFwProtocol->GetModelFamily (&ModelFamily);
+ if (Status != EFI_SUCCESS) {
+ DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi model family: %r\n", Status));
+ } else {
+ DEBUG ((DEBUG_INFO, "Current Raspberry Pi model family is 0x%x\n", ModelFamily));
+ }
+
+
+ if (ModelFamily == 3) {
/*
- * Route SDIO to SdHost.
+ * Pi 3: either Arasan or SdHost goes to SD card.
+ *
+ * Switching two groups around, so disable both first.
+ *
+ * No, I've not seen a problem, but having a group be
+ * routed to two sets of pins seems like asking for trouble.
*/
- Gpio34Group = GPIO_FSEL_ALT0;
- } else {
- DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
- Gpio48Group = GPIO_FSEL_ALT0;
+ GpioPinFuncSet (34, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (35, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (36, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (37, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (38, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (39, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (48, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (49, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (50, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (51, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (52, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (53, GPIO_FSEL_INPUT);
+
+ if (PcdGet32 (PcdSdIsArasan)) {
+ DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
+ Gpio48Group = GPIO_FSEL_ALT3;
+ /*
+ * Route SDIO to SdHost.
+ */
+ Gpio34Group = GPIO_FSEL_ALT0;
+ } else {
+ DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
+ Gpio48Group = GPIO_FSEL_ALT0;
+ /*
+ * Route SDIO to Arasan.
+ */
+ Gpio34Group = GPIO_FSEL_ALT3;
+ }
+ GpioPinFuncSet (34, Gpio34Group);
+ GpioPinFuncSet (35, Gpio34Group);
+ GpioPinFuncSet (36, Gpio34Group);
+ GpioPinFuncSet (37, Gpio34Group);
+ GpioPinFuncSet (38, Gpio34Group);
+ GpioPinFuncSet (39, Gpio34Group);
+ GpioPinFuncSet (48, Gpio48Group);
+ GpioPinFuncSet (49, Gpio48Group);
+ GpioPinFuncSet (50, Gpio48Group);
+ GpioPinFuncSet (51, Gpio48Group);
+ GpioPinFuncSet (52, Gpio48Group);
+ GpioPinFuncSet (53, Gpio48Group);
+
+ } else if (ModelFamily == 4){
/*
- * Route SDIO to Arasan.
+ * Pi 4: either Arasan or eMMC2 goes to SD card.
*/
- Gpio34Group = GPIO_FSEL_ALT3;
+ if (PcdGet32 (PcdSdIsArasan)) {
+ /*
+ * WiFi disabled.
+ */
+ GpioPinFuncSet (34, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (35, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (36, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (37, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (38, GPIO_FSEL_INPUT);
+ GpioPinFuncSet (39, GPIO_FSEL_INPUT);
+ /*
+ * SD card pins go to Arasan.
+ */
+ MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
+ MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
+ } else {
+ /*
+ * SD card pins back to eMMC2.
+ */
+ MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
+ MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
+ /*
+ * WiFi back to Arasan.
+ */
+ GpioPinFuncSet (34, GPIO_FSEL_ALT3);
+ GpioPinFuncSet (35, GPIO_FSEL_ALT3);
+ GpioPinFuncSet (36, GPIO_FSEL_ALT3);
+ GpioPinFuncSet (37, GPIO_FSEL_ALT3);
+ GpioPinFuncSet (38, GPIO_FSEL_ALT3);
+ GpioPinFuncSet (39, GPIO_FSEL_ALT3);
+ }
+ } else {
+ DEBUG ((DEBUG_ERROR, "Model Family %d not supported...\n", ModelFamily));
}
- GpioPinFuncSet (34, Gpio34Group);
- GpioPinFuncSet (35, Gpio34Group);
- GpioPinFuncSet (36, Gpio34Group);
- GpioPinFuncSet (37, Gpio34Group);
- GpioPinFuncSet (38, Gpio34Group);
- GpioPinFuncSet (39, Gpio34Group);
- GpioPinFuncSet (48, Gpio48Group);
- GpioPinFuncSet (49, Gpio48Group);
- GpioPinFuncSet (50, Gpio48Group);
- GpioPinFuncSet (51, Gpio48Group);
- GpioPinFuncSet (52, Gpio48Group);
- GpioPinFuncSet (53, Gpio48Group);
/*
* JTAG pin JTAG sig GPIO Mode Header pin
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model
2019-11-27 12:37 ` [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model Pete Batard
@ 2019-11-27 15:24 ` Philippe Mathieu-Daudé
2019-11-27 16:33 ` Pete Batard
0 siblings, 1 reply; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-27 15:24 UTC (permalink / raw)
To: Pete Batard, devel
Cc: ard.biesheuvel, leif.lindholm, samer.el-haj-mahmoud,
andrey.warkentin
On 11/27/19 1:37 PM, Pete Batard wrote:
> From: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
>
> The Raspberry Pi 4 has a new SD controller. As a result we must
> handle SD routing according to the model, which we perform in
> the Config driver by using the GetModelFamily () call that was
> recently introduced.
>
> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 137 ++++++++++++++------
> 1 file changed, 96 insertions(+), 41 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> index 98e58a560ed4..26bc92f28185 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> @@ -1,6 +1,7 @@
> /** @file
> *
> - * Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> + * Copyright (c) 2019, ARM Limited. All rights reserved.
"All rights reserved."?
> + * Copyright (c) 2018 - 2019, Andrei Warkentin <andrey.warkentin@gmail.com>
> *
> * SPDX-License-Identifier: BSD-2-Clause-Patent
> *
> @@ -9,10 +10,12 @@
> #include <Uefi.h>
> #include <Library/HiiLib.h>
> #include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiRuntimeServicesTableLib.h>
> #include <Library/DevicePathLib.h>
> #include <IndustryStandard/RpiMbox.h>
> +#include <IndustryStandard/Bcm2836.h>
> #include <IndustryStandard/Bcm2836Gpio.h>
> #include <Library/GpioLib.h>
> #include <Protocol/RpiFirmware.h>
> @@ -212,6 +215,7 @@ ApplyVariables (
> UINT32 CpuClock = PcdGet32 (PcdCpuClock);
> UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock);
> UINT32 Rate = 0;
> + UINT32 ModelFamily = 0;
>
> if (CpuClock != 0) {
> if (CpuClock == 2) {
> @@ -245,51 +249,102 @@ ApplyVariables (
> DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
> }
>
> - /*
> - * Switching two groups around, so disable both first.
> - *
> - * No, I've not seen a problem, but having a group be
> - * routed to two sets of pins seems like asking for trouble.
> - */
> - GpioPinFuncSet (34, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (35, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (36, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (37, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (38, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (39, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (48, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (49, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (50, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (51, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (52, GPIO_FSEL_INPUT);
> - GpioPinFuncSet (53, GPIO_FSEL_INPUT);
> - if (PcdGet32 (PcdSdIsArasan)) {
> - DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
> - Gpio48Group = GPIO_FSEL_ALT3;
> + Status = mFwProtocol->GetModelFamily (&ModelFamily);
> + if (Status != EFI_SUCCESS) {
> + DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi model family: %r\n", Status));
> + } else {
> + DEBUG ((DEBUG_INFO, "Current Raspberry Pi model family is 0x%x\n", ModelFamily));
> + }
> +
> +
> + if (ModelFamily == 3) {
> /*
> - * Route SDIO to SdHost.
> + * Pi 3: either Arasan or SdHost goes to SD card.
> + *
> + * Switching two groups around, so disable both first.
> + *
> + * No, I've not seen a problem, but having a group be
> + * routed to two sets of pins seems like asking for trouble.
> */
> - Gpio34Group = GPIO_FSEL_ALT0;
> - } else {
> - DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
> - Gpio48Group = GPIO_FSEL_ALT0;
> + GpioPinFuncSet (34, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (35, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (36, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (37, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (38, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (39, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (48, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (49, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (50, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (51, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (52, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (53, GPIO_FSEL_INPUT);
> +
> + if (PcdGet32 (PcdSdIsArasan)) {
> + DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
> + Gpio48Group = GPIO_FSEL_ALT3;
> + /*
> + * Route SDIO to SdHost.
> + */
> + Gpio34Group = GPIO_FSEL_ALT0;
> + } else {
> + DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
> + Gpio48Group = GPIO_FSEL_ALT0;
> + /*
> + * Route SDIO to Arasan.
> + */
> + Gpio34Group = GPIO_FSEL_ALT3;
> + }
> + GpioPinFuncSet (34, Gpio34Group);
> + GpioPinFuncSet (35, Gpio34Group);
> + GpioPinFuncSet (36, Gpio34Group);
> + GpioPinFuncSet (37, Gpio34Group);
> + GpioPinFuncSet (38, Gpio34Group);
> + GpioPinFuncSet (39, Gpio34Group);
> + GpioPinFuncSet (48, Gpio48Group);
> + GpioPinFuncSet (49, Gpio48Group);
> + GpioPinFuncSet (50, Gpio48Group);
> + GpioPinFuncSet (51, Gpio48Group);
> + GpioPinFuncSet (52, Gpio48Group);
> + GpioPinFuncSet (53, Gpio48Group);
> +
> + } else if (ModelFamily == 4){
Missing space before opening brackets.
Patch looks good otherwise.
> /*
> - * Route SDIO to Arasan.
> + * Pi 4: either Arasan or eMMC2 goes to SD card.
> */
> - Gpio34Group = GPIO_FSEL_ALT3;
> + if (PcdGet32 (PcdSdIsArasan)) {
> + /*
> + * WiFi disabled.
> + */
> + GpioPinFuncSet (34, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (35, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (36, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (37, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (38, GPIO_FSEL_INPUT);
> + GpioPinFuncSet (39, GPIO_FSEL_INPUT);
> + /*
> + * SD card pins go to Arasan.
> + */
> + MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
> + MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
> + } else {
> + /*
> + * SD card pins back to eMMC2.
> + */
> + MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
> + MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
> + /*
> + * WiFi back to Arasan.
> + */
> + GpioPinFuncSet (34, GPIO_FSEL_ALT3);
> + GpioPinFuncSet (35, GPIO_FSEL_ALT3);
> + GpioPinFuncSet (36, GPIO_FSEL_ALT3);
> + GpioPinFuncSet (37, GPIO_FSEL_ALT3);
> + GpioPinFuncSet (38, GPIO_FSEL_ALT3);
> + GpioPinFuncSet (39, GPIO_FSEL_ALT3);
> + }
> + } else {
> + DEBUG ((DEBUG_ERROR, "Model Family %d not supported...\n", ModelFamily));
> }
> - GpioPinFuncSet (34, Gpio34Group);
> - GpioPinFuncSet (35, Gpio34Group);
> - GpioPinFuncSet (36, Gpio34Group);
> - GpioPinFuncSet (37, Gpio34Group);
> - GpioPinFuncSet (38, Gpio34Group);
> - GpioPinFuncSet (39, Gpio34Group);
> - GpioPinFuncSet (48, Gpio48Group);
> - GpioPinFuncSet (49, Gpio48Group);
> - GpioPinFuncSet (50, Gpio48Group);
> - GpioPinFuncSet (51, Gpio48Group);
> - GpioPinFuncSet (52, Gpio48Group);
> - GpioPinFuncSet (53, Gpio48Group);
>
> /*
> * JTAG pin JTAG sig GPIO Mode Header pin
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model
2019-11-27 15:24 ` Philippe Mathieu-Daudé
@ 2019-11-27 16:33 ` Pete Batard
2019-11-27 17:04 ` Leif Lindholm
0 siblings, 1 reply; 21+ messages in thread
From: Pete Batard @ 2019-11-27 16:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, devel
Cc: ard.biesheuvel, leif.lindholm, samer.el-haj-mahmoud,
andrey.warkentin
On 2019.11.27 15:24, Philippe Mathieu-Daudé wrote:
> On 11/27/19 1:37 PM, Pete Batard wrote:
>> From: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
>>
>> The Raspberry Pi 4 has a new SD controller. As a result we must
>> handle SD routing according to the model, which we perform in
>> the Config driver by using the GetModelFamily () call that was
>> recently introduced.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 137
>> ++++++++++++++------
>> 1 file changed, 96 insertions(+), 41 deletions(-)
>>
>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> index 98e58a560ed4..26bc92f28185 100644
>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>> @@ -1,6 +1,7 @@
>> /** @file
>> *
>> - * Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
>> + * Copyright (c) 2019, ARM Limited. All rights reserved.
>
> "All rights reserved."?
To be honest, that's something that's been bothering me too in this
codebase (and some other ones too, where you get to see the same), since
there are only so many rights one can reserve when the code is actually
governed by the Open Source license being used, and therefore asserting
that you reserve "all rights" seems to be in direct conflict with that.
However, I am not a lawyer, and this seems to be standard boilerplate
being imposed by large companies. For instance, you'll find plenty of
instances of it in the existing codebase. E.g.
https://github.com/tianocore/edk2/blob/master/ArmPkg/Include/AsmMacroIoLib.h
has three separate entities that appear to state that each one holds all
the rights to the source, which I can't help by find amusing.
I guess we're supposed to understand that each entity reserves all
rights to the code they've actually written (including the right to do
something that might go against the license, since "All rights" >
"Rights to the extent being granted by the BSD"), and that it's up to
legal departments to sort up the mess, if mess there is...
Then again, while I think I can wrap my head against what copyright
entails, I'm not sure I completely get what these additional "rights"
are supposed to mean in this context (my current take being that we're
supposed to be believe that there exists an implicit grandfathered
license, which gives all rights to the parent company, and that governs
a virtual version of the source code containing only the changes that
the developer applied, and therefore that the BSD licensed version of
the source that is then made public is meant to be seen as a derivative
of this virtual "All rights reserved" incomplete source, hence granting
a partial "All rights" for said source to the company, if that makes any
sense), so it may be good for someone with better understanding of this
to clarify, or point to a place where this might be explained.
>
>> + * Copyright (c) 2018 - 2019, Andrei Warkentin
>> <andrey.warkentin@gmail.com>
>> *
>> * SPDX-License-Identifier: BSD-2-Clause-Patent
>> *
>> @@ -9,10 +10,12 @@
>> #include <Uefi.h>
>> #include <Library/HiiLib.h>
>> #include <Library/DebugLib.h>
>> +#include <Library/IoLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>> #include <Library/UefiRuntimeServicesTableLib.h>
>> #include <Library/DevicePathLib.h>
>> #include <IndustryStandard/RpiMbox.h>
>> +#include <IndustryStandard/Bcm2836.h>
>> #include <IndustryStandard/Bcm2836Gpio.h>
>> #include <Library/GpioLib.h>
>> #include <Protocol/RpiFirmware.h>
>> @@ -212,6 +215,7 @@ ApplyVariables (
>> UINT32 CpuClock = PcdGet32 (PcdCpuClock);
>> UINT32 CustomCpuClock = PcdGet32 (PcdCustomCpuClock);
>> UINT32 Rate = 0;
>> + UINT32 ModelFamily = 0;
>> if (CpuClock != 0) {
>> if (CpuClock == 2) {
>> @@ -245,51 +249,102 @@ ApplyVariables (
>> DEBUG ((DEBUG_INFO, "Current CPU speed is %uHz\n", Rate));
>> }
>> - /*
>> - * Switching two groups around, so disable both first.
>> - *
>> - * No, I've not seen a problem, but having a group be
>> - * routed to two sets of pins seems like asking for trouble.
>> - */
>> - GpioPinFuncSet (34, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (35, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (36, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (37, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (38, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (39, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (48, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (49, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (50, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (51, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (52, GPIO_FSEL_INPUT);
>> - GpioPinFuncSet (53, GPIO_FSEL_INPUT);
>> - if (PcdGet32 (PcdSdIsArasan)) {
>> - DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
>> - Gpio48Group = GPIO_FSEL_ALT3;
>> + Status = mFwProtocol->GetModelFamily (&ModelFamily);
>> + if (Status != EFI_SUCCESS) {
>> + DEBUG ((DEBUG_ERROR, "Couldn't get the Raspberry Pi model family:
>> %r\n", Status));
>> + } else {
>> + DEBUG ((DEBUG_INFO, "Current Raspberry Pi model family is
>> 0x%x\n", ModelFamily));
>> + }
>> +
>> +
>> + if (ModelFamily == 3) {
>> /*
>> - * Route SDIO to SdHost.
>> + * Pi 3: either Arasan or SdHost goes to SD card.
>> + *
>> + * Switching two groups around, so disable both first.
>> + *
>> + * No, I've not seen a problem, but having a group be
>> + * routed to two sets of pins seems like asking for trouble.
>> */
>> - Gpio34Group = GPIO_FSEL_ALT0;
>> - } else {
>> - DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
>> - Gpio48Group = GPIO_FSEL_ALT0;
>> + GpioPinFuncSet (34, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (35, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (36, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (37, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (38, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (39, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (48, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (49, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (50, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (51, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (52, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (53, GPIO_FSEL_INPUT);
>> +
>> + if (PcdGet32 (PcdSdIsArasan)) {
>> + DEBUG ((DEBUG_INFO, "Routing SD to Arasan\n"));
>> + Gpio48Group = GPIO_FSEL_ALT3;
>> + /*
>> + * Route SDIO to SdHost.
>> + */
>> + Gpio34Group = GPIO_FSEL_ALT0;
>> + } else {
>> + DEBUG ((DEBUG_INFO, "Routing SD to SdHost\n"));
>> + Gpio48Group = GPIO_FSEL_ALT0;
>> + /*
>> + * Route SDIO to Arasan.
>> + */
>> + Gpio34Group = GPIO_FSEL_ALT3;
>> + }
>> + GpioPinFuncSet (34, Gpio34Group);
>> + GpioPinFuncSet (35, Gpio34Group);
>> + GpioPinFuncSet (36, Gpio34Group);
>> + GpioPinFuncSet (37, Gpio34Group);
>> + GpioPinFuncSet (38, Gpio34Group);
>> + GpioPinFuncSet (39, Gpio34Group);
>> + GpioPinFuncSet (48, Gpio48Group);
>> + GpioPinFuncSet (49, Gpio48Group);
>> + GpioPinFuncSet (50, Gpio48Group);
>> + GpioPinFuncSet (51, Gpio48Group);
>> + GpioPinFuncSet (52, Gpio48Group);
>> + GpioPinFuncSet (53, Gpio48Group);
>> +
>> + } else if (ModelFamily == 4){
>
> Missing space before opening brackets.
Thanks for the review. Will fix that.
Regards,
/Pete
>
> Patch looks good otherwise.
>
>> /*
>> - * Route SDIO to Arasan.
>> + * Pi 4: either Arasan or eMMC2 goes to SD card.
>> */
>> - Gpio34Group = GPIO_FSEL_ALT3;
>> + if (PcdGet32 (PcdSdIsArasan)) {
>> + /*
>> + * WiFi disabled.
>> + */
>> + GpioPinFuncSet (34, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (35, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (36, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (37, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (38, GPIO_FSEL_INPUT);
>> + GpioPinFuncSet (39, GPIO_FSEL_INPUT);
>> + /*
>> + * SD card pins go to Arasan.
>> + */
>> + MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
>> + MmioRead32(GPIO_BASE_ADDRESS + 0xD0) | 0x2);
>> + } else {
>> + /*
>> + * SD card pins back to eMMC2.
>> + */
>> + MmioWrite32((GPIO_BASE_ADDRESS + 0xD0),
>> + MmioRead32(GPIO_BASE_ADDRESS + 0xD0) & ~0x2);
>> + /*
>> + * WiFi back to Arasan.
>> + */
>> + GpioPinFuncSet (34, GPIO_FSEL_ALT3);
>> + GpioPinFuncSet (35, GPIO_FSEL_ALT3);
>> + GpioPinFuncSet (36, GPIO_FSEL_ALT3);
>> + GpioPinFuncSet (37, GPIO_FSEL_ALT3);
>> + GpioPinFuncSet (38, GPIO_FSEL_ALT3);
>> + GpioPinFuncSet (39, GPIO_FSEL_ALT3);
>> + }
>> + } else {
>> + DEBUG ((DEBUG_ERROR, "Model Family %d not supported...\n",
>> ModelFamily));
>> }
>> - GpioPinFuncSet (34, Gpio34Group);
>> - GpioPinFuncSet (35, Gpio34Group);
>> - GpioPinFuncSet (36, Gpio34Group);
>> - GpioPinFuncSet (37, Gpio34Group);
>> - GpioPinFuncSet (38, Gpio34Group);
>> - GpioPinFuncSet (39, Gpio34Group);
>> - GpioPinFuncSet (48, Gpio48Group);
>> - GpioPinFuncSet (49, Gpio48Group);
>> - GpioPinFuncSet (50, Gpio48Group);
>> - GpioPinFuncSet (51, Gpio48Group);
>> - GpioPinFuncSet (52, Gpio48Group);
>> - GpioPinFuncSet (53, Gpio48Group);
>> /*
>> * JTAG pin JTAG sig GPIO Mode Header pin
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model
2019-11-27 16:33 ` Pete Batard
@ 2019-11-27 17:04 ` Leif Lindholm
2019-11-27 17:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 21+ messages in thread
From: Leif Lindholm @ 2019-11-27 17:04 UTC (permalink / raw)
To: Pete Batard, Philippe Mathieu-Daudé
Cc: devel, ard.biesheuvel, samer.el-haj-mahmoud, andrey.warkentin
On Wed, Nov 27, 2019 at 16:33:28 +0000, Pete Batard wrote:
> > > Signed-off-by: Pete Batard <pete@akeo.ie>
> > > ---
> > > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 137
> > > ++++++++++++++------
> > > 1 file changed, 96 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> > > b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> > > index 98e58a560ed4..26bc92f28185 100644
> > > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> > > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
> > > @@ -1,6 +1,7 @@
> > > /** @file
> > > *
> > > - * Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> > > + * Copyright (c) 2019, ARM Limited. All rights reserved.
> >
> > "All rights reserved."?
>
> To be honest, that's something that's been bothering me too in this codebase
> (and some other ones too, where you get to see the same), since there are
> only so many rights one can reserve when the code is actually governed by
> the Open Source license being used, and therefore asserting that you reserve
> "all rights" seems to be in direct conflict with that.
>
> However, I am not a lawyer, and this seems to be standard boilerplate being
> imposed by large companies. For instance, you'll find plenty of instances of
> it in the existing codebase. E.g.
> https://github.com/tianocore/edk2/blob/master/ArmPkg/Include/AsmMacroIoLib.h
> has three separate entities that appear to state that each one holds all the
> rights to the source, which I can't help by find amusing.
>
> I guess we're supposed to understand that each entity reserves all rights to
> the code they've actually written (including the right to do something that
> might go against the license, since "All rights" > "Rights to the extent
> being granted by the BSD"), and that it's up to legal departments to sort up
> the mess, if mess there is...
Yeah, that mostly matches my interpretation.
My understanding is that there are certain paranoid interpretations
under which you *give away* rights to code you contribute to an open
source project - like the right to also publish/contribute the same
code under some other license.
I don't know if this stems from things like copyright assignment
agreements, which (for similar reasons) may explicitly grant back to
the contributor a bunch of rights to the contributed code, and various
corporate legal departments just blindly require it to be included
everywhere.
Phil: do a grep in linux, u-boot or qemu.
This is silly, but it's commonplace and non-controversial.
/
Leif
> Then again, while I think I can wrap my head against what copyright entails,
> I'm not sure I completely get what these additional "rights" are supposed to
> mean in this context (my current take being that we're supposed to be
> believe that there exists an implicit grandfathered license, which gives all
> rights to the parent company, and that governs a virtual version of the
> source code containing only the changes that the developer applied, and
> therefore that the BSD licensed version of the source that is then made
> public is meant to be seen as a derivative of this virtual "All rights
> reserved" incomplete source, hence granting a partial "All rights" for said
> source to the company, if that makes any sense), so it may be good for
> someone with better understanding of this to clarify, or point to a place
> where this might be explained.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-platforms][PATCH 5/5] Platform/RPi: Set SD routing according to model
2019-11-27 17:04 ` Leif Lindholm
@ 2019-11-27 17:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 21+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-27 17:17 UTC (permalink / raw)
To: Leif Lindholm, Pete Batard
Cc: devel, ard.biesheuvel, samer.el-haj-mahmoud, andrey.warkentin
On 11/27/19 6:04 PM, Leif Lindholm wrote:
> On Wed, Nov 27, 2019 at 16:33:28 +0000, Pete Batard wrote:
>>>> Signed-off-by: Pete Batard <pete@akeo.ie>
>>>> ---
>>>> Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 137
>>>> ++++++++++++++------
>>>> 1 file changed, 96 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>>>> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>>>> index 98e58a560ed4..26bc92f28185 100644
>>>> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>>>> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c
>>>> @@ -1,6 +1,7 @@
>>>> /** @file
>>>> *
>>>> - * Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
>>>> + * Copyright (c) 2019, ARM Limited. All rights reserved.
>>>
>>> "All rights reserved."?
>>
>> To be honest, that's something that's been bothering me too in this codebase
>> (and some other ones too, where you get to see the same), since there are
>> only so many rights one can reserve when the code is actually governed by
>> the Open Source license being used, and therefore asserting that you reserve
>> "all rights" seems to be in direct conflict with that.
>>
>> However, I am not a lawyer, and this seems to be standard boilerplate being
>> imposed by large companies. For instance, you'll find plenty of instances of
>> it in the existing codebase. E.g.
>> https://github.com/tianocore/edk2/blob/master/ArmPkg/Include/AsmMacroIoLib.h
>> has three separate entities that appear to state that each one holds all the
>> rights to the source, which I can't help by find amusing.
>>
>> I guess we're supposed to understand that each entity reserves all rights to
>> the code they've actually written (including the right to do something that
>> might go against the license, since "All rights" > "Rights to the extent
>> being granted by the BSD"), and that it's up to legal departments to sort up
>> the mess, if mess there is...
>
> Yeah, that mostly matches my interpretation.
>
> My understanding is that there are certain paranoid interpretations
> under which you *give away* rights to code you contribute to an open
> source project - like the right to also publish/contribute the same
> code under some other license.
>
> I don't know if this stems from things like copyright assignment
> agreements, which (for similar reasons) may explicitly grant back to
> the contributor a bunch of rights to the contributed code, and various
> corporate legal departments just blindly require it to be included
> everywhere.
>
> Phil: do a grep in linux, u-boot or qemu.
> This is silly, but it's commonplace and non-controversial.
Sorry for keeping asking about licensing, I'm trying to understand
better the situation / status quo, I also am not a lawyer.
Thanks both for your explanations :)
>> Then again, while I think I can wrap my head against what copyright entails,
>> I'm not sure I completely get what these additional "rights" are supposed to
>> mean in this context (my current take being that we're supposed to be
>> believe that there exists an implicit grandfathered license, which gives all
>> rights to the parent company, and that governs a virtual version of the
>> source code containing only the changes that the developer applied, and
>> therefore that the BSD licensed version of the source that is then made
>> public is meant to be seen as a derivative of this virtual "All rights
>> reserved" incomplete source, hence granting a partial "All rights" for said
>> source to the company, if that makes any sense), so it may be good for
>> someone with better understanding of this to clarify, or point to a place
>> where this might be explained.
>
^ permalink raw reply [flat|nested] 21+ messages in thread