Hi Frank, Am Mittwoch, 15. Juni 2016, 18:58:43 schrieb Frank Wang: > On 2016/6/15 17:04, Heiko Stübner wrote: > > Am Mittwoch, 15. Juni 2016, 11:23:26 schrieb Frank Wang: > >> On 2016/6/14 21:27, Heiko Stübner wrote: > >>> Am Montag, 13. Juni 2016, 10:10:10 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.w...@rock-chips.com> > >>>> --- > >>>> > >>>> Changes in v5: > >>>> - Added 'reg' in the data block to match the different phy-blocks in > >>>> dt. > >>>> > >>>> Changes in v4: > >>>> - Removed some processes related to 'vbus_host-supply'. > >>>> > >>>> Changes in v3: > >>>> - Resolved the mapping defect between fixed value in driver and the > >>>> > >>>> property in devicetree. > >>>> > >>>> - Optimized 480m output clock register function. > >>>> - Code cleanup. > >>>> > >>>> Changes in v2: > >>>> - Changed vbus_host operation from gpio to regulator in *_probe. > >>>> - Improved the fault treatment relate to 480m clock register. > >>>> - Cleaned up some meaningless codes in *_clk480m_disable. > >>>> - made more clear the comment of *_sm_work. > >>>> > >>>> drivers/phy/Kconfig | 7 + > >>>> drivers/phy/Makefile | 1 + > >>>> drivers/phy/phy-rockchip-inno-usb2.c | 645 > >>>> > >>>> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+) > >>>> > >>>> [...] > >>>> > >>>> + > >>>> +static int rockchip_usb2phy_exit(struct phy *phy) > >>>> +{ > >>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy); > >>>> + > >>>> > >>> if (!rport->port_cfg) > >>> > >>> return 0; > >>>> > >>>> + if (rport->port_id == USB2PHY_PORT_HOST) > >>>> + cancel_delayed_work_sync(&rport->sm_work); > >>>> + > >>> > >>> you will also need to resume the port here, if it is suspended at this > >>> point, as phy_power_off gets called after phy_exit and would probably > >>> produce clk enable/disable mismatches otherwise. > >> > >> Hmm, from my personal point of view, when canceling sm_work here, it may > >> not cause the port goes to suspend, isn't it? besides, clk only prepared > >> in *_usb2phy_resume(), and unprepared in *_usb2phy_suspend(), so if we > >> resume port here, the prepare_count of clk will be increased again, I > >> am afraid this is not correct, and am I wrong? would you like to tell me > >> more details? > > > > usb2phy_resume gets called both initially through phy_power_on as well. > > So it's on but through the first scheduled work call, might get suspended > > when nothing is connected. (clk_enable and clk_disable will run). > > If nothing is connected on unload phy_power_off will get called while the > > clock actually is still disabled. > > > > So I think it's either resuming on exit, or at least making sure to do > > nothing in that case in the phy_power_off callback of the driver. > > Well, I got what you mean. I saw phy_power_on() gets called twice at > ehci/ohci driver probe time, one is at pdata->power_on(); another is at > usb_add_hcd(), so after that, the power_count of phy increases to two. > however, when ehci/ohci driver goes to suspend, there is only once to > call phy_power_off(), and the power_count of phy decreases to 1, so our > usb2phy_resume() could not be invoked :-) . > > If so, is it still need to care it?
You should never have to rely on oddities in other drivers :-) > How about use suspended member of > rockchip_usb2phy_port as a conditional to check in *_usb2phy_suspend(), > if necessary. That looks also sane and should work. Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html