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

[Matthias Brugger]

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@xxxxxxx>
>>>
>>> ---
>>>
>>> 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.

Regards,
Matthias

> Regards,
> Nicolas
> 
>> 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");
>>> +}
>>> +#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_ */
>>>
>