Message-ID: <Z8SsKQXt4SwEvP2I@shell.armlinux.org.uk>
Date: Sun, 2 Mar 2025 19:06:17 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	Jose Abreu <joabreu@...opsys.com>, netdev <netdev@...r.kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [QUERY] : STMMAC Clocks

On Sun, Mar 02, 2025 at 05:37:32PM +0000, Lad, Prabhakar wrote:
> Hi Russell,
> 
> On Sat, Mar 1, 2025 at 10:35 AM Russell King (Oracle)
> <linux@...linux.org.uk> wrote:
> >
> > On Fri, Feb 28, 2025 at 09:51:15PM +0000, Lad, Prabhakar wrote:
> > > Hi All,
> > >
> > > I am bit confused related clocks naming in with respect to STMMAC driver,
> > >
> > > We have the below clocks in the binding doc:
> > > - stmmaceth
> > > - pclk
> > > - ptp_ref
> > >
> > > But there isn't any description for this. Based on this patch [0]
> > > which isn't in mainline we have,
> > > - stmmaceth - system clock
> > > - pclk - CSR clock
> > > - ptp_ref - PTP reference clock.
> > >
> > > [0] https://patches.linaro.org/project/netdev/patch/20210208135609.7685-23-Sergey.Semin@baikalelectronics.ru/
> > >
> > > Can somebody please clarify on the above as I am planning to add a
> > > platform which supports the below clocks:
> > > - CSR clock
> > > - AXI system clock
> > > - Tx & Tx-180
> > > - Rx & Rx-180
> >
> > I'm afraid the stmmac driver is a mess when it comes to clocks.
> >
> :-)
> 
> > According to the databook, the DW GMAC IP has several clocks:
> >
> > clk_tx_i - 0° transmit clock
> > clk_tx_180_i - 180° transmit clock (synchronous to the above)
> >
> Ive named them as tx, tx-180 in the vendor specific binding.

Note that although there are separate inputs to the GMAC, they shouldn't
be treated separately - there should be no separate control of each of
them as its required that clk_tx_180_i is merely 180° out of phase with
clk_tx_i. The purpose of these two clocks is to be able to cope with
data that is transferred at both edges of e.g. a 125MHz clock without
requiring an exact 50% duty cycle 125MHz clock.

> > I've recently added generic support for clk_tx_i that platforms can
> > re-use rather than implementing the same thing over and over. You can
> > find that in net-next as of yesterday.
> >
> Thanks for the pointer, Ive rebased my changes on net-next.
> 
> > clk_rx_i - 0° receive clock
> > clk_rx_180_i - 180° of above
> >
> > These are synchronous to the datastream from the PHY, and generally
> > come from the PHY's RXC or from the PCS block integrated with the
> > GMAC. Normally these require no configuration, and thus generally
> > don't need mentioning in firmware.
> >
> On the SoC which I'm working on, these have an ON/OFF bit, so I had to
> extend my binding.
> 
> > The host specific interface clocks in your case are:
> >
> > - clock for AXI (for AXI DMA interface)
> > - clock for CSR (for register access and MDC)
> >
> > There are several different possible synthesis options for these
> > clocks, so there will be quite a bit of variability in these. I haven't
> > yet reviewed the driver for these, but I would like there to be
> > something more generic rather than each platform implementing basically
> > the same thing but differently.
> >
> I agree.

Having looked at this at various points over the weekend, stmmac_clk
seems to be the CSR clock - it's used by stmmac_main.c to calculate
the MDIO divisor. So for all intents and purposes, stmmac_clk is
csr_clk_i.

> > snps,dwc-qos-ethernet.txt lists alternative names for these clocks:
> >
> > "tx" - clk_tx_i (even mentions the official name in the description!)
> > "rx" - clk_rx_i (ditto)
> > "slave_bus" - says this is the CSR clock - however depending on
> >    synthesis options, could be one of several clocks
> > "master_bus" - AHB or AXI clock (which have different hardware names)
> > "ptp_ref" - clk_ptp_ref_i
> >
> I think it was for the older version of the IPs.
> 
> > I would encourage a new platform to either use the DW GMAC naming for
> > these clocks so we can start to have some uniformity, or maybe we could
> > standardise on the list in dwc-qos-ethernet.
> >
> I agree, in that case we need to update the driver and have fallbacks
> to maintain compatibility.
> 
> > However, I would like some standardisation around this. The names used
> > in snps,dwmac with the exception of ptp_ref make no sense as they don't
> > correspond with documentation, and convey no meaning.
> >
> > If we want to go fully with the documentation, then I would suggest:
> >
> >         hclk_i, aclk_i, clk_app_i - optional (depends on interface)
> >         clk_csr_i - optional (if not one of the above should be supplied
> >                               as CSR clock may be the same as one of the
> >                               above.)
> >         clk_tx_i - transmit clock
> >         clk_rx_i - receive clock
> >
> > As there is a configuration where aclk_i and hclk_i could be present
> > (where aclk_i is used for the interface and hclk_i is used for the CSR)
> > it may be better to deviate for clk_csr_i and use "csr" - which would
> > always point at the same clock as one of hclk_i, aclk_i, clk_app_i or
> > the separate clk_csr_i.
> >
> I agree, I think the DT maintainers wouldn't prefer "clk" in the
> prefix and "_i" in the postfix.

Really the DT maintainers shouldn't care about the format of the
clock names - there's a clock-names property, and it takes strings
that is used by the code internally. clock-names already identifies
that what follows are the names of clocks, so "clk" in "clk_foo" is
rather redundant as far as working out if the identifier is a clock
or not.

I suspect DT maintainers would much prefer clock names to have some
meaning back to hardware documentation rather than something randomly
made up.

As clk API maintainer, clk_get() as I designed the API takes the
consumer device and the clock name as defined by the *consumer*. The
clock name is not supposed to be some global identifier. The intention
of the clk API is that the hardware names used by the consumer should
always be used.

Sadly, in the beginning lots of people decided to ignore the "dev"
argument and use global clock names... and then ended up having to
pass clock names through platform data to drivers. That's just dumb
and very short sighted. I think people have generally seen the light
more recently though, especially with DT where the binding defines
the clock names (thus making them device specific) and not some
global clock name.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists