linux-kernel - Re: [PATCH V3] phy: bcm-ns-usb2: new driver for USB 2.0 PHY on Northstar

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 14 Apr 2016 09:28:14 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Kishon Vijay Abraham I <kishon@...com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Hauke Mehrtens <hauke@...ke-m.de>,
	Felix Fietkau <nbd@...nwrt.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jon Mason <jon.mason@...adcom.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
	Felipe Balbi <balbi@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH V3] phy: bcm-ns-usb2: new driver for USB 2.0 PHY on Northstar

Hi and thanks for your review!

On 13 April 2016 at 15:54, Kishon Vijay Abraham I <kishon@...com> wrote:
> On Monday 11 April 2016 03:13 PM, Rafał Miłecki wrote:
>> +Example:
>> +     usb2-phy {
>> +             compatible = "brcm,ns-usb2-phy";
>> +             reg = <0x1800c000 0x1000>;
>> +             reg-names = "dmu";
>> +             #phy-cells = <0>;
>> +             #clock-cells = <0>;
>
> Is clock-cells required here? It's generally added for clock providers right?

You're right, it's not.


>> +static int bcm_ns_usb2_phy_init(struct phy *phy)
>> +{
>> +     struct bcm_ns_usb2 *usb2 = phy_get_drvdata(phy);
>> +     struct device *dev = usb2->dev;
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct resource *res;
>> +     void __iomem *dmu;
>> +     u32 ref_clk_rate, usb2ctl, usb_pll_ndiv, usb_pll_pdiv;
>> +     int err = 0;
>> +
>> +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dmu");
>> +     dmu = devm_ioremap_resource(dev, res);
>> +     if (IS_ERR(dmu)) {
>> +             dev_err(dev, "Failed to map DMU regs\n");
>> +             err = -EIO;
>> +             goto err_out;
>> +     }
>
> ioremap should ideally be in probe function.

Sure, will change it.


>> +     err = clk_prepare_enable(usb2->ref_clk);
>> +     if (err < 0) {
>> +             dev_err(dev, "Failed to prepare ref clock: %d\n", err);
>> +             goto err_iounmap;
>> +     }
>> +
>> +     ref_clk_rate = clk_get_rate(usb2->ref_clk);
>> +     if (!ref_clk_rate) {
>
> use IS_ERR?

clk_get_rate returns unsigned long, not a pointer


>> +             dev_err(dev, "Failed to get ref clock rate\n");
>> +             err = -EINVAL;
>> +             goto err_clk_off;
>> +     }
>> +
>> +     usb2ctl = ioread32(dmu + BCMA_DMU_CRU_USB2_CONTROL);
>
> use readl and friends.

OK


>> +     usb_pll_pdiv = usb2ctl;
>> +     usb_pll_pdiv &= BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_PDIV_MASK;
>> +     usb_pll_pdiv >>= BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_PDIV_SHIFT;
>> +     if (!usb_pll_pdiv)
>> +             usb_pll_pdiv = 1 << 3;
>
> this can be
> if (!(usb2ctl & BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_PDIV_MASK))
>         usb_pll_pdiv = 1 << 3;



>> +     /* Calculate ndiv based on a solid 1920 MHz that is for USB2 PHY */
>> +     usb_pll_ndiv = (1920000000 * usb_pll_pdiv) / ref_clk_rate;
>> +
>> +     /* Unlock DMU PLL settings */
>> +     iowrite32(0x0000ea68, dmu + BCMA_DMU_CRU_CLKSET_KEY);
>
> What is 0x0000ea68? Please avoid hard coding values. It makes difficult to review.

I'd love to define every single bit, but I don't know them. I didn't
get or see any programming guide from Broadcom for 10 years now. I'm
just using magic value I found in reference code in Broadcom's SDK.


>> +     /* Write USB 2.0 PLL control setting */
>> +     usb2ctl &= ~BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_NDIV_MASK;
>> +     usb2ctl |= usb_pll_ndiv << BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_NDIV_SHIFT;
>> +     iowrite32(usb2ctl, dmu + BCMA_DMU_CRU_USB2_CONTROL);
>> +
>> +     /* Lock DMU PLL settings */
>> +     iowrite32(0x00000000, dmu + BCMA_DMU_CRU_CLKSET_KEY);
>
> So the PHY has only a PLL that has to be configured?

Yes to my best knowledge.


>> diff --git a/include/linux/bcma/bcma_driver_arm_c9.h b/include/linux/bcma/bcma_driver_arm_c9.h
>> new file mode 100644
>> index 0000000..93bd73d
>> --- /dev/null
>> +++ b/include/linux/bcma/bcma_driver_arm_c9.h
>> @@ -0,0 +1,15 @@
>> +#ifndef LINUX_BCMA_DRIVER_ARM_C9_H_
>> +#define LINUX_BCMA_DRIVER_ARM_C9_H_
>> +
>> +/* DMU (Device Management Unit) */
>> +#define BCMA_DMU_CRU_USB2_CONTROL                    0x0164
>> +#define  BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_NDIV_MASK 0x00000FFC
>> +#define  BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_NDIV_SHIFT        2
>> +#define  BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_PDIV_MASK 0x00007000
>> +#define  BCMA_DMU_CRU_USB2_CONTROL_USB_PLL_PDIV_SHIFT        12
>> +#define BCMA_DMU_CRU_CLKSET_KEY                              0x0180
>> +#define BCMA_DMU_CRU_STRAPS_CTRL                     0x02A0
>> +#define  BCMA_DMU_CRU_STRAPS_CTRL_USB3                       0x00000010
>> +#define  BCMA_DMU_CRU_STRAPS_CTRL_4BYTE                      0x00008000
>
> If these are specific to PHY, then add it inside the PHY driver.

DMU registers are also used by other drivers, but I should definitely
mention that, I'll update commit message in next version.

-- 
Rafał