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?
>
> > + * FIXME: This part should going up to arch/powerpc -- but where?
>
> s/going/go/ ?
> s/rrr/rr/ ?
yep.
>
> > +int ll_temac_halt_sdma(struct eth_device *dev)
> > +{
> > + unsigned timeout = 2000;
>
> See comments above.
>
> > + if (!timeout) {
> > + printf("%s: Timeout\n", __func__);
> > + return 1;
> > + }
>
> Interesting - here you return an error code, above you don't :-(
Grr, ache on my head. I need one more look to the timeouts (see above).
> Same issues again. Please fix timout and error handling globally.
>
> > +
> > +#if defined(CONFIG_XILINX_440) || defined(CONFIG_XILINX_405)
> > +/* Check for TX and RX channel errrors. */
> > +static inline int ll_temac_dmac_error(struct eth_device *dev)
> > +{
> > + int err;
> > +--snip--
>
> Umm.... this code is more or less exactly the same as for
> ll_temac_recv_sdma(). Can we please avoid this duplication of code?
>
> Ditto for the other duplicated functions.
You are completely right, but for more than one week I have not found
any practicable way to avoid duplicate code here. The only really good
approach would be to use private function pointers in struct ll_temac
for all the in_*/out_* calls we need in sdma code and refactor the DCR
code to use its special in_*/out_* functions with the same prototype.
This special 'DCR' function can map into indirect DCR access. But
unfortunately in_be32() and out_be32() for Microblaze are not real
functions. They are CPP defines :-(
I will review arch/microblaze/include/asm/io.h together with Michal.
Until then it would be nice if we could keep this code as it is. I will
fix the dublicate code if we have in/out functions -- no defines.
--
Viele Grüße,
Stephan Linz
______________________________________________________________________________
MB-Ref: http://www.li-pro.de/xilinx_mb:mbref:start
OpenDCC: http://www.li-pro.net/opendcc.phtml
PC/M: http://www.li-pro.net/pcm.phtml
Sourceforge: http://sourceforge.net/users/slz
Gitorious: https://gitorious.org/~slz
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
