| Message ID | 45dc876980d7537694d4471aae3c0bad62d287ce.1637237351.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | net: zynq: Add support for PHY configuration in SGMII mode | expand |
On 11/18/21 7:09 AM, Michal Simek wrote:> SGMII configuration depends on proper GT setting that's why when node has > phys property call PSGTR driver to configure it properly. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > drivers/net/zynq_gem.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c > index 5cbe8d28304b..fece077066df 100644 > --- a/drivers/net/zynq_gem.c > +++ b/drivers/net/zynq_gem.c > @@ -12,6 +12,7 @@ > #include <common.h> > #include <cpu_func.h> > #include <dm.h> > +#include <generic-phy.h> > #include <log.h> > #include <net.h> > #include <netdev.h> > @@ -716,6 +717,21 @@ static int zynq_gem_probe(struct udevice *dev) > struct zynq_gem_priv *priv = dev_get_priv(dev); > int ret; > > + if (priv->interface == PHY_INTERFACE_MODE_SGMII) { > + struct phy phy; > + > + ret = generic_phy_get_by_index(dev, 0, &phy); You should check the return value here. See f56db163ad ("usb: host: xhci-dwc3: Add generic PHY support") for an example. > + if (!ret) { > + ret = generic_phy_init(&phy); > + if (ret) > + return ret; > + > + ret = generic_phy_power_on(&phy); > + if (ret) > + return ret; > + } > + } > + > ret = zynq_gem_reset_init(dev); > if (ret) > return ret; > Otherwise LGTM
On Fri, Nov 19, 2021 at 7:01 PM Sean Anderson <sean.anderson@seco.com> wrote: > > > > On 11/18/21 7:09 AM, Michal Simek wrote:> SGMII configuration depends on proper GT setting that's why when node has > > phys property call PSGTR driver to configure it properly. > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > --- > > > > drivers/net/zynq_gem.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c > > index 5cbe8d28304b..fece077066df 100644 > > --- a/drivers/net/zynq_gem.c > > +++ b/drivers/net/zynq_gem.c > > @@ -12,6 +12,7 @@ > > #include <common.h> > > #include <cpu_func.h> > > #include <dm.h> > > +#include <generic-phy.h> > > #include <log.h> > > #include <net.h> > > #include <netdev.h> > > @@ -716,6 +717,21 @@ static int zynq_gem_probe(struct udevice *dev) > > struct zynq_gem_priv *priv = dev_get_priv(dev); > > int ret; > > > > + if (priv->interface == PHY_INTERFACE_MODE_SGMII) { > > + struct phy phy; > > + > > + ret = generic_phy_get_by_index(dev, 0, &phy); > > You should check the return value here. See f56db163ad ("usb: host: > xhci-dwc3: Add generic PHY support") for an example. He does check, he just doesn't print an error message. what am I missing here ? > > > + if (!ret) { > > + ret = generic_phy_init(&phy); > > + if (ret) > > + return ret; > > + > > + ret = generic_phy_power_on(&phy); > > + if (ret) > > + return ret; > > + } > > + } > > + > > ret = zynq_gem_reset_init(dev); > > if (ret) > > return ret; > > > > Otherwise LGTM
On 11/21/21 2:16 PM, Ramon Fried wrote: > On Fri, Nov 19, 2021 at 7:01 PM Sean Anderson <sean.anderson@seco.com> wrote: >> >> >> >> On 11/18/21 7:09 AM, Michal Simek wrote:> SGMII configuration depends on proper GT setting that's why when node has >>> phys property call PSGTR driver to configure it properly. >>> >>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>> --- >>> >>> drivers/net/zynq_gem.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c >>> index 5cbe8d28304b..fece077066df 100644 >>> --- a/drivers/net/zynq_gem.c >>> +++ b/drivers/net/zynq_gem.c >>> @@ -12,6 +12,7 @@ >>> #include <common.h> >>> #include <cpu_func.h> >>> #include <dm.h> >>> +#include <generic-phy.h> >>> #include <log.h> >>> #include <net.h> >>> #include <netdev.h> >>> @@ -716,6 +717,21 @@ static int zynq_gem_probe(struct udevice *dev) >>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>> int ret; >>> >>> + if (priv->interface == PHY_INTERFACE_MODE_SGMII) { >>> + struct phy phy; >>> + >>> + ret = generic_phy_get_by_index(dev, 0, &phy); >> >> You should check the return value here. See f56db163ad ("usb: host: >> xhci-dwc3: Add generic PHY support") for an example. > He does check, he just doesn't print an error message. what am I missing here ? The correct behavior here is something like ret = generic_phy_get_by_index(dev, 0, &phy); if (!ret) { ... } else if (ret != -ENOENT) { dev_dbg(dev, "could not get phy (err %d)\n", ret); return ret; } which will give an error message if e.g. the reference is malformed, or the phy driver isn't compiled in --Sean >>> + if (!ret) { >>> + ret = generic_phy_init(&phy); >>> + if (ret) >>> + return ret; >>> + >>> + ret = generic_phy_power_on(&phy); >>> + if (ret) >>> + return ret; >>> + } >>> + } >>> + >>> ret = zynq_gem_reset_init(dev); >>> if (ret) >>> return ret; >>> >> >> Otherwise LGTM
On 11/21/21 21:58, Sean Anderson wrote: > On 11/21/21 2:16 PM, Ramon Fried wrote: >> On Fri, Nov 19, 2021 at 7:01 PM Sean Anderson <sean.anderson@seco.com> wrote: >>> >>> >>> >>> On 11/18/21 7:09 AM, Michal Simek wrote:> SGMII configuration depends on >>> proper GT setting that's why when node has >>>> phys property call PSGTR driver to configure it properly. >>>> >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>> --- >>>> >>>> drivers/net/zynq_gem.c | 16 ++++++++++++++++ >>>> 1 file changed, 16 insertions(+) >>>> >>>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c >>>> index 5cbe8d28304b..fece077066df 100644 >>>> --- a/drivers/net/zynq_gem.c >>>> +++ b/drivers/net/zynq_gem.c >>>> @@ -12,6 +12,7 @@ >>>> #include <common.h> >>>> #include <cpu_func.h> >>>> #include <dm.h> >>>> +#include <generic-phy.h> >>>> #include <log.h> >>>> #include <net.h> >>>> #include <netdev.h> >>>> @@ -716,6 +717,21 @@ static int zynq_gem_probe(struct udevice *dev) >>>> struct zynq_gem_priv *priv = dev_get_priv(dev); >>>> int ret; >>>> >>>> + if (priv->interface == PHY_INTERFACE_MODE_SGMII) { >>>> + struct phy phy; >>>> + >>>> + ret = generic_phy_get_by_index(dev, 0, &phy); >>> >>> You should check the return value here. See f56db163ad ("usb: host: >>> xhci-dwc3: Add generic PHY support") for an example. >> He does check, he just doesn't print an error message. what am I missing here ? > > The correct behavior here is something like > > ret = generic_phy_get_by_index(dev, 0, &phy); > if (!ret) { > ... > } else if (ret != -ENOENT) { > dev_dbg(dev, "could not get phy (err %d)\n", ret); > return ret; > } > > which will give an error message if e.g. the reference is malformed, or the phy > driver isn't compiled in v2 sent with this added. Also I have tested it on real board. Thanks, Michal
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index 5cbe8d28304b..fece077066df 100644 --- a/drivers/net/zynq_gem.c +++ b/drivers/net/zynq_gem.c @@ -12,6 +12,7 @@ #include <common.h> #include <cpu_func.h> #include <dm.h> +#include <generic-phy.h> #include <log.h> #include <net.h> #include <netdev.h> @@ -716,6 +717,21 @@ static int zynq_gem_probe(struct udevice *dev) struct zynq_gem_priv *priv = dev_get_priv(dev); int ret; + if (priv->interface == PHY_INTERFACE_MODE_SGMII) { + struct phy phy; + + ret = generic_phy_get_by_index(dev, 0, &phy); + if (!ret) { + ret = generic_phy_init(&phy); + if (ret) + return ret; + + ret = generic_phy_power_on(&phy); + if (ret) + return ret; + } + } + ret = zynq_gem_reset_init(dev); if (ret) return ret;
SGMII configuration depends on proper GT setting that's why when node has phys property call PSGTR driver to configure it properly. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/net/zynq_gem.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)