| Message ID | 1517579628-8607-1-git-send-email-stefan@olimex.com |
|---|---|
| State | Accepted |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,v2,1/3] Move CONFIG_PHY_ADDR to Kconfig | expand |
Hi, > From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Stefan > Mavrodiev > Sent: Friday, February 2, 2018 7:24 PM <snip> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index > 95b7534..c934aed 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -12,6 +12,13 @@ menuconfig PHYLIB > > if PHYLIB > > +config PHY_ADDR > + int "PHY address" > + default 1 if ARCH_SUNXI > + default 0 > + help > + The address of PHY on MII bus. Usually in range of 0 to 31. Isn't board Kconfig, the right place to keep this? Regards Calvin
> Hi, > > > From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Stefan > > Mavrodiev > > Sent: Friday, February 2, 2018 7:24 PM > > <snip> > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index > > 95b7534..c934aed 100644 > > --- a/drivers/net/phy/Kconfig > > +++ b/drivers/net/phy/Kconfig > > @@ -12,6 +12,13 @@ menuconfig PHYLIB > > > > if PHYLIB > > > > +config PHY_ADDR > > + int "PHY address" > > + default 1 if ARCH_SUNXI Sorry, I meant the default value can be defined in board/sunxi/Kconfig. Remaining definition of PHY_ADDR config can be here. > > + default 0 > > + help > > + The address of PHY on MII bus. Usually in range of 0 to 31. > > Isn't board Kconfig, the right place to keep this?
On Sat, Feb 03, 2018 at 04:37:15AM +0000, Calvin Johnson wrote: > > Hi, > > > > > From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Stefan > > > Mavrodiev > > > Sent: Friday, February 2, 2018 7:24 PM > > > > <snip> > > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index > > > 95b7534..c934aed 100644 > > > --- a/drivers/net/phy/Kconfig > > > +++ b/drivers/net/phy/Kconfig > > > @@ -12,6 +12,13 @@ menuconfig PHYLIB > > > > > > if PHYLIB > > > > > > +config PHY_ADDR > > > + int "PHY address" > > > + default 1 if ARCH_SUNXI > > Sorry, I meant the default value can be defined in board/sunxi/Kconfig. > Remaining definition of PHY_ADDR config can be here. Tom has asked a few times not to do this but to put the defaults where the Kconfig option is defined. I'm not sure I remember the details though. Maxime
On Mon, Feb 05, 2018 at 09:07:40AM +0100, Maxime Ripard wrote: > On Sat, Feb 03, 2018 at 04:37:15AM +0000, Calvin Johnson wrote: > > > Hi, > > > > > > > From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Stefan > > > > Mavrodiev > > > > Sent: Friday, February 2, 2018 7:24 PM > > > > > > <snip> > > > > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index > > > > 95b7534..c934aed 100644 > > > > --- a/drivers/net/phy/Kconfig > > > > +++ b/drivers/net/phy/Kconfig > > > > @@ -12,6 +12,13 @@ menuconfig PHYLIB > > > > > > > > if PHYLIB > > > > > > > > +config PHY_ADDR > > > > + int "PHY address" > > > > + default 1 if ARCH_SUNXI > > > > Sorry, I meant the default value can be defined in board/sunxi/Kconfig. > > Remaining definition of PHY_ADDR config can be here. > > Tom has asked a few times not to do this but to put the defaults where > the Kconfig option is defined. I'm not sure I remember the details > though. For now, yes, the preferred way of dealing with this is 'default FOO if BAR' in the main Kconfig entry line. What I wish for long term is for 'imply' to get extended to support 'imply FOO BAR' but I've not had more to add there other than "Gee, this would make life easier".
Hi, > -----Original Message----- > From: Tom Rini [mailto:trini@konsulko.com] > > On Mon, Feb 05, 2018 at 09:07:40AM +0100, Maxime Ripard wrote: > > On Sat, Feb 03, 2018 at 04:37:15AM +0000, Calvin Johnson wrote: > > > > Hi, > > > > > > > > > From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of > > > > > Stefan Mavrodiev > > > > > Sent: Friday, February 2, 2018 7:24 PM > > > > > > > > <snip> > > > > > > > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > > > > > index 95b7534..c934aed 100644 > > > > > --- a/drivers/net/phy/Kconfig > > > > > +++ b/drivers/net/phy/Kconfig > > > > > @@ -12,6 +12,13 @@ menuconfig PHYLIB > > > > > > > > > > if PHYLIB > > > > > > > > > > +config PHY_ADDR > > > > > + int "PHY address" > > > > > + default 1 if ARCH_SUNXI > > > > > > Sorry, I meant the default value can be defined in board/sunxi/Kconfig. > > > Remaining definition of PHY_ADDR config can be here. > > > > Tom has asked a few times not to do this but to put the defaults where > > the Kconfig option is defined. I'm not sure I remember the details > > though. > > For now, yes, the preferred way of dealing with this is 'default FOO if BAR' in > the main Kconfig entry line. What I wish for long term is for 'imply' to get > extended to support 'imply FOO BAR' but I've not had more to add there > other than "Gee, this would make life easier". Got it. Thanks! Calvin
On Fri, Feb 2, 2018 at 7:23 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: > CONFIG_PHY_ADDR is used for old-style configuration. This makes > impossible changing the PHY address, if multiple boards share a same > config header file (for example include/configs/sunxi-common.h). > > Moving this to Kconfig helps overcoming this issue. It's defined > as entry inside PHYLIB section. > > After the implemention, moveconfig was run. The issues are: > - edb9315a - CONFIG_PHYLIB is not enabled. Entry is > deleted. > > - ds414 - CONFIG_PHYLIB is in incompatible format: > { 0x1, 0x0 }. This entry is also deleted. > > - devkit3250 - The PHY_ADDR is in hex format (0x1F). > Manually CONFIG_PHY_ADDR=31 is added in > the defconfig. > > After the changes the suspicious defconfigs passes building. > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- rebased on master and Applied to u-boot-sunxi/master
Hi, On Fri, Feb 2, 2018 at 9:53 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: > CONFIG_PHY_ADDR is used for old-style configuration. This makes > impossible changing the PHY address, if multiple boards share a same > config header file (for example include/configs/sunxi-common.h). > > Moving this to Kconfig helps overcoming this issue. It's defined > as entry inside PHYLIB section. > > After the implemention, moveconfig was run. The issues are: > - edb9315a - CONFIG_PHYLIB is not enabled. Entry is > deleted. > > - ds414 - CONFIG_PHYLIB is in incompatible format: > { 0x1, 0x0 }. This entry is also deleted. > > - devkit3250 - The PHY_ADDR is in hex format (0x1F). > Manually CONFIG_PHY_ADDR=31 is added in > the defconfig. > > After the changes the suspicious defconfigs passes building. > > Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > Changes for v2: > - Replaced CONFIG_SUNXI_PHY_ADDR with a common one > CONFIG_PHY_ADDR, using moveconfig. > > README | 4 ---- > configs/devkit3250_defconfig | 1 + > configs/khadas-vim_defconfig | 1 + > configs/libretech-cc_defconfig | 1 + > configs/p212_defconfig | 1 + > drivers/net/phy/Kconfig | 7 +++++++ > include/configs/am335x_shc.h | 1 - > include/configs/baltos.h | 1 - > include/configs/devkit3250.h | 1 - > include/configs/ds414.h | 1 - > include/configs/edb93xx.h | 1 - > include/configs/khadas-vim.h | 2 -- > include/configs/libretech-cc.h | 2 -- > include/configs/p212.h | 2 -- > include/configs/pepper.h | 1 - > include/configs/sunxi-common.h | 2 -- > include/configs/work_92105.h | 1 - > include/configs/x600.h | 1 - > scripts/config_whitelist.txt | 1 - > 19 files changed, 11 insertions(+), 21 deletions(-) > [snip] > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index 95b7534..c934aed 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -12,6 +12,13 @@ menuconfig PHYLIB > > if PHYLIB > > +config PHY_ADDR > + int "PHY address" > + default 1 if ARCH_SUNXI > + default 0 > + help > + The address of PHY on MII bus. Usually in range of 0 to 31. > + Sorry for jumping out so late, but this commit breaks Intel Galileo ethernet. Previously the board boots with the following log: Net: eth0: eth_designware#0, eth1: eth_designware#1 With this commit it becomes: Net: No ethernet found. The reason is that the board has two designware ethernet controllers, and PHY_ADDR has been set to zero for both. A simple fix is to: diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 43670a7..1394119 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -471,10 +471,6 @@ static int dw_phy_init(struct dw_eth_dev *priv, void *dev) struct phy_device *phydev; int mask = 0xffffffff, ret; -#ifdef CONFIG_PHY_ADDR - mask = 1 << CONFIG_PHY_ADDR; -#endif - phydev = phy_find_by_mask(priv->bus, mask, priv->interface); if (!phydev) return -ENODEV; But the real question is that: why do we introduce this PHY_ADDR Kconfig? It for sure won't work for multiple ethernet controllers.This should be eliminated IMHO. Comments? [snip] Regards, Bin
On Thu, Mar 22, 2018 at 9:46 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi, > > On Fri, Feb 2, 2018 at 9:53 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: >> CONFIG_PHY_ADDR is used for old-style configuration. This makes >> impossible changing the PHY address, if multiple boards share a same >> config header file (for example include/configs/sunxi-common.h). >> >> Moving this to Kconfig helps overcoming this issue. It's defined >> as entry inside PHYLIB section. >> >> After the implemention, moveconfig was run. The issues are: >> - edb9315a - CONFIG_PHYLIB is not enabled. Entry is >> deleted. >> >> - ds414 - CONFIG_PHYLIB is in incompatible format: >> { 0x1, 0x0 }. This entry is also deleted. >> >> - devkit3250 - The PHY_ADDR is in hex format (0x1F). >> Manually CONFIG_PHY_ADDR=31 is added in >> the defconfig. >> >> After the changes the suspicious defconfigs passes building. >> >> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> --- >> Changes for v2: >> - Replaced CONFIG_SUNXI_PHY_ADDR with a common one >> CONFIG_PHY_ADDR, using moveconfig. >> >> README | 4 ---- >> configs/devkit3250_defconfig | 1 + >> configs/khadas-vim_defconfig | 1 + >> configs/libretech-cc_defconfig | 1 + >> configs/p212_defconfig | 1 + >> drivers/net/phy/Kconfig | 7 +++++++ >> include/configs/am335x_shc.h | 1 - >> include/configs/baltos.h | 1 - >> include/configs/devkit3250.h | 1 - >> include/configs/ds414.h | 1 - >> include/configs/edb93xx.h | 1 - >> include/configs/khadas-vim.h | 2 -- >> include/configs/libretech-cc.h | 2 -- >> include/configs/p212.h | 2 -- >> include/configs/pepper.h | 1 - >> include/configs/sunxi-common.h | 2 -- >> include/configs/work_92105.h | 1 - >> include/configs/x600.h | 1 - >> scripts/config_whitelist.txt | 1 - >> 19 files changed, 11 insertions(+), 21 deletions(-) >> > > [snip] > >> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >> index 95b7534..c934aed 100644 >> --- a/drivers/net/phy/Kconfig >> +++ b/drivers/net/phy/Kconfig >> @@ -12,6 +12,13 @@ menuconfig PHYLIB >> >> if PHYLIB >> >> +config PHY_ADDR >> + int "PHY address" >> + default 1 if ARCH_SUNXI >> + default 0 >> + help >> + The address of PHY on MII bus. Usually in range of 0 to 31. >> + > > Sorry for jumping out so late, but this commit breaks Intel Galileo > ethernet. Previously the board boots with the following log: > > Net: eth0: eth_designware#0, eth1: eth_designware#1 > > With this commit it becomes: > > Net: No ethernet found. > > The reason is that the board has two designware ethernet controllers, > and PHY_ADDR has been set to zero for both. A simple fix is to: > > diff --git a/drivers/net/designware.c b/drivers/net/designware.c > index 43670a7..1394119 100644 > --- a/drivers/net/designware.c > +++ b/drivers/net/designware.c > @@ -471,10 +471,6 @@ static int dw_phy_init(struct dw_eth_dev *priv, void *dev) > struct phy_device *phydev; > int mask = 0xffffffff, ret; > > -#ifdef CONFIG_PHY_ADDR > - mask = 1 << CONFIG_PHY_ADDR; > -#endif > - > phydev = phy_find_by_mask(priv->bus, mask, priv->interface); > if (!phydev) > return -ENODEV; > > But the real question is that: why do we introduce this PHY_ADDR > Kconfig? It for sure won't work for multiple ethernet controllers.This > should be eliminated IMHO. Comments? This should be able to come from the device tree, ultimately. Can you undefine the phy addr for the Galileo board? > [snip] > > Regards, > Bin > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Joe, On Sat, Mar 24, 2018 at 1:11 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: > On Thu, Mar 22, 2018 at 9:46 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi, >> >> On Fri, Feb 2, 2018 at 9:53 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: >>> CONFIG_PHY_ADDR is used for old-style configuration. This makes >>> impossible changing the PHY address, if multiple boards share a same >>> config header file (for example include/configs/sunxi-common.h). >>> >>> Moving this to Kconfig helps overcoming this issue. It's defined >>> as entry inside PHYLIB section. >>> >>> After the implemention, moveconfig was run. The issues are: >>> - edb9315a - CONFIG_PHYLIB is not enabled. Entry is >>> deleted. >>> >>> - ds414 - CONFIG_PHYLIB is in incompatible format: >>> { 0x1, 0x0 }. This entry is also deleted. >>> >>> - devkit3250 - The PHY_ADDR is in hex format (0x1F). >>> Manually CONFIG_PHY_ADDR=31 is added in >>> the defconfig. >>> >>> After the changes the suspicious defconfigs passes building. >>> >>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>> --- >>> Changes for v2: >>> - Replaced CONFIG_SUNXI_PHY_ADDR with a common one >>> CONFIG_PHY_ADDR, using moveconfig. >>> >>> README | 4 ---- >>> configs/devkit3250_defconfig | 1 + >>> configs/khadas-vim_defconfig | 1 + >>> configs/libretech-cc_defconfig | 1 + >>> configs/p212_defconfig | 1 + >>> drivers/net/phy/Kconfig | 7 +++++++ >>> include/configs/am335x_shc.h | 1 - >>> include/configs/baltos.h | 1 - >>> include/configs/devkit3250.h | 1 - >>> include/configs/ds414.h | 1 - >>> include/configs/edb93xx.h | 1 - >>> include/configs/khadas-vim.h | 2 -- >>> include/configs/libretech-cc.h | 2 -- >>> include/configs/p212.h | 2 -- >>> include/configs/pepper.h | 1 - >>> include/configs/sunxi-common.h | 2 -- >>> include/configs/work_92105.h | 1 - >>> include/configs/x600.h | 1 - >>> scripts/config_whitelist.txt | 1 - >>> 19 files changed, 11 insertions(+), 21 deletions(-) >>> >> >> [snip] >> >>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >>> index 95b7534..c934aed 100644 >>> --- a/drivers/net/phy/Kconfig >>> +++ b/drivers/net/phy/Kconfig >>> @@ -12,6 +12,13 @@ menuconfig PHYLIB >>> >>> if PHYLIB >>> >>> +config PHY_ADDR >>> + int "PHY address" >>> + default 1 if ARCH_SUNXI >>> + default 0 >>> + help >>> + The address of PHY on MII bus. Usually in range of 0 to 31. >>> + >> >> Sorry for jumping out so late, but this commit breaks Intel Galileo >> ethernet. Previously the board boots with the following log: >> >> Net: eth0: eth_designware#0, eth1: eth_designware#1 >> >> With this commit it becomes: >> >> Net: No ethernet found. >> >> The reason is that the board has two designware ethernet controllers, >> and PHY_ADDR has been set to zero for both. A simple fix is to: >> >> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >> index 43670a7..1394119 100644 >> --- a/drivers/net/designware.c >> +++ b/drivers/net/designware.c >> @@ -471,10 +471,6 @@ static int dw_phy_init(struct dw_eth_dev *priv, void *dev) >> struct phy_device *phydev; >> int mask = 0xffffffff, ret; >> >> -#ifdef CONFIG_PHY_ADDR >> - mask = 1 << CONFIG_PHY_ADDR; >> -#endif >> - >> phydev = phy_find_by_mask(priv->bus, mask, priv->interface); >> if (!phydev) >> return -ENODEV; >> >> But the real question is that: why do we introduce this PHY_ADDR >> Kconfig? It for sure won't work for multiple ethernet controllers.This >> should be eliminated IMHO. Comments? > > This should be able to come from the device tree, ultimately. Can you > undefine the phy addr for the Galileo board? > >> [snip] >> #undf the PHY_ADDR in Galileo board looks weird. This to me is a workaround. Since the designware ethernet controller driver supports finding any PHY attached to its mdio bus, the changes suggested above can be a proper fix. Regards, Bin
On Sun, Mar 25, 2018 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Sat, Mar 24, 2018 at 1:11 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: >> On Thu, Mar 22, 2018 at 9:46 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi, >>> >>> On Fri, Feb 2, 2018 at 9:53 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: >>>> CONFIG_PHY_ADDR is used for old-style configuration. This makes >>>> impossible changing the PHY address, if multiple boards share a same >>>> config header file (for example include/configs/sunxi-common.h). >>>> >>>> Moving this to Kconfig helps overcoming this issue. It's defined >>>> as entry inside PHYLIB section. >>>> >>>> After the implemention, moveconfig was run. The issues are: >>>> - edb9315a - CONFIG_PHYLIB is not enabled. Entry is >>>> deleted. >>>> >>>> - ds414 - CONFIG_PHYLIB is in incompatible format: >>>> { 0x1, 0x0 }. This entry is also deleted. >>>> >>>> - devkit3250 - The PHY_ADDR is in hex format (0x1F). >>>> Manually CONFIG_PHY_ADDR=31 is added in >>>> the defconfig. >>>> >>>> After the changes the suspicious defconfigs passes building. >>>> >>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>> --- >>>> Changes for v2: >>>> - Replaced CONFIG_SUNXI_PHY_ADDR with a common one >>>> CONFIG_PHY_ADDR, using moveconfig. >>>> >>>> README | 4 ---- >>>> configs/devkit3250_defconfig | 1 + >>>> configs/khadas-vim_defconfig | 1 + >>>> configs/libretech-cc_defconfig | 1 + >>>> configs/p212_defconfig | 1 + >>>> drivers/net/phy/Kconfig | 7 +++++++ >>>> include/configs/am335x_shc.h | 1 - >>>> include/configs/baltos.h | 1 - >>>> include/configs/devkit3250.h | 1 - >>>> include/configs/ds414.h | 1 - >>>> include/configs/edb93xx.h | 1 - >>>> include/configs/khadas-vim.h | 2 -- >>>> include/configs/libretech-cc.h | 2 -- >>>> include/configs/p212.h | 2 -- >>>> include/configs/pepper.h | 1 - >>>> include/configs/sunxi-common.h | 2 -- >>>> include/configs/work_92105.h | 1 - >>>> include/configs/x600.h | 1 - >>>> scripts/config_whitelist.txt | 1 - >>>> 19 files changed, 11 insertions(+), 21 deletions(-) >>>> >>> >>> [snip] >>> >>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >>>> index 95b7534..c934aed 100644 >>>> --- a/drivers/net/phy/Kconfig >>>> +++ b/drivers/net/phy/Kconfig >>>> @@ -12,6 +12,13 @@ menuconfig PHYLIB >>>> >>>> if PHYLIB >>>> >>>> +config PHY_ADDR >>>> + int "PHY address" >>>> + default 1 if ARCH_SUNXI >>>> + default 0 >>>> + help >>>> + The address of PHY on MII bus. Usually in range of 0 to 31. >>>> + >>> >>> Sorry for jumping out so late, but this commit breaks Intel Galileo >>> ethernet. Previously the board boots with the following log: >>> >>> Net: eth0: eth_designware#0, eth1: eth_designware#1 >>> >>> With this commit it becomes: >>> >>> Net: No ethernet found. >>> >>> The reason is that the board has two designware ethernet controllers, >>> and PHY_ADDR has been set to zero for both. A simple fix is to: >>> >>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >>> index 43670a7..1394119 100644 >>> --- a/drivers/net/designware.c >>> +++ b/drivers/net/designware.c >>> @@ -471,10 +471,6 @@ static int dw_phy_init(struct dw_eth_dev *priv, void *dev) >>> struct phy_device *phydev; >>> int mask = 0xffffffff, ret; >>> >>> -#ifdef CONFIG_PHY_ADDR >>> - mask = 1 << CONFIG_PHY_ADDR; >>> -#endif >>> - >>> phydev = phy_find_by_mask(priv->bus, mask, priv->interface); >>> if (!phydev) >>> return -ENODEV; >>> >>> But the real question is that: why do we introduce this PHY_ADDR >>> Kconfig? It for sure won't work for multiple ethernet controllers.This >>> should be eliminated IMHO. Comments? >> >> This should be able to come from the device tree, ultimately. Can you >> undefine the phy addr for the Galileo board? >> >>> [snip] >>> > > #undf the PHY_ADDR in Galileo board looks weird. This to me is a workaround. I didn't mean to add a #undef. I was just saying that if the "default 0" in the Kconfig were instead "default 0 if !X86" or something (or maybe if the board defconfig explicitly does unselects it). > Since the designware ethernet controller driver supports finding any > PHY attached to its mdio bus, the changes suggested above can be a > proper fix. That is good for your board, but some board may have more than one phy and want to specify which to use in U-Boot. Ultimately it should be possible to read it from the DT. Cheers, -Joe > Regards, > Bin > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Joe, On Thu, Mar 29, 2018 at 2:17 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: > On Sun, Mar 25, 2018 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Joe, >> >> On Sat, Mar 24, 2018 at 1:11 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: >>> On Thu, Mar 22, 2018 at 9:46 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi, >>>> >>>> On Fri, Feb 2, 2018 at 9:53 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>> CONFIG_PHY_ADDR is used for old-style configuration. This makes >>>>> impossible changing the PHY address, if multiple boards share a same >>>>> config header file (for example include/configs/sunxi-common.h). >>>>> >>>>> Moving this to Kconfig helps overcoming this issue. It's defined >>>>> as entry inside PHYLIB section. >>>>> >>>>> After the implemention, moveconfig was run. The issues are: >>>>> - edb9315a - CONFIG_PHYLIB is not enabled. Entry is >>>>> deleted. >>>>> >>>>> - ds414 - CONFIG_PHYLIB is in incompatible format: >>>>> { 0x1, 0x0 }. This entry is also deleted. >>>>> >>>>> - devkit3250 - The PHY_ADDR is in hex format (0x1F). >>>>> Manually CONFIG_PHY_ADDR=31 is added in >>>>> the defconfig. >>>>> >>>>> After the changes the suspicious defconfigs passes building. >>>>> >>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>>> --- >>>>> Changes for v2: >>>>> - Replaced CONFIG_SUNXI_PHY_ADDR with a common one >>>>> CONFIG_PHY_ADDR, using moveconfig. >>>>> >>>>> README | 4 ---- >>>>> configs/devkit3250_defconfig | 1 + >>>>> configs/khadas-vim_defconfig | 1 + >>>>> configs/libretech-cc_defconfig | 1 + >>>>> configs/p212_defconfig | 1 + >>>>> drivers/net/phy/Kconfig | 7 +++++++ >>>>> include/configs/am335x_shc.h | 1 - >>>>> include/configs/baltos.h | 1 - >>>>> include/configs/devkit3250.h | 1 - >>>>> include/configs/ds414.h | 1 - >>>>> include/configs/edb93xx.h | 1 - >>>>> include/configs/khadas-vim.h | 2 -- >>>>> include/configs/libretech-cc.h | 2 -- >>>>> include/configs/p212.h | 2 -- >>>>> include/configs/pepper.h | 1 - >>>>> include/configs/sunxi-common.h | 2 -- >>>>> include/configs/work_92105.h | 1 - >>>>> include/configs/x600.h | 1 - >>>>> scripts/config_whitelist.txt | 1 - >>>>> 19 files changed, 11 insertions(+), 21 deletions(-) >>>>> >>>> >>>> [snip] >>>> >>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >>>>> index 95b7534..c934aed 100644 >>>>> --- a/drivers/net/phy/Kconfig >>>>> +++ b/drivers/net/phy/Kconfig >>>>> @@ -12,6 +12,13 @@ menuconfig PHYLIB >>>>> >>>>> if PHYLIB >>>>> >>>>> +config PHY_ADDR >>>>> + int "PHY address" >>>>> + default 1 if ARCH_SUNXI >>>>> + default 0 >>>>> + help >>>>> + The address of PHY on MII bus. Usually in range of 0 to 31. >>>>> + >>>> >>>> Sorry for jumping out so late, but this commit breaks Intel Galileo >>>> ethernet. Previously the board boots with the following log: >>>> >>>> Net: eth0: eth_designware#0, eth1: eth_designware#1 >>>> >>>> With this commit it becomes: >>>> >>>> Net: No ethernet found. >>>> >>>> The reason is that the board has two designware ethernet controllers, >>>> and PHY_ADDR has been set to zero for both. A simple fix is to: >>>> >>>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >>>> index 43670a7..1394119 100644 >>>> --- a/drivers/net/designware.c >>>> +++ b/drivers/net/designware.c >>>> @@ -471,10 +471,6 @@ static int dw_phy_init(struct dw_eth_dev *priv, void *dev) >>>> struct phy_device *phydev; >>>> int mask = 0xffffffff, ret; >>>> >>>> -#ifdef CONFIG_PHY_ADDR >>>> - mask = 1 << CONFIG_PHY_ADDR; >>>> -#endif >>>> - >>>> phydev = phy_find_by_mask(priv->bus, mask, priv->interface); >>>> if (!phydev) >>>> return -ENODEV; >>>> >>>> But the real question is that: why do we introduce this PHY_ADDR >>>> Kconfig? It for sure won't work for multiple ethernet controllers.This >>>> should be eliminated IMHO. Comments? >>> >>> This should be able to come from the device tree, ultimately. Can you >>> undefine the phy addr for the Galileo board? >>> >>>> [snip] >>>> >> >> #undf the PHY_ADDR in Galileo board looks weird. This to me is a workaround. > > I didn't mean to add a #undef. I was just saying that if the "default > 0" in the Kconfig were instead "default 0 if !X86" or something (or > maybe if the board defconfig explicitly does unselects it). > This cannot be done as CONFIG_PHY_ADDR is an "int", not a "bool". We cannot do "# CONFIG_PHY_ADDR is not set" in the galileo_defconfig. >> Since the designware ethernet controller driver supports finding any >> PHY attached to its mdio bus, the changes suggested above can be a >> proper fix. > > That is good for your board, but some board may have more than one phy > and want to specify which to use in U-Boot. > > Ultimately it should be possible to read it from the DT. > When will this be fixed? At present the CONFIG_PHY_ADDR concept is just wrong. It can only handle one eth and one PHY. Regards, Bin
On Fri, Mar 30, 2018 at 2:49 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Thu, Mar 29, 2018 at 2:17 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: >> On Sun, Mar 25, 2018 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Joe, >>> >>> On Sat, Mar 24, 2018 at 1:11 AM, Joe Hershberger <joe.hershberger@ni.com> wrote: >>>> On Thu, Mar 22, 2018 at 9:46 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi, >>>>> >>>>> On Fri, Feb 2, 2018 at 9:53 PM, Stefan Mavrodiev <stefan@olimex.com> wrote: >>>>>> CONFIG_PHY_ADDR is used for old-style configuration. This makes >>>>>> impossible changing the PHY address, if multiple boards share a same >>>>>> config header file (for example include/configs/sunxi-common.h). >>>>>> >>>>>> Moving this to Kconfig helps overcoming this issue. It's defined >>>>>> as entry inside PHYLIB section. >>>>>> >>>>>> After the implemention, moveconfig was run. The issues are: >>>>>> - edb9315a - CONFIG_PHYLIB is not enabled. Entry is >>>>>> deleted. >>>>>> >>>>>> - ds414 - CONFIG_PHYLIB is in incompatible format: >>>>>> { 0x1, 0x0 }. This entry is also deleted. >>>>>> >>>>>> - devkit3250 - The PHY_ADDR is in hex format (0x1F). >>>>>> Manually CONFIG_PHY_ADDR=31 is added in >>>>>> the defconfig. >>>>>> >>>>>> After the changes the suspicious defconfigs passes building. >>>>>> >>>>>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> >>>>>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>>>>> --- >>>>>> Changes for v2: >>>>>> - Replaced CONFIG_SUNXI_PHY_ADDR with a common one >>>>>> CONFIG_PHY_ADDR, using moveconfig. >>>>>> >>>>>> README | 4 ---- >>>>>> configs/devkit3250_defconfig | 1 + >>>>>> configs/khadas-vim_defconfig | 1 + >>>>>> configs/libretech-cc_defconfig | 1 + >>>>>> configs/p212_defconfig | 1 + >>>>>> drivers/net/phy/Kconfig | 7 +++++++ >>>>>> include/configs/am335x_shc.h | 1 - >>>>>> include/configs/baltos.h | 1 - >>>>>> include/configs/devkit3250.h | 1 - >>>>>> include/configs/ds414.h | 1 - >>>>>> include/configs/edb93xx.h | 1 - >>>>>> include/configs/khadas-vim.h | 2 -- >>>>>> include/configs/libretech-cc.h | 2 -- >>>>>> include/configs/p212.h | 2 -- >>>>>> include/configs/pepper.h | 1 - >>>>>> include/configs/sunxi-common.h | 2 -- >>>>>> include/configs/work_92105.h | 1 - >>>>>> include/configs/x600.h | 1 - >>>>>> scripts/config_whitelist.txt | 1 - >>>>>> 19 files changed, 11 insertions(+), 21 deletions(-) >>>>>> >>>>> >>>>> [snip] >>>>> >>>>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig >>>>>> index 95b7534..c934aed 100644 >>>>>> --- a/drivers/net/phy/Kconfig >>>>>> +++ b/drivers/net/phy/Kconfig >>>>>> @@ -12,6 +12,13 @@ menuconfig PHYLIB >>>>>> >>>>>> if PHYLIB >>>>>> >>>>>> +config PHY_ADDR >>>>>> + int "PHY address" >>>>>> + default 1 if ARCH_SUNXI >>>>>> + default 0 >>>>>> + help >>>>>> + The address of PHY on MII bus. Usually in range of 0 to 31. >>>>>> + >>>>> >>>>> Sorry for jumping out so late, but this commit breaks Intel Galileo >>>>> ethernet. Previously the board boots with the following log: >>>>> >>>>> Net: eth0: eth_designware#0, eth1: eth_designware#1 >>>>> >>>>> With this commit it becomes: >>>>> >>>>> Net: No ethernet found. >>>>> >>>>> The reason is that the board has two designware ethernet controllers, >>>>> and PHY_ADDR has been set to zero for both. A simple fix is to: >>>>> >>>>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >>>>> index 43670a7..1394119 100644 >>>>> --- a/drivers/net/designware.c >>>>> +++ b/drivers/net/designware.c >>>>> @@ -471,10 +471,6 @@ static int dw_phy_init(struct dw_eth_dev *priv, void *dev) >>>>> struct phy_device *phydev; >>>>> int mask = 0xffffffff, ret; >>>>> >>>>> -#ifdef CONFIG_PHY_ADDR >>>>> - mask = 1 << CONFIG_PHY_ADDR; >>>>> -#endif >>>>> - >>>>> phydev = phy_find_by_mask(priv->bus, mask, priv->interface); >>>>> if (!phydev) >>>>> return -ENODEV; >>>>> >>>>> But the real question is that: why do we introduce this PHY_ADDR >>>>> Kconfig? It for sure won't work for multiple ethernet controllers.This >>>>> should be eliminated IMHO. Comments? >>>> >>>> This should be able to come from the device tree, ultimately. Can you >>>> undefine the phy addr for the Galileo board? >>>> >>>>> [snip] >>>>> >>> >>> #undf the PHY_ADDR in Galileo board looks weird. This to me is a workaround. >> >> I didn't mean to add a #undef. I was just saying that if the "default >> 0" in the Kconfig were instead "default 0 if !X86" or something (or >> maybe if the board defconfig explicitly does unselects it). >> > > This cannot be done as CONFIG_PHY_ADDR is an "int", not a "bool". We > cannot do "# CONFIG_PHY_ADDR is not set" in the galileo_defconfig. I just sent an RFC. >>> Since the designware ethernet controller driver supports finding any >>> PHY attached to its mdio bus, the changes suggested above can be a >>> proper fix. >> >> That is good for your board, but some board may have more than one phy >> and want to specify which to use in U-Boot. >> >> Ultimately it should be possible to read it from the DT. >> > > When will this be fixed? At present the CONFIG_PHY_ADDR concept is > just wrong. It can only handle one eth and one PHY. I'm not sure exactly when I'll have the DT sourced phy addr ready, but hopefully the RFC will fix things in the meantime. Cheers, -Joe > Regards, > Bin > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
diff --git a/README b/README index b53ea7d..499184d 100644 --- a/README +++ b/README @@ -1436,10 +1436,6 @@ The following options need to be configured: be at least 4MB. - MII/PHY support: - CONFIG_PHY_ADDR - - The address of PHY on MII bus. - CONFIG_PHY_CLOCK_FREQ (ppc4xx) The clock frequency of the MII bus diff --git a/configs/devkit3250_defconfig b/configs/devkit3250_defconfig index 2ad08ae..930b631 100644 --- a/configs/devkit3250_defconfig +++ b/configs/devkit3250_defconfig @@ -36,6 +36,7 @@ CONFIG_MTD_NOR_FLASH=y CONFIG_NAND=y CONFIG_SPL_NAND_SIMPLE=y CONFIG_PHYLIB=y +CONFIG_PHY_ADDR=31 CONFIG_DM_SERIAL=y CONFIG_SYS_NS16550=y CONFIG_USB=y diff --git a/configs/khadas-vim_defconfig b/configs/khadas-vim_defconfig index 4ceab90..e9a96d3 100644 --- a/configs/khadas-vim_defconfig +++ b/configs/khadas-vim_defconfig @@ -20,6 +20,7 @@ CONFIG_NET_RANDOM_ETHADDR=y CONFIG_DM_GPIO=y CONFIG_DM_MMC=y CONFIG_MMC_MESON_GX=y +CONFIG_PHY_ADDR=8 CONFIG_PHY_MESON_GXL=y CONFIG_DM_ETH=y CONFIG_ETH_DESIGNWARE=y diff --git a/configs/libretech-cc_defconfig b/configs/libretech-cc_defconfig index 3bccff1..86852b1 100644 --- a/configs/libretech-cc_defconfig +++ b/configs/libretech-cc_defconfig @@ -20,6 +20,7 @@ CONFIG_NET_RANDOM_ETHADDR=y CONFIG_DM_GPIO=y CONFIG_DM_MMC=y CONFIG_MMC_MESON_GX=y +CONFIG_PHY_ADDR=8 CONFIG_PHY_MESON_GXL=y CONFIG_DM_ETH=y CONFIG_ETH_DESIGNWARE=y diff --git a/configs/p212_defconfig b/configs/p212_defconfig index cb9be4a..9466238 100644 --- a/configs/p212_defconfig +++ b/configs/p212_defconfig @@ -20,6 +20,7 @@ CONFIG_NET_RANDOM_ETHADDR=y CONFIG_DM_GPIO=y CONFIG_DM_MMC=y CONFIG_MMC_MESON_GX=y +CONFIG_PHY_ADDR=8 CONFIG_PHY_MESON_GXL=y CONFIG_DM_ETH=y CONFIG_ETH_DESIGNWARE=y diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 95b7534..c934aed 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -12,6 +12,13 @@ menuconfig PHYLIB if PHYLIB +config PHY_ADDR + int "PHY address" + default 1 if ARCH_SUNXI + default 0 + help + The address of PHY on MII bus. Usually in range of 0 to 31. + config B53_SWITCH bool "Broadcom BCM53xx (RoboSwitch) Ethernet switch PHY support." help diff --git a/include/configs/am335x_shc.h b/include/configs/am335x_shc.h index e2d329a..2b705fc 100644 --- a/include/configs/am335x_shc.h +++ b/include/configs/am335x_shc.h @@ -264,7 +264,6 @@ #define CONFIG_BOOTP_GATEWAY #define CONFIG_BOOTP_SUBNETMASK #define CONFIG_NET_RETRY_COUNT 10 -#define CONFIG_PHY_ADDR 0 #define CONFIG_PHY_SMSC /* I2C configuration */ diff --git a/include/configs/baltos.h b/include/configs/baltos.h index 75dd0c5..31ae20c 100644 --- a/include/configs/baltos.h +++ b/include/configs/baltos.h @@ -285,7 +285,6 @@ #endif /* Network. */ -#define CONFIG_PHY_ADDR 0 #define CONFIG_PHY_SMSC #define CONFIG_MII #define CONFIG_PHY_ATHEROS diff --git a/include/configs/devkit3250.h b/include/configs/devkit3250.h index 526a81a..b66b90e 100644 --- a/include/configs/devkit3250.h +++ b/include/configs/devkit3250.h @@ -73,7 +73,6 @@ #define CONFIG_RMII #define CONFIG_PHY_SMSC #define CONFIG_LPC32XX_ETH -#define CONFIG_PHY_ADDR 0x1F #define CONFIG_SYS_FAULT_ECHO_LINK_DOWN /* diff --git a/include/configs/ds414.h b/include/configs/ds414.h index c840c93..03e96df 100644 --- a/include/configs/ds414.h +++ b/include/configs/ds414.h @@ -41,7 +41,6 @@ #define CONFIG_ENV_SECT_SIZE (64 << 10) /* 64KiB sectors */ #define CONFIG_PHY_MARVELL /* there is a marvell phy */ -#define CONFIG_PHY_ADDR { 0x1, 0x0 } #define CONFIG_SYS_NETA_INTERFACE_TYPE PHY_INTERFACE_MODE_RGMII #define CONFIG_SYS_ALT_MEMTEST diff --git a/include/configs/edb93xx.h b/include/configs/edb93xx.h index fcad7c4..03a1eff 100644 --- a/include/configs/edb93xx.h +++ b/include/configs/edb93xx.h @@ -97,7 +97,6 @@ #define CONFIG_DRIVER_EP93XX_MAC #define CONFIG_MII_SUPPRESS_PREAMBLE #define CONFIG_MII -#define CONFIG_PHY_ADDR 1 #undef CONFIG_NETCONSOLE /* SDRAM configuration */ diff --git a/include/configs/khadas-vim.h b/include/configs/khadas-vim.h index 9d99bc5..f44b388 100644 --- a/include/configs/khadas-vim.h +++ b/include/configs/khadas-vim.h @@ -12,8 +12,6 @@ #define CONFIG_MISC_INIT_R -#define CONFIG_PHY_ADDR 8 - #define MESON_FDTFILE_SETTING "fdtfile=amlogic/meson-gxl-s905x-khadas-vim.dtb\0" #include <configs/meson-gxbb-common.h> diff --git a/include/configs/libretech-cc.h b/include/configs/libretech-cc.h index ffaca26..08dfb30 100644 --- a/include/configs/libretech-cc.h +++ b/include/configs/libretech-cc.h @@ -12,8 +12,6 @@ #define CONFIG_MISC_INIT_R -#define CONFIG_PHY_ADDR 8 - #define MESON_FDTFILE_SETTING "fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb\0" #include <configs/meson-gxbb-common.h> diff --git a/include/configs/p212.h b/include/configs/p212.h index 793b556..0477384 100644 --- a/include/configs/p212.h +++ b/include/configs/p212.h @@ -12,8 +12,6 @@ #define CONFIG_MISC_INIT_R -#define CONFIG_PHY_ADDR 8 - /* Serial setup */ #define CONFIG_CONS_INDEX 0 diff --git a/include/configs/pepper.h b/include/configs/pepper.h index 7ef2529..960afa0 100644 --- a/include/configs/pepper.h +++ b/include/configs/pepper.h @@ -77,7 +77,6 @@ #define CONFIG_SYS_NS16550_COM1 0x44e09000 /* Ethernet support */ -#define CONFIG_PHY_ADDR 0 #define CONFIG_PHY_RESET_DELAY 1000 /* SPL */ diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 9b3944a..e252e7fb 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -296,12 +296,10 @@ extern int soft_i2c_gpio_scl; /* Ethernet support */ #ifdef CONFIG_SUN4I_EMAC -#define CONFIG_PHY_ADDR 1 #define CONFIG_MII /* MII PHY management */ #endif #ifdef CONFIG_SUN7I_GMAC -#define CONFIG_PHY_ADDR 1 #define CONFIG_MII /* MII PHY management */ #define CONFIG_PHY_REALTEK #endif diff --git a/include/configs/work_92105.h b/include/configs/work_92105.h index 7faab4e..c0a9833 100644 --- a/include/configs/work_92105.h +++ b/include/configs/work_92105.h @@ -57,7 +57,6 @@ #define CONFIG_PHY_SMSC #define CONFIG_LPC32XX_ETH -#define CONFIG_PHY_ADDR 0 #define CONFIG_SYS_FAULT_ECHO_LINK_DOWN /* FIXME: remove "Waiting for PHY auto negotiation to complete..." message */ diff --git a/include/configs/x600.h b/include/configs/x600.h index 66a8e88..ea9500a 100644 --- a/include/configs/x600.h +++ b/include/configs/x600.h @@ -71,7 +71,6 @@ /* Ethernet config options */ #define CONFIG_MII #define CONFIG_PHY_RESET_DELAY 10000 /* in usec */ -#define CONFIG_PHY_ADDR 0 /* PHY address */ #define CONFIG_SPEAR_GPIO diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt index ef83c00..c94e6ce 100644 --- a/scripts/config_whitelist.txt +++ b/scripts/config_whitelist.txt @@ -1593,7 +1593,6 @@ CONFIG_PERIF2_FREQ CONFIG_PERIF3_FREQ CONFIG_PERIF4_FREQ CONFIG_PHYSMEM -CONFIG_PHY_ADDR CONFIG_PHY_BASE_ADR CONFIG_PHY_BCM5421S CONFIG_PHY_ET1011C_TX_CLK_FIX
CONFIG_PHY_ADDR is used for old-style configuration. This makes impossible changing the PHY address, if multiple boards share a same config header file (for example include/configs/sunxi-common.h). Moving this to Kconfig helps overcoming this issue. It's defined as entry inside PHYLIB section. After the implemention, moveconfig was run. The issues are: - edb9315a - CONFIG_PHYLIB is not enabled. Entry is deleted. - ds414 - CONFIG_PHYLIB is in incompatible format: { 0x1, 0x0 }. This entry is also deleted. - devkit3250 - The PHY_ADDR is in hex format (0x1F). Manually CONFIG_PHY_ADDR=31 is added in the defconfig. After the changes the suspicious defconfigs passes building. Signed-off-by: Stefan Mavrodiev <stefan@olimex.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- Changes for v2: - Replaced CONFIG_SUNXI_PHY_ADDR with a common one CONFIG_PHY_ADDR, using moveconfig. README | 4 ---- configs/devkit3250_defconfig | 1 + configs/khadas-vim_defconfig | 1 + configs/libretech-cc_defconfig | 1 + configs/p212_defconfig | 1 + drivers/net/phy/Kconfig | 7 +++++++ include/configs/am335x_shc.h | 1 - include/configs/baltos.h | 1 - include/configs/devkit3250.h | 1 - include/configs/ds414.h | 1 - include/configs/edb93xx.h | 1 - include/configs/khadas-vim.h | 2 -- include/configs/libretech-cc.h | 2 -- include/configs/p212.h | 2 -- include/configs/pepper.h | 1 - include/configs/sunxi-common.h | 2 -- include/configs/work_92105.h | 1 - include/configs/x600.h | 1 - scripts/config_whitelist.txt | 1 - 19 files changed, 11 insertions(+), 21 deletions(-)