On 06/09/2014 10:26 AM, Jisheng Zhang wrote:

Dear Sebastian and Antoine,

On Fri, 6 Jun 2014 03:54:06 -0700
Sebastian Hesselbarth [off-list ref] wrote:

quoted

quoted

+
+#define to_berlin_phy_priv(p)	container_of((p), struct
berlin_phy_priv, phy) +
+struct berlin_phy_priv {
+	void __iomem		*base;
+	struct usb_phy		phy;
+	struct reset_control	*rst_ctrl;
+	int			pwr_gpio;

Is the GPIO used for USB power? If so, we should not rely on

The GPIO is used for vbus. Sorry for using the confusing "pwr". Do we still
need to use regulator API?

Yes, I guess using regulator is still the way to go. Also, I think
it should be up to the controller to power on/off the device. That
way, we could make use of the dual role controller features on BG2
and BG2CD.

quoted

GPIO at all but use regulator API. Thinking of Chromecast which
is externally powered over USB, there will be no regulator nor
GPIO at all.

quoted

+};
+
+static int berlin_phy_init(struct usb_phy *phy)
+{
+	struct berlin_phy_priv *priv = to_berlin_phy_priv(phy);
+	int ret;
+
+	reset_control_reset(priv->rst_ctrl);
+
+	writel(CLK_REF_DIV(0xc) | FEEDBACK_CLK_DIV(0x54),
+	       priv->base + USB_PHY_PLL);

@Jisheng: IIRC the dividers above are different for BG2? Can you please
evaluate?

Yes, BG2 uses different refdiv and fbdiv. Is there any suggestions about how to
handle this difference? The value is chosen after carefully tunning

I guess it depends on how many different div values you expect for
berlin2 usb PHYs. If it is just the two, we can go with different
compatibles for e.g. "berlin2-usb-phy" and "berlin2cd-usb-phy".

If you know of more PHYs with different div, a corresponding vendor-
specific property should do the trick, e.g.
marvell,pll-divider = <0x54c0>;

I am fine with both.

quoted

quoted

+	writel(CLK_STABLE | PLL_CTRL_REG | PHASE_OFF_TOL_250 |
KVC0_REG_CTRL |
+	       CLK_BLK_EN, priv->base + USB_PHY_PLL_CONTROL);
+	writel(V2I_VCO_RATIO(0x5) | R_ROTATE_0 | ANA_TEST_DC_CTRL(0x5),
+	       priv->base + USB_PHY_ANALOG);
+	writel(PHASE_FREEZE_DLY_4_CL | ACK_LENGTH_16_CL | SQ_LENGTH_12 |
+	       DISCON_THRESHOLD_260 | SQ_THRESHOLD(0xa) | LPF_COEF(0x2) |
+	       INTPL_CUR_30, priv->base + USB_PHY_RX_CTRL);
+
+	writel(TX_VDD12_13 | TX_OUT_AMP(0x3), priv->base +
USB_PHY_TX_CTRL1);
+	writel(EXT_HS_RCAL_EN | IMPCAL_VTH_DIV(0x3) |
EXT_RS_RCAL_DIV(0x4),
+	       priv->base + USB_PHY_TX_CTRL0);
+
+	writel(EXT_HS_RCAL_EN | IMPCAL_VTH_DIV(0x3) |
EXT_RS_RCAL_DIV(0x4) |
+	       EXT_FS_RCAL_DIV(0x2), priv->base + USB_PHY_TX_CTRL0);
+
+	writel(EXT_HS_RCAL_EN | IMPCAL_VTH_DIV(0x3) |
EXT_RS_RCAL_DIV(0x4),
+	       priv->base + USB_PHY_TX_CTRL0);
+	writel(TX_CHAN_CTRL_REG(0xf) | DRV_SLEWRATE(0x3) |
IMP_CAL_FS_HS_DLY_3 |
+	       FS_DRV_EN_MASK(0xd), priv->base + USB_PHY_TX_CTRL2);
+
+	ret = gpio_direction_output(priv->pwr_gpio, 0);

As mentioned above, this should be using regulator API. And also, if
there is no dummy regulator allowed, it should be optional.

quoted

+	if (ret)
+		return ret;
+
+	gpio_set_value(priv->pwr_gpio, 1);
+
+	return 0;
+}
+
+static void berlin_phy_shutdown(struct usb_phy *phy)
+{
+	struct berlin_phy_priv *priv = to_berlin_phy_priv(phy);
+
+	gpio_set_value(priv->pwr_gpio, 0);
+}
+
+static int berlin_phy_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct berlin_phy_priv *priv;
+	struct resource *res;
+	int ret, gpio;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->rst_ctrl = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->rst_ctrl)) {
+		ret = PTR_ERR(priv->rst_ctrl);
+		dev_err(&pdev->dev, "cannot get reset controller: %d\n",
ret);

Hmm, considering a non arch_init call registered reset driver, it does
also spit out an error for -EPROBE_DEFER, does it?

quoted

+		return ret;
+	}
+
+	gpio = of_get_named_gpio(np, "power-gpio", 0);
+	if (!gpio_is_valid(gpio))
+		return gpio;

Some BG2Q boards hardwired the vbus to be always powered on, we should continue
the probe if vbus gpio is missing.

Yeah, the same applies for regulators. But with the comments above, it
should move to the controller stub instead.

Sebastian