Hi and thanks for your review!
On 13 April 2016 at 15:54, Kishon Vijay Abraham I <kishon@xxxxxx> 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ł
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
