[QUERY] : STMMAC Clocks

[QUERY] : STMMAC Clocks

[Lad, Prabhakar]

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

Cheers,
Prabhakar

Re: [QUERY] : STMMAC Clocks

[Andrew Lunn]

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

Please take a look at the recent patches to stmmac for clock handling,
in particular the clocks used for RGMII

For the meaning of the clocks, you need to look at the vendors binding
document. Vendors tend to call the clocks whatever they want, rather
than have one consistent naming between vendors. The IP might be
licensed, but each vendor integrates it differently, inventing their
own clock names. It might of helped if Synopsis had requested in there
databook what each clock was called, so there was some consistency,
but this does not appear to of happened.

    Andrew

Re: [QUERY] : STMMAC Clocks

[Russell King (Oracle)]

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)

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.

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.

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.

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

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.

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

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

Re: [QUERY] : STMMAC Clocks

[Russell King (Oracle)]

On Sat, Mar 01, 2025 at 12:38:04AM +0100, Andrew Lunn 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
> 
> Please take a look at the recent patches to stmmac for clock handling,
> in particular the clocks used for RGMII
> 
> For the meaning of the clocks, you need to look at the vendors binding
> document. Vendors tend to call the clocks whatever they want, rather
> than have one consistent naming between vendors. The IP might be
> licensed, but each vendor integrates it differently, inventing their
> own clock names. It might of helped if Synopsis had requested in there
> databook what each clock was called, so there was some consistency,
> but this does not appear to of happened.

Part of the problem is that vendors can place clock muxes and gates
and divisors around the Synopsys block, where some muxes/divisors can
be controlled by signals output by the Synopsys block (and thus are
hidden from software). This is especially true of the pair of clk_tx_i
and pair of clk_rx_i clocks.

Thus, the clocks that are visible may be functionally different from
the Synopsys defined clocks.

However, I think that we should push to standardise on the Synopsys
named clock names where they exist (essentially optional) and then
allow platform specific clocks where they're buried out of view in
the way I describe above.

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

Re: [QUERY] : STMMAC Clocks

[Andrew Lunn]

> However, I think that we should push to standardise on the Synopsys
> named clock names where they exist (essentially optional) and then
> allow platform specific clocks where they're buried out of view in
> the way I describe above.

Interestingly snps,dwc-qos-ethernet.txt has a pretty good description:

- clock-names: May contain any/all of the following depending on the IP
  configuration, in any order:
  - "tx"
    The EQOS transmit path clock. The HW signal name is clk_tx_i.
    In some configurations (e.g. GMII/RGMII), this clock also drives the PHY TX
    path. In other configurations, other clocks (such as tx_125, rmii) may
    drive the PHY TX path.
  - "rx"
    The EQOS receive path clock. The HW signal name is clk_rx_i.
    In some configurations (e.g. GMII/RGMII), this clock is derived from the
    PHY's RX clock output. In other configurations, other clocks (such as
    rx_125, rmii) may drive the EQOS RX path.
    In cases where the PHY clock is directly fed into the EQOS receive path
    without intervening logic, the DT need not represent this clock, since it
    is assumed to be fully under the control of the PHY device/driver. In
    cases where SoC integration adds additional logic to this path, such as a
    SW-controlled clock gate, this clock should be represented in DT.
  - "slave_bus"
    The CPU/slave-bus (CSR) interface clock. This applies to any bus type;
    APB, AHB, AXI, etc. The HW signal name is hclk_i (AHB) or clk_csr_i (other
    buses).
  - "master_bus"
    The master bus interface clock. Only required in configurations that use a
    separate clock for the master and slave bus interfaces. The HW signal name
    is hclk_i (AHB) or aclk_i (AXI).
  - "ptp_ref"
    The PTP reference clock. The HW signal name is clk_ptp_ref_i.
  - "phy_ref_clk"
    This clock is deprecated and should not be used by new compatible values.
    It is equivalent to "tx".
  - "apb_pclk"
    This clock is deprecated and should not be used by new compatible values.
    It is equivalent to "slave_bus".

But snps,dwmac.yaml only has:

 clock-names:
    minItems: 1
    maxItems: 8
    additionalItems: true
    contains:
      enum:
        - stmmaceth
        - pclk
        - ptp_ref

Could you improve the description in snps,dwmac.yaml, based on what
you seen in the data book?

	Andrew

Re: [QUERY] : STMMAC Clocks

[Russell King (Oracle)]

On Sat, Mar 01, 2025 at 04:15:17PM +0100, Andrew Lunn wrote:
> > However, I think that we should push to standardise on the Synopsys
> > named clock names where they exist (essentially optional) and then
> > allow platform specific clocks where they're buried out of view in
> > the way I describe above.
> 
> Interestingly snps,dwc-qos-ethernet.txt has a pretty good description:
> 
> - clock-names: May contain any/all of the following depending on the IP
>   configuration, in any order:
>   - "tx"
>     The EQOS transmit path clock. The HW signal name is clk_tx_i.
>     In some configurations (e.g. GMII/RGMII), this clock also drives the PHY TX
>     path. In other configurations, other clocks (such as tx_125, rmii) may
>     drive the PHY TX path.
>   - "rx"
>     The EQOS receive path clock. The HW signal name is clk_rx_i.
>     In some configurations (e.g. GMII/RGMII), this clock is derived from the
>     PHY's RX clock output. In other configurations, other clocks (such as
>     rx_125, rmii) may drive the EQOS RX path.
>     In cases where the PHY clock is directly fed into the EQOS receive path
>     without intervening logic, the DT need not represent this clock, since it
>     is assumed to be fully under the control of the PHY device/driver. In
>     cases where SoC integration adds additional logic to this path, such as a
>     SW-controlled clock gate, this clock should be represented in DT.
>   - "slave_bus"
>     The CPU/slave-bus (CSR) interface clock. This applies to any bus type;
>     APB, AHB, AXI, etc. The HW signal name is hclk_i (AHB) or clk_csr_i (other
>     buses).
>   - "master_bus"
>     The master bus interface clock. Only required in configurations that use a
>     separate clock for the master and slave bus interfaces. The HW signal name
>     is hclk_i (AHB) or aclk_i (AXI).
>   - "ptp_ref"
>     The PTP reference clock. The HW signal name is clk_ptp_ref_i.
>   - "phy_ref_clk"
>     This clock is deprecated and should not be used by new compatible values.
>     It is equivalent to "tx".
>   - "apb_pclk"
>     This clock is deprecated and should not be used by new compatible values.
>     It is equivalent to "slave_bus".
> 
> But snps,dwmac.yaml only has:
> 
>  clock-names:
>     minItems: 1
>     maxItems: 8
>     additionalItems: true
>     contains:
>       enum:
>         - stmmaceth
>         - pclk
>         - ptp_ref
> 
> Could you improve the description in snps,dwmac.yaml, based on what
> you seen in the data book?

I'm afraid I can't, because the description there is basically rubbish.
As I stated in my previous email, the only one listed there which
means anything as far as the databook is concerned is "ptp_ref". The
other two are just made up names that have no basis for anything in
the databook.

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

Re: [QUERY] : STMMAC Clocks

[Lad, Prabhakar]

Hi Andrew,

On Fri, Feb 28, 2025 at 11:38 PM Andrew Lunn <andrew@lunn.ch> 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
>
> Please take a look at the recent patches to stmmac for clock handling,
> in particular the clocks used for RGMII
>
Thank you for the pointer, Ive rebased my changes on this.

> For the meaning of the clocks, you need to look at the vendors binding
> document. Vendors tend to call the clocks whatever they want, rather
> than have one consistent naming between vendors. The IP might be
> licensed, but each vendor integrates it differently, inventing their
> own clock names. It might of helped if Synopsis had requested in there
> databook what each clock was called, so there was some consistency,
> but this does not appear to of happened.
>
Thanks, I will have a look at vendors binding.

Cheers,
Prabhakar

Re: [QUERY] : STMMAC Clocks

[Lad, Prabhakar]

Hi Russell,

On Sat, Mar 1, 2025 at 10:35 AM Russell King (Oracle)
<linux@armlinux.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

Re: [QUERY] : STMMAC Clocks

[Russell King (Oracle)]

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@armlinux.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!