On Thu, Oct 20, 2016 at 2:19 PM, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 10/20/2016 12:48 PM, Simon Glass wrote: >> >> Hi, >> >> On 19 October 2016 at 15:14, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 10/19/2016 12:29 PM, Joe Hershberger wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swar...@wwwdotorg.org> >>>> wrote: >>>>> >>>>> >>>>> On 10/13/2016 05:46 PM, Joe Hershberger wrote: >>>>>> >>>>>> >>>>>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren >>>>>> <swar...@wwwdotorg.org> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren >>>>>>>> <swar...@wwwdotorg.org> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren >>>>>>>>>> <swar...@wwwdotorg.org> wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This driver supports the Synopsys Designware Ethernet QoS >>>>>>>>>>> (Quality >>>>>>>>>>> of >>>>>>>>>>> Service) a/k/a eqos IP block, which is a different design than >>>>>>>>>>> the >>>>>>>>>>> HW >>>>>>>>>>> supported by the existing designware.c driver. The IP supports >>>>>>>>>>> many >>>>>>>>>>> options for bus type, clocking/reset structure, and feature list. >>>>>>>>>>> This >>>>>>>>>>> driver currently supports the specific configuration used in >>>>>>>>>>> NVIDIA's >>>>>>>>>>> Tegra186 chip, but should be extensible to other combinations >>>>>>>>>>> quite >>>>>>>>>>> easily, as explained in the source. >>> >>> >>> ... >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> +static int eqos_start(struct udevice *dev) >>> >>> >>> ... >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> + /* Update the MAC address */ >>>>>>>>>>> + val = (plat->enetaddr[5] << 8) | >>>>>>>>>>> + (plat->enetaddr[4]); >>>>>>>>>>> + writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH); >>>>>>>>>>> + val = (plat->enetaddr[3] << 24) | >>>>>>>>>>> + (plat->enetaddr[2] << 16) | >>>>>>>>>>> + (plat->enetaddr[1] << 8) | >>>>>>>>>>> + (plat->enetaddr[0]); >>>>>>>>>>> + writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW); >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> This should be implemented in write_hwaddr() op. >>> >>> >>> ... >>>>>>> >>>>>>> >>>>>>> Anyway, I still don't believe using write_hwaddr() is correct for >>>>>>> this >>>>>>> HW. >>>>>>> It's marked optional in include/net.h; it would be implemented in >>>>>>> cases >>>>>>> where the MAC address should be passed to subsequent SW in Ethernet >>>>>>> controller registers. That's not the case here. The master location >>>>>>> for >>>>>>> the MAC address is in an unrelated EEPROM that all drivers must read. >>>>>> >>>>>> >>>>>> >>>>>> That sounds more like a NV storage location for a read_rom_hwaddr() op >>>>>> to get a default mac addr that can be overridden with the env. >>>>> >>>>> >>>>> >>>>> If the EQoS HW module contained the interface to this EEPROM, such that >>>>> all >>>>> instances of the HW module always accessed the EEPROM in the same way >>>>> and >>>>> the layout of data in the EEPROM was fixed by the HW module, then yes. >>>>> >>>>> However, the EqoS HW module doesn't define any mechanism for >>>>> non-volatile >>>>> MAC address storage; only the runtime registers. So, we can't implement >>>>> read_rom_hwaddr() inside the EQoS driver unfortunately. >>>> >>>> >>>> >>>> OK. >>>> >>>>>> The >>>>>> write_hwaddr is about what the mac uses to filter for limiting packet >>>>>> ingress. One reason to support it as an op is so that when the env var >>>>>> for the mac address is changed, the mac filter in the hw is also >>>>>> updated. >>>>> >>>>> >>>>> >>>>> I believe that every time the Ethernet device is used, the start() op >>>>> is >>>>> called first, followed by packet transfer, followed by the stop() op. >>>>> If >>>>> start() always programs the MAC address, the driver will always end up >>>>> using >>>>> the value requested by the user. >>>> >>>> >>>> >>>> That may be. I still don't understand the reluctance to implement it. >>> >>> >>> >>> I don't want to implement it because it can't work. >>> >>> write_hwaddr() is called before start() is called. At that point, clocks >>> to >>> the EQoS HW are not running and the EQoS HW is in reset, and hence it >>> cannot >>> accept any register accesses; attempting any accesses will hang the bus >>> and >>> CPU. >>> >>> These are the possible solutions: >>> >>> a) Don't implement write_hwaddr() >>> >>> b) Make write_hwaddr() turn on the clock and clear the reset, program the >>> register, then reset the device and assert the reset. Re-asserting reset >>> is >>> required so that setting the MAC address doesn't leave the clock running >>> even when the device isn't in use.This is pointless since the written >>> value >>> will not last beyond the end of the function. >>> >>> c) Make probe() start the clock and clear the reset. Then write_hwaddr() >>> can >>> successfully write the MAC address registers at any time. This would >>> waste >>> power running the clock to the device when it's not in use. Also, Simon >>> Glass continually asks that U-Boot not initialize HW that the user hasn't >>> attempted to use. I believe turning on the clocks in probe() violates >>> this. >> >> >> Not quite...or at least if I did I was mistaken. Of course we should >> limit init in probe() to what is necessary, but it is the bind() >> method which must not touch hardware. >> >> It is fine to turn clocks on in probe if you want to. > > > Even for a device that the user never ultimately makes use of? If so, this > might be a reasonable solution. It feels like U-Boot should turn off the > clocks before booting an OS though so that the overall system state isn't > any different between the cases where this driver is present and enabled vs > not. With the clocks manipulated by start()/stop() we already do this. If > the clocks are enabled in probe() instead, this won't be the case. > >> However I am wondering whether we have something wrong in the Ethernet >> uclass interface. Should we move the MAC setting to after start(), or >> similar? > > > That's option d right below. > > Joe's objection to this is that for some hardware, downstream OSs expect the > MAC address to be programmed into controller registers before the OS starts. > This required write_hwaddr() to be called even if the user doesn't actually > make use of the Ethernet device, and hence in cases where start()/stop() are > never called. This is probably more common for e.g. PCI devices where the > bus imposes a certain system structure including the ability to access > device registers immediately after boot without manual clock programming, > rather than SoC-based designs where all bets are off regarding consistency, > and system configuration data is more typically passed by either out-of-band > configuration data structures like DT, or via entirely custom mechanisms.
Maybe we should just make it a config option to select between the two behaviors. Or is there already something that says "This system is structured" vs "This system is a SoC / all bets are off"? >>> d) Modify the network core to only call write_hwaddr() between the >>> device's >>> start() and stop() functions. I haven't looked into this at all, but I >>> imagine it will have a fairly significant impact across many parts of the >>> network core and other drivers. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
