From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752746AbcF2OQf (ORCPT ); Wed, 29 Jun 2016 10:16:35 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:41845 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752346AbcF2OQc (ORCPT ); Wed, 29 Jun 2016 10:16:32 -0400 Subject: Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy To: =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1465783810-18756-1-git-send-email-frank.wang@rock-chips.com> <1465783810-18756-3-git-send-email-frank.wang@rock-chips.com> <5763E506.1060500@ti.com> <1653733.7f4MgdqfFf@diego> CC: Frank Wang , , , , , , , , , , , , , , , , , From: Kishon Vijay Abraham I Message-ID: <5773D7DC.9050902@ti.com> Date: Wed, 29 Jun 2016 19:44:52 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <1653733.7f4MgdqfFf@diego> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Friday 17 June 2016 10:16 PM, Heiko Stübner wrote: > Hi Kishon, > > Am Freitag, 17. Juni 2016, 17:24:46 schrieb Kishon Vijay Abraham I: > >>> + ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m); >>> + 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 0; >>> + >>> +err_unreg_action: >>> + of_clk_del_provider(node); >>> +err_clk_provider: >>> + clk_unregister(rphy->clk480m); >>> +err_register: >>> + if (rphy->clk) >>> + clk_put(rphy->clk); >>> + return ret; >>> +} >> >> I'm seeing lot of similarities specifically w.r.t to clock handling part in >> drivers/phy/phy-rockchip-usb.c. Why not just re-use that driver? > > It's a completely different phy block (Designware vs. Innosilicon) and a lot > of stuff also is handled differently. > > For one on the old block, each phy was somewhat independent and had for examle > its own clock-supply, while on this one there is only one for both ports of > the phy. Similarly with the clock getting fed back to the clock-controller > (one clock per port on the old block, now one clock for the whole phy). > > Then as you can see, the handling for power-up and down is a bit different and > I guess one big block might be the still missing special otg handling, Frank > wrote about. All right then. > > > [...] > >>> + /* >>> + * we don't need to rearm the delayed work when the phy port >>> + * is suspended. >>> + */ >>> + mutex_unlock(&rport->mutex); >>> + return; >>> + default: >>> + dev_dbg(&rport->phy->dev, "unknown phy state\n"); >>> + break; >>> + } >>> + >>> +next_schedule: >>> + mutex_unlock(&rport->mutex); >>> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY); >> >> Why are you scheduling the work again? Interrupt handlers can invoke this >> right? > > Frank said, that the phy is only able to detect the plug-in event via > interrupts, not the removal, so once a plugged device is detected, this gets > rescheduled until the device gets removed. okay. > > [...] > >>> + /* find out a proper config which can be matched with dt. */ >>> + index = 0; >>> + while (phy_cfgs[index].reg) { >>> + if (phy_cfgs[index].reg == reg) { >> >> Why not pass these config values from dt? Moreover finding the config using >> register offset is bound to break. > > As you have probably seen, this phy block is no stand-alone (mmio-)device, but > gets controlled through special register/bits in the so called "General > Register Files" syscon. > > The values stored and accessed here, are the location and layout of those > control registers. Bits in those phy control registers at times move between > phy-versions in different socs (rk3036, rk3228, rk3366, rk3368, rk3399) and > some are even missing. So I don't really see a nice way to describe that in dt > without describing the register and offset of each of those 22 used bits > individually. > > > I'm also not sure where you expect it to break? The reg-offset is the offset > of the phy inside the GRF and the Designware-phy also already does something > similar to select some appropriate values. I'm concerned the phy can be placed in the different reg-offset within GRF (like in the next silicon revision) and this driver can't be used. Thanks Kishon >