Re: [PATCH v2 2/2] usb: xhci: Load Raspberry Pi 4 VL805's firmware

[Matthias Brugger <mbrugger@suse.com>]

On 05/05/2020 15:47, Nicolas Saenz Julienne wrote:
> On Tue, 2020-05-05 at 15:39 +0200, Matthias Brugger wrote:
>>
>> On 05/05/2020 14:53, Nicolas Saenz Julienne wrote:
>>> Hi Matthias,
>>>
>>> On Tue, 2020-05-05 at 14:15 +0200, Matthias Brugger wrote:
>>>> On 30/04/2020 15:04, Nicolas Saenz Julienne wrote:
>>>>> When needed, RPi4's co-processor (called VideoCore) has to be instructed
>>>>> to load VL805's firmware (the chip providing xHCI support). VideoCore's
>>>>> firmware expects the board's PCIe bus to be already configured in order
>>>>> for it to load the xHCI chip firmware. So we have to make sure this
>>>>> happens in between the PCIe configuration and xHCI startup.
>>>>>
>>>>> Introduce a callback in xhci_pci_probe() to run this platform specific
>>>>> routine.
>>>>>
>>>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes since v1:
>>>>>  - Create callback
>>>>>
>>>>>  board/raspberrypi/rpi/rpi.c | 12 ++++++++++++
>>>>>  drivers/usb/host/xhci-pci.c |  6 ++++++
>>>>>  include/usb/xhci.h          |  3 +++
>>>>>  3 files changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
>>>>> index e367ba3092..8aa78d1f48 100644
>>>>> --- a/board/raspberrypi/rpi/rpi.c
>>>>> +++ b/board/raspberrypi/rpi/rpi.c
>>>>> @@ -14,6 +14,7 @@
>>>>>  #include <lcd.h>
>>>>>  #include <memalign.h>
>>>>>  #include <mmc.h>
>>>>> +#include <usb/xhci.h>
>>>>>  #include <asm/gpio.h>
>>>>>  #include <asm/arch/mbox.h>
>>>>>  #include <asm/arch/msg.h>
>>>>> @@ -494,3 +495,14 @@ int ft_board_setup(void *blob, bd_t *bd)
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>> +
>>>>> +#ifdef CONFIG_BCM2711
>>>>
>>>> This won't work with rpi_arm64_defconfig.
>>>> Can't we just evaluate at runtime if we need to do anything in
>>>> xhci_pci_fixup.
>>>
>>> I can't see why, who is going to call xhci_pci_probe() in RPi3?
>>>
>>
>> If you do make rpi_arm64_defconfig and inspect the .config, you will see that
>> CONFIG_BCM2711 is not defined, so this code won't be called on RPi4.
> 
> Oh! Understood.
> 
> Well, given that only xhci_pci_probe() is called if we're running on RPi4, I
> think I can disregard those defines altogether. I'll double-check that.
> 

Yes but from my understanding we only need to call the function on newer
revisions of RPi4. Does it have any side effect on older revisions, e.g. we get
error messages (see below)?

[...]>>>> I wonder if the newer RPi4 have also a newer revision ID (see
>>>> get_board_rev).
>>>> If
>>>> so we could add another bool to struct rpi_model which will indicate us if
>>>> we
>>>> need to notify VideoCore about vl805's firmware.
>>>>
>>>>> +void xhci_pci_fixup(struct udevice *dev)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = bcm2711_notify_vl805_reset();
>>>>> +	if (ret)
>>>>> +		printf("RPI: Failed to notify VideoCore about vl805's
>>>>> firmware\n");

We already have
printf("bcm2711: Faild to load vl805's firmware, %d\n", ret); in
bcm2711_notify_vl805_reset(). Do we really need two error messages?

Regards,
Matthias

>>>>> +}
>>>>> +#endif
>>>>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>>>>> index c1f60da541..1285dde1ef 100644
>>>>> --- a/drivers/usb/host/xhci-pci.c
>>>>> +++ b/drivers/usb/host/xhci-pci.c
>>>>> @@ -11,6 +11,10 @@
>>>>>  #include <usb.h>
>>>>>  #include <usb/xhci.h>
>>>>>  
>>>>> +__weak void xhci_pci_fixup(struct udevice *dev)
>>>>> +{
>>>>> +}
>>>>> +
>>>>>  static void xhci_pci_init(struct udevice *dev, struct xhci_hccr
>>>>> **ret_hccr,
>>>>>  			  struct xhci_hcor **ret_hcor)
>>>>>  {
>>>>> @@ -40,6 +44,8 @@ static int xhci_pci_probe(struct udevice *dev)
>>>>>  	struct xhci_hccr *hccr;
>>>>>  	struct xhci_hcor *hcor;
>>>>>  
>>>>> +	xhci_pci_fixup(dev);
>>>>> +
>>>>>  	xhci_pci_init(dev, &hccr, &hcor);
>>>>>  
>>>>>  	return xhci_register(dev, hccr, hcor);
>>>>> diff --git a/include/usb/xhci.h b/include/usb/xhci.h
>>>>> index c16106a2fc..57feed7603 100644
>>>>> --- a/include/usb/xhci.h
>>>>> +++ b/include/usb/xhci.h
>>>>> @@ -16,6 +16,7 @@
>>>>>  #ifndef HOST_XHCI_H_
>>>>>  #define HOST_XHCI_H_
>>>>>  
>>>>> +#include <usb.h>
>>>>>  #include <asm/types.h>
>>>>>  #include <asm/cache.h>
>>>>>  #include <asm/io.h>
>>>>> @@ -1281,4 +1282,6 @@ extern struct dm_usb_ops xhci_usb_ops;
>>>>>  
>>>>>  struct xhci_ctrl *xhci_get_ctrl(struct usb_device *udev);
>>>>>  
>>>>> +extern void xhci_pci_fixup(struct udevice *dev);
>>>>> +
>>>>>  #endif /* HOST_XHCI_H_ */
>>>>>
>