RE: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
Hi Marek, > From: Christophe ROULLIER > Sent: lundi 13 janvier 2020 17:15 > > > On 1/13/20 2:33 PM, Marek Vasut wrote: > > On 1/13/20 2:26 PM, Patrick DELAUNAY wrote: > >> Hi Marek, > > Hi, > > > >> + Christophe (Maintainer in kernel) > >> > >>> From: Marek Vasut > >>> Sent: vendredi 10 janvier 2020 01:29 > >>> > >>> Add missing 'eth-ck' clock to the ethernet node. These clock are > >>> used to generate external clock signal for the PHY in case > >>> 'st,eth_ref_clk_sel' > >>> is specified. > >>> > >>> Signed-off-by: Marek Vasut > >>> Cc: Patrick Delaunay > >>> Cc: Patrice Chotard > >>> --- > >>> arch/arm/dts/stm32mp157c.dtsi | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/arch/arm/dts/stm32mp157c.dtsi > >>> b/arch/arm/dts/stm32mp157c.dtsi index > >>> 6c670cf9a3..41aea75213 100644 > >>> --- a/arch/arm/dts/stm32mp157c.dtsi > >>> +++ b/arch/arm/dts/stm32mp157c.dtsi > >>> @@ -1404,11 +1404,13 @@ > >>> clock-names = "stmmaceth", > >>> "mac-clk-tx", > >>> "mac-clk-rx", > >>> + "eth-ck", > >>> "ethstp", > >>> "syscfg-clk"; > >>> clocks = < ETHMAC>, > >>>< ETHTX>, > >>>< ETHRX>, > >>> + < ETHCK_K>, > >>>< ETHSTP>, > >>>< SYSCFG>; > >>> st,syscon = < 0x4>; > >> This clock is optional, not needed by default for STM32MP15x: > >> this clock is used only for a specific case: Phy without quartz, for all > >> the other > case we don't need to switch on this clock. > > That's the usecase here. > > > >> Normally if it is the case, it should be done in board part, not in SOC > >> device > tree (is is the strategy chosen by kernel). > >> Moreover stm32mp157c.dtsi is just a copy of the kernel one, so this patch > >> also > need to be accepted by Kernel Maintainers. > >> > >> NB: if you are in the Phy without crytal 50MHz (rmii + > >> "st,eth_ref_clk_sel" + > "eth-ck"), > >> You should have also the associated pin in the device tree of the > >> board : > >> > >> + , /* ETH_RMII_ETHCK */ > > My PHY is _NOT_ connected to this pin, the output clock of the SoC are PA1. > > > >> Christophe you can complete my answer if it is not enough clear. > > I would expect that you should describe _all_ the clock which are > > routed into the ethernet IP in the DT and if the driver doesn't need > > some of those clock, the driver should just not enable such clock. > > Hi Marek, > > You are right, we will add by default all clocks used by stm32mp157, and > modify > driver to decide if we need to enable it or not. > > So ACK to add ETHCK_K in stm32mp157c.dtsi Christophe: thanks for ACK, Reviewed-by: Patrick Delaunay And I add the patch in U-Boot device tree by advance (the only possible impact is to switch on a not used clock). Applied to u-boot-stm32/master, thanks! Regards Patrick
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/14/20 3:36 PM, Marek Vasut wrote: > On 1/14/20 8:20 AM, Christophe ROULLIER wrote: >> On 1/13/20 6:30 PM, Marek Vasut wrote: >>> On 1/13/20 5:14 PM, Christophe ROULLIER wrote: >>> [...] >> Christophe you can complete my answer if it is not enough clear. > I would expect that you should describe _all_ the clock which are routed > into the ethernet IP in the DT and if the driver doesn't need some of > those clock, the driver should just not enable such clock. Hi Marek, >>> Hi, >>> You are right, we will add by default all clocks used by stm32mp157, and modify driver to decide if we need to enable it or not. So ACK to add ETHCK_K in stm32mp157c.dtsi >>> Do you want me to send the kernel patch ? I have it somewhere here too, >>> I can roll it out tomorrow-ish. >> Hi Marek, >> >> I have not changed yet, the kernel and uboot driver code, to manage this >> behavior. I will implement and push new code soon. > The kernel code already does what is needed. You only need similar patch > to the DT, cfr attachment. Yes, I'm aligned Marek (I've also done the glue driver in kernel :-)) But what I need to do in uboot and kernel driver it is to add by default all clocks in dtsi file and modified driver to manage them in my driver (enable or disable it in function of phy mode, if with or without quartz in PHY etc..) Regards, Christophe.
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/14/20 8:20 AM, Christophe ROULLIER wrote: > On 1/13/20 6:30 PM, Marek Vasut wrote: >> On 1/13/20 5:14 PM, Christophe ROULLIER wrote: >> [...] > Christophe you can complete my answer if it is not enough clear. I would expect that you should describe _all_ the clock which are routed into the ethernet IP in the DT and if the driver doesn't need some of those clock, the driver should just not enable such clock. >>> Hi Marek, >> Hi, >> >>> You are right, we will add by default all clocks used by stm32mp157, and >>> modify driver to decide if we need to enable it or not. >>> >>> So ACK to add ETHCK_K in stm32mp157c.dtsi >> Do you want me to send the kernel patch ? I have it somewhere here too, >> I can roll it out tomorrow-ish. > > Hi Marek, > > I have not changed yet, the kernel and uboot driver code, to manage this > behavior. I will implement and push new code soon. The kernel code already does what is needed. You only need similar patch to the DT, cfr attachment. >From a84e5d5680c2b1e3e08cba67aaff0b5417f80790 Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Tue, 3 Dec 2019 00:50:25 +0100 Subject: [PATCH] ARM: dts: stm32: Add missing ETHCK clock to ethernet node Add missing 'eth-ck' clock to the ethernet node. These clock are used to generate external clock signal for the PHY in case 'st,eth_ref_clk_sel' is specified. Signed-off-by: Marek Vasut Cc: Patrick Delaunay Cc: Patrice Chotard --- arch/arm/boot/dts/stm32mp157c.dtsi | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi index f20e0101b577..9e7e6328f97c 100644 --- a/arch/arm/boot/dts/stm32mp157c.dtsi +++ b/arch/arm/boot/dts/stm32mp157c.dtsi @@ -1380,11 +1380,13 @@ clock-names = "stmmaceth", "mac-clk-tx", "mac-clk-rx", + "eth-ck", "ethstp", "syscfg-clk"; clocks = < ETHMAC>, < ETHTX>, < ETHRX>, + < ETHCK_K>, < ETHSTP>, < SYSCFG>; st,syscon = < 0x4>; -- 2.24.1
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/13/20 6:30 PM, Marek Vasut wrote: > On 1/13/20 5:14 PM, Christophe ROULLIER wrote: > [...] Christophe you can complete my answer if it is not enough clear. >>> I would expect that you should describe _all_ the clock which are routed >>> into the ethernet IP in the DT and if the driver doesn't need some of >>> those clock, the driver should just not enable such clock. >> Hi Marek, > Hi, > >> You are right, we will add by default all clocks used by stm32mp157, and >> modify driver to decide if we need to enable it or not. >> >> So ACK to add ETHCK_K in stm32mp157c.dtsi > Do you want me to send the kernel patch ? I have it somewhere here too, > I can roll it out tomorrow-ish. Hi Marek, I have not changed yet, the kernel and uboot driver code, to manage this behavior. I will implement and push new code soon. Regards Christophe.
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/13/20 2:33 PM, Marek Vasut wrote: > On 1/13/20 2:26 PM, Patrick DELAUNAY wrote: >> Hi Marek, > Hi, > >> + Christophe (Maintainer in kernel) >> >>> From: Marek Vasut >>> Sent: vendredi 10 janvier 2020 01:29 >>> >>> Add missing 'eth-ck' clock to the ethernet node. These clock are used to >>> generate >>> external clock signal for the PHY in case 'st,eth_ref_clk_sel' >>> is specified. >>> >>> Signed-off-by: Marek Vasut >>> Cc: Patrick Delaunay >>> Cc: Patrice Chotard >>> --- >>> arch/arm/dts/stm32mp157c.dtsi | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/arch/arm/dts/stm32mp157c.dtsi b/arch/arm/dts/stm32mp157c.dtsi >>> index >>> 6c670cf9a3..41aea75213 100644 >>> --- a/arch/arm/dts/stm32mp157c.dtsi >>> +++ b/arch/arm/dts/stm32mp157c.dtsi >>> @@ -1404,11 +1404,13 @@ >>> clock-names = "stmmaceth", >>> "mac-clk-tx", >>> "mac-clk-rx", >>> + "eth-ck", >>> "ethstp", >>> "syscfg-clk"; >>> clocks = < ETHMAC>, >>> < ETHTX>, >>> < ETHRX>, >>> +< ETHCK_K>, >>> < ETHSTP>, >>> < SYSCFG>; >>> st,syscon = < 0x4>; >> This clock is optional, not needed by default for STM32MP15x: >> this clock is used only for a specific case: Phy without quartz, for all the >> other case we don't need to switch on this clock. > That's the usecase here. > >> Normally if it is the case, it should be done in board part, not in SOC >> device tree (is is the strategy chosen by kernel). >> Moreover stm32mp157c.dtsi is just a copy of the kernel one, so this patch >> also need to be accepted by Kernel Maintainers. >> >> NB: if you are in the Phy without crytal 50MHz (rmii + "st,eth_ref_clk_sel" >> + "eth-ck"), >> You should have also the associated pin in the device tree of the >> board : >> >> + , /* ETH_RMII_ETHCK */ > My PHY is _NOT_ connected to this pin, the output clock of the SoC are PA1. > >> Christophe you can complete my answer if it is not enough clear. > I would expect that you should describe _all_ the clock which are routed > into the ethernet IP in the DT and if the driver doesn't need some of > those clock, the driver should just not enable such clock. Hi Marek, You are right, we will add by default all clocks used by stm32mp157, and modify driver to decide if we need to enable it or not. So ACK to add ETHCK_K in stm32mp157c.dtsi Regards, Christophe
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/13/20 5:14 PM, Christophe ROULLIER wrote: [...] >>> Christophe you can complete my answer if it is not enough clear. >> I would expect that you should describe _all_ the clock which are routed >> into the ethernet IP in the DT and if the driver doesn't need some of >> those clock, the driver should just not enable such clock. > > Hi Marek, Hi, > You are right, we will add by default all clocks used by stm32mp157, and > modify driver to decide if we need to enable it or not. > > So ACK to add ETHCK_K in stm32mp157c.dtsi Do you want me to send the kernel patch ? I have it somewhere here too, I can roll it out tomorrow-ish.
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/13/20 2:26 PM, Patrick DELAUNAY wrote: > Hi Marek, Hi, > + Christophe (Maintainer in kernel) > >> From: Marek Vasut >> Sent: vendredi 10 janvier 2020 01:29 >> >> Add missing 'eth-ck' clock to the ethernet node. These clock are used to >> generate >> external clock signal for the PHY in case 'st,eth_ref_clk_sel' >> is specified. >> >> Signed-off-by: Marek Vasut >> Cc: Patrick Delaunay >> Cc: Patrice Chotard >> --- >> arch/arm/dts/stm32mp157c.dtsi | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/arm/dts/stm32mp157c.dtsi b/arch/arm/dts/stm32mp157c.dtsi >> index >> 6c670cf9a3..41aea75213 100644 >> --- a/arch/arm/dts/stm32mp157c.dtsi >> +++ b/arch/arm/dts/stm32mp157c.dtsi >> @@ -1404,11 +1404,13 @@ >> clock-names = "stmmaceth", >>"mac-clk-tx", >>"mac-clk-rx", >> + "eth-ck", >>"ethstp", >>"syscfg-clk"; >> clocks = < ETHMAC>, >> < ETHTX>, >> < ETHRX>, >> + < ETHCK_K>, >> < ETHSTP>, >> < SYSCFG>; >> st,syscon = < 0x4>; > > This clock is optional, not needed by default for STM32MP15x: > this clock is used only for a specific case: Phy without quartz, for all the > other case we don't need to switch on this clock. That's the usecase here. > Normally if it is the case, it should be done in board part, not in SOC > device tree (is is the strategy chosen by kernel). > Moreover stm32mp157c.dtsi is just a copy of the kernel one, so this patch > also need to be accepted by Kernel Maintainers. > > NB: if you are in the Phy without crytal 50MHz (rmii + "st,eth_ref_clk_sel" + > "eth-ck"), >You should have also the associated pin in the device tree of the > board : > > + , /* ETH_RMII_ETHCK */ My PHY is _NOT_ connected to this pin, the output clock of the SoC are PA1. > Christophe you can complete my answer if it is not enough clear. I would expect that you should describe _all_ the clock which are routed into the ethernet IP in the DT and if the driver doesn't need some of those clock, the driver should just not enable such clock.
RE: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
Hi Marek, + Christophe (Maintainer in kernel) > From: Marek Vasut > Sent: vendredi 10 janvier 2020 01:29 > > Add missing 'eth-ck' clock to the ethernet node. These clock are used to > generate > external clock signal for the PHY in case 'st,eth_ref_clk_sel' > is specified. > > Signed-off-by: Marek Vasut > Cc: Patrick Delaunay > Cc: Patrice Chotard > --- > arch/arm/dts/stm32mp157c.dtsi | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/dts/stm32mp157c.dtsi b/arch/arm/dts/stm32mp157c.dtsi > index > 6c670cf9a3..41aea75213 100644 > --- a/arch/arm/dts/stm32mp157c.dtsi > +++ b/arch/arm/dts/stm32mp157c.dtsi > @@ -1404,11 +1404,13 @@ > clock-names = "stmmaceth", > "mac-clk-tx", > "mac-clk-rx", > + "eth-ck", > "ethstp", > "syscfg-clk"; > clocks = < ETHMAC>, >< ETHTX>, >< ETHRX>, > + < ETHCK_K>, >< ETHSTP>, >< SYSCFG>; > st,syscon = < 0x4>; This clock is optional, not needed by default for STM32MP15x: this clock is used only for a specific case: Phy without quartz, for all the other case we don't need to switch on this clock. Normally if it is the case, it should be done in board part, not in SOC device tree (is is the strategy chosen by kernel). Moreover stm32mp157c.dtsi is just a copy of the kernel one, so this patch also need to be accepted by Kernel Maintainers. NB: if you are in the Phy without crytal 50MHz (rmii + "st,eth_ref_clk_sel" + "eth-ck"), You should have also the associated pin in the device tree of the board : + , /* ETH_RMII_ETHCK */ Christophe you can complete my answer if it is not enough clear. regards Patrick
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
On 1/13/20 1:50 PM, Patrice CHOTARD wrote: > Hi Marek Hi, > Adding Christophe Roullier > > > On 1/10/20 1:28 AM, Marek Vasut wrote: >> Add missing 'eth-ck' clock to the ethernet node. These clock are used to >> generate external clock signal for the PHY in case 'st,eth_ref_clk_sel' >> is specified. >> >> Signed-off-by: Marek Vasut >> Cc: Patrick Delaunay >> Cc: Patrice Chotard >> --- >> arch/arm/dts/stm32mp157c.dtsi | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/arm/dts/stm32mp157c.dtsi b/arch/arm/dts/stm32mp157c.dtsi >> index 6c670cf9a3..41aea75213 100644 >> --- a/arch/arm/dts/stm32mp157c.dtsi >> +++ b/arch/arm/dts/stm32mp157c.dtsi >> @@ -1404,11 +1404,13 @@ >> clock-names = "stmmaceth", >>"mac-clk-tx", >>"mac-clk-rx", >> + "eth-ck", >>"ethstp", >>"syscfg-clk"; >> clocks = < ETHMAC>, >> < ETHTX>, >> < ETHRX>, >> + < ETHCK_K>, >> < ETHSTP>, >> < SYSCFG>; >> st,syscon = < 0x4>; > > Adding this clock is a particular case, it should be added in the board DT > file, not in the SoC DT one. > > Our ethernet expert ( Christophe -in CC) is surprised that you don't need > the following pin: > > , /* ETH_RMII_ETHCK */ > > To confirm, can you describe your ethernet config ? I need 50 MHz RMII clock output for PHY on PA1, PG8 is completely unused.
Re: [PATCH 4/5] ARM: dts: stm32: Add missing ETHCK clock to ethernet node
Hi Marek Adding Christophe Roullier On 1/10/20 1:28 AM, Marek Vasut wrote: > Add missing 'eth-ck' clock to the ethernet node. These clock are used to > generate external clock signal for the PHY in case 'st,eth_ref_clk_sel' > is specified. > > Signed-off-by: Marek Vasut > Cc: Patrick Delaunay > Cc: Patrice Chotard > --- > arch/arm/dts/stm32mp157c.dtsi | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm/dts/stm32mp157c.dtsi b/arch/arm/dts/stm32mp157c.dtsi > index 6c670cf9a3..41aea75213 100644 > --- a/arch/arm/dts/stm32mp157c.dtsi > +++ b/arch/arm/dts/stm32mp157c.dtsi > @@ -1404,11 +1404,13 @@ > clock-names = "stmmaceth", > "mac-clk-tx", > "mac-clk-rx", > + "eth-ck", > "ethstp", > "syscfg-clk"; > clocks = < ETHMAC>, >< ETHTX>, >< ETHRX>, > + < ETHCK_K>, >< ETHSTP>, >< SYSCFG>; > st,syscon = < 0x4>; Adding this clock is a particular case, it should be added in the board DT file, not in the SoC DT one. Our ethernet expert ( Christophe -in CC) is surprised that you don't need the following pin: , /* ETH_RMII_ETHCK */ To confirm, can you describe your ethernet config ? Thanks
