Hi,
> -----Original Message-----
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, October 15, 2015 10:51 AM
> To: linux-arm-ker...@lists.infradead.org
> Cc: Kwok, WingMan; 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-kernel@vger.kernel.org; linux-
> p...@vger.kernel.org
> Subject: Re: [PATCH v1 1/2] phy: keystone: serdes driver for gbe 10gbe and
> pcie
>
> On Thursday 15 October 2015 10:25:44 WingMan Kwok wrote:
> > On TI's Keystone platforms, several peripherals such as the
> > gbe ethernet switch, 10gbe ethernet switch and PCIe controller
> > require the use of a SerDes for converting SoC parallel data into
> > serialized data that can be output over a high-speed electrical
> > interface, and also converting high-speed serial input data
> > into parallel data that can be processed by the SoC. The
> > SerDeses used by those peripherals, though they may be different,
> > are largely similar in functionality and setup.
> >
> > This patch provides a SerDes phy driver implementation that can be
> > used by the above mentioned peripheral drivers to configure their
> > respective SerDeses.
> >
> > v1:
> > - see cover letter for review comments addressed.
> >
> > Signed-off-by: WingMan Kwok <w-kw...@ti.com>
> > ---
> > Documentation/devicetree/bindings/phy/ti-phy.txt | 278 +++
> > drivers/phy/Kconfig | 8 +
> > drivers/phy/Makefile | 1 +
> > drivers/phy/phy-keystone-serdes.c | 2373
> ++++++++++++++++++++++
> > 4 files changed, 2660 insertions(+)
> > create mode 100644 drivers/phy/phy-keystone-serdes.c
>
> This is quite a bit of code. Are you very sure that this PHY is
> not used on any other SoC family, and that it is not licensed
> from a third party? I would hate to see multiple copies of
> this getting merged into the kernel over time, so thename should
> be chosen carefully to let the next person know when they have
> related hardware.
>
> > +
> > +gbe_serdes0: gbe_serdes@232a000 {
>
>
> make that phy@232a000, the name should be one of the usual identifiers,
> not specific to the instance.
>
will change to something like gbe_serdes0: phy@232a000 {};
> > +config PHY_TI_KEYSTONE_SERDES
> > + tristate "TI Keystone SerDes PHY support"
> > + depends on OF && ARCH_KEYSTONE
> > + select GENERIC_PHY
> > + help
> > + This option enables support for TI Keystone SerDes PHY found
> > + in peripherals GBE, 10GBE and PCIe.
> > +
>
> (ARCH_KEYSTONE || COMPILE_TEST) ?
>
will add COMPILE_TEST
> > + * Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the
> > + * distribution.
>
> The current code does not do this when compiled, which might be a
> problem for distributors. Can you clarify the license?
>
will investigate
> > +#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))
> > +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()
> > +
> > + 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()
> > +
> > +static int __init keystone_serdes_phy_init(void)
> > +{
> > + return platform_driver_register(&kserdes_driver);
> > +}
> > +module_init(keystone_serdes_phy_init);
> > +
> > +static void __exit keystone_serdes_phy_exit(void)
> > +{
> > + platform_driver_unregister(&kserdes_driver);
> > +}
> > +module_exit(keystone_serdes_phy_exit);
>
> module_platform_driver()
>
will do.
> Arnd
Thanks,
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
