Hi,

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

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
register function.

Hi,

I know I am a bit late with this, but while adapting the U-Boot driver
to the new binding I was wondering about the internal PHY detection:


So here you seem to deduce the usage of the internal PHY by the PHY
interface specified in the DT (MII = internal, RGMII = external).
I think I raised this question before, but isn't it perfectly legal for
a board to use MII with an external PHY even on those SoCs that feature
an internal PHY?
On the first glance that does not make too much sense, but apart from
not being the correct binding to describe all of the SoCs features I see
two scenarios:
1) A board vendor might choose to not use the internal PHY because it
has bugs, lacks features (configurability) or has other issues. For
instance I have heard reports that the internal PHY makes the SoC go
rather hot, possibly limiting the CPU frequency. By using an external
MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
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
IC or some other network circuitry, for instance fibre connectors.

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
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
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
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
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>;
                    reg = <1>;
                    clocks = <&ccu CLK_BUS_EPHY>;
                    resets = <&ccu RST_BUS_EPHY>;
                };
            };
        };

And then move the internal-PHY setup code into a separate PHY driver?

That looks like the architecturally best solution to me, but is probably
also a bit involved since it would require a separate PHY driver.
Or can we make it simpler, but still use this binding?

Cheers,
Andre.