Messages in this thread | | | From | "Wan Mohamad, Wan Ahmad Zainie" <> | Subject | RE: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support | Date | Wed, 11 Nov 2020 09:28:34 +0000 |
| |
Hi Andy.
Thanks for the review and sorry for the late reply.
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Monday, November 9, 2020 7:41 PM > To: Wan Mohamad, Wan Ahmad Zainie > <wan.ahmad.zainie.wan.mohamad@intel.com> > Cc: kishon@ti.com; vkoul@kernel.org; robh+dt@kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; > mgross@linux.intel.com; Raja Subramanian, Lakshmi Bai > <lakshmi.bai.raja.subramanian@intel.com> > Subject: Re: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support > > On Mon, Nov 09, 2020 at 11:16:54AM +0800, Wan Ahmad Zainie wrote: > > Add support for USB PHY on Intel Keem Bay SoC. > > ... > > > +config PHY_INTEL_KEEMBAY_USB > > + tristate "Intel Keem Bay USB PHY driver" > > + depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST) > > > + depends on OF && HAS_IOMEM > > Do you really need dependency to OF (yes, I see that it will be not functional, > but still can be compile tested)?
No, I can drop OF. I will remove in v3.
> > > + select GENERIC_PHY > > + select REGMAP_MMIO > > + help > > + Choose this option if you have an Intel Keem Bay SoC. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called phy-keembay-usb.ko. > > ... > > > +#include <linux/bitfield.h> > > +#include <linux/bits.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/module.h> > > > +#include <linux/of.h> > > No evidence of anything being used in this code. > mod_devicetable.h is missed, though.
I will fix in v3. Remove of.h and add mod_devicetable.h.
> > > +#include <linux/phy/phy.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > ... > > > + usleep_range(30, 50); > > Why 30-50?
I take this value from boot firmware. There is a delay of 30us after clearing IDDQ_enable bit. I believe the purpose is to ensure all analog blocks are powered up.
> > ... > > > + usleep_range(20, 50); > > Why these numbers?
In Keem Bay data book, under USB initialization section, there is step that there must be a minimum 20us wait after clock enable, before bringing PHYs out of reset.
50 is the value that I picked randomly. Is usleep_range(20, 20) Better?
> > ... > > > + usleep_range(2, 10); > > Ditto.
Under the same section above, there is a step for 2us wait. I believe it is for register write to go through.
> > ... > > > + usleep_range(20, 50); > > Ditto.
Under the same section above, there is a step to wait 20us after setting SRAM load bit, before release the controller reset.
I will add comment for those 4 delay above.
Before I proceed with v3, I would like to know if I should use udelay(), instead of usleep_range()? I refer to https://www.kernel.org/doc/Documentation/timers/timers-howto.txt.
> > > ... > > > + struct device_node *np = dev->of_node; > > It's being used only once and it doesn't bring any benefit.
I will fix in v3. Use dev->of_node directly.
> > -- > With Best Regards, > Andy Shevchenko >
Best regards, Zainie
| |