On 10/20/2016 01:30 PM, Joe Hershberger wrote:
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"?
Perhaps we should make a run-time decision rather than compile-time.
This will allow a single system to contain multiple instances of the
EQoS HW block with different behaviour, e.g. 1 embedded in the SoC, and
1 plug-in PCIe device.
We can assume that for any PCIe device, we can implement write_hwaddr()
without issue.
For devices instantiated from DT, the SoC-level integration details will
drive whether there is SW control over the HW block's clock/reset
signals. We can use struct udevice_id's .data field to provide
parameters for each different DT compatible value that the driver
supports, which can indicate how write_hwaddr() should work.
Then, we can implement roughly the following:
write_hwaddr:
if (!dev->reg_access_ok && !dev->reg_access_always_ok)
return 0;
write MAC address registers
That should also allow write_hwaddr to operate correctly if
start()/stop() are drivers are refactored not to start()/stop() for
every separate network transaction, since presumably the code that sets
reg_access_ok will be refactored as part of that.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
