| Message ID | 8ec19c20bf163a3734e6080179c1d4ca434d2b45.1581075110.git.michal.simek@xilinx.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | net: phy: dp83867: Do not check sgmii if rgmii is already used | expand |
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.simek@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.
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.simek@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? Thanks, Michal
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.simek@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 - code readability
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.simek@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. Can you please sync it if you want to keep it in the same was as is done in Linux? > - 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. Thanks, Michal
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.simek@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. > >> - 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.
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.simek@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. Thanks, Michal
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.simek@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.
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.simek@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
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));
There is no reason to check sgmii branch again when it is clear that phy interface is rgmii. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/net/phy/dp83867.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)