Hello,
> -----Original Message-----
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 4:53 PM
> To: Kwok, WingMan
> Cc: linux-arm-ker...@lists.infradead.org; robh...@kernel.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; KISHON VIJAY ABRAHAM; Quadros, Roger; Karicheri,
> Muralidharan; bhelg...@google.com; ssant...@kernel.org;
> li...@arm.linux.org.uk; devicet...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
>
> On Thursday 15 October 2015 20:08:32 Kwok, WingMan wrote:
> >
> > > > +#define reg_rmw(addr, value, mask) \
> > > > + __raw_writel(((__raw_readl(addr) & (~(mask))) | \
> > > > + (value & (mask))), (addr))
> > >
> > > not endian safe, and potentially racy.
> > >
> >
> > will change to
> >
> > #define reg_rmw(addr, value, mask) \
> > writel(((readl(addr) & (~(mask))) | \
> > (value & (mask))), (addr))
>
> Ok, sounds good. Note that if any of this is performance critical,
> better use readl_relaxed(), but as long as this is just for setup
> code and not for data transfers, staying with readl() as you
> suggest is better.
>
since this is only for initialization, I'll probably stay with readl().
> > > > +static inline void _kserdes_reset_cdr(void __iomem *sregs, int lane)
> > > > +{
> > > > + /* toggle signal detect */
> > > > + _kserdes_force_signal_detect_low(sregs, lane);
> > > > + mdelay(1);
> > > > + _kserdes_force_signal_detect_high(sregs, lane);
> > > > +}
> > >
> > > Can you change the code so you can use msleep(1) here?
> > >
> >
> > will replace delays with usleep_range()
>
> Ok.
>
> > > > +
> > > > + do {
> > > > + mdelay(10);
> > > > + memset(lane_down, 0, sizeof(lane_down));
> > > > +
> > > > + link_up = _kserdes_check_link_status(dev, sregs,
> > > > + pcsr_regmap, lanes,
> > > > + lanes_enable,
> > > > + current_state,
> lane_down);
> > > > +
> > > > + /* if we did not get link up then wait 100ms
> > > > + * before calling it again
> > > > + */
> > > > + if (link_up)
> > > > + break;
> > > > +
> > > > + for (i = 0; i < lanes; i++) {
> > > > + if ((lanes_enable & (1 << i)) && lane_down[i])
> > > > + dev_dbg(dev,
> > > > + "XGE: detected lane down on lane
> %d\n",
> > > > + i);
> > > > + }
> > > > +
> > > > + if (++retries > 100)
> > > > + return -ETIMEDOUT;
> > > > +
> > > > + } while (!link_up);
> > >
> > > an more importantly here. Blocking the CPU for over one second is not
> good.
> > >
> > > Any use of mdelay() should have a comment explaining why you cannot use
> > > msleep() in that instance.
> > >
> >
> > will replace delays with usleep_range()
>
> Here you have to be careful with the total runtime. Using usleep_range()
> is a good idea, and you can have a particularly wide range, but then you
> should changen the timeout condition from number of retries to total
> elapsed time like
>
> unsigned long timeout = jiffies + HZ; /* 1 second maximum */
> do {
> ...
>
> if (link_up)
> break;
>
> if (time_after(jiffies, timeout)
> return -ETIMEOUT;
>
> usleep_range(1000, 50000);
> } while (1);
>
will do.
> Arnd
Thanks so much for your comments.
WingMan
N�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf��^jǫy�m��@A�a���
0��h���i
