Hi Marek,
Thanks for the feedback.
Subject: Re: [PATCH] net: ravb: Fix NULL pointer access
On 9/18/20 5:26 PM, Biju Das wrote: [...]
+++ b/drivers/net/ravb.c @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev)
writel(mask, eth->iobase + RAVB_REG_ECMR);
-phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); +if (phy->drv->writeext) +phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19);
Shouldn't we rather move this into the PHY driver altogether ?
If I fix, the phydriver with empty function, compiler will complain about
unused parameter right?
What about other phy's which doesn't have this callback.
I mean, should we not move this entire code which configures something in a Micrel PHY, and is specific to Micrel PHY, into the Micrel PHY driver and remove the whole call of writeext from this
ethernet driver ?
Ok , this function is invoked during data transfer.
The ravb_config() is invoked when starting the interface, see ravb_start(), it is not repeatedly called during data transfer.
Yep you are correct , it will be called during starting of the interface. I have seen the crash when I started doing tftp or ping, which in turn calls ravb_start.
We could remove this function and test on all RCar boards to see any issues
present or not?
No, because calling the writeext configures something in the Micrel KSZ90x1 PHY, and by removing it, I suspect the Micrel PHY would stop working.
I agree, otherwise it will use default values which may cause issues.
By looking at [1], only this driver is using writeext. [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext
git grep indicates a couple more sites where the writeext is called. But look into the KSZ9031 datasheet, that particular writeext call seems to be setting up RGMII Clock Pad Skew (MMD Address 2, Register 8), and I think there is a matching DT binding to set those up too, rxc-skew-ps and txc- skew-ps I think.
Thanks for the pointers. I checked the configs[2] which uses renesas ravb driver and found that we are defining only rxc-skew-ps in all dts.
since CONFIG DM_ETH is defined it is already picking the value corresponding to "rxc-skew-ps".
For txc-skew-ps anyway the value is default one. So we don't care.
So I think if you set those two DT properties in rcar2/3 DTs accordingly (and that might already be the case, but please double-check), you can even drop this entire writeext altogether.
I did a grep and the below configs enables renesas ravb driver .Only Gen3 uses renesas ravb driver and all of them have "rxc-skew-ps" in Devicetree. So I agree with you, it is safe to remove writephyext. I will post V2, removing this
[2] u-boot/configs/rcar3_ulcb_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a77990_ebisu_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a774a1_beacon_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/hihope_rzg2_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a77995_draak_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a77970_eagle_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/rcar3_salvator-x_defconfig:CONFIG_RENESAS_RAVB=y
Cheers, Biju
Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647