From: Corentin Labbe <clabbe.montjoie@gmail.com>
To: Andre Przywara <andre.przywara@arm.com>
Cc: "Icenowy Zheng" <icenowy@aosc.io>,
"Maxime Ripard" <maxime.ripard@free-electrons.com>,
"Chen-Yu Tsai" <wens@csie.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Mark Rutland" <mark.rutland@arm.com>,
"Russell King" <linux@armlinux.org.uk>,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Will Deacon" <will.deacon@arm.com>,
"Giuseppe Cavallaro" <peppe.cavallaro@st.com>,
alexandre.torgue@st.com, devicetree <devicetree@vger.kernel.org>,
netdev <netdev@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
linux-sunxi <linux-sunxi@googlegroups.com>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
david.wu@rock-chips.com,
"Florian Fainelli" <f.fainelli@gmail.com>,
"Andrew Lunn" <andrew@lunn.ch>, "Heiko Stübner" <heiko@sntech.de>
Subject: Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i
Date: Tue, 27 Jun 2017 14:37:48 +0200 [thread overview]
Message-ID: <20170627123748.GA8403@Red> (raw)
In-Reply-To: <858b5361-fde5-8ab4-b9ba-aeb7859b9a7d@arm.com>
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@arm.com> 写到:
> >> 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@gmail.com> 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
next prev parent reply other threads:[~2017-06-27 12:38 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-31 7:18 [PATCH v6 00/21] net-next: stmmac: add dwmac-sun8i ethernet driver Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 01/21] net-next: stmmac: export stmmac_set_mac_addr/stmmac_get_mac_addr Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 02/21] net-next: stmmac: add optional setup function Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 03/21] dt-bindings: net-next: Add DT bindings documentation for Allwinner dwmac-sun8i Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 04/21] dt-bindings: syscon: Add DT bindings documentation for Allwinner syscon Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i Corentin Labbe
2017-06-26 0:18 ` André Przywara
2017-06-27 8:05 ` Corentin Labbe
2017-06-27 8:11 ` Chen-Yu Tsai
2017-06-27 8:21 ` Corentin Labbe
2017-06-27 9:02 ` Andre Przywara
2017-06-27 9:41 ` Maxime Ripard
2017-06-27 10:11 ` Chen-Yu Tsai
2017-06-27 10:17 ` [linux-sunxi] " Icenowy Zheng
2017-06-27 10:20 ` Chen-Yu Tsai
2017-06-27 10:15 ` Andre Przywara
2017-06-27 10:22 ` Chen-Yu Tsai
2017-06-27 10:23 ` Icenowy Zheng
2017-06-27 10:33 ` Andre Przywara
2017-06-27 12:37 ` Corentin Labbe [this message]
2017-06-27 17:29 ` Maxime Ripard
2017-06-27 17:37 ` Corentin Labbe
2017-06-27 17:37 ` Florian Fainelli
2017-07-01 6:53 ` Corentin Labbe
2017-07-01 21:42 ` Florian Fainelli
2017-07-02 8:36 ` Corentin Labbe
2017-06-27 16:00 ` Maxime Ripard
2017-05-31 7:18 ` [PATCH v6 06/21] arm: sun8i: sunxi-h3-h5: Add dt node for the syscon control module Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 07/21] arm: sun8i: sunxi-h3-h5: add dwmac-sun8i ethernet driver Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 08/21] arm: sun8i: orangepi-pc: Enable dwmac-sun8i Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 09/21] arm: sun8i: orangepi-zero: " Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 10/21] arm: sun8i: orangepi-one: " Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 11/21] arm: sun8i: orangepi-2: " Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 12/21] arm: sun8i: orangepi-pc-plus: Set EMAC activity LEDs to active high Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 13/21] arm: sun8i: nanopi-neo: Enable dwmac-sun8i Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 14/21] arm64: allwinner: sun50i-a64: Add dt node for the syscon control module Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 15/21] arm64: allwinner: sun50i-a64: add dwmac-sun8i Ethernet driver Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 16/21] arm64: allwinner: pine64: Enable dwmac-sun8i Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 17/21] arm64: allwinner: pine64-plus: " Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 18/21] arm64: allwinner: bananapi-m64: " Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 19/21] arm: sunxi: Enable dwmac-sun8i driver on sunxi_defconfig Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 20/21] arm: multi_v7: Enable dwmac-sun8i driver on multi_v7_defconfig Corentin Labbe
2017-05-31 7:18 ` [PATCH v6 21/21] arm64: defconfig: Enable dwmac-sun8i driver on defconfig Corentin Labbe
2017-06-01 18:58 ` [PATCH v6 00/21] net-next: stmmac: add dwmac-sun8i ethernet driver David Miller
2017-06-02 6:37 ` Maxime Ripard
2017-06-02 9:13 ` Maxime Ripard
2017-06-02 14:22 ` David Miller
2017-06-02 22:24 ` Maxime Ripard
2017-06-09 12:50 ` Maxime Ripard
2017-06-02 14:08 ` David Miller
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=20170627123748.GA8403@Red \
--to=clabbe.montjoie@gmail.com \
--cc=alexandre.torgue@st.com \
--cc=andre.przywara@arm.com \
--cc=andrew@lunn.ch \
--cc=catalin.marinas@arm.com \
--cc=david.wu@rock-chips.com \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=heiko@sntech.de \
--cc=icenowy@aosc.io \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sunxi@googlegroups.com \
--cc=linux@armlinux.org.uk \
--cc=mark.rutland@arm.com \
--cc=maxime.ripard@free-electrons.com \
--cc=netdev@vger.kernel.org \
--cc=peppe.cavallaro@st.com \
--cc=robh+dt@kernel.org \
--cc=wens@csie.org \
--cc=will.deacon@arm.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).