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
---> Converter driver is disabled
---> Converter driver is enabled but the converter probe is not called from the
macb driver.
>
> > + 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
