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

[Corentin Labbe]

On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> Hi,
> 
> On 27/06/17 11:23, Icenowy Zheng wrote:
> > 
> > 
> > ä 2017å6æ27æ GMT+08:00 äå6:15:58, Andre Przywara <andre.przywara@xxxxxxx> åå:
> >> Hi,
> >>
> >> On 27/06/17 10:41, Maxime Ripard wrote:
> >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> >>>> Hi,
> >>>>
> >>>> (CC:ing some people from that Rockchip dmwac series)
> >>>>
> >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >>>>>> <clabbe.montjoie@xxxxxxxxx> wrote:
> >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, Andrà Przywara wrote:
> >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>>>>>> 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
> >>>>>
> >>>>>>
> >>>>>> 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 ...
> >>>>
> >>>>> 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>;
> > 
> > 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.
> 
> Cheers,
> Andre.
> 

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)

Regards