[PATCH] net: phy: dp83867: Do not check sgmii if rgmii is already used

[Michal Simek]

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 at 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