| Message ID | 20170606123529.8852-1-christian.gmeiner@gmail.com |
|---|---|
| State | Accepted |
| Commit | 8f0b169382734d0f1f7bf89ec1e686e51a75cd67 |
| Delegated to: | Joe Hershberger |
| Headers | show |
"U-Boot" <u-boot-bounces@lists.denx.de> schrieb am 06.06.2017 14:35:29: > Von: Christian Gmeiner <christian.gmeiner@gmail.com> > An: u-boot@lists.denx.de, > Kopie: joe.hershberger@ni.com, oe5hpm@oevsv.at > Datum: 06.06.2017 14:35 > Betreff: [U-Boot] [PATCH] drivers/net/phy/fixed: do not overwrite addr > Gesendet von: "U-Boot" <u-boot-bounces@lists.denx.de> > > phy_device_create(..) sets the addr of phy_device with a sane value. > There is no need overwrite it. Hi Christian, The addr=0 was not an accident. Since there is no real phy in this case outside, i did set the address to zero (no real phy has address #0), later on in the board_phy_config(...) i look at the phydev->addr for detecting that i deal with the "fixed-link" driver. But for sure there are better ways to handle this, any suggestion ? cheers, Hannes > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > --- > drivers/net/phy/fixed.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c > index df82356..e8e9099 100644 > --- a/drivers/net/phy/fixed.c > +++ b/drivers/net/phy/fixed.c > @@ -34,7 +34,6 @@ int fixedphy_probe(struct phy_device *phydev) > memset(priv, 0, sizeof(*priv)); > > phydev->priv = priv; > - phydev->addr = 0; > > priv->link_speed = val; > priv->duplex = fdtdec_get_bool(gd->fdt_blob, ofnode, "full-duplex"); > -- > 2.9.4 >
2017-06-06 14:51 GMT+02:00 Hannes Schmelzer <Hannes.Schmelzer@br-automation.com>: > "U-Boot" <u-boot-bounces@lists.denx.de> schrieb am 06.06.2017 14:35:29: > >> Von: Christian Gmeiner <christian.gmeiner@gmail.com> >> An: u-boot@lists.denx.de, >> Kopie: joe.hershberger@ni.com, oe5hpm@oevsv.at >> Datum: 06.06.2017 14:35 >> Betreff: [U-Boot] [PATCH] drivers/net/phy/fixed: do not overwrite addr >> Gesendet von: "U-Boot" <u-boot-bounces@lists.denx.de> >> >> phy_device_create(..) sets the addr of phy_device with a sane value. >> There is no need overwrite it. > > Hi Christian, > > The addr=0 was not an accident. > Since there is no real phy in this case outside, i did set the address to > zero (no real phy has address #0), > later on in the board_phy_config(...) i look at the phydev->addr for > detecting that i deal with the "fixed-link" driver. > > But for sure there are better ways to handle this, any suggestion ? > I have a similar use case where I have one variant of a device with fixed-link and an other variant with a 'real' phy. Here is how I handle that case: -------8<------ int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) { int phy_reg; /* MR variant has a fixed phy at address 5 */ if (addr == 5) { *phy_id = PHY_FIXED_ID; return 0; } /* Grab the bits from PHYIR1, and put them * in the upper half */ phy_reg = bus->read(bus, addr, devad, MII_PHYSID1); if (phy_reg < 0) return -EIO; *phy_id = (phy_reg & 0xffff) << 16; /* Grab the bits from PHYIR2, and put them in the lower half */ phy_reg = bus->read(bus, addr, devad, MII_PHYSID2); if (phy_reg < 0) return -EIO; *phy_id |= (phy_reg & 0xffff); return 0; } int board_eth_init(bd_t *bis) { uint32_t base = IMX_FEC_BASE; struct mii_dev *bus = NULL; struct phy_device *phydev = NULL; int ret; setup_iomux_enet(); bus = fec_get_miibus(base, -1); if (!bus) return -EINVAL; /* scan phy 0 and 5 */ phydev = phy_find_by_mask(bus, 0x21, PHY_INTERFACE_MODE_RGMII); if (!phydev) { ret = -EINVAL; goto free_bus; } /* depending on the phy address we can detect our board version */ if (phydev->addr == 0) setenv("boardver", ""); else setenv("boardver", "mr"); ret = fec_probe(bis, -1, base, bus, phydev); if (ret) goto free_phydev; return 0; free_phydev: free(phydev); free_bus: free(bus); return ret; } -------8<------ As you can see I use phydev->addr to detect my board variant and with this patch everything works as expected. greets -- Christian Gmeiner, MSc https://www.youtube.com/user/AloryOFFICIAL https://soundcloud.com/christian-gmeiner
On 06/06/2017 02:57 PM, Christian Gmeiner wrote: > 2017-06-06 14:51 GMT+02:00 Hannes Schmelzer > <Hannes.Schmelzer@br-automation.com>: >> "U-Boot" <u-boot-bounces@lists.denx.de> schrieb am 06.06.2017 14:35:29: >> >>> Von: Christian Gmeiner <christian.gmeiner@gmail.com> >>> An: u-boot@lists.denx.de, >>> Kopie: joe.hershberger@ni.com, oe5hpm@oevsv.at >>> Datum: 06.06.2017 14:35 >>> Betreff: [U-Boot] [PATCH] drivers/net/phy/fixed: do not overwrite addr >>> Gesendet von: "U-Boot" <u-boot-bounces@lists.denx.de> >>> >>> phy_device_create(..) sets the addr of phy_device with a sane value. >>> There is no need overwrite it. >> Hi Christian, >> >> The addr=0 was not an accident. >> Since there is no real phy in this case outside, i did set the address to >> zero (no real phy has address #0), >> later on in the board_phy_config(...) i look at the phydev->addr for >> detecting that i deal with the "fixed-link" driver. >> >> But for sure there are better ways to handle this, any suggestion ? >> > I have a similar use case where I have one variant of a device with > fixed-link and an > other variant with a 'real' phy. > > Here is how I handle that case: > > -------8<------ > > int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) > { > int phy_reg; > > /* MR variant has a fixed phy at address 5 */ > if (addr == 5) { > *phy_id = PHY_FIXED_ID; > return 0; > } > > /* Grab the bits from PHYIR1, and put them > * in the upper half */ > phy_reg = bus->read(bus, addr, devad, MII_PHYSID1); > > if (phy_reg < 0) > return -EIO; > > *phy_id = (phy_reg & 0xffff) << 16; > > /* Grab the bits from PHYIR2, and put them in the lower half */ > phy_reg = bus->read(bus, addr, devad, MII_PHYSID2); > > if (phy_reg < 0) > return -EIO; > > *phy_id |= (phy_reg & 0xffff); > > return 0; > } > > int board_eth_init(bd_t *bis) > { > uint32_t base = IMX_FEC_BASE; > struct mii_dev *bus = NULL; > struct phy_device *phydev = NULL; > int ret; > > setup_iomux_enet(); > > bus = fec_get_miibus(base, -1); > if (!bus) > return -EINVAL; > > /* scan phy 0 and 5 */ > phydev = phy_find_by_mask(bus, 0x21, PHY_INTERFACE_MODE_RGMII); > if (!phydev) { > ret = -EINVAL; > goto free_bus; > } > > /* depending on the phy address we can detect our board version */ > if (phydev->addr == 0) > setenv("boardver", ""); > else > setenv("boardver", "mr"); > > ret = fec_probe(bis, -1, base, bus, phydev); > if (ret) > goto free_phydev; > > return 0; > > free_phydev: > free(phydev); > free_bus: > free(bus); > return ret; > } > > -------8<------ > > As you can see I use phydev->addr to detect my board variant and with this patch > everything works as expected. Hi Christian, yes doing that stuff using the phy-id is much more elegant. I will adapt my board-code doing so. Reviewed-by: Hannes Schmelzer <oe5hpm@oevsv.at> Tested-by: Hannes Schmelzer <oe5hpm@oevsv.at> > greets > -- > Christian Gmeiner, MSc > > https://www.youtube.com/user/AloryOFFICIAL > https://soundcloud.com/christian-gmeiner cheers, Hannes
2017-06-07 6:33 GMT+02:00 Hannes Schmelzer <hannes@schmelzer.or.at>: > On 06/06/2017 02:57 PM, Christian Gmeiner wrote: >> >> 2017-06-06 14:51 GMT+02:00 Hannes Schmelzer >> <Hannes.Schmelzer@br-automation.com>: >>> >>> "U-Boot" <u-boot-bounces@lists.denx.de> schrieb am 06.06.2017 14:35:29: >>> >>>> Von: Christian Gmeiner <christian.gmeiner@gmail.com> >>>> An: u-boot@lists.denx.de, >>>> Kopie: joe.hershberger@ni.com, oe5hpm@oevsv.at >>>> Datum: 06.06.2017 14:35 >>>> Betreff: [U-Boot] [PATCH] drivers/net/phy/fixed: do not overwrite addr >>>> Gesendet von: "U-Boot" <u-boot-bounces@lists.denx.de> >>>> >>>> phy_device_create(..) sets the addr of phy_device with a sane value. >>>> There is no need overwrite it. >>> >>> Hi Christian, >>> >>> The addr=0 was not an accident. >>> Since there is no real phy in this case outside, i did set the address to >>> zero (no real phy has address #0), >>> later on in the board_phy_config(...) i look at the phydev->addr for >>> detecting that i deal with the "fixed-link" driver. >>> >>> But for sure there are better ways to handle this, any suggestion ? >>> >> I have a similar use case where I have one variant of a device with >> fixed-link and an >> other variant with a 'real' phy. >> >> Here is how I handle that case: >> >> -------8<------ >> >> int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) >> { >> int phy_reg; >> >> /* MR variant has a fixed phy at address 5 */ >> if (addr == 5) { >> *phy_id = PHY_FIXED_ID; >> return 0; >> } >> >> /* Grab the bits from PHYIR1, and put them >> * in the upper half */ >> phy_reg = bus->read(bus, addr, devad, MII_PHYSID1); >> >> if (phy_reg < 0) >> return -EIO; >> >> *phy_id = (phy_reg & 0xffff) << 16; >> >> /* Grab the bits from PHYIR2, and put them in the lower half */ >> phy_reg = bus->read(bus, addr, devad, MII_PHYSID2); >> >> if (phy_reg < 0) >> return -EIO; >> >> *phy_id |= (phy_reg & 0xffff); >> >> return 0; >> } >> >> int board_eth_init(bd_t *bis) >> { >> uint32_t base = IMX_FEC_BASE; >> struct mii_dev *bus = NULL; >> struct phy_device *phydev = NULL; >> int ret; >> >> setup_iomux_enet(); >> >> bus = fec_get_miibus(base, -1); >> if (!bus) >> return -EINVAL; >> >> /* scan phy 0 and 5 */ >> phydev = phy_find_by_mask(bus, 0x21, PHY_INTERFACE_MODE_RGMII); >> if (!phydev) { >> ret = -EINVAL; >> goto free_bus; >> } >> >> /* depending on the phy address we can detect our board version */ >> if (phydev->addr == 0) >> setenv("boardver", ""); >> else >> setenv("boardver", "mr"); >> >> ret = fec_probe(bis, -1, base, bus, phydev); >> if (ret) >> goto free_phydev; >> >> return 0; >> >> free_phydev: >> free(phydev); >> free_bus: >> free(bus); >> return ret; >> } >> >> -------8<------ >> >> As you can see I use phydev->addr to detect my board variant and with this >> patch >> everything works as expected. > > Hi Christian, > > yes doing that stuff using the phy-id is much more elegant. > I will adapt my board-code doing so. > > Reviewed-by: Hannes Schmelzer <oe5hpm@oevsv.at> > Tested-by: Hannes Schmelzer <oe5hpm@oevsv.at> >> >> greets >> -- >> Christian Gmeiner, MSc >> >> https://www.youtube.com/user/AloryOFFICIAL >> https://soundcloud.com/christian-gmeiner > > cheers, > Hannes > gentle ping greets -- Christian Gmeiner, MSc https://www.youtube.com/user/AloryOFFICIAL https://soundcloud.com/christian-gmeiner
On Tue, Jun 6, 2017 at 7:35 AM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > phy_device_create(..) sets the addr of phy_device with a sane value. > There is no need overwrite it. > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Hi Christian, https://patchwork.ozlabs.org/patch/771848/ was applied to u-boot-net.git. Thanks! -Joe
diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c index df82356..e8e9099 100644 --- a/drivers/net/phy/fixed.c +++ b/drivers/net/phy/fixed.c @@ -34,7 +34,6 @@ int fixedphy_probe(struct phy_device *phydev) memset(priv, 0, sizeof(*priv)); phydev->priv = priv; - phydev->addr = 0; priv->link_speed = val; priv->duplex = fdtdec_get_bool(gd->fdt_blob, ofnode, "full-duplex");
phy_device_create(..) sets the addr of phy_device with a sane value. There is no need overwrite it. Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> --- drivers/net/phy/fixed.c | 1 - 1 file changed, 1 deletion(-)