linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Heiko Stübner" <heiko@sntech.de>
To: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Frank Wang <frank.wang@rock-chips.com>,
	dianders@chromium.org, linux@roeck-us.net, groeck@chromium.org,
	jwerner@chromium.org, robh+dt@kernel.org, pawel.moll@arm.com,
	mark.rutland@arm.com, ijc+devicetree@hellion.org.uk,
	galak@codeaurora.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-rockchip@lists.infradead.org, xzy.xu@rock-chips.com,
	kever.yang@rock-chips.com, huangtao@rock-chips.com,
	william.wu@rock-chips.com
Subject: Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
Date: Fri, 17 Jun 2016 18:46:25 +0200	[thread overview]
Message-ID: <1653733.7f4MgdqfFf@diego> (raw)
In-Reply-To: <5763E506.1060500@ti.com>

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.


[...]

> > +		/*
> > +		 * 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.

[...]

> > +	/* 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.

  reply	other threads:[~2016-06-17 16:46 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-13  2:10 [PATCH v5 0/2] Add a new Rockchip usb2 phy driver Frank Wang
2016-06-13  2:10 ` [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY Frank Wang
2016-06-13  8:38   ` Heiko Stübner
2016-06-14 13:28     ` Heiko Stübner
2016-06-15  1:24       ` Frank Wang
2016-06-14 22:26   ` Rob Herring
2016-06-13  2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
2016-06-14 13:27   ` Heiko Stübner
2016-06-14 13:50     ` Guenter Roeck
2016-06-14 14:00       ` Heiko Stübner
2016-06-15  1:14         ` Frank Wang
2016-06-15 15:47           ` Guenter Roeck
2016-06-16  1:47             ` Frank Wang
2016-06-16 13:12               ` Guenter Roeck
2016-06-17  0:57                 ` Frank Wang
2016-06-15  3:23     ` Frank Wang
2016-06-15  9:04       ` Heiko Stübner
2016-06-15 10:58         ` Frank Wang
2016-06-15 18:49           ` Heiko Stübner
2016-06-14 14:52   ` Guenter Roeck
2016-06-14 15:20     ` Heiko Stübner
2016-06-17 11:54   ` Kishon Vijay Abraham I
2016-06-17 16:46     ` Heiko Stübner [this message]
2016-06-29 14:14       ` Kishon Vijay Abraham I
2016-06-29 14:27         ` Heiko Stuebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1653733.7f4MgdqfFf@diego \
    --to=heiko@sntech.de \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=frank.wang@rock-chips.com \
    --cc=galak@codeaurora.org \
    --cc=groeck@chromium.org \
    --cc=huangtao@rock-chips.com \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=jwerner@chromium.org \
    --cc=kever.yang@rock-chips.com \
    --cc=kishon@ti.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mark.rutland@arm.com \
    --cc=pawel.moll@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=william.wu@rock-chips.com \
    --cc=xzy.xu@rock-chips.com \
    --subject='Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).