Re: [PATCH 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy

[Frank Wang <frank.wang@rock-chips.com>]

Hi Heiko,

On 06/02/2016 07:46 AM, Heiko Stübner wrote:
> Hi Frank,
>
> Am Dienstag, 31. Mai 2016, 14:40:11 schrieb Frank Wang:
>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>> than rk3288 and before, and most of phy-related registers are also
>> different from the past, so a new phy driver is required necessarily.
>>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>
> [...]
>
>> +static void rockchip_usb2phy_clk480m_disable(struct clk_hw *hw)
>> +{
>> +	struct rockchip_usb2phy *rphy =
>> +		container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>> +	int index;
>> +
>> +	/* make sure all ports in suspended mode */
>> +	for (index = 0; index != rphy->phy_cfg->num_ports; index++)
>> +		if (!rphy->ports[index].suspended)
>> +			return;
>
> This function can only get called when all clk-references have disabled the
> clock, so you should never reach that point that one phy port is not
> suspended?
>

Yeah, you are right, I will clean them up in the next patches.

>> +
>> +	/* turn off 480m clk output */
>> +	property_enable(rphy, &rphy->phy_cfg->clkout_ctl, false);
>> +}
>> +
>
> [...]
>
> add something like:
>
> static void rockchip_usb2phy_clk480m_unregister(void *data)
> {
> 	struct rockchip_usb2phy *rphy = data;
>
> 	of_clk_del_provider(rphy->dev->of_node);
> 	clk_unregister(rphy->clk480m);
> }
>
>> +static struct clk *
>> +rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>> +{
>> +	struct device_node *node = rphy->dev->of_node;
>> +	struct clk *clk;
>> +	struct clk_init_data init;
> 	int ret;
>
>> +
>> +	init.name = "clk_usbphy_480m";
>> +	init.ops = &rockchip_usb2phy_clkout_ops;
>> +	init.flags = CLK_IS_ROOT;
>> +	init.parent_names = NULL;
>> +	init.num_parents = 0;
>> +	rphy->clk480m_hw.init = &init;
>> +
>> +	/* optional override of the clockname */
>> +	of_property_read_string(node, "clock-output-names", &init.name);
>> +
>> +	/* register the clock */
>> +	clk = clk_register(rphy->dev, &rphy->clk480m_hw);
> 	if (IS_ERR(clk))
> 		return clk:
>
> 	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> 	if (ret < 0)
> 		goto err_clk_provider;
>
> 	ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
> rphy);
> 	if (ret < 0)
> 		goto err_unreg_action;
>
> 	return clk;
>
> err_unreg_action:
> 	of_clk_del_provider(node);
> err_clk_provider:
> 	clk_unregister(clk);
> 	return ERR_PTR(ret);
>
>> +}
>

Good comments! Those codes will be included in the next.

> [...]
>
>> +/*
>> + * The function manage host-phy port state and suspend/resume phy port
>> + * to save power.
>> + *
>> + * we rely on utmi_linestate and utmi_hostdisconnect to identify whether
>> + * FS/HS is disconnect or not. Besides, we do not need care it is FS
>> + * disconnected or HS disconnected, actually, we just only need get the
>> + * device is disconnected at last through rearm the delayed work,
>> + * to suspend the phy port in _PHY_STATE_DISCONNECT_ case.
>> + *
>> + * NOTE: It will invoke some clk related APIs, so do not invoke it from
>> + * interrupt context.
>
> This does not seem to match the code, as I don't see any clk_* calls,

Sorry for not describing the comment clearly. In a fact, *_sm_work will 
invoke *phy_suspend or *phy_resume which will invoke some *clk APIs.

Anyway, I will correct it to be more clear.

>
>> + */
>> +static void rockchip_usb2phy_sm_work(struct work_struct *work)
>> +{
 >> +
>>  [...]
 >> +
>> +		/* fall through */
>> +	case PHY_STATE_HS_CONNECT:
>> +		if (rport->suspended) {
>> +			dev_dbg(&rport->phy->dev, "HS/FS connected\n");
>> +			rockchip_usb2phy_resume(rport->phy);
>> +			rport->suspended = false;
>> +		}
>> +		break;
>> +	case PHY_STATE_DISCONNECT:
>> +		if (!rport->suspended) {
>> +			dev_dbg(&rport->phy->dev, "HS/FS disconnected\n");
>> +			rockchip_usb2phy_suspend(rport->phy);
>> +			rport->suspended = true;
>> +		}
>>  [...]
 >> +
>> +	}
>> +
>> +next_schedule:
>> +	mutex_unlock(&rport->mutex);
>> +	schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
>> +}
>
> [...]
>
>> +static int rockchip_usb2phy_probe(struct platform_device *pdev)
>> +{
 >> +
 >>  [...]
 >> +
>> +
>> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +	return PTR_ERR_OR_ZERO(provider);
>> +
>> +put_child:
>> +	of_node_put(child_np);
>
>> +	of_clk_del_provider(np);
>> +	clk_unregister(rphy->clk480m);
>
> these two can go away, once you have the devm_action described
> near your clk_register function.

Done

>
>> +	return ret;
>> +}

BR.
Frank