Re: [PATCH 3/3] ARM64: dts: stratix10: Add stmmac ptp_ref clock

[Dalon L Westergreen <dalon.westergreen@linux.intel.com>]

On Tue, 2019-05-21 at 15:27 -0500, Dinh Nguyen wrote:
> 
> On 5/21/19 8:09 AM, Dalon L Westergreen wrote:
> > On Mon, 2019-05-20 at 11:39 -0500, Thor Thayer wrote:
> > > On 5/15/19 11:20 AM, Dalon Westergreen wrote:
> > > > Add the default stmmac ptp_ref clock for stratix10.  The stmmac
> > > > driver defaults the ptp_ref clock to the main stmmac clock
> > > > if the ptp_ref clock is not set in the devicetree.  This is
> > > > inappropriate for the stratix10.  The default ptp_ref clock is
> > > > STRATIX10_PERI_EMAC_PTP_CLK in the clock manager.
> > > > 
> > > > Signed-off-by: Dalon Westergreen <
> > > > dalon.westergreen@linux.intel.com
> > > > 
> > > > 
> > > > ---
> > > >   arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi | 12 ++++++------
> > > >   1 file changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > > > b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > > > index adedd563125a..f464e7ba3402 100644
> > > > --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > > > +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> > > > @@ -160,8 +160,8 @@
> > > >   			mac-address = [00 00 00 00 00 00];
> > > >   			resets = <&rst EMAC0_RESET>, <&rst
> > > > EMAC0_OCP_RESET>;
> > > >   			reset-names = "stmmaceth", "stmmaceth-ocp";
> > > > -			clocks = <&clkmgr STRATIX10_EMAC0_CLK>;
> > > > -			clock-names = "stmmaceth";
> > > > +			clocks = <&clkmgr STRATIX10_EMAC0_CLK>, <&clkmgr
> > > > STRATIX10_PERI_EMAC_PTP_CLK>;
> > > > +			clock-names = "stmmaceth", "ptp_ref";
> > > >   			tx-fifo-depth = <16384>;
> > > >   			rx-fifo-depth = <16384>;
> > > >   			snps,multicast-filter-bins = <256>;
> > > > @@ -176,8 +176,8 @@
> > > >   			mac-address = [00 00 00 00 00 00];
> > > >   			resets = <&rst EMAC1_RESET>, <&rst
> > > > EMAC1_OCP_RESET>;
> > > >   			reset-names = "stmmaceth", "stmmaceth-ocp";
> > > > -			clocks = <&clkmgr STRATIX10_EMAC1_CLK>;
> > > > -			clock-names = "stmmaceth";
> > > > +			clocks = <&clkmgr STRATIX10_EMAC1_CLK>, <&clkmgr
> > > > STRATIX10_PERI_EMAC_PTP_CLK>;
> > > > +			clock-names = "stmmaceth", "ptp_ref";
> > > >   			tx-fifo-depth = <16384>;
> > > >   			rx-fifo-depth = <16384>;
> > > >   			snps,multicast-filter-bins = <256>;
> > > > @@ -192,8 +192,8 @@
> > > >   			mac-address = [00 00 00 00 00 00];
> > > >   			resets = <&rst EMAC2_RESET>, <&rst
> > > > EMAC2_OCP_RESET>;
> > > >   			reset-names = "stmmaceth", "stmmaceth-ocp";
> > > > -			clocks = <&clkmgr STRATIX10_EMAC2_CLK>;
> > > > -			clock-names = "stmmaceth";
> > > > +			clocks = <&clkmgr STRATIX10_EMAC2_CLK>, <&clkmgr
> > > > STRATIX10_PERI_EMAC_PTP_CLK>;
> > > > +			clock-names = "stmmaceth", "ptp_ref";
> > > >   			tx-fifo-depth = <16384>;
> > > >   			rx-fifo-depth = <16384>;
> > > >   			snps,multicast-filter-bins = <256>;
> > > > 
> > > 
> > > Should STRATIX10_EMAC_PTP_CLK be used instead of 
> > > STRATIX10_PERI_EMAC_PTP_CLK since this is the gate of the clkgmr?
> > > 
> > 
> > Either can be used, but the default in the hps configuration is the
> > peripheral pll output and not the main pll output.
> > 
> 
> I think it should be the emac_ptp_clk. If you look at the clock tree,
> the emac_ptp_clk(50 MHz) and is derived from the peri_emac_ptp_clk(200
> MHz).
> 
> You can look at the clock tree by doing a 'cat
> /sys/kernel/debug/clk/clk_summary'.
> 
> Dinh
> 
I dont disagree, but this devicetree is a representation of what was implemented
in the devkit no?  Should we first request that this is changed in the design?
or should i set this to STRATIX10_EMAC_PTP_CLK and overide it in the top level
socdk devicetree?

dalon