Re: [RFC PATCH 2/2] net: macb: Add gmii2rgmii phy converter support

[Nicolas Ferre]

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 <appanad@xxxxxxxxxx>
>>> ---
>>>  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