Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

From: Corentin Labbe <hidden>
Date: 2017-06-27 17:38:10
Also in: linux-arm-kernel, linux-devicetree, lkml

On Tue, Jun 27, 2017 at 07:29:37PM +0200, Maxime Ripard wrote:

On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:

quoted

On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:

quoted

Hi,

On 27/06/17 11:23, Icenowy Zheng wrote:

quoted


于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara [off-list ref] 写到:

quoted

Hi,

On 27/06/17 10:41, Maxime Ripard wrote:

quoted

On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:

quoted

Hi,

(CC:ing some people from that Rockchip dmwac series)

On 27/06/17 09:21, Corentin Labbe wrote:

quoted

On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:

quoted

On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
[off-list ref] wrote:

quoted

On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:

quoted

On 31/05/17 08:18, Corentin Labbe wrote:

quoted

The dwmac-sun8i is a heavy hacked version of stmmac hardware by
allwinner.
In fact the only common part is the descriptor management and

the first

quoted

register function.

Hi,

I know I am a bit late with this, but while adapting the U-Boot

driver

quoted

quoted
quoted
quoted
quoted
quoted
to the new binding I was wondering about the internal PHY

detection:

quoted

quoted
quoted
quoted
quoted
quoted
So here you seem to deduce the usage of the internal PHY by the

PHY

quoted

quoted
quoted
quoted
quoted
quoted
interface specified in the DT (MII = internal, RGMII =

external).

quoted

quoted
quoted
quoted
quoted
quoted
I think I raised this question before, but isn't it perfectly

legal for

quoted

quoted
quoted
quoted
quoted
quoted
a board to use MII with an external PHY even on those SoCs that

feature

quoted

an internal PHY?
On the first glance that does not make too much sense, but apart

from

quoted

quoted
quoted
quoted
quoted
quoted
not being the correct binding to describe all of the SoCs

features I see

quoted

two scenarios:
1) A board vendor might choose to not use the internal PHY

because it

quoted

quoted
quoted
quoted
quoted
quoted
has bugs, lacks features (configurability) or has other issues.

For

quoted

quoted
quoted
quoted
quoted
quoted
instance I have heard reports that the internal PHY makes the

SoC go

quoted

quoted
quoted
quoted
quoted
quoted
rather hot, possibly limiting the CPU frequency. By using an

external

quoted

quoted
quoted
quoted
quoted
quoted
MII PHY (which are still cheaper than RGMII PHYs) this can be

avoided.

quoted

2) A PHY does not necessarily need to be directly connected to
magnetics. Indeed quite some boards use (RG)MII to connect to a

switch

quoted

quoted
quoted
quoted
quoted
quoted
IC or some other network circuitry, for instance fibre

connectors.

quoted

So I was wondering if we would need an explicit:
      allwinner,use-internal-phy;
boolean DT property to signal the usage of the internal PHY?
Alternatively we could go with the negative version:
      allwinner,disable-internal-phy;

Or what about introducing a new "allwinner,internal-mii-phy"

compatible

quoted

string for the *PHY* node and use that?

I just want to avoid that we introduce a binding that causes us
headaches later. I think we can still fix this with a followup

patch

quoted

before the driver and its binding hit a release kernel.

Cheers,
Andre.

I just see some patch, where "phy-mode = internal" is valid.
I will try to find a way to use it

Can you provide a link?

https://lkml.org/lkml/2017/6/23/479

quoted

I'm not a fan of using phy-mode for this. There's no guarantee

what

quoted

mode the internal PHY uses. That's what phy-mode is for.

I can understand Chen-Yu's concerns, but ...

quoted

For each soc the internal PHY mode is know and setted in

emac_variant/internal_phy

quoted

So its not a problem.

that is true as well, at least for now.

So while I agree that having a separate property to indicate
the usage of the internal PHY would be nice, I am bit tempted
to use this easier approach and piggy back on the existing
phy-mode property.

We're trying to fix an issue that works for now too.

If we want to consider future weird cases, then we must
consider all of them. And the phy mode changing is definitely
not really far fetched.

I agree with Chen-Yu, and I really feel like the compatible
solution you suggested would cover both your concerns, and
ours.

So something like this?
	emac: emac@1c30000 {
	    compatible = "allwinner,sun8i-h3-emac";
	    ...
	    phy-mode = "mii";
	    phy-handle = <&int_mii_phy>;
	    ...

	    mdio: mdio {
               #address-cells = <1>;
               #size-cells = <0>;
               int_mii_phy: ethernet-phy@1 {
                   compatible = "allwinner,sun8i-h3-ephy";
                   syscon = <&syscon>;

The MAC still needs to set some bits of syscon register.

Yes, the syscon property needs also to be in the MAC node, that
was meant to be somewhere in the second "..." ;-)

But now since Chen-Yu mentioned that we need to set up the PHY *first*
to make it actually discoverable via MDIO, I wonder if we could change
this to:
1) have the DT as described here
2) Let the dwmac-sun8i driver peek into the node referenced by
phy-handle and check the compatible string there.
3) If that matches some allwinner internal PHY name, it sets up the PHY
to make it respond when the MDIO driver queries its bus.

Or can a PHY driver set itself up (since we have clocks and resets
properties in there) *before* the MDIO bus gets scanned?
Chen-Yu's comment in the other mail hints at that this is not easily
possible.

I think adding phy compatible just make things more complex.

I think the patch series I sent early fix all problems without more
complexity since:

- it does not add more DT stuff
- it use an already used in tree DT phy-mode "internal" (and so phy
  mode PHY_INTERFACE_MODE_INTERNAL)

  - it doesn't cover all the concerns we had
  - it uses an undocumented value, with an unclear implication

The answer from Florian anyway breaks my logic, internal is for "internal phy with non-xMII protocol" not just internal PHY

Regards

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help