linux-kernel - RE: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Nov 2020 09:28:34 +0000
From:   "Wan Mohamad, Wan Ahmad Zainie" 
        <wan.ahmad.zainie.wan.mohamad@...el.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC:     "kishon@...com" <kishon@...com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "mgross@...ux.intel.com" <mgross@...ux.intel.com>,
        "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>
Subject: RE: [PATCH v2 2/2] phy: intel: Add Keem Bay USB PHY support

Hi Andy.

Thanks for the review and sorry for the late reply.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Sent: Monday, November 9, 2020 7:41 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@...el.com>
> Cc: kishon@...com; vkoul@...nel.org; robh+dt@...nel.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org;
> mgross@...ux.intel.com; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@...el.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