From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grygorii Strashko Date: Wed, 19 Feb 2020 18:36:37 +0200 Subject: [PATCH] net: phy: dp83867: Do not check sgmii if rgmii is already used In-Reply-To: <680217c9-d470-b2b3-954e-aae296a555a0@xilinx.com> References: <8ec19c20bf163a3734e6080179c1d4ca434d2b45.1581075110.git.michal.simek@xilinx.com> <5ecc9571-160f-a85d-8f17-67b78cc4ba9d@ti.com> <1e614e08-1633-b8b3-7305-4117ea5ee734@xilinx.com> <1694fd94-00fb-5372-6f0a-aab7292675b1@ti.com> <680217c9-d470-b2b3-954e-aae296a555a0@xilinx.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>> --- >>>>>>> >>>>>>> ??? 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. -- Best regards, grygorii