Le 01/07/2016 11:02, Appana Durga Kedareswara Rao a écrit :
> Hi Nicolas Ferre,
>
> Thanks for the quick review...
>
>>
>> Le 01/07/2016 08:20, Kedareswara rao Appana a écrit :
>>> This patch adds support for gmii2rgmii phy converter in the macb
>>> driver.
>>
>> Okay, I'd like more explanation here.
>> Hints & key words:
>> - dt property
>> - mdio bus
>> - mac speed
>
> Sure will fix in the next version...
>
>>
>>
>>> Signed-off-by: Kedareswara rao Appana <appa...@xilinx.com>
>>> ---
>>> drivers/net/ethernet/cadence/macb.c | 21 ++++++++++++++++++++-
>>> drivers/net/ethernet/cadence/macb.h | 3 +++
>>> 2 files changed, 23 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.c
>>> b/drivers/net/ethernet/cadence/macb.c
>>> index cb07d95..de0ad71 100644
>>> --- a/drivers/net/ethernet/cadence/macb.c
>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>> @@ -257,6 +257,14 @@ static int macb_mdio_write(struct mii_bus *bus, int
>> mii_id, int regnum,
>>> return 0;
>>> }
>>>
>>> +static inline void macb_hw_fix_mac_speed(struct macb *bp,
>>> + struct phy_device *phydev)
>>> +{
>>> + if (likely(bp->converter_phy.fix_mac_speed))
>>
>> What is the purpose of this branch bias? The code isn't in some hot path, so
>> I
>> suspect that its not needed.
>
> If we won't put this check driver will crash with NULL pointer dereference
> for the below cases
I know that...
> ---> Converter driver is disabled
> ---> Converter driver is enabled but the converter probe is not called from
> the macb driver.
I didn't make myself clear: It's not the branch itself that I'm talking
about it's the branch profiling directive "likely()" that seems not
necessary.
>>> + bp->converter_phy.fix_mac_speed(&bp->converter_phy,
>>> + phydev->speed);
>>> +}
>>> +
>>> /**
>>> * macb_set_tx_clk() - Set a clock to a new frequency
>>> * @clk Pointer to the clock to change
>>> @@ -329,6 +337,7 @@ static void macb_handle_link_change(struct
>> net_device *dev)
>>> reg |= GEM_BIT(GBE);
>>>
>>> macb_or_gem_writel(bp, NCFGR, reg);
>>> + macb_hw_fix_mac_speed(bp, phydev);
>>>
>>> bp->speed = phydev->speed;
>>> bp->duplex = phydev->duplex;
>>> @@ -2884,7 +2893,7 @@ static int macb_probe(struct platform_device
>> *pdev)
>>> struct clk **, struct clk **)
>>> = macb_clk_init;
>>> int (*init)(struct platform_device *) = macb_init;
>>> - struct device_node *np = pdev->dev.of_node;
>>> + struct device_node *np = pdev->dev.of_node, *np1, *np11;
>>
>> Nitpicking:
>> Be more explicit on variable names. Simple name for the iterator is okay, the
>> other is better if changed.
>> I also like to see variable on separated lines.
>
> Ok sure will fix in the next version...
>
>>
>>> struct device_node *phy_node;
>>> const struct macb_config *macb_config = NULL;
>>> struct clk *pclk, *hclk = NULL, *tx_clk = NULL; @@ -3011,6 +3020,16
>>> @@ static int macb_probe(struct platform_device *pdev)
>>> goto err_out_free_netdev;
>>>
>>> phydev = bp->phy_dev;
>>> + np1 = of_get_next_child(np, NULL);
>>> + for_each_child_of_node(np1, np11) {
>>> + if (of_device_is_compatible(np11, "xlnx,gmiitorgmii")) {
>>
>> This would definitively need documentation and at least link in the macb
>> binding
>> to show how this emulated phy connect to the mdio bus...
>>
>> I'm not able to judge if the node arrangement is okay: I let Florian tell
>> his view
>> on this...
>
> Ok will document in the macb binding doc.
>
>>
>>> + bp->converter_phy.dev = dev;
>>> + bp->converter_phy.mii_bus = bp->mii_bus;
>>> + bp->converter_phy.mdio_write = macb_mdio_write;
>>> + bp->converter_phy.platform_data = bp->pdev-
>>> dev.of_node;
>>> + gmii2rgmii_phyprobe(&bp->converter_phy);
>>> + }
>>> + }
>>>
>>> netif_carrier_off(dev);
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.h
>>> b/drivers/net/ethernet/cadence/macb.h
>>> index 8a13824..735bce2 100644
>>> --- a/drivers/net/ethernet/cadence/macb.h
>>> +++ b/drivers/net/ethernet/cadence/macb.h
>>> @@ -10,6 +10,8 @@
>>> #ifndef _MACB_H
>>> #define _MACB_H
>>>
>>> +#include <linux/xilinx_gmii2rgmii.h>
>>
>> No, put it in the macb.c.
>
> Ok will fix in v2...
>
>>
>>> +
>>> #define MACB_GREGS_NBR 16
>>> #define MACB_GREGS_VERSION 2
>>> #define MACB_MAX_QUEUES 8
>>> @@ -846,6 +848,7 @@ struct macb {
>>> unsigned int jumbo_max_len;
>>>
>>> u32 wol;
>>> + struct gmii2rgmii converter_phy;
>>> };
>>>
>>> static inline bool macb_is_gem(struct macb *bp)
>>
>>
>> If Florian and phy guys are okay with the approach, I'm fine with this
>> patch, once
>> corrected.
>
> Ok thanks...
>
> Regards,
> Kedar.
>
>>
>> Thanks, bye,
>> --
>> Nicolas Ferre
>
--
Nicolas Ferre
