| Message ID | 20171204143049.20244-1-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thomas, On Mon, Dec 4, 2017 at 3:30 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > The sh_eth driver (used for the Ethernet controller on SH7786) uses > devm_clk_get(&pdev->dev, NULL) to get its clock. It means that con_id > = NULL, and therefore a clock lookup made on the con_id doesn't work. > > In order to make sure that the sh_eth driver is able to find its clock > on SH7786, we switch from using a con_id to a dev_id in the clock > lookup table for this SoC. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> The clock is optional, right? Do you need it? It's "needed" for WoL only, but that dependency is planned to be removed, cfr. "sh_eth: Remove obsolete explicit clock handling for WoL". Regardless: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, 4 Dec 2017 15:58:42 +0100, Geert Uytterhoeven wrote: > The clock is optional, right? Do you need it? It's "needed" for WoL only, > but that dependency is planned to be removed, cfr. "sh_eth: Remove obsolete > explicit clock handling for WoL". Hm, that's a good point what you raise here. Indeed looking at the code, the clock isn't enabled unless you go in suspend() and WoL support is enabled. I don't go into suspend, and I don't have WoL enabled, but if I revert my patch, the kernel hangs at: Generic PHY sh7786-ether-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=sh7786-ether-0:00, irq=POLL) Waiting up to 110 more seconds for network. (I booting over NFS, so the kernel tries to get an IP address over DHCP at boot time) With my patch applied: Generic PHY sh7786-ether-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=sh7786-ether-0:00, irq=POLL) sh-eth sh7786-ether.0 eth0: Link is Up - 100Mbps/Full - flow control rx/tx Sending DHCP requests ., OK IP-Config: Got DHCP answer from 192.168.0.254, my address is 192.168.0.202 IP-Config: Complete: device=eth0, hwaddr=d4:81:d7:45:df:5a, ipaddr=192.168.0.202, mask=255.255.255.0, gw=192.168.0.254 host=192.168.0.202, domain=lan, nis-domain=(none) bootserver=192.168.0.254, rootserver=192.168.0.11, rootpath= nameserver0=192.168.0.254 VFS: Mounted root (nfs filesystem) on device 0:13. devtmpfs: mounted Freeing unused kernel memory: 132K So it seems like getting the reference to the clock has some side-effect. I guess this needs more debugging :-) Best regards, Thomas
Hi Thomas, On Mon, Dec 4, 2017 at 4:23 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Mon, 4 Dec 2017 15:58:42 +0100, Geert Uytterhoeven wrote: >> The clock is optional, right? Do you need it? It's "needed" for WoL only, >> but that dependency is planned to be removed, cfr. "sh_eth: Remove obsolete >> explicit clock handling for WoL". > > Hm, that's a good point what you raise here. Indeed looking at the > code, the clock isn't enabled unless you go in suspend() and WoL > support is enabled. I don't go into suspend, and I don't have WoL > enabled, but if I revert my patch, the kernel hangs at: > > Generic PHY sh7786-ether-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=sh7786-ether-0:00, irq=POLL) > Waiting up to 110 more seconds for network. > > (I booting over NFS, so the kernel tries to get an IP address over DHCP > at boot time) > > With my patch applied: > > Generic PHY sh7786-ether-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=sh7786-ether-0:00, irq=POLL) > sh-eth sh7786-ether.0 eth0: Link is Up - 100Mbps/Full - flow control rx/tx > Sending DHCP requests ., OK > IP-Config: Got DHCP answer from 192.168.0.254, my address is 192.168.0.202 > IP-Config: Complete: > device=eth0, hwaddr=d4:81:d7:45:df:5a, ipaddr=192.168.0.202, mask=255.255.255.0, gw=192.168.0.254 > host=192.168.0.202, domain=lan, nis-domain=(none) > bootserver=192.168.0.254, rootserver=192.168.0.11, rootpath= nameserver0=192.168.0.254 > VFS: Mounted root (nfs filesystem) on device 0:13. > devtmpfs: mounted > Freeing unused kernel memory: 132K > > So it seems like getting the reference to the clock has some > side-effect. I guess this needs more debugging :-) Ah, it makes sure the legacy clock domain code in drivers/sh/pm_runtime.c finds the clock, and enables it when pm_runtime_get_sync() is called. So yes, you do need the patch ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Mon, 4 Dec 2017 15:30:49 +0100, Thomas Petazzoni wrote: > The sh_eth driver (used for the Ethernet controller on SH7786) uses > devm_clk_get(&pdev->dev, NULL) to get its clock. It means that con_id > = NULL, and therefore a clock lookup made on the con_id doesn't work. > > In order to make sure that the sh_eth driver is able to find its clock > on SH7786, we switch from using a con_id to a dev_id in the clock > lookup table for this SoC. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Is it possible to get this patch merged? It has a Reviewed-by from Geert. Thanks! Thomas
Hello, On Mon, 4 Dec 2017 15:30:49 +0100, Thomas Petazzoni wrote: > The sh_eth driver (used for the Ethernet controller on SH7786) uses > devm_clk_get(&pdev->dev, NULL) to get its clock. It means that con_id > = NULL, and therefore a clock lookup made on the con_id doesn't work. > > In order to make sure that the sh_eth driver is able to find its clock > on SH7786, we switch from using a con_id to a dev_id in the clock > lookup table for this SoC. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Here as well: patch sent almost 3 months ago, it has a Reviewed-by from Geert. Is it possible to apply this patch ? Thanks, Thomas
diff --git a/arch/sh/kernel/cpu/sh4a/clock-sh7786.c b/arch/sh/kernel/cpu/sh4a/clock-sh7786.c index ac3dcfe5d303..fa183280a62c 100644 --- a/arch/sh/kernel/cpu/sh4a/clock-sh7786.c +++ b/arch/sh/kernel/cpu/sh4a/clock-sh7786.c @@ -170,7 +170,7 @@ static struct clk_lookup lookups[] = { CLKDEV_CON_ID("dmac_11_6_fck", &mstp_clks[MSTP105]), CLKDEV_CON_ID("dmac_5_0_fck", &mstp_clks[MSTP104]), CLKDEV_CON_ID("du_fck", &mstp_clks[MSTP103]), - CLKDEV_CON_ID("ether_fck", &mstp_clks[MSTP102]), + CLKDEV_DEV_ID("sh7786-ether.0", &mstp_clks[MSTP102]), }; int __init arch_clk_init(void)
The sh_eth driver (used for the Ethernet controller on SH7786) uses devm_clk_get(&pdev->dev, NULL) to get its clock. It means that con_id = NULL, and therefore a clock lookup made on the con_id doesn't work. In order to make sure that the sh_eth driver is able to find its clock on SH7786, we switch from using a con_id to a dev_id in the clock lookup table for this SoC. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- arch/sh/kernel/cpu/sh4a/clock-sh7786.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)