| Message ID | 20191029174819.3502-1-michael@walle.cc |
|---|---|
| Headers | show |
| Series | net: phy: initialize PHYs via device tree properties | expand |
On 10/29/19 10:48 AM, Michael Walle wrote: > I was trying to configure the Atheros PHY for my board. There are fixups > all over the place, for example to enable the 125MHz clock output in almost > any i.MX architecture. Instead of adding another fixup in architecture > specific code, try to provide a generic way to init the PHY registers. > > This patch series tries to pick up the "broadcom,reg-init" and > "marvell,reg-init" device tree properties idea and make it a more generic > "reg-init" which is handled by phy_device instead of a particular phy > driver. These two examples are actually quite bad and were symptomatic of a few things at the time: - rush to get a specific feature/device supported without thinking about the big picture - lack of appropriate review on the Device Tree bindings Fortunately, the last item is now not happening anymore. The problem with letting that approach go through is that the Device Tree can now hold a configuration policy which is passed through as-is from DT to the PHY device, this is bad on so many different levels, starting with abstraction. If all you need is to enable a particular clock, introduce device specific properties that describe the hardware, and make the necessary change to the local driver that needs to act on those. You can always define a more generic scope property if you see a recurring pattern. So just to be clear on the current approach: NACK. > > Michael Walle (3): > dt-bindings: net: phy: Add reg-init property > net: phy: export __phy_{read|write}_page > net: phy: Use device tree properties to initialize any PHYs > > .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++ > MAINTAINERS | 1 + > drivers/net/phy/phy-core.c | 24 ++++- > drivers/net/phy/phy_device.c | 97 ++++++++++++++++++- > include/dt-bindings/net/phy.h | 18 ++++ > include/linux/phy.h | 2 + > 6 files changed, 170 insertions(+), 3 deletions(-) > create mode 100644 include/dt-bindings/net/phy.h > > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> >
> So just to be clear on the current approach: NACK.
Agreed.
And the Marvell one has only been used to set LEDs, as far as i
know. I would definitely push back on using it for anything else.
Andrew
Am 29. Oktober 2019 18:59:07 MEZ schrieb Florian Fainelli <f.fainelli@gmail.com>: >On 10/29/19 10:48 AM, Michael Walle wrote: >> I was trying to configure the Atheros PHY for my board. There are >fixups >> all over the place, for example to enable the 125MHz clock output in >almost >> any i.MX architecture. Instead of adding another fixup in >architecture >> specific code, try to provide a generic way to init the PHY >registers. >> >> This patch series tries to pick up the "broadcom,reg-init" and >> "marvell,reg-init" device tree properties idea and make it a more >generic >> "reg-init" which is handled by phy_device instead of a particular phy >> driver. > >These two examples are actually quite bad and were symptomatic of a few >things at the time: > >- rush to get a specific feature/device supported without thinking >about >the big picture >- lack of appropriate review on the Device Tree bindings > >Fortunately, the last item is now not happening anymore. > >The problem with letting that approach go through is that the Device >Tree can now hold a configuration policy which is passed through as-is >from DT to the PHY device, this is bad on so many different levels, >starting with abstraction. I see. >If all you need is to enable a particular clock, introduce device >specific properties that describe the hardware, and make the necessary >change to the local driver that needs to act on those. You can always >define a more generic scope property if you see a recurring pattern. Could you have a quick look at the following patch I made for u-boot, which adds a binding for the Atheros PHY. If that is the right direction. Yeah, I should have made it first to Linux to get some feedback on the binding :p https://patchwork.ozlabs.org/patch/1184516/ I'd then prepare another patch for Linux based on your suggestions. -michael > >So just to be clear on the current approach: NACK. > >> >> Michael Walle (3): >> dt-bindings: net: phy: Add reg-init property >> net: phy: export __phy_{read|write}_page >> net: phy: Use device tree properties to initialize any PHYs >> >> .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++ >> MAINTAINERS | 1 + >> drivers/net/phy/phy-core.c | 24 ++++- >> drivers/net/phy/phy_device.c | 97 >++++++++++++++++++- >> include/dt-bindings/net/phy.h | 18 ++++ >> include/linux/phy.h | 2 + >> 6 files changed, 170 insertions(+), 3 deletions(-) >> create mode 100644 include/dt-bindings/net/phy.h >> >> Cc: Andrew Lunn <andrew@lunn.ch> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Heiner Kallweit <hkallweit1@gmail.com> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Hi Florian,
On 10/29/19 1:54 PM, Michael Walle wrote: > Am 29. Oktober 2019 18:59:07 MEZ schrieb Florian Fainelli <f.fainelli@gmail.com>: >> On 10/29/19 10:48 AM, Michael Walle wrote: >>> I was trying to configure the Atheros PHY for my board. There are >> fixups >>> all over the place, for example to enable the 125MHz clock output in >> almost >>> any i.MX architecture. Instead of adding another fixup in >> architecture >>> specific code, try to provide a generic way to init the PHY >> registers. >>> >>> This patch series tries to pick up the "broadcom,reg-init" and >>> "marvell,reg-init" device tree properties idea and make it a more >> generic >>> "reg-init" which is handled by phy_device instead of a particular phy >>> driver. >> >> These two examples are actually quite bad and were symptomatic of a few >> things at the time: >> >> - rush to get a specific feature/device supported without thinking >> about >> the big picture >> - lack of appropriate review on the Device Tree bindings >> >> Fortunately, the last item is now not happening anymore. >> >> The problem with letting that approach go through is that the Device >> Tree can now hold a configuration policy which is passed through as-is >>from DT to the PHY device, this is bad on so many different levels, >> starting with abstraction. > > I see. > >> If all you need is to enable a particular clock, introduce device >> specific properties that describe the hardware, and make the necessary >> change to the local driver that needs to act on those. You can always >> define a more generic scope property if you see a recurring pattern. > > Could you have a quick look at the following patch I made for u-boot, which adds a binding for the Atheros PHY. If that is the right direction. Yeah, I should have made it first to Linux to get some feedback on the binding :p > > https://patchwork.ozlabs.org/patch/1184516/ > > I'd then prepare another patch for Linux based on your suggestions. This looks like the right direction IMHO.
Am 2019-10-29 21:59, schrieb Florian Fainelli: >>> If all you need is to enable a particular clock, introduce device >>> specific properties that describe the hardware, and make the >>> necessary >>> change to the local driver that needs to act on those. You can always >>> define a more generic scope property if you see a recurring pattern. >> >> Could you have a quick look at the following patch I made for u-boot, >> which adds a binding for the Atheros PHY. If that is the right >> direction. Yeah, I should have made it first to Linux to get some >> feedback on the binding :p >> >> https://patchwork.ozlabs.org/patch/1184516/ >> >> I'd then prepare another patch for Linux based on your suggestions. > > This looks like the right direction IMHO. Ok, one question though: There is a clock output, but it just supports four frequencies. Should there be a property like "atheros,clk-out-frequency = <25000000>" which can take an arbitrary number or should it be something like "atheros,clk-out-frequency = <AT803X_CLK_OUT_25_MHZ>" (the ti dp83867 uses the latter). -michael
I was trying to configure the Atheros PHY for my board. There are fixups all over the place, for example to enable the 125MHz clock output in almost any i.MX architecture. Instead of adding another fixup in architecture specific code, try to provide a generic way to init the PHY registers. This patch series tries to pick up the "broadcom,reg-init" and "marvell,reg-init" device tree properties idea and make it a more generic "reg-init" which is handled by phy_device instead of a particular phy driver. Michael Walle (3): dt-bindings: net: phy: Add reg-init property net: phy: export __phy_{read|write}_page net: phy: Use device tree properties to initialize any PHYs .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++ MAINTAINERS | 1 + drivers/net/phy/phy-core.c | 24 ++++- drivers/net/phy/phy_device.c | 97 ++++++++++++++++++- include/dt-bindings/net/phy.h | 18 ++++ include/linux/phy.h | 2 + 6 files changed, 170 insertions(+), 3 deletions(-) create mode 100644 include/dt-bindings/net/phy.h Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Heiner Kallweit <hkallweit1@gmail.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Rob Herring <robh+dt@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>