| Message ID | 1369651702-9207-1-git-send-email-alexandre.belloni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On May 27, 2013, at 6:48 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > Fixes: > arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup': > :(.text+0x1174): undefined reference to `mdiobus_write' > :(.text+0x1188): undefined reference to `mdiobus_write' > :(.text+0x119c): undefined reference to `mdiobus_write' > :(.text+0x11b0): undefined reference to `mdiobus_write' > arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init': > :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid' > > when CONFIG_PHYLIB is not selected. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> no for this change add an inline version of phy_register_fixup_for_uid this will do the same for every platform > --- > Changes in v3: > added SoB > added Acked-By > > Changes in v2: > use IS_BUILTIN > use CONFIG_PHYLIB and not CONFIG_PHY > > arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c > index 705305e..e9ce541 100644 > --- a/arch/arm/mach-at91/board-dt-sama5.c > +++ b/arch/arm/mach-at91/board-dt-sama5.c > @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) > #define GMII_ERCR 11 > #define GMII_ERDWR 12 > > - /* Set delay values */ > - value = GMII_RCCPSR | 0x8000; > - phy_write(phy, GMII_ERCR, value); > - value = 0xF2F4; > - phy_write(phy, GMII_ERDWR, value); > - value = GMII_RRDPSR | 0x8000; > - phy_write(phy, GMII_ERCR, value); > - value = 0x2222; > - phy_write(phy, GMII_ERDWR, value); > + if (IS_BUILTIN(CONFIG_PHYLIB)) { > + /* Set delay values */ > + value = GMII_RCCPSR | 0x8000; > + phy_write(phy, GMII_ERCR, value); > + value = 0xF2F4; > + phy_write(phy, GMII_ERDWR, value); > + value = GMII_RRDPSR | 0x8000; > + phy_write(phy, GMII_ERCR, value); > + value = 0x2222; > + phy_write(phy, GMII_ERDWR, value); > + } > > return 0; > } > > static void __init sama5_dt_device_init(void) > { > - if (of_machine_is_compatible("atmel,sama5d3xcm")) > + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) > phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, > ksz9021rn_phy_fixup); > > -- > 1.8.1.2 >
On 27/05/2013 12:48, Alexandre Belloni : > Fixes: > arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup': > :(.text+0x1174): undefined reference to `mdiobus_write' > :(.text+0x1188): undefined reference to `mdiobus_write' > :(.text+0x119c): undefined reference to `mdiobus_write' > :(.text+0x11b0): undefined reference to `mdiobus_write' > arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init': > :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid' > > when CONFIG_PHYLIB is not selected. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> > --- > Changes in v3: > added SoB > added Acked-By A v3 is not needed simply for this. Thanks, anyway! One question though: why not just add select PHYLIB In Kconfig entry: config MACH_SAMA5_DT ? (note that I like to play with everybody's nerves ;-)) Bye, > Changes in v2: > use IS_BUILTIN > use CONFIG_PHYLIB and not CONFIG_PHY > > arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c > index 705305e..e9ce541 100644 > --- a/arch/arm/mach-at91/board-dt-sama5.c > +++ b/arch/arm/mach-at91/board-dt-sama5.c > @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) > #define GMII_ERCR 11 > #define GMII_ERDWR 12 > > - /* Set delay values */ > - value = GMII_RCCPSR | 0x8000; > - phy_write(phy, GMII_ERCR, value); > - value = 0xF2F4; > - phy_write(phy, GMII_ERDWR, value); > - value = GMII_RRDPSR | 0x8000; > - phy_write(phy, GMII_ERCR, value); > - value = 0x2222; > - phy_write(phy, GMII_ERDWR, value); > + if (IS_BUILTIN(CONFIG_PHYLIB)) { > + /* Set delay values */ > + value = GMII_RCCPSR | 0x8000; > + phy_write(phy, GMII_ERCR, value); > + value = 0xF2F4; > + phy_write(phy, GMII_ERDWR, value); > + value = GMII_RRDPSR | 0x8000; > + phy_write(phy, GMII_ERCR, value); > + value = 0x2222; > + phy_write(phy, GMII_ERDWR, value); > + } > > return 0; > } > > static void __init sama5_dt_device_init(void) > { > - if (of_machine_is_compatible("atmel,sama5d3xcm")) > + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) > phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, > ksz9021rn_phy_fixup); > >
On Monday 27 May 2013 14:05:13 Nicolas Ferre wrote: > On 27/05/2013 12:48, Alexandre Belloni : > A v3 is not needed simply for this. Thanks, anyway! > > One question though: why not just add > > select PHYLIB > > In Kconfig entry: > > config MACH_SAMA5_DT > > ? > > (note that I like to play with everybody's nerves ;-)) That would make it impossible to build a sama5 kernel without networking support, which is probably not what you want. Jean-Christophe's suggestion to add the inline helper is best. Aside from that, please note that > > diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c > > index 705305e..e9ce541 100644 > > --- a/arch/arm/mach-at91/board-dt-sama5.c > > +++ b/arch/arm/mach-at91/board-dt-sama5.c > > @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) > > #define GMII_ERCR 11 > > #define GMII_ERDWR 12 > > > > - /* Set delay values */ > > - value = GMII_RCCPSR | 0x8000; > > - phy_write(phy, GMII_ERCR, value); > > - value = 0xF2F4; > > - phy_write(phy, GMII_ERDWR, value); > > - value = GMII_RRDPSR | 0x8000; > > - phy_write(phy, GMII_ERCR, value); > > - value = 0x2222; > > - phy_write(phy, GMII_ERDWR, value); > > + if (IS_BUILTIN(CONFIG_PHYLIB)) { > > + /* Set delay values */ > > + value = GMII_RCCPSR | 0x8000; > > + phy_write(phy, GMII_ERCR, value); > > + value = 0xF2F4; > > + phy_write(phy, GMII_ERDWR, value); > > + value = GMII_RRDPSR | 0x8000; > > + phy_write(phy, GMII_ERCR, value); > > + value = 0x2222; > > + phy_write(phy, GMII_ERDWR, value); > > + } > > > > return 0; > > } This part of the patch is not actually needed if you > > static void __init sama5_dt_device_init(void) > > { > > - if (of_machine_is_compatible("atmel,sama5d3xcm")) > > + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) > > phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, > > ksz9021rn_phy_fixup); do this change, or the inline helper. Arnd
On May 27, 2013, at 8:05 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > On 27/05/2013 12:48, Alexandre Belloni : >> Fixes: >> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup': >> :(.text+0x1174): undefined reference to `mdiobus_write' >> :(.text+0x1188): undefined reference to `mdiobus_write' >> :(.text+0x119c): undefined reference to `mdiobus_write' >> :(.text+0x11b0): undefined reference to `mdiobus_write' >> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init': >> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid' >> >> when CONFIG_PHYLIB is not selected. >> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> >> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> >> --- >> Changes in v3: >> added SoB >> added Acked-By > > A v3 is not needed simply for this. Thanks, anyway! > > One question though: why not just add > > select PHYLIB > > In Kconfig entry: > > config MACH_SAMA5_DT > > ? > > (note that I like to play with everybody's nerves ;-)) > I prefer this than the IS_BUILTIN > Bye, > >> Changes in v2: >> use IS_BUILTIN >> use CONFIG_PHYLIB and not CONFIG_PHY >> >> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c >> index 705305e..e9ce541 100644 >> --- a/arch/arm/mach-at91/board-dt-sama5.c >> +++ b/arch/arm/mach-at91/board-dt-sama5.c >> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) >> #define GMII_ERCR 11 >> #define GMII_ERDWR 12 >> >> - /* Set delay values */ >> - value = GMII_RCCPSR | 0x8000; >> - phy_write(phy, GMII_ERCR, value); >> - value = 0xF2F4; >> - phy_write(phy, GMII_ERDWR, value); >> - value = GMII_RRDPSR | 0x8000; >> - phy_write(phy, GMII_ERCR, value); >> - value = 0x2222; >> - phy_write(phy, GMII_ERDWR, value); >> + if (IS_BUILTIN(CONFIG_PHYLIB)) { >> + /* Set delay values */ >> + value = GMII_RCCPSR | 0x8000; >> + phy_write(phy, GMII_ERCR, value); >> + value = 0xF2F4; >> + phy_write(phy, GMII_ERDWR, value); >> + value = GMII_RRDPSR | 0x8000; >> + phy_write(phy, GMII_ERCR, value); >> + value = 0x2222; >> + phy_write(phy, GMII_ERDWR, value); >> + } >> >> return 0; >> } >> >> static void __init sama5_dt_device_init(void) >> { >> - if (of_machine_is_compatible("atmel,sama5d3xcm")) >> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) >> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, >> ksz9021rn_phy_fixup); >> >> > > > -- > Nicolas Ferre
On 27/05/2013 14:05, Nicolas Ferre wrote: > On 27/05/2013 12:48, Alexandre Belloni : >> Fixes: >> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup': >> :(.text+0x1174): undefined reference to `mdiobus_write' >> :(.text+0x1188): undefined reference to `mdiobus_write' >> :(.text+0x119c): undefined reference to `mdiobus_write' >> :(.text+0x11b0): undefined reference to `mdiobus_write' >> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init': >> :(.init.text+0x1e34): undefined reference to >> `phy_register_fixup_for_uid' >> >> when CONFIG_PHYLIB is not selected. >> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> >> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> >> --- >> Changes in v3: >> added SoB >> added Acked-By > > A v3 is not needed simply for this. Thanks, anyway! > It was not much and I have to stop forgetting my SoB ;) > One question though: why not just add > > select PHYLIB > > In Kconfig entry: > > config MACH_SAMA5_DT > > ? > That would simply mean that you have to enable NET when using a SAMA5 which is not what every user will want. Regards,
On 27/05/2013 13:26, Jean-Christophe PLAGNIOL-VILLARD wrote: > On May 27, 2013, at 6:48 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > >> Fixes: >> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup': >> :(.text+0x1174): undefined reference to `mdiobus_write' >> :(.text+0x1188): undefined reference to `mdiobus_write' >> :(.text+0x119c): undefined reference to `mdiobus_write' >> :(.text+0x11b0): undefined reference to `mdiobus_write' >> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init': >> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid' >> >> when CONFIG_PHYLIB is not selected. >> >> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> >> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> > no for this change > > add an inline version of phy_register_fixup_for_uid > > this will do the same for every platform I don't actually see the added value of doing that. Using IS_BUILTIN is readable and ensure that you will definitely be able to link. The functions will be optimized out and removed by gcc even when you have CONFIG_PHYLIB=m. >> --- >> Changes in v3: >> added SoB >> added Acked-By >> >> Changes in v2: >> use IS_BUILTIN >> use CONFIG_PHYLIB and not CONFIG_PHY >> >> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c >> index 705305e..e9ce541 100644 >> --- a/arch/arm/mach-at91/board-dt-sama5.c >> +++ b/arch/arm/mach-at91/board-dt-sama5.c >> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) >> #define GMII_ERCR 11 >> #define GMII_ERDWR 12 >> >> - /* Set delay values */ >> - value = GMII_RCCPSR | 0x8000; >> - phy_write(phy, GMII_ERCR, value); >> - value = 0xF2F4; >> - phy_write(phy, GMII_ERDWR, value); >> - value = GMII_RRDPSR | 0x8000; >> - phy_write(phy, GMII_ERCR, value); >> - value = 0x2222; >> - phy_write(phy, GMII_ERDWR, value); >> + if (IS_BUILTIN(CONFIG_PHYLIB)) { >> + /* Set delay values */ >> + value = GMII_RCCPSR | 0x8000; >> + phy_write(phy, GMII_ERCR, value); >> + value = 0xF2F4; >> + phy_write(phy, GMII_ERDWR, value); >> + value = GMII_RRDPSR | 0x8000; >> + phy_write(phy, GMII_ERCR, value); >> + value = 0x2222; >> + phy_write(phy, GMII_ERDWR, value); >> + } >> >> return 0; >> } >> >> static void __init sama5_dt_device_init(void) >> { >> - if (of_machine_is_compatible("atmel,sama5d3xcm")) >> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) >> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, >> ksz9021rn_phy_fixup); >> >> -- >> 1.8.1.2 >>
On May 27, 2013, at 8:50 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 27/05/2013 13:26, Jean-Christophe PLAGNIOL-VILLARD wrote: >> On May 27, 2013, at 6:48 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: >> >>> Fixes: >>> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup': >>> :(.text+0x1174): undefined reference to `mdiobus_write' >>> :(.text+0x1188): undefined reference to `mdiobus_write' >>> :(.text+0x119c): undefined reference to `mdiobus_write' >>> :(.text+0x11b0): undefined reference to `mdiobus_write' >>> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init': >>> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid' >>> >>> when CONFIG_PHYLIB is not selected. >>> >>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> >>> Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> >> no for this change >> >> add an inline version of phy_register_fixup_for_uid >> >> this will do the same for every platform > > I don't actually see the added value of doing that. Using IS_BUILTIN is > readable and ensure that you will definitely be able to link. The > functions will be optimized out and removed by gcc even when you have > CONFIG_PHYLIB=m. no will not as you do ifdef CONFIG_PHYLIB and if CONFIG_PHYLIB=m the macro is defended we do this all the time in the kernel I add the IS_BUILTIN & co to be used when a simple inline is not sufficient Best Regards, J/ > >>> --- >>> Changes in v3: >>> added SoB >>> added Acked-By >>> >>> Changes in v2: >>> use IS_BUILTIN >>> use CONFIG_PHYLIB and not CONFIG_PHY >>> >>> arch/arm/mach-at91/board-dt-sama5.c | 22 ++++++++++++---------- >>> 1 file changed, 12 insertions(+), 10 deletions(-) >>> >>> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c >>> index 705305e..e9ce541 100644 >>> --- a/arch/arm/mach-at91/board-dt-sama5.c >>> +++ b/arch/arm/mach-at91/board-dt-sama5.c >>> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) >>> #define GMII_ERCR 11 >>> #define GMII_ERDWR 12 >>> >>> - /* Set delay values */ >>> - value = GMII_RCCPSR | 0x8000; >>> - phy_write(phy, GMII_ERCR, value); >>> - value = 0xF2F4; >>> - phy_write(phy, GMII_ERDWR, value); >>> - value = GMII_RRDPSR | 0x8000; >>> - phy_write(phy, GMII_ERCR, value); >>> - value = 0x2222; >>> - phy_write(phy, GMII_ERDWR, value); >>> + if (IS_BUILTIN(CONFIG_PHYLIB)) { >>> + /* Set delay values */ >>> + value = GMII_RCCPSR | 0x8000; >>> + phy_write(phy, GMII_ERCR, value); >>> + value = 0xF2F4; >>> + phy_write(phy, GMII_ERDWR, value); >>> + value = GMII_RRDPSR | 0x8000; >>> + phy_write(phy, GMII_ERCR, value); >>> + value = 0x2222; >>> + phy_write(phy, GMII_ERDWR, value); >>> + } >>> >>> return 0; >>> } >>> >>> static void __init sama5_dt_device_init(void) >>> { >>> - if (of_machine_is_compatible("atmel,sama5d3xcm")) >>> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) >>> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, >>> ksz9021rn_phy_fixup); >>> >>> -- >>> 1.8.1.2 >>> > > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com >
On 27/05/2013 14:59, Jean-Christophe PLAGNIOL-VILLARD wrote: > > no will not as you do ifdef CONFIG_PHYLIB and if CONFIG_PHYLIB=m the macro is defended > > we do this all the time in the kernel > > I add the IS_BUILTIN & co to be used when a simple inline is not sufficient Ok, it took me a while to understand what you were trying to explain. I'm sending a patch set now. I don't believe it will be taken in for 3.10 though. Regards,
On 27/05/2013 14:42, Arnd Bergmann : > On Monday 27 May 2013 14:05:13 Nicolas Ferre wrote: >> On 27/05/2013 12:48, Alexandre Belloni : >> A v3 is not needed simply for this. Thanks, anyway! >> >> One question though: why not just add >> >> select PHYLIB >> >> In Kconfig entry: >> >> config MACH_SAMA5_DT >> >> ? >> >> (note that I like to play with everybody's nerves ;-)) > > That would make it impossible to build a sama5 kernel without networking > support, which is probably not what you want. > > Jean-Christophe's suggestion to add the inline helper is best. It seems that it has been clearly rejected by David. So the best solution remains to... > Aside from > that, please note that > >>> diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c >>> index 705305e..e9ce541 100644 >>> --- a/arch/arm/mach-at91/board-dt-sama5.c >>> +++ b/arch/arm/mach-at91/board-dt-sama5.c >>> @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) >>> #define GMII_ERCR 11 >>> #define GMII_ERDWR 12 >>> >>> - /* Set delay values */ >>> - value = GMII_RCCPSR | 0x8000; >>> - phy_write(phy, GMII_ERCR, value); >>> - value = 0xF2F4; >>> - phy_write(phy, GMII_ERDWR, value); >>> - value = GMII_RRDPSR | 0x8000; >>> - phy_write(phy, GMII_ERCR, value); >>> - value = 0x2222; >>> - phy_write(phy, GMII_ERDWR, value); >>> + if (IS_BUILTIN(CONFIG_PHYLIB)) { >>> + /* Set delay values */ >>> + value = GMII_RCCPSR | 0x8000; >>> + phy_write(phy, GMII_ERCR, value); >>> + value = 0xF2F4; >>> + phy_write(phy, GMII_ERDWR, value); >>> + value = GMII_RRDPSR | 0x8000; >>> + phy_write(phy, GMII_ERCR, value); >>> + value = 0x2222; >>> + phy_write(phy, GMII_ERDWR, value); >>> + } >>> >>> return 0; >>> } > > This part of the patch is not actually needed if you > >>> static void __init sama5_dt_device_init(void) >>> { >>> - if (of_machine_is_compatible("atmel,sama5d3xcm")) >>> + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) >>> phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, >>> ksz9021rn_phy_fixup); > > do this change, or the inline helper. ... simply use this "IS_BUILTIN(CONFIG_PHYLIB)" here -----^^^^^^^^^ (and not above as suggested by Arnd). Jean-Christophe, Alexandre: do you agree with this conclusion? I plan to stack this patch for next at91-fixes batch. Bye,
On 04/06/2013 15:49, Nicolas Ferre wrote: > > ... simply use this "IS_BUILTIN(CONFIG_PHYLIB)" here -----^^^^^^^^^ > (and not above as suggested by Arnd). > > Jean-Christophe, Alexandre: do you agree with this conclusion? > > I plan to stack this patch for next at91-fixes batch. > Yeah, I prepared and tested that last week but didn't send it yet. I can do it now if you want but you could probably just edit my patch. I'd like to fix the other platforms too but that probably can wait. I believe we would still have to fix the phy_register_fixup*() works as it is always called from arch/ and is clearly not fool proofed enough. Maybe DT could help ? Arnd, do you have any input ?
On Tuesday 04 June 2013 16:12:24 Alexandre Belloni wrote: > On 04/06/2013 15:49, Nicolas Ferre wrote: > > > > ... simply use this "IS_BUILTIN(CONFIG_PHYLIB)" here -----^^^^^^^^^ > > (and not above as suggested by Arnd). > > > > Jean-Christophe, Alexandre: do you agree with this conclusion? > > > > I plan to stack this patch for next at91-fixes batch. > > > > Yeah, I prepared and tested that last week but didn't send it yet. I can > do it now if you want but you could probably just edit my patch. I'd > like to fix the other platforms too but that probably can wait. > > I believe we would still have to fix the phy_register_fixup*() works as > it is always called from arch/ and is clearly not fool proofed enough. > Maybe DT could help ? > > Arnd, do you have any input ? I've replied to the earlier thread now. Using if(IS_ENABLED(CONFIG_NET)) or if(IS_ENABLED(CONFIG_PHYLIB)) in platform code should be just fine and is not a lof of extra effort. As I explained in the other mail, I think IS_ENABLED() is better than IS_BUILTIN() because the case where the network driver is a loadable module would cause a silent run-time error with IS_BUILTIN, but a link time error with IS_ENABLED, and we can prevent that error by correctly expressing the dependency in Kconfig, e.g. config THIS_BOARD select PHYLIB if THIS_BOARDS_NIC Arnd
diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c index 705305e..e9ce541 100644 --- a/arch/arm/mach-at91/board-dt-sama5.c +++ b/arch/arm/mach-at91/board-dt-sama5.c @@ -47,22 +47,24 @@ static int ksz9021rn_phy_fixup(struct phy_device *phy) #define GMII_ERCR 11 #define GMII_ERDWR 12 - /* Set delay values */ - value = GMII_RCCPSR | 0x8000; - phy_write(phy, GMII_ERCR, value); - value = 0xF2F4; - phy_write(phy, GMII_ERDWR, value); - value = GMII_RRDPSR | 0x8000; - phy_write(phy, GMII_ERCR, value); - value = 0x2222; - phy_write(phy, GMII_ERDWR, value); + if (IS_BUILTIN(CONFIG_PHYLIB)) { + /* Set delay values */ + value = GMII_RCCPSR | 0x8000; + phy_write(phy, GMII_ERCR, value); + value = 0xF2F4; + phy_write(phy, GMII_ERDWR, value); + value = GMII_RRDPSR | 0x8000; + phy_write(phy, GMII_ERCR, value); + value = 0x2222; + phy_write(phy, GMII_ERDWR, value); + } return 0; } static void __init sama5_dt_device_init(void) { - if (of_machine_is_compatible("atmel,sama5d3xcm")) + if (of_machine_is_compatible("atmel,sama5d3xcm") && IS_BUILTIN(CONFIG_PHYLIB)) phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK, ksz9021rn_phy_fixup);