Message-ID: <CA+V-a8uWcgOsyG8Fy=ivs_zNqU7ur4OHzESQW=4EfYx+q2VJHg@mail.gmail.com>
Date: Sun, 2 Mar 2025 17:37:32 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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

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.

> 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.

> 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.

> Essentially, this needs discussion before settling on something saner
> than what we currently have.
>
Indeed.

Cheers,
Prabhakar

Powered by blists - more mailing lists