Stephan Linz wrote:
Am Sonntag, den 27.11.2011, 20:09 +0100 schrieb Wolfgang Denk:
Dear Stephan Linz,
In message <1322419397-26326-2-git-send-email-l...@li-pro.net> you wrote:
Xilinx LocalLink Tri-Mode Ether MAC driver can be
used by Xilinx Microblaze or Xilinx ppc405/440 in
SDMA and FIFO mode. DCR or XPS bus can be used.
The driver uses and requires MII and PHYLIB.
Signed-off-by: Stephan Linz <l...@li-pro.net>
...
+static inline void xps_ll_temac_check_status(struct temac_reg *regs, u32 mask)
+{
+ unsigned timeout = 2000;
+ while (timeout && (!(in_be32(®s->rdy) & mask)))
+ timeout--;
This has been asked before: what exactly is the limit for the timeout
here? 2000 loops though that loop is not exactly a known value.
Please add some udelay() here (which is also good for triggering the
watchdog, should you have one running).
Hi Wolfgang,
I'll try to make the timeout handling more precise. Your argument with
the watchdog is an interesting issue I forget completely. Thanks for the
hint.
+ if (!timeout)
+ printf("%s: Timeout\n", __func__);
Why is this function void when you are know that you might have to
handle error situations (like a timeout)?
+static void xps_ll_temac_hostif_set(struct eth_device *dev, u8 phy_addr,
+ u8 reg_addr, u16 phy_data)
+{
+ struct temac_reg *regs = (struct temac_reg *)dev->iobase;
+
+ out_be32(®s->lsw, (phy_data & LSW_REGDAT_MASK));
+ out_be32(®s->ctl, CTL_WEN | TEMAC_MIIMWD);
+ out_be32(®s->lsw,
+ ((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
+ (reg_addr & LSW_REGAD_MASK));
+ out_be32(®s->ctl, CTL_WEN | TEMAC_MIIMAI);
+ xps_ll_temac_check_status(regs, RSE_MIIM_WR);
+}
So what happens here if we have a timeout in
xps_ll_temac_check_status()? SHould such an error not be handled?
Yes we should. I'll fix it.
+ * Undirect hostif read to ll_temac.
"Undirect"?? Or "Indirect" ?
"read to"?? Or "read from" ?
Please fix globally.
yep.
+#ifdef DEBUG
+static inline void read_phy_reg(struct eth_device *dev, u8 phy_addr)
+{
+ int j, result;
+ debug("phy%d ", phy_addr);
Please always have one blank line between declarations and code.
Please fix globally.
yep.
+ if (ll_temac->ctrlreset)
+ if (ll_temac->ctrlreset(dev))
+ return -1;
Braces needed for multiline statements. Please fix globally.
yep.
+/* halt device */
+static void ll_temac_halt(struct eth_device *dev)
+{
+#ifdef ETH_HALTING
+ struct ll_temac *ll_temac = dev->priv;
+
+ /* Disable Receiver */
+ xps_ll_temac_indirect_set(dev, TEMAC_RCW0, 0);
+
+ /* Disable Transmitter */
+ xps_ll_temac_indirect_set(dev, TEMAC_TC, 0);
+
+ if (ll_temac->ctrlhalt)
+ ll_temac->ctrlhalt(dev);
+#endif
+}
ETH_HALTING is permanently undef'ed, so all this is dead code. Please
remove.
@Michal: Is there any platform which can not halt the TEMAC? I have no
problem with this code but I left ETH_HALTING undefined from original
driver code. I would like to remove all the ETH_HALTING statements and
hold this code.
+static int ll_temac_bus_reset(struct mii_dev *bus)
+{
+ debug("Just bus reset\n");
+ return 0;
+}
Can we remove this function? It does not appear to perform any useful
operation?
@Michal: Have you any objections?
@all: What is the meaning of mii_dev bus reset? What is expacted from
the perspective of the MII driver code?
It was necessary before this patch.
phylib: reset mii bus only if reset handler is registered
sha1: e3a77218a256edbe201112a39beeed8adcabae3f
Currently you can simple remove.
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
