[PATCH v2 0/3] Ethernet support for Raspberry Pi 4

[Matthias Brugger]

On 24/01/2020 01:26, André Przywara wrote:
> On 23/01/2020 19:37, Matthias Brugger wrote:
> 
> Hi,
> 
>> On 23/01/2020 12:29, Andre Przywara wrote:
>>> On Wed, 22 Jan 2020 19:05:10 +0100
>>> Matthias Brugger <mbrugger at suse.com> wrote:
>>>
>>> Hi,
>>>
>>> Matthias, many thanks for looking at this and giving it a try!
>>>
>>>> On 22/01/2020 18:34, Andre Przywara wrote:
>>>>> On Wed, 22 Jan 2020 18:18:39 +0100
>>>>> Matthias Brugger <mbrugger at suse.com> wrote:
>>>>>
>>>>> Hi Matthias,
>>>>>   
>>>>>> On 17/01/2020 02:20, Andre Przywara wrote:  
>>>>>>> This series adds Ethernet support for the Raspberry Pi 4. The SoC
>>>>>>> includes a "Broadcom Genet v5 MAC" IP, connected as a proper platform
>>>>>>> device (no USB anymore!). Patch 1 provides a driver for that. There does
>>>>>>> not seem to be publicly available documentation, so this is based on the
>>>>>>> Linux driver, but stripped down to just provide what U-Boot needs.
>>>>>>> Patch 2 fixes up the RPi4 memory map to accommodate the MMIO area the
>>>>>>> MAC lives in, while patch 3 enables it in the respective defconfigs.
>>>>>>>
>>>>>>> This version addresses the comments by the diligent reviewers and testers,
>>>>>>> for a changelog see below.
>>>>>>> To see the individual changes as patches, refer to [1].
>>>>>>>
>>>>>>> Please have a look and test it, I hope this helps to simplify
>>>>>>> development, as you spare the SD card and its slot from heavy swapping.
>>>>>>>
>>>>>>> I dropped the Tested-by's, as there were changes in the code. Happy
>>>>>>> to reapply them when people confirm that it still works for them.
>>>>>>>     
>>>>>>
>>>>>> I having problems to actually boot a kernel when the genet driver is build into
>>>>>> U-Boot.  
>>>>>
>>>>> Ah! Sorry, I misread the former reports, I thought this was about booting kernels in general, with mainline U-Boot, without this series.
>>>>>   
>>>>>> If I boot grub and linux-next from there, I get the following SError (when using
>>>>>> earlycon):
>>>>>> https://pastebin.com/c1sw2uZk
>>>>>>
>>>>>> If I skip grub and boot the kernel directly from the SD:
>>>>>> load mmc 0:1 $kernel_addr_r Image
>>>>>> load mmc 0:1 $fdt_addr_r bcm2711-rpi-4-b.dtb
>>>>>> setenv bootargs "earlycon=uart8250,mmio32,0xfe215040"
>>>>>> booti $kernel_addr_r - $fdt_addr_r
>>>>>>
>>>>>> Gives a similar result.
>>>>>>
>>>>>> Do you see similar issues?  
>>>>>
>>>>> I didn't manage to start some kernel even without this series, I think, but didn't investigate further. I *loaded* several kernel images via TFTP and verified them with md5sum.
>>>>>   
>>>>
>>>> I think linux-next with defconfig should work.
>>>
>>> Yeah, I had some success with 5.5-rc, at least till it goes into userland, which is good enough for this purpose.
>>> And indeed I could reproduce the early crash with genet compiled in vs. mainline U-Boot.
>>>
>>>>> Some questions:
>>>>> - Does this happen even without touching the Ethernet in U-Boot at all (no dhcp command, no tftpboot, etc.)?  
>>>>
>>>> Yes, as soon as the genet is compiled into U-Boot I'm not able to boot a Linux
>>>> kernel.
>>>>
>>>>>   (I wonder if we have still DMA going on, even after the kernel already started. But if we just call probe(), there shouldn't be much going on).  
>>>>
>>>> At least when we start grub, we are actually starting the genet. I played with
>>>> the DMA shutdown in bcmgenet_gmac_eth_stop() but wasn't lucky.
>>>
>>> Stray DMA was more of a hope, as it would be easy to fix ;-)
>>> Without actively using the Ethernet in U-Boot, we never call eth_start, so don't enable DMA in the first place. Actually we don't even reset the MAC. See below.
>>>
>>>>> - Does reverting patch 2/3 change anything?  
>>>>
>>>> That was my first bet, but it hangs the board when it tries to initialize the
>>>> network driver.
>>>
>>> True, it just looked promising ;-)
>>>
>>> So I did some experiments, and it seems like we only call ofdata_to_platdata() and probe() from the driver. The latter is not doing much, but it starts the whole PHY init process.
>>> I could actually avoid the crash by just *not* returning 0 in bcmgenet_phy_init(). So every hardware setup step was still performed, but U-Boot *thinks* it didn't work.
>>>
>>> Looking at this more closely I see that we don't actually reset the MAC before accessing the PHY via MDIO. This might be a lead, but I don't see immediately why this would lead to an SError interrupt later on.
>>>
>>> I don't have a working setup here at work, so if someone could try to insert:
>>>
>>>     writel(0, priv->mac_reg + SYS_RBUF_FLUSH_CTRL);
>>>     udelay(10);
>>>     /* disable MAC while updating its registers */
>>>     writel(0, priv->mac_reg + UMAC_CMD);
>>>     /* issue soft reset with (rg)mii loopback to ensure a stable rxclk */
>>>     writel(CMD_SW_RESET | CMD_LCL_LOOP_EN, priv->mac_reg + UMAC_CMD);
>>>
>>> before calling bcmgenet_mdio_init() in bcmgenet_eth_probe() and give this a try.
>>>
>>
>> I'd say works by accident. If I use $fdtcontroladdr it works, but if I load the
>> dtb into $fdt_addr_r the kernel crashes at boot.
> 
> Weird, thanks for giving this a try.
> 
>>> Also it would be good to know if the PHY accesses via MDIO actually work at this point. As far as I remember the actual PHY init happens on first usage (typically the dhcp call).
>>>
>>
>> I see bcmgenet_eth_probe() being called on U-Boot startup.
>>
>> After that I can read registers from the phy, e.g.:
>> mdio read genet at 7d580000 1
>> Reading from bus mdio at e14
>> PHY at address 1:
>> 1 - 0x7969
> 
> So MDIO accesses seem to work fine, but actually MAC accesses seem to be
> a different story, see below.
> 
>>> Another thing to check is whether we would need to put the MAC back into reset upon exiting U-Boot. I quickly wired in a debug print in a .remove routine, but that didn't show up, so not sure it gets actually called in our case?
>>
>> You are right, it get's not called. I think last function called is
>> bcmgenet_gmac_eth_stop() but that's not an analytic claim (speak, I only saw
>> this sometimes when booting the kernel)
> 
> _stop() should only be called when _start() was called before. And this
> sequence happens on every network command, but not automatically without
> one.
> 
> And I was hoping that probe() would be called on traversing the DT,
> fishing for devices (which it does), and remove() at the very end
> (EXIT_BOOTSERVICES).
> 
>> Will keep digging tomorrow.
> 
> Found the culprit, after following a lead started by an over-lunch
> discussion: Colleagues pointed out the SError (interrupts) early in the
> kernel could just show because they just got unmasked when dropping into
> EL1. And indeed in AArch64 U-Boot we keep Aborts masked - we don't clear
> the A bit in DAIF normally (only for Freescale).
> So allowing SError exceptions in U-Boot's start.S revealed that the
> SError interrupt was actually triggered by the writel in write_hwaddr(),
> I guess because the MAC wasn't reset before. And the SError condition
> stayed pending all the time, until the kernel announced its interest in
> being told about fatal errors - then it inherited U-Boot's error.
> 

Thanks for the explanation. I think the situation leaves space for improving.
Either should we warn about a pending Abort before leaving U-Boot or we should
allow aborts in general.

> So for me the issue is fixed after adding the reset routine I sketched
> in that thread before.
> 
> But you mentioned that it still didn't work for you?
> 

I just double checked and everything works fine. Please feel free to send a new
version :)

Regards,
Matthias