On 19. 02. 20 17:36, Grygorii Strashko wrote: > > > On 13/02/2020 18:05, Michal Simek wrote: >> On 13. 02. 20 16:49, Grygorii Strashko wrote: >>> >>> >>> On 13/02/2020 08:23, Michal Simek wrote: >>>> On 12. 02. 20 21:24, Grygorii Strashko wrote: >>>>> >>>>> >>>>> On 11/02/2020 10:11, Michal Simek wrote: >>>>>> On 10. 02. 20 13:07, Grygorii Strashko wrote: >>>>>>> >>>>>>> >>>>>>> On 07/02/2020 13:31, Michal Simek wrote: >>>>>>>> There is no reason to check sgmii branch again when it is clear >>>>>>>> that >>>>>>>> phy >>>>>>>> interface is rgmii. >>>>>>>> >>>>>>>> Signed-off-by: Michal Simek <michal.si...@xilinx.com> >>>>>>>> --- >>>>>>>> >>>>>>>> drivers/net/phy/dp83867.c | 4 +--- >>>>>>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c >>>>>>>> index 4d796e289c45..3178787ff1c7 100644 >>>>>>>> --- a/drivers/net/phy/dp83867.c >>>>>>>> +++ b/drivers/net/phy/dp83867.c >>>>>>>> @@ -327,9 +327,7 @@ static int dp83867_config(struct phy_device >>>>>>>> *phydev) >>>>>>>> phy_write_mmd(phydev, DP83867_DEVADDR, >>>>>>>> DP83867_RGMIIDCTL, delay); >>>>>>>> - } >>>>>>>> - >>>>>>>> - if (phy_interface_is_sgmii(phydev)) { >>>>>>>> + } else if (phy_interface_is_sgmii(phydev)) { >>>>>>>> phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, >>>>>>>> (BMCR_ANENABLE | BMCR_FULLDPLX | >>>>>>>> BMCR_SPEED1000)); >>>>>>>> >>>>>>> >>>>>>> From one side I have no objections, but from another - I'd >>>>>>> prefer to >>>>>>> keep as is. >>>>>> >>>>>> Can you please be elaborate on this one more? >>>>> >>>>> - keep the same way as in the Kernel >>>> >>>> If kernel does it in the same way it should be also fixed. >>>> >>>> I have been checking yesterday dt binding docs in u-boot and in Linux >>>> and surprisingly they are different. >>>> >>>> ti,dp83867-rxctrl-strap-quirk is supported in u-boot but not described >>>> >>>> ti,clk-output-sel is supported but even in code is said that it is >>>> optional property. >>>> >>>> ti,min-output-impedance, ti,max-output-impedance and ti,fifo-depth are >>>> not documented in dt binding doc >>>> >>>> ti,sgmii-ref-clock-output-enable is not supported in u-boot but it >>>> is in >>>> Linux and we are using this feature. >>>> >>> >>> My understanding is that u-boot goal is to have uboot-dt == kernel-dt >>> and current >>> approach add DT+bindings to the kernel first. >>> >>> So, if you check most of u-boot bindings are missed or obsolete. >>> >>>> Can you please sync it if you want to keep it in the same was as is >>>> done >>>> in Linux? >>> >>> So, may be it can be just deleted. >> >> I am ok with that. >> >>> >>>> >>>>> - code readability >>>> >>>> I don't think this is really changing code readability. For improving >>>> readability would be the best to move bodies of these ifs to separate >>>> functions and not have dp83867_config() ~140 lines long. >>> >>> It really too minor change to fight for. But if you wish you can update >>> kernel >>> driver as per-above your proposal and then port it in u-boot. >> >> I was looking at latest kernel code and it is designed differently there. >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/net/phy/dp83867.c?h=next-20200213#n479 >> >> >> >> there is >> if rgmii or sgmii >> if rgmii >> io impedance >> if sgmii >> >> in u-boot you have >> if rgmii >> if sgmii >> io impedance >> >> I am ok with having this in sync but that's not what we have today. > > Yes. Unfortunately, The kernel moves forward faster than u-boot and > people not always > interesting to port their patches in u-boot :( > Plus, not all functionality, enabled in the kernel, is really required > by u-boot.
Anyway I have sent the patch for sgmii-ref-clock. If you can review that will be great. And if you want to sync it later I am fine with it. Thanks, Michal