| Message ID | 1440487347-10517-3-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > With driver model, board_eth_init() or cpu_eth_init() is not needed. > Remove the call to these in eth_common_init(). I'm pretty sure Simon needed this when he ported some allwinner board originally. 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing Ethernet init for driver model) Cheers, -Joe
Hi Joe, On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger <joe.hershberger@gmail.com> wrote: > Hi Bin, > > On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >> With driver model, board_eth_init() or cpu_eth_init() is not needed. >> Remove the call to these in eth_common_init(). > > I'm pretty sure Simon needed this when he ported some allwinner board > originally. > > 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing > Ethernet init for driver model) > I think my patch does not break Simon's. My patch only comments out the call to board_eth_init() or cpu_eth_init() which is not needed for driver model. Other stuff in eth_common_init() is still there. In fact, my patch series also needs phy_init() call (required by pch_gbe driver). Regards, Bin
Hi Simon, On Wed, Aug 26, 2015 at 9:29 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Bin, >> >> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> With driver model, board_eth_init() or cpu_eth_init() is not needed. >>> Remove the call to these in eth_common_init(). >> >> I'm pretty sure Simon needed this when he ported some allwinner board >> originally. >> >> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing >> Ethernet init for driver model) >> > > I think my patch does not break Simon's. My patch only comments out > the call to board_eth_init() or cpu_eth_init() which is not needed for > driver model. Other stuff in eth_common_init() is still there. In > fact, my patch series also needs phy_init() call (required by pch_gbe > driver). > Would you confirm this does not break your boards? Regards, Bin
Hi Bin, On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Bin, >> >> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> With driver model, board_eth_init() or cpu_eth_init() is not needed. >>> Remove the call to these in eth_common_init(). >> >> I'm pretty sure Simon needed this when he ported some allwinner board >> originally. >> >> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing >> Ethernet init for driver model) >> > > I think my patch does not break Simon's. My patch only comments out > the call to board_eth_init() or cpu_eth_init() which is not needed for > driver model. Other stuff in eth_common_init() is still there. In > fact, my patch series also needs phy_init() call (required by pch_gbe > driver). Even if it doesn't break Simon's board, why remove the ability for a board to add eth_init code. You're trying to say that there is no case where a DM board would need to init anything related to eth. That seems unlikely. Also, why is it hurting your board to have an optional call to such a function. Presumably you just don't define those functions and you're fine, right? I guess it can just be put back when such a board is converted. -Joe
Hi Joe, On Wed, Aug 26, 2015 at 10:24 AM, Joe Hershberger <joe.hershberger@gmail.com> wrote: > Hi Bin, > > On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Joe, >> >> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger >> <joe.hershberger@gmail.com> wrote: >>> Hi Bin, >>> >>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> With driver model, board_eth_init() or cpu_eth_init() is not needed. >>>> Remove the call to these in eth_common_init(). >>> >>> I'm pretty sure Simon needed this when he ported some allwinner board >>> originally. >>> >>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing >>> Ethernet init for driver model) >>> >> >> I think my patch does not break Simon's. My patch only comments out >> the call to board_eth_init() or cpu_eth_init() which is not needed for >> driver model. Other stuff in eth_common_init() is still there. In >> fact, my patch series also needs phy_init() call (required by pch_gbe >> driver). > > Even if it doesn't break Simon's board, why remove the ability for a > board to add eth_init code. You're trying to say that there is no case > where a DM board would need to init anything related to eth. That > seems unlikely. > I believe with driver model, we should avoid these calls. All the drive initialization needs to be done in the bind or probe phase, and called by driver model automatically. Like pci_eth_init() which just calls non-dm ethernet drivers, and pci_eth_init() can be called from board_eth_init() or cpu_eth_init(). But I think you are right, there might be some board-specific thing to be put there, even right now it does not break any board. But that's maybe we don't have proper driver model uclass to handle these misc things? > Also, why is it hurting your board to have an optional call to such a > function. Presumably you just don't define those functions and you're > fine, right? > It does not hurt but I think at least we should comment out the following printf for DM. } else { printf("Net Initialization Skipped\n"); } This is misleading message. > I guess it can just be put back when such a board is converted. > Regards, Bin
Hi Bin, On Tue, Aug 25, 2015 at 9:36 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Wed, Aug 26, 2015 at 10:24 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Bin, >> >> On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Joe, >>> >>> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger >>> <joe.hershberger@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> With driver model, board_eth_init() or cpu_eth_init() is not needed. >>>>> Remove the call to these in eth_common_init(). >>>> >>>> I'm pretty sure Simon needed this when he ported some allwinner board >>>> originally. >>>> >>>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing >>>> Ethernet init for driver model) >>>> >>> >>> I think my patch does not break Simon's. My patch only comments out >>> the call to board_eth_init() or cpu_eth_init() which is not needed for >>> driver model. Other stuff in eth_common_init() is still there. In >>> fact, my patch series also needs phy_init() call (required by pch_gbe >>> driver). >> >> Even if it doesn't break Simon's board, why remove the ability for a >> board to add eth_init code. You're trying to say that there is no case >> where a DM board would need to init anything related to eth. That >> seems unlikely. >> > > I believe with driver model, we should avoid these calls. All the > drive initialization needs to be done in the bind or probe phase, and > called by driver model automatically. Like pci_eth_init() which just > calls non-dm ethernet drivers, and pci_eth_init() can be called from > board_eth_init() or cpu_eth_init(). I agree we shouldn't use them if it makes sense not to. > But I think you are right, there > might be some board-specific thing to be put there, even right now it > does not break any board. But that's maybe we don't have proper driver > model uclass to handle these misc things? I think we can evaluate that for each case. We don't necessarily want the uclass to be a union of all crazy board features. >> Also, why is it hurting your board to have an optional call to such a >> function. Presumably you just don't define those functions and you're >> fine, right? >> > > It does not hurt but I think at least we should comment out the > following printf for DM. > > } else { > printf("Net Initialization Skipped\n"); > } > > This is misleading message. I agree. >> I guess it can just be put back when such a board is converted. >> > > Regards, > Bin
Hi, On 25 August 2015 at 19:36, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Wed, Aug 26, 2015 at 10:24 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Bin, >> >> On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Joe, >>> >>> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger >>> <joe.hershberger@gmail.com> wrote: >>>> Hi Bin, >>>> >>>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> With driver model, board_eth_init() or cpu_eth_init() is not needed. >>>>> Remove the call to these in eth_common_init(). >>>> >>>> I'm pretty sure Simon needed this when he ported some allwinner board >>>> originally. >>>> >>>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing >>>> Ethernet init for driver model) >>>> >>> >>> I think my patch does not break Simon's. My patch only comments out >>> the call to board_eth_init() or cpu_eth_init() which is not needed for >>> driver model. Other stuff in eth_common_init() is still there. In >>> fact, my patch series also needs phy_init() call (required by pch_gbe >>> driver). >> >> Even if it doesn't break Simon's board, why remove the ability for a >> board to add eth_init code. You're trying to say that there is no case >> where a DM board would need to init anything related to eth. That >> seems unlikely. >> > > I believe with driver model, we should avoid these calls. All the > drive initialization needs to be done in the bind or probe phase, and > called by driver model automatically. Like pci_eth_init() which just > calls non-dm ethernet drivers, and pci_eth_init() can be called from > board_eth_init() or cpu_eth_init(). But I think you are right, there > might be some board-specific thing to be put there, even right now it > does not break any board. But that's maybe we don't have proper driver > model uclass to handle these misc things? Normally the board init should happen in board_init(). We don't currently have a good way to handle 'associated init' for drivers. For example, if an Ethernet port needs some pinmux settings, or some clock settings, with driver model we currently have to init this in board_init() so it is not 'lazy init', it always happens at start of day. I don't actually think this is a problem in practice, but it is not in the spirit of driver model. Drivers should 'pull in' the init that they need and we should make that work automatically. But it is very important the we do not put this init into the drivers themselves. The drivers need to be generic. E.g. an Ethernet driver should be able to operate on any board that has that Ethernet controller. The setup to make the controller available on pins, enable it clock it correctly should not be in driver code. Soon we will have pinctrl which will deal with the pinmux problem. We already have a clock framework would could handle clocking. So progress is being made. I wonder whether we will end up wanting a way to request that the resources for a driver be activated, in a general sense. But until we have a use case I'm not keen to spend much time thinking about it. > >> Also, why is it hurting your board to have an optional call to such a >> function. Presumably you just don't define those functions and you're >> fine, right? >> > > It does not hurt but I think at least we should comment out the > following printf for DM. > > } else { > printf("Net Initialization Skipped\n"); > } > > This is misleading message. > >> I guess it can just be put back when such a board is converted. >> > > Regards, > Bin Regards, Simon
diff --git a/net/eth.c b/net/eth.c index d3ec8d6..0b4b08a 100644 --- a/net/eth.c +++ b/net/eth.c @@ -96,6 +96,7 @@ static void eth_common_init(void) phy_init(); #endif +#ifndef CONFIG_DM_ETH /* * If board-specific initialization exists, call it. * If not, call a CPU-specific one @@ -109,6 +110,7 @@ static void eth_common_init(void) } else { printf("Net Initialization Skipped\n"); } +#endif } #ifdef CONFIG_DM_ETH
With driver model, board_eth_init() or cpu_eth_init() is not needed. Remove the call to these in eth_common_init(). Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- net/eth.c | 2 ++ 1 file changed, 2 insertions(+)