| Message ID | 20180827143200.8597-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues | expand |
Quoting Hans de Goede (2018-08-27 07:31:58) > On some boards a platform clock is used as clock for the r8169 chip, > this commit adds support for getting and enabling this clock (assuming > it has an "ether_clk" alias set on it). > > This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks > enabled by the firmware") which is a previous attempt to fix this for some > x86 boards, but this causes all Cherry Trail SoC using boards to not reach > there lowest power states when suspending. > > This commit (together with an atom-pmc-clk driver commit adding the alias) > fixes things properly by making the r8169 get the clock and enable it when > it needs it. > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102 > Cc: Johannes Stezenbach <js@sig21.net> > Cc: Carlo Caione <carlo@endlessm.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Stephen Boyd <sboyd@kernel.org> > @@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_private *tp) > } > } > > +static void rtl_disable_clk(void *data) > +{ > + clk_disable_unprepare(data); > +} > + > static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > { > const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data; > @@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > mii->reg_num_mask = 0x1f; > mii->supports_gmii = cfg->has_gmii; > > + /* Get the *optional* external "ether_clk" used on some boards */ > + tp->clk = devm_clk_get(&pdev->dev, "ether_clk"); An "optional" clk API is in flight, but it's easier to wait for that to merge and then this to be updated, so just take that as an FYI. It would be interesting to see how to support optional clks with clkdev lookups though, which would be needed in this case. How would you know that a clk device driver hasn't probed yet and isn't the driver that's actually providing the clk to this device on x86 systems? With DT systems we can figure that out by looking at the DT and seeing if the device driver requesting the clk has the clocks property. On x86 systems it's all clkdev which doesn't really lend itself to solving this problem. > + if (IS_ERR(tp->clk)) { > + rc = PTR_ERR(tp->clk);
Hi, On 27-08-18 20:47, Stephen Boyd wrote: > Quoting Hans de Goede (2018-08-27 07:31:58) >> On some boards a platform clock is used as clock for the r8169 chip, >> this commit adds support for getting and enabling this clock (assuming >> it has an "ether_clk" alias set on it). >> >> This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks >> enabled by the firmware") which is a previous attempt to fix this for some >> x86 boards, but this causes all Cherry Trail SoC using boards to not reach >> there lowest power states when suspending. >> >> This commit (together with an atom-pmc-clk driver commit adding the alias) >> fixes things properly by making the r8169 get the clock and enable it when >> it needs it. >> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102 >> Cc: Johannes Stezenbach <js@sig21.net> >> Cc: Carlo Caione <carlo@endlessm.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Stephen Boyd <sboyd@kernel.org> Thanks. >> @@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_private *tp) >> } >> } >> >> +static void rtl_disable_clk(void *data) >> +{ >> + clk_disable_unprepare(data); >> +} >> + >> static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> { >> const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data; >> @@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> mii->reg_num_mask = 0x1f; >> mii->supports_gmii = cfg->has_gmii; >> >> + /* Get the *optional* external "ether_clk" used on some boards */ >> + tp->clk = devm_clk_get(&pdev->dev, "ether_clk"); > > An "optional" clk API is in flight, but it's easier to wait for that to > merge and then this to be updated, so just take that as an FYI. That is good to know. > It would > be interesting to see how to support optional clks with clkdev lookup > though, which would be needed in this case. Ack. > How would you know that a clk device driver hasn't probed yet and isn't > the driver that's actually providing the clk to this device on x86 > systems? With DT systems we can figure that out by looking at the DT and > seeing if the device driver requesting the clk has the clocks property. > On x86 systems it's all clkdev which doesn't really lend itself to > solving this problem. Right on x86 the assumption is that the clk driver will be builtin and will probe before the consumer. In this case that is true as the pmc-atom-clk driver can only be builtin and its platform device is instantiated from the acpi_lpss code and acpi init happens before the PCI bus is scanned. We have the same problem with ACPI OpRegions and drivers who have _PS0 / _PS3 methods using those OpRegions and the "solution" so far is the same, make sure all drivers providing OpRegion handlers are builtin. Basically we (x86) miss the nice dependency info and complete hw description devicetree gives, so we rely on initialization order for some of this. I added the -EPROBE_DEFER handling for completeness sake / for other platforms, as you point out it will never trigger on x86. Regards, Hans > >> + if (IS_ERR(tp->clk)) { >> + rc = PTR_ERR(tp->clk);
Quoting Hans de Goede (2018-08-27 11:53:19) > On 27-08-18 20:47, Stephen Boyd wrote: > > How would you know that a clk device driver hasn't probed yet and isn't > > the driver that's actually providing the clk to this device on x86 > > systems? With DT systems we can figure that out by looking at the DT and > > seeing if the device driver requesting the clk has the clocks property. > > On x86 systems it's all clkdev which doesn't really lend itself to > > solving this problem. > > Right on x86 the assumption is that the clk driver will be builtin and > will probe before the consumer. In this case that is true as the > pmc-atom-clk driver can only be builtin and its platform device is > instantiated from the acpi_lpss code and acpi init happens before > the PCI bus is scanned. If we can go with this assumption then we can make the optional clk API work even on clkdev based systems. Maybe if x86 had some way of indicating that all builtin clks are registered? That might work but it's not very clean. Or if we could check to see if we're running on an ACPI based system in clkdev we could use that to assume that clk_get() will only be called after all providers have registered their lookups. > > We have the same problem with ACPI OpRegions and drivers who have > _PS0 / _PS3 methods using those OpRegions and the "solution" so far > is the same, make sure all drivers providing OpRegion handlers are > builtin. > > Basically we (x86) miss the nice dependency info and complete hw > description devicetree gives, so we rely on initialization order > for some of this. I added the -EPROBE_DEFER handling for completeness > sake / for other platforms, as you point out it will never trigger > on x86. > Thanks for the info!
Hi, On 27-08-18 21:14, Stephen Boyd wrote: > Quoting Hans de Goede (2018-08-27 11:53:19) >> On 27-08-18 20:47, Stephen Boyd wrote: >>> How would you know that a clk device driver hasn't probed yet and isn't >>> the driver that's actually providing the clk to this device on x86 >>> systems? With DT systems we can figure that out by looking at the DT and >>> seeing if the device driver requesting the clk has the clocks property. >>> On x86 systems it's all clkdev which doesn't really lend itself to >>> solving this problem. >> >> Right on x86 the assumption is that the clk driver will be builtin and >> will probe before the consumer. In this case that is true as the >> pmc-atom-clk driver can only be builtin and its platform device is >> instantiated from the acpi_lpss code and acpi init happens before >> the PCI bus is scanned. > > If we can go with this assumption then we can make the optional clk API > work even on clkdev based systems. Maybe if x86 had some way of > indicating that all builtin clks are registered? Unfortunately there is no such thing I'm afraid. > That might work but > it's not very clean. Or if we could check to see if we're running on an > ACPI based system in clkdev we could use that to assume that clk_get() > will only be called after all providers have registered their lookups. Yes some check for x86 + ACPI (ARM also uses ACPI, but there we should no do this AFAICT) is probably best. That or not use the new optional clk API on x86, but that means that any cross platform driver cannot use it, which would be a pain. BTW does your Acked-by indicate you are ok with merging this series through the netdev tree as I suggested in the cover-letter? If so can I also add your Acked-by to the 3th patch ? Regards, Hans
Quoting Hans de Goede (2018-08-29 10:09:57) > Hi, > > On 27-08-18 21:14, Stephen Boyd wrote: > > Quoting Hans de Goede (2018-08-27 11:53:19) > >> On 27-08-18 20:47, Stephen Boyd wrote: > >>> How would you know that a clk device driver hasn't probed yet and isn't > >>> the driver that's actually providing the clk to this device on x86 > >>> systems? With DT systems we can figure that out by looking at the DT and > >>> seeing if the device driver requesting the clk has the clocks property. > >>> On x86 systems it's all clkdev which doesn't really lend itself to > >>> solving this problem. > >> > >> Right on x86 the assumption is that the clk driver will be builtin and > >> will probe before the consumer. In this case that is true as the > >> pmc-atom-clk driver can only be builtin and its platform device is > >> instantiated from the acpi_lpss code and acpi init happens before > >> the PCI bus is scanned. > > > > If we can go with this assumption then we can make the optional clk API > > work even on clkdev based systems. Maybe if x86 had some way of > > indicating that all builtin clks are registered? > > Unfortunately there is no such thing I'm afraid. Ugh! > > > That might work but > > it's not very clean. Or if we could check to see if we're running on an > > ACPI based system in clkdev we could use that to assume that clk_get() > > will only be called after all providers have registered their lookups. > > Yes some check for x86 + ACPI (ARM also uses ACPI, but there we > should no do this AFAICT) is probably best. That or not use the > new optional clk API on x86, but that means that any cross platform > driver cannot use it, which would be a pain. Right. The optional clk API will be not so great until we can get ACPI to move way from clkdev. > > BTW does your Acked-by indicate you are ok with merging this series > through the netdev tree as I suggested in the cover-letter? If so > can I also add your Acked-by to the 3th patch ? > Yep, I thought I did that but now I've really done it.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index eaedc11ed686..779b02979493 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -13,6 +13,7 @@ #include <linux/pci.h> #include <linux/netdevice.h> #include <linux/etherdevice.h> +#include <linux/clk.h> #include <linux/delay.h> #include <linux/ethtool.h> #include <linux/mii.h> @@ -765,6 +766,7 @@ struct rtl8169_private { u16 event_slow; const struct rtl_coalesce_info *coalesce_info; + struct clk *clk; struct mdio_ops { void (*write)(struct rtl8169_private *, int, int); @@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_private *tp) } } +static void rtl_disable_clk(void *data) +{ + clk_disable_unprepare(data); +} + static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) { const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data; @@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) mii->reg_num_mask = 0x1f; mii->supports_gmii = cfg->has_gmii; + /* Get the *optional* external "ether_clk" used on some boards */ + tp->clk = devm_clk_get(&pdev->dev, "ether_clk"); + if (IS_ERR(tp->clk)) { + rc = PTR_ERR(tp->clk); + if (rc == -ENOENT) { + /* clk-core allows NULL (for suspend / resume) */ + tp->clk = NULL; + } else if (rc == -EPROBE_DEFER) { + return rc; + } else { + dev_err(&pdev->dev, "failed to get clk: %d\n", rc); + return rc; + } + } else { + rc = clk_prepare_enable(tp->clk); + if (rc) { + dev_err(&pdev->dev, "failed to enable clk: %d\n", rc); + return rc; + } + + rc = devm_add_action_or_reset(&pdev->dev, rtl_disable_clk, + tp->clk); + if (rc) + return rc; + } + /* disable ASPM completely as that cause random device stop working * problems as well as full system hangs for some PCIe devices users */ pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
On some boards a platform clock is used as clock for the r8169 chip, this commit adds support for getting and enabling this clock (assuming it has an "ether_clk" alias set on it). This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware") which is a previous attempt to fix this for some x86 boards, but this causes all Cherry Trail SoC using boards to not reach there lowest power states when suspending. This commit (together with an atom-pmc-clk driver commit adding the alias) fixes things properly by making the r8169 get the clock and enable it when it needs it. Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102 Cc: Johannes Stezenbach <js@sig21.net> Cc: Carlo Caione <carlo@endlessm.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/net/ethernet/realtek/r8169.c | 33 ++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)