RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

Hello netdev,

We are planning to publish driver patches for adding Ethernet support
for Qualcomm's IPQ9574 SoC, and looking for some advice on the approach
to follow. There are two new drivers (described below) split across four
patch series, totaling to 40 patches. These two drivers depend on a
couple of clock controller drivers which are currently in review with
the community.

Support is currently being added only for IPQ9574 SoC. However the
drivers are written for the Qualcomm PPE (packet process engine)
architecture, and are easily extendable for additional IPQ SoC (Ex:
IPQ5332) that belong to the same network architecture family.

Given the number of patches for IPQ9574, we were wondering whether it is
preferred to publish the four series together, since having all the code
available could help clarify the inter-workings of the code. Or whether
it is preferred to publish the patches sequentially, depending on the
review progress?

As part of this email, we also wanted to give a brief overview of the
various hardware blocks involved, the various driver patch series, and
list the dependencies between them. Hopefully this will help the review
process (this info will be also added to driver doc). The rest of the
email addresses this.

Thank you for your time!

With Regards
Kiran

=========================================================================
Section Layout
==============
1. IPQ Ethernet hardware overview
   1.1 PPE: Internal blocks overview
   1.2 Clock controllers for network function
2. List of Patch series and dependencies


1. IPQ Ethernet hardware overview
=================================
The PPE (packet process engine) is a hardware block in IPQ SoC which
provides Ethernet and L2/L3 networking offload functions. These L2/L3
functions help offload network processing functions from the CPU.
Specifically w.r.to Ethernet functionality, it broadly comprises of
three components: Ethernet DMA, Switch core and GMACs/xGMACs.

On IPQ9574 SoC, the PPE includes 6 ethernet ports (1G/10G MACs) which
can connect to external PHY. For packet transfer between host CPU and
these ethernet ports, the PPE interfaces with the host CPU using
Ethernet-DMA (EDMA) on a special CPU connected port.

It also includes a switch core which can transfer packets between the
switch GMAC ports and CPU, or transfer packets between the switch ports.

The below diagram for IPQ9574 depicts the hardware blocks within and
outside PPE, which work together to support Ethernet functionality.

Fig.1.1 PPE and hardware block connectivity diagram for IPQ9574
---------------------------------------------------------------
         +---------+
         |  48MHZ  |
         +----+----+
              |(clock)
              v
         +----+----+
  +------| CMN PLL |
  |      +----+----+
  |           |(clock)
  |           v
  |      +----+----+           +----+----+  clock   +----+----+
  |  +---|  NSSCC  |           |   GCC   |--------->|   MDIO  |
  |  |   +----+----+           +----+----+          +----+----+
  |  |        |(clock & reset)      |(clock & reset)
  |  |        v                     v
  |  |   +-----------------------------+----------+----------+---------+
  |  |   |       +-----+               |EDMA FIFO |          | EIP FIFO|
  |  |   |       | SCH |               +----------+          +---------+
  |  |   |       +-----+                      |               |        |
  |  |   |  +------+   +------+            +-------------------+       |
  |  |   |  |  BM  |   |  QM  |            | L2/L3 Switch Core |       |
  |  |   |  +------+   +------+            +-------------------+       |
  |  |   |                                   |                         |
  |  |   | +-------+ +-------+ +-------+ +-------+ +-------+ +-------+ |
  |  |   | |  MAC0 | |  MAC1 | |  MAC2 | |  MAC3 | | XGMAC4| |XGMAC5 | |
  |  |   | +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ |
  |  |   |     |         |         |         |         |         |     |
  |  |   +-----+---------+---------+---------+---------+---------+-----+
  |  |         |         |         |         |         |         |
  |  |     +---+---------+---------+---------+---+ +---+---+ +---+---+
  +--+---->|             PCS0                    | |  PCS1 | | PCS2  |
  | clock  +---+---------+---------+---------+---+ +---+---+ +---+---+
  |            |         |         |         |         |         |
  |        +---+---------+---------+---------+---+ +---+---+ +---+---+
  | clock  +----------------+                    | |       | |       |
  +------->|Clock Controller|   4-port Eth PHY   | | PHY4  | | PHY5  |
           +----------------+--------------------+ +-------+ +-------+


1.1 PPE: Internal blocks overview
=================================

The Switch core
---------------
It has maximum 8 ports, comprising 6 GMAC ports and two DMA interfaces
(for Ethernet DMA and EIP security processor) on the IPQ9574.

GMAC/xGMAC
----------
There are 6 GMAC and 6 XGMAC in IPQ9574. Depending on the board ethernet
configuration, either GMAC or XGMAC is selected by the PPE driver to
interface with the PCS. The PPE driver initializes and manages these
GMACs, and registers one netdevice per GMAC.

EDMA (Ethernet DMA)
-------------------
This is a common ethernet DMA block inside PPE, which is used to
transmit and receive packets between Ethernet ports in the PPE switch,
and the ARM CPU cores. The PPE driver includes the ethernet DMA driver,
and registers one netdevice per PPE port.

PCS
---
The PCS blocks are outside the PPE, and provides the connection between
PPE's GMAC/XGMAC and the external ethernet PHY. There are 3 PCS
instances supported by IPQ9574. The PCS provides the PCS/xPCS function
to support modes such as SGMII/2500BASE-X/QSGMII/USXGMII/10G-BASE-R modes.

SCH (Scheduler)
---------------
The PPE driver initializes this block to enable traffic scheduling for
switch ports at egress.

QM (Queue Manager)
------------------
This block manages the various egress queues of the PPE switch.

The queues inside the switch core in PPE are mapped to the switch
ports. The PPE driver initializes this block to enable the switch
port to queue mappings as per the SoC's port configuration for
IPQ9574.

BM (Buffer Manager)
-------------------
This block manages the buffer thresholds for PPE port flow control.
The buffer availability for a port at run time will influence the
behavior of flow control on that port.

1.2 Clock controllers for network function
==========================================
Common PLL block
----------------
This block takes in a fixed reference clock as input and provide
clocks to other hardware blocks and peripherals such as NSS clock
controller, and to external PHYs as output.

NSS clock controller
--------------------
The NSS clock controller supplies clock to ethernet hardware blocks in
the IPQ such as PPE and PCS blocks, as shown in the diagram. It takes
clock input from Common PLL.

2. List of patch series and dependencies
========================================

Clock drivers (currently in review)
===================================
1) CMN PLL driver patch series:
	Currently in review with community.
	https://lore.kernel.org/linux-arm-msm/20240827-qcom_ipq_cmnpll-v3-0-8e009cece8b2@quicinc.com/


2) NSS clock controller (NSSCC) driver patch series
	Currently in review with community.
	https://lore.kernel.org/linux-arm-msm/20240626143302.810632-1-quic_devipriy@quicinc.com/


Networking drivers (to be posted for review next week)
======================================================

The following patch series are planned to be pushed for the PPE and PCS
drivers, to support ethernet function. These patch series are listed
below in dependency order.

3) PCS driver patch series:
        Driver for the PCS block in IPQ9574. New IPQ PCS driver will
        be enabled in drivers/net/pcs/
	Dependent on NSS CC patch series (2).

4) PPE base driver patch series
        Base PPE driver support for IPQ9574.
	Configures scheduler, BM, QM, MAC during initialization for
		IPQ9574.
	Dependent on NSS CC patch series (2).

	We plan to update the below patch series which was paused
        earlier, with the new code which addresses the earlier
        shortcomings.

	https://lore.kernel.org/netdev/20240110114033.32575-1-quic_luoj@quicinc.com/

5) PPE MAC support patch series
        PPE driver patches to configure the various functions of
        ethernet MAC in PPE hardware and to enable phylink support for
        each ethernet port.

        Dependent on NSS CC (2) and PPE base (4) patch series

6) Ethernet DMA driver patch series (part of PPE driver)
        Enables DMA driver support for PPE EDMA, netdevice
        registration and operation for each of the PPE ports.

        Dependendent on NSS CC (2), PPE base (4), PPE MAC (5) patch
        series.

Other notable dependencies
===============================
MDIO driver:
        Already merged in net-next via the below patch from Christian.

https://lore.kernel.org/linux-arm-msm/170686862773.17682.10435156329986246682.git-patchwork-notify@kernel.org/

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Andrew Lunn]

On Thu, Oct 03, 2024 at 02:07:10AM +0530, Kiran Kumar C.S.K wrote:
> Hello netdev,
> 
> We are planning to publish driver patches for adding Ethernet support
> for Qualcomm's IPQ9574 SoC, and looking for some advice on the approach
> to follow. There are two new drivers (described below) split across four
> patch series, totaling to 40 patches. These two drivers depend on a
> couple of clock controller drivers which are currently in review with
> the community.
> 
> Support is currently being added only for IPQ9574 SoC. However the
> drivers are written for the Qualcomm PPE (packet process engine)
> architecture, and are easily extendable for additional IPQ SoC (Ex:
> IPQ5332) that belong to the same network architecture family.
> 
> Given the number of patches for IPQ9574, we were wondering whether it is
> preferred to publish the four series together, since having all the code
> available could help clarify the inter-workings of the code. Or whether
> it is preferred to publish the patches sequentially, depending on the
> review progress?

Sequentially. You are likely to learn about working with mainline code
from the first patch series, which will allow you to improve the
following series before posting them.

>          +---------+
>          |  48MHZ  |
>          +----+----+
>               |(clock)
>               v
>          +----+----+
>   +------| CMN PLL |
>   |      +----+----+
>   |           |(clock)
>   |           v
>   |      +----+----+           +----+----+  clock   +----+----+
>   |  +---|  NSSCC  |           |   GCC   |--------->|   MDIO  |
>   |  |   +----+----+           +----+----+          +----+----+
>   |  |        |(clock & reset)      |(clock & reset)
>   |  |        v                     v
>   |  |   +-----------------------------+----------+----------+---------+
>   |  |   |       +-----+               |EDMA FIFO |          | EIP FIFO|
>   |  |   |       | SCH |               +----------+          +---------+
>   |  |   |       +-----+                      |               |        |
>   |  |   |  +------+   +------+            +-------------------+       |
>   |  |   |  |  BM  |   |  QM  |            | L2/L3 Switch Core |       |
>   |  |   |  +------+   +------+            +-------------------+       |
>   |  |   |                                   |                         |
>   |  |   | +-------+ +-------+ +-------+ +-------+ +-------+ +-------+ |
>   |  |   | |  MAC0 | |  MAC1 | |  MAC2 | |  MAC3 | | XGMAC4| |XGMAC5 | |
>   |  |   | +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ |
>   |  |   |     |         |         |         |         |         |     |
>   |  |   +-----+---------+---------+---------+---------+---------+-----+
>   |  |         |         |         |         |         |         |
>   |  |     +---+---------+---------+---------+---+ +---+---+ +---+---+
>   +--+---->|             PCS0                    | |  PCS1 | | PCS2  |
>   | clock  +---+---------+---------+---------+---+ +---+---+ +---+---+
>   |            |         |         |         |         |         |
>   |        +---+---------+---------+---------+---+ +---+---+ +---+---+
>   | clock  +----------------+                    | |       | |       |
>   +------->|Clock Controller|   4-port Eth PHY   | | PHY4  | | PHY5  |
>            +----------------+--------------------+ +-------+ +-------+
> 
> 
> 1.1 PPE: Internal blocks overview
> =================================
> 
> The Switch core
> ---------------
> It has maximum 8 ports, comprising 6 GMAC ports and two DMA interfaces
> (for Ethernet DMA and EIP security processor) on the IPQ9574.

How are packets from the host directed to a specific egress port? Is
there bits in the DMA descriptor of the EDMA? Or is there an
additional header in the fields? This will determine if you are
writing a DSA switch driver, or a pure switchdev driver. 

> GMAC/xGMAC
> ----------
> There are 6 GMAC and 6 XGMAC in IPQ9574. Depending on the board ethernet
> configuration, either GMAC or XGMAC is selected by the PPE driver to
> interface with the PCS. The PPE driver initializes and manages these
> GMACs, and registers one netdevice per GMAC.

That suggests you are doing a pure switchdev driver.

> 2. List of patch series and dependencies
> ========================================
> 
> Clock drivers (currently in review)
> ===================================
> 1) CMN PLL driver patch series:
> 	Currently in review with community.
> 	https://lore.kernel.org/linux-arm-msm/20240827-qcom_ipq_cmnpll-v3-0-8e009cece8b2@quicinc.com/
> 
> 
> 2) NSS clock controller (NSSCC) driver patch series
> 	Currently in review with community.
> 	https://lore.kernel.org/linux-arm-msm/20240626143302.810632-1-quic_devipriy@quicinc.com/
> 
> 
> Networking drivers (to be posted for review next week)
> ======================================================
> 
> The following patch series are planned to be pushed for the PPE and PCS
> drivers, to support ethernet function. These patch series are listed
> below in dependency order.
> 
> 3) PCS driver patch series:
>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
>         be enabled in drivers/net/pcs/
> 	Dependent on NSS CC patch series (2).

I assume this dependency is pure at runtime? So the code will build
without the NSS CC patch series?

This should be a good way to start, PCS drivers are typically nice and
simple.

	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/3/2024 2:58 AM, Andrew Lunn wrote:
> On Thu, Oct 03, 2024 at 02:07:10AM +0530, Kiran Kumar C.S.K wrote:
>> Hello netdev,
>>
>> We are planning to publish driver patches for adding Ethernet support
>> for Qualcomm's IPQ9574 SoC, and looking for some advice on the approach
>> to follow. There are two new drivers (described below) split across four
>> patch series, totaling to 40 patches. These two drivers depend on a
>> couple of clock controller drivers which are currently in review with
>> the community.
>>
>> Support is currently being added only for IPQ9574 SoC. However the
>> drivers are written for the Qualcomm PPE (packet process engine)
>> architecture, and are easily extendable for additional IPQ SoC (Ex:
>> IPQ5332) that belong to the same network architecture family.
>>
>> Given the number of patches for IPQ9574, we were wondering whether it is
>> preferred to publish the four series together, since having all the code
>> available could help clarify the inter-workings of the code. Or whether
>> it is preferred to publish the patches sequentially, depending on the
>> review progress?
> 
> Sequentially. You are likely to learn about working with mainline code
> from the first patch series, which will allow you to improve the
> following series before posting them.
> 

OK, thanks.

>>          +---------+
>>          |  48MHZ  |
>>          +----+----+
>>               |(clock)
>>               v
>>          +----+----+
>>   +------| CMN PLL |
>>   |      +----+----+
>>   |           |(clock)
>>   |           v
>>   |      +----+----+           +----+----+  clock   +----+----+
>>   |  +---|  NSSCC  |           |   GCC   |--------->|   MDIO  |
>>   |  |   +----+----+           +----+----+          +----+----+
>>   |  |        |(clock & reset)      |(clock & reset)
>>   |  |        v                     v
>>   |  |   +-----------------------------+----------+----------+---------+
>>   |  |   |       +-----+               |EDMA FIFO |          | EIP FIFO|
>>   |  |   |       | SCH |               +----------+          +---------+
>>   |  |   |       +-----+                      |               |        |
>>   |  |   |  +------+   +------+            +-------------------+       |
>>   |  |   |  |  BM  |   |  QM  |            | L2/L3 Switch Core |       |
>>   |  |   |  +------+   +------+            +-------------------+       |
>>   |  |   |                                   |                         |
>>   |  |   | +-------+ +-------+ +-------+ +-------+ +-------+ +-------+ |
>>   |  |   | |  MAC0 | |  MAC1 | |  MAC2 | |  MAC3 | | XGMAC4| |XGMAC5 | |
>>   |  |   | +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ |
>>   |  |   |     |         |         |         |         |         |     |
>>   |  |   +-----+---------+---------+---------+---------+---------+-----+
>>   |  |         |         |         |         |         |         |
>>   |  |     +---+---------+---------+---------+---+ +---+---+ +---+---+
>>   +--+---->|             PCS0                    | |  PCS1 | | PCS2  |
>>   | clock  +---+---------+---------+---------+---+ +---+---+ +---+---+
>>   |            |         |         |         |         |         |
>>   |        +---+---------+---------+---------+---+ +---+---+ +---+---+
>>   | clock  +----------------+                    | |       | |       |
>>   +------->|Clock Controller|   4-port Eth PHY   | | PHY4  | | PHY5  |
>>            +----------------+--------------------+ +-------+ +-------+
>>
>>
>> 1.1 PPE: Internal blocks overview
>> =================================
>>
>> The Switch core
>> ---------------
>> It has maximum 8 ports, comprising 6 GMAC ports and two DMA interfaces
>> (for Ethernet DMA and EIP security processor) on the IPQ9574.
> 
> How are packets from the host directed to a specific egress port? Is
> there bits in the DMA descriptor of the EDMA? Or is there an
> additional header in the fields? This will determine if you are
> writing a DSA switch driver, or a pure switchdev driver. 

The DMA descriptor carries the information on the destination port.
There is no additional header required in the packet.

> 
>> GMAC/xGMAC
>> ----------
>> There are 6 GMAC and 6 XGMAC in IPQ9574. Depending on the board ethernet
>> configuration, either GMAC or XGMAC is selected by the PPE driver to
>> interface with the PCS. The PPE driver initializes and manages these
>> GMACs, and registers one netdevice per GMAC.
> 
> That suggests you are doing a pure switchdev driver.
> 

Agree that switchdev is the right model for this device. We were
planning to enable base Ethernet functionality using regular
(non-switchdev) netdevice representation for the ports initially,
without offload support. As the next step, L2/VLAN offload support using
switchdev will be enabled on top. Hope this phased approach is fine.

>> 2. List of patch series and dependencies
>> ========================================
>>
>> Clock drivers (currently in review)
>> ===================================
>> 1) CMN PLL driver patch series:
>> 	Currently in review with community.
>> 	https://lore.kernel.org/linux-arm-msm/20240827-qcom_ipq_cmnpll-v3-0-8e009cece8b2@quicinc.com/
>>
>>
>> 2) NSS clock controller (NSSCC) driver patch series
>> 	Currently in review with community.
>> 	https://lore.kernel.org/linux-arm-msm/20240626143302.810632-1-quic_devipriy@quicinc.com/
>>
>>
>> Networking drivers (to be posted for review next week)
>> ======================================================
>>
>> The following patch series are planned to be pushed for the PPE and PCS
>> drivers, to support ethernet function. These patch series are listed
>> below in dependency order.
>>
>> 3) PCS driver patch series:
>>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
>>         be enabled in drivers/net/pcs/
>> 	Dependent on NSS CC patch series (2).
> 
> I assume this dependency is pure at runtime? So the code will build
> without the NSS CC patch series?

The MII Rx/Tx clocks are supplied from the NSS clock controller to the
PCS's MII channels. To represent this in the DTS, the PCS node in the
DTS is configured with the MII Rx/Tx clock that it consumes, using
macros for clocks which are exported from the NSS CC driver in a header
file. So, there will be a compile-time dependency for the dtbindings/DTS
on the NSS CC patch series. We will clearly call out this dependency in
the cover letter of the PCS driver. Hope that this approach is ok.

> 
> This should be a good way to start, PCS drivers are typically nice and
> simple.
> 

Sure, thanks for the inputs.

> 	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Andrew Lunn]

> Agree that switchdev is the right model for this device. We were
> planning to enable base Ethernet functionality using regular
> (non-switchdev) netdevice representation for the ports initially,
> without offload support. As the next step, L2/VLAN offload support using
> switchdev will be enabled on top. Hope this phased approach is fine.

Since it is not a DSA switch, yes, a phased approach should be O.K.

> >> 3) PCS driver patch series:
> >>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
> >>         be enabled in drivers/net/pcs/
> >> 	Dependent on NSS CC patch series (2).
> > 
> > I assume this dependency is pure at runtime? So the code will build
> > without the NSS CC patch series?
> 
> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
> PCS's MII channels. To represent this in the DTS, the PCS node in the
> DTS is configured with the MII Rx/Tx clock that it consumes, using
> macros for clocks which are exported from the NSS CC driver in a header
> file. So, there will be a compile-time dependency for the dtbindings/DTS
> on the NSS CC patch series. We will clearly call out this dependency in
> the cover letter of the PCS driver. Hope that this approach is ok.

Since there is a compile time dependency, you might want to ask for
the clock patches to be put into a stable branch which can be merged
into netdev.

Or you need to wait a kernel cycle.

   Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Bjorn Andersson]

On Thu, Oct 03, 2024 at 11:20:03PM +0530, Kiran Kumar C.S.K wrote:
> On 10/3/2024 2:58 AM, Andrew Lunn wrote:
> > On Thu, Oct 03, 2024 at 02:07:10AM +0530, Kiran Kumar C.S.K wrote:
[..]
> >> 2. List of patch series and dependencies
> >> ========================================
> >>
> >> Clock drivers (currently in review)
> >> ===================================
> >> 1) CMN PLL driver patch series:
> >> 	Currently in review with community.
> >> 	https://lore.kernel.org/linux-arm-msm/20240827-qcom_ipq_cmnpll-v3-0-8e009cece8b2@quicinc.com/
> >>
> >>
> >> 2) NSS clock controller (NSSCC) driver patch series
> >> 	Currently in review with community.
> >> 	https://lore.kernel.org/linux-arm-msm/20240626143302.810632-1-quic_devipriy@quicinc.com/
> >>
> >>
> >> Networking drivers (to be posted for review next week)
> >> ======================================================
> >>
> >> The following patch series are planned to be pushed for the PPE and PCS
> >> drivers, to support ethernet function. These patch series are listed
> >> below in dependency order.
> >>
> >> 3) PCS driver patch series:
> >>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
> >>         be enabled in drivers/net/pcs/
> >> 	Dependent on NSS CC patch series (2).
> > 
> > I assume this dependency is pure at runtime? So the code will build
> > without the NSS CC patch series?
> 
> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
> PCS's MII channels. To represent this in the DTS, the PCS node in the
> DTS is configured with the MII Rx/Tx clock that it consumes, using
> macros for clocks which are exported from the NSS CC driver in a header
> file. So, there will be a compile-time dependency for the dtbindings/DTS
> on the NSS CC patch series. We will clearly call out this dependency in
> the cover letter of the PCS driver. Hope that this approach is ok.
> 

So you're not going to expose these clocks through the common clock
framework and use standard DeviceTree properties for consuming the
clocks?

I expect the bindings for these things to go through respective tree
(clock and netdev) and then the DeviceTree source (dts) addition to go
through the qcom tree.

The only remaining dependency I was expecting is the qcom tree depending
on the clock and netdev trees to have picked up the bindings, and for
new bindings I do accept dts changes in the same cycle (I validate dts
against bindings in linux-next).

Regards,
Bjorn

> > 
> > This should be a good way to start, PCS drivers are typically nice and
> > simple.
> > 
> 
> Sure, thanks for the inputs.
> 
> > 	Andrew
> 
> 

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/4/2024 12:12 AM, Andrew Lunn wrote:
>> Agree that switchdev is the right model for this device. We were
>> planning to enable base Ethernet functionality using regular
>> (non-switchdev) netdevice representation for the ports initially,
>> without offload support. As the next step, L2/VLAN offload support using
>> switchdev will be enabled on top. Hope this phased approach is fine.
> 
> Since it is not a DSA switch, yes, a phased approach should be O.K.
> 

Ok.

>>>> 3) PCS driver patch series:
>>>>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
>>>>         be enabled in drivers/net/pcs/
>>>> 	Dependent on NSS CC patch series (2).
>>>
>>> I assume this dependency is pure at runtime? So the code will build
>>> without the NSS CC patch series?
>>
>> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
>> PCS's MII channels. To represent this in the DTS, the PCS node in the
>> DTS is configured with the MII Rx/Tx clock that it consumes, using
>> macros for clocks which are exported from the NSS CC driver in a header
>> file. So, there will be a compile-time dependency for the dtbindings/DTS
>> on the NSS CC patch series. We will clearly call out this dependency in
>> the cover letter of the PCS driver. Hope that this approach is ok.
> 
> Since there is a compile time dependency, you might want to ask for
> the clock patches to be put into a stable branch which can be merged
> into netdev.
> 

Sure. We will request for such a stable branch merge once the NSS CC
patches are accepted by the reviewers. Could the 'net' tree be one such
stable branch option to merge the NSS CC driver?


> Or you need to wait a kernel cycle.>
Understand.

>    Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Andrew Lunn]

On Fri, Oct 04, 2024 at 06:36:59PM +0530, Kiran Kumar C.S.K wrote:
> 
> 
> On 10/4/2024 12:12 AM, Andrew Lunn wrote:
> >> Agree that switchdev is the right model for this device. We were
> >> planning to enable base Ethernet functionality using regular
> >> (non-switchdev) netdevice representation for the ports initially,
> >> without offload support. As the next step, L2/VLAN offload support using
> >> switchdev will be enabled on top. Hope this phased approach is fine.
> > 
> > Since it is not a DSA switch, yes, a phased approach should be O.K.
> > 
> 
> Ok.
> 
> >>>> 3) PCS driver patch series:
> >>>>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
> >>>>         be enabled in drivers/net/pcs/
> >>>> 	Dependent on NSS CC patch series (2).
> >>>
> >>> I assume this dependency is pure at runtime? So the code will build
> >>> without the NSS CC patch series?
> >>
> >> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
> >> PCS's MII channels. To represent this in the DTS, the PCS node in the
> >> DTS is configured with the MII Rx/Tx clock that it consumes, using
> >> macros for clocks which are exported from the NSS CC driver in a header
> >> file. So, there will be a compile-time dependency for the dtbindings/DTS
> >> on the NSS CC patch series. We will clearly call out this dependency in
> >> the cover letter of the PCS driver. Hope that this approach is ok.
> > 
> > Since there is a compile time dependency, you might want to ask for
> > the clock patches to be put into a stable branch which can be merged
> > into netdev.
> > 
> 
> Sure. We will request for such a stable branch merge once the NSS CC
> patches are accepted by the reviewers. Could the 'net' tree be one such
> stable branch option to merge the NSS CC driver?

Given Bjorn reply, maybe you should explain in detail why you have a
build dependency.

A stable branch has some overhead. We should not create it just to
find out you have your architecture wrong when we start reviewing the
code, and it is not actually needed when you fix your architecture.

So, details please.

	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Krzysztof Kozlowski]

On 03/10/2024 20:42, Andrew Lunn wrote:
>> Agree that switchdev is the right model for this device. We were
>> planning to enable base Ethernet functionality using regular
>> (non-switchdev) netdevice representation for the ports initially,
>> without offload support. As the next step, L2/VLAN offload support using
>> switchdev will be enabled on top. Hope this phased approach is fine.
> 
> Since it is not a DSA switch, yes, a phased approach should be O.K.
> 
>>>> 3) PCS driver patch series:
>>>>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
>>>>         be enabled in drivers/net/pcs/
>>>> 	Dependent on NSS CC patch series (2).
>>>
>>> I assume this dependency is pure at runtime? So the code will build
>>> without the NSS CC patch series?
>>
>> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
>> PCS's MII channels. To represent this in the DTS, the PCS node in the
>> DTS is configured with the MII Rx/Tx clock that it consumes, using
>> macros for clocks which are exported from the NSS CC driver in a header

Huh? How is this anyhow related to the point discussed here? That's DTS.
You *CANNOT* send DTS to net-next/net.

>> file. So, there will be a compile-time dependency for the dtbindings/DTS
>> on the NSS CC patch series. We will clearly call out this dependency in
>> the cover letter of the PCS driver. Hope that this approach is ok.
> 
> Since there is a compile time dependency, you might want to ask for
> the clock patches to be put into a stable branch which can be merged
> into netdev.
> 
> Or you need to wait a kernel cycle.

Sorry guys, but this is not accurate. There is no such dependency, but
what's more: there cannot be such dependency.

If there is such dependency and above cross-tree merging is needed, then
it is a clear NAK from me, because patchset is utterly broken.

Upstreaming SoCs have clear rules, already documented in the kernel, in
various emails and in Qualcomm internal guideline (I think, if it is not
in your internal guideline, then please update because we keep repeating
it once per month to you guys).

Best regards,
Krzysztof

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Krzysztof Kozlowski]

On 04/10/2024 15:06, Kiran Kumar C.S.K wrote:
>>> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
>>> PCS's MII channels. To represent this in the DTS, the PCS node in the
>>> DTS is configured with the MII Rx/Tx clock that it consumes, using
>>> macros for clocks which are exported from the NSS CC driver in a header
>>> file. So, there will be a compile-time dependency for the dtbindings/DTS
>>> on the NSS CC patch series. We will clearly call out this dependency in
>>> the cover letter of the PCS driver. Hope that this approach is ok.
>>
>> Since there is a compile time dependency, you might want to ask for
>> the clock patches to be put into a stable branch which can be merged
>> into netdev.
>>
> 
> Sure. We will request for such a stable branch merge once the NSS CC
> patches are accepted by the reviewers. Could the 'net' tree be one such
> stable branch option to merge the NSS CC driver?

NAK.

This is not needed and cannot happen.

Best regards,
Krzysztof

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/4/2024 12:50 AM, Bjorn Andersson wrote:
> On Thu, Oct 03, 2024 at 11:20:03PM +0530, Kiran Kumar C.S.K wrote:
>> On 10/3/2024 2:58 AM, Andrew Lunn wrote:
>>> On Thu, Oct 03, 2024 at 02:07:10AM +0530, Kiran Kumar C.S.K wrote:
> [..]
>>>> 2. List of patch series and dependencies
>>>> ========================================
>>>>
>>>> Clock drivers (currently in review)
>>>> ===================================
>>>> 1) CMN PLL driver patch series:
>>>> 	Currently in review with community.
>>>> 	https://lore.kernel.org/linux-arm-msm/20240827-qcom_ipq_cmnpll-v3-0-8e009cece8b2@quicinc.com/
>>>>
>>>>
>>>> 2) NSS clock controller (NSSCC) driver patch series
>>>> 	Currently in review with community.
>>>> 	https://lore.kernel.org/linux-arm-msm/20240626143302.810632-1-quic_devipriy@quicinc.com/
>>>>
>>>>
>>>> Networking drivers (to be posted for review next week)
>>>> ======================================================
>>>>
>>>> The following patch series are planned to be pushed for the PPE and PCS
>>>> drivers, to support ethernet function. These patch series are listed
>>>> below in dependency order.
>>>>
>>>> 3) PCS driver patch series:
>>>>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
>>>>         be enabled in drivers/net/pcs/
>>>> 	Dependent on NSS CC patch series (2).
>>>
>>> I assume this dependency is pure at runtime? So the code will build
>>> without the NSS CC patch series?
>>
>> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
>> PCS's MII channels. To represent this in the DTS, the PCS node in the
>> DTS is configured with the MII Rx/Tx clock that it consumes, using
>> macros for clocks which are exported from the NSS CC driver in a header
>> file. So, there will be a compile-time dependency for the dtbindings/DTS
>> on the NSS CC patch series. We will clearly call out this dependency in
>> the cover letter of the PCS driver. Hope that this approach is ok.
>>
> 
> So you're not going to expose these clocks through the common clock
> framework and use standard DeviceTree properties for consuming the
> clocks?

The MII Rx/Tx clocks are being registered by the NSS CC driver using the
common clock framework, and the PCS driver consumes these clocks using
the 'clocks'/'clocks-names' devicetree property defined in the PCS MII
instance's DTS node.

Below is the link to the latest version of the NSS CC driver currently
under review for reference. These clocks can be referred using
'NSS_CC_UNIPHY_PORTx_RX_CLK' and 'NSS_CC_UNIPHY_PORTx_TX_CLK' for each
of the six ports (x == 1...6).

https://lore.kernel.org/linux-arm-msm/20241004080332.853503-6-quic_mmanikan@quicinc.com/


> 
> I expect the bindings for these things to go through respective tree
> (clock and netdev) and then the DeviceTree source (dts) addition to go
> through the qcom tree.
> 
Yes, the respective bindings will be included along with the drivers
posted to netdev and clock.

> The only remaining dependency I was expecting is the qcom tree depending
> on the clock and netdev trees to have picked up the bindings, and for
> new bindings I do accept dts changes in the same cycle (I validate dts
> against bindings in linux-next).
> 

The only compile-time dependency from PCS driver to NSS CC driver is
with the example section in PCS driver's dtbindings file. The PCS DTS
node example definitions include a header file exported by the NSS CC
driver, to access certain macros for referring to the MII Rx/Tx clocks.
So, although there is no dependency in the driver code, a successful
dtbindings check will require the NSS CC driver to be available. Could
you suggest how such dependencies can be worked around? Would it be
acceptable to defer enabling the example node for dtbindings compilation
using its 'status' property, until the NSS CC driver is merged?

> Regards,
> Bjorn
> 
>>>
>>> This should be a good way to start, PCS drivers are typically nice and
>>> simple.
>>>
>>
>> Sure, thanks for the inputs.
>>
>>> 	Andrew
>>
>>

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Russell King (Oracle)]

On Thu, Oct 03, 2024 at 11:20:03PM +0530, Kiran Kumar C.S.K wrote:
> >>          +---------+
> >>          |  48MHZ  |
> >>          +----+----+
> >>               |(clock)
> >>               v
> >>          +----+----+
> >>   +------| CMN PLL |
> >>   |      +----+----+
> >>   |           |(clock)
> >>   |           v
> >>   |      +----+----+           +----+----+  clock   +----+----+
> >>   |  +---|  NSSCC  |           |   GCC   |--------->|   MDIO  |
> >>   |  |   +----+----+           +----+----+          +----+----+
> >>   |  |        |(clock & reset)      |(clock & reset)
> >>   |  |        v                     v
> >>   |  |   +-----------------------------+----------+----------+---------+
> >>   |  |   |       +-----+               |EDMA FIFO |          | EIP FIFO|
> >>   |  |   |       | SCH |               +----------+          +---------+
> >>   |  |   |       +-----+                      |               |        |
> >>   |  |   |  +------+   +------+            +-------------------+       |
> >>   |  |   |  |  BM  |   |  QM  |            | L2/L3 Switch Core |       |
> >>   |  |   |  +------+   +------+            +-------------------+       |
> >>   |  |   |                                   |                         |
> >>   |  |   | +-------+ +-------+ +-------+ +-------+ +-------+ +-------+ |
> >>   |  |   | |  MAC0 | |  MAC1 | |  MAC2 | |  MAC3 | | XGMAC4| |XGMAC5 | |
> >>   |  |   | +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ |
> >>   |  |   |     |         |         |         |         |         |     |
> >>   |  |   +-----+---------+---------+---------+---------+---------+-----+
> >>   |  |         |         |         |         |         |         |
> >>   |  |     +---+---------+---------+---------+---+ +---+---+ +---+---+
> >>   +--+---->|             PCS0                    | |  PCS1 | | PCS2  |
> >>   | clock  +---+---------+---------+---------+---+ +---+---+ +---+---+
> >>   |            |         |         |         |         |         |
> >>   |        +---+---------+---------+---------+---+ +---+---+ +---+---+
> >>   | clock  +----------------+                    | |       | |       |
> >>   +------->|Clock Controller|   4-port Eth PHY   | | PHY4  | | PHY5  |
> >>            +----------------+--------------------+ +-------+ +-------+
...
> >> 3) PCS driver patch series:
> >>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
> >>         be enabled in drivers/net/pcs/
> >> 	Dependent on NSS CC patch series (2).
> > 
> > I assume this dependency is pure at runtime? So the code will build
> > without the NSS CC patch series?
> 
> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
> PCS's MII channels. To represent this in the DTS, the PCS node in the
> DTS is configured with the MII Rx/Tx clock that it consumes, using
> macros for clocks which are exported from the NSS CC driver in a header
> file. So, there will be a compile-time dependency for the dtbindings/DTS
> on the NSS CC patch series. We will clearly call out this dependency in
> the cover letter of the PCS driver. Hope that this approach is ok.

Please distinguish between the clocks that are part of the connection
between the PCS and PHY and additional clocks.

For example, RGMII has its own clocks that are part of the RGMII
interface. Despite DT having a way to describe clocks, these clocks
are fundamental to the RGMII interface and are outside of the scope
of DT to describe. Their description is implicit in the relationship
between the PHY and network driver.

Also, the PCS itself is a subset of the network driver, and we do
not (as far as I know) ever describe any kind of connection between
a PCS and PHY. That would be madness when we have situations where
the PHY can change its serdes mode, causing the MAC to switch
between several PCS - which PCS would one associate the PHY with in
DT when the "mux" is embedded in the ethernet driver and may be
effectively transparent?

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

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Andrew Lunn]

> The only compile-time dependency from PCS driver to NSS CC driver is
> with the example section in PCS driver's dtbindings file. The PCS DTS
> node example definitions include a header file exported by the NSS CC
> driver, to access certain macros for referring to the MII Rx/Tx clocks.

> So, although there is no dependency in the driver code, a successful
> dtbindings check will require the NSS CC driver to be available.

You are doing something wrong. A clock is just a phandle. The
dtbindings check does not care where the phandle points to, just that
it looks like a phandle. You can hard code the instance to 42 and all
is good.

And this is all just basic getting SoC stuff merged, nothing
special. So why do you not know this? Have you not been subscribed to
arm-soc for the last six months and watched other drivers get merged?
I also really hope you have been on the netdev list for the last few
months and have watched other pcs and ethernet drivers get merged.

	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Bjorn Andersson]

On Fri, Oct 04, 2024 at 07:47:15PM GMT, Kiran Kumar C.S.K wrote:
> 
> 
> On 10/4/2024 12:50 AM, Bjorn Andersson wrote:
> > On Thu, Oct 03, 2024 at 11:20:03PM +0530, Kiran Kumar C.S.K wrote:
> >> On 10/3/2024 2:58 AM, Andrew Lunn wrote:
> >>> On Thu, Oct 03, 2024 at 02:07:10AM +0530, Kiran Kumar C.S.K wrote:
[..]
> > The only remaining dependency I was expecting is the qcom tree depending
> > on the clock and netdev trees to have picked up the bindings, and for
> > new bindings I do accept dts changes in the same cycle (I validate dts
> > against bindings in linux-next).
> > 
> 
> The only compile-time dependency from PCS driver to NSS CC driver is
> with the example section in PCS driver's dtbindings file. The PCS DTS
> node example definitions include a header file exported by the NSS CC
> driver, to access certain macros for referring to the MII Rx/Tx clocks.
> So, although there is no dependency in the driver code, a successful
> dtbindings check will require the NSS CC driver to be available. Could
> you suggest how such dependencies can be worked around? Would it be
> acceptable to defer enabling the example node for dtbindings compilation
> using its 'status' property, until the NSS CC driver is merged?
> 

You can avoid this dependency by making the example...an example.

By using just descriptive phandles you can present an example of the
client device without creating a dependency on the specific provider.

Regards,
Bjorn

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/4/2024 8:20 PM, Andrew Lunn wrote:
>> The only compile-time dependency from PCS driver to NSS CC driver is
>> with the example section in PCS driver's dtbindings file. The PCS DTS
>> node example definitions include a header file exported by the NSS CC
>> driver, to access certain macros for referring to the MII Rx/Tx clocks.
> 
>> So, although there is no dependency in the driver code, a successful
>> dtbindings check will require the NSS CC driver to be available.
> 
> You are doing something wrong. A clock is just a phandle. The
> dtbindings check does not care where the phandle points to, just that
> it looks like a phandle. You can hard code the instance to 42 and all
> is good.
> 

Understand, we will specify it as a phandle and get rid of this dependency.

> And this is all just basic getting SoC stuff merged, nothing
> special. So why do you not know this? Have you not been subscribed to
> arm-soc for the last six months and watched other drivers get merged?
> I also really hope you have been on the netdev list for the last few
> months and have watched other pcs and ethernet drivers get merged.
>

We will pay closer attention to ongoing reviews going forward to avoid
these gaps. Thank you for all the inputs in this thread.

> 	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/6/2024 12:00 AM, Bjorn Andersson wrote:
> On Fri, Oct 04, 2024 at 07:47:15PM GMT, Kiran Kumar C.S.K wrote:
>>
>>
>> On 10/4/2024 12:50 AM, Bjorn Andersson wrote:
>>> On Thu, Oct 03, 2024 at 11:20:03PM +0530, Kiran Kumar C.S.K wrote:
>>>> On 10/3/2024 2:58 AM, Andrew Lunn wrote:
>>>>> On Thu, Oct 03, 2024 at 02:07:10AM +0530, Kiran Kumar C.S.K wrote:
> [..]
>>> The only remaining dependency I was expecting is the qcom tree depending
>>> on the clock and netdev trees to have picked up the bindings, and for
>>> new bindings I do accept dts changes in the same cycle (I validate dts
>>> against bindings in linux-next).
>>>
>>
>> The only compile-time dependency from PCS driver to NSS CC driver is
>> with the example section in PCS driver's dtbindings file. The PCS DTS
>> node example definitions include a header file exported by the NSS CC
>> driver, to access certain macros for referring to the MII Rx/Tx clocks.
>> So, although there is no dependency in the driver code, a successful
>> dtbindings check will require the NSS CC driver to be available. Could
>> you suggest how such dependencies can be worked around? Would it be
>> acceptable to defer enabling the example node for dtbindings compilation
>> using its 'status' property, until the NSS CC driver is merged?
>>
> 
> You can avoid this dependency by making the example...an example.
> 
> By using just descriptive phandles you can present an example of the
> client device without creating a dependency on the specific provider.
> 

Sure, will represent this as a descriptive phandle and nothing more. Thanks.

> Regards,
> Bjorn

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/4/2024 7:54 PM, Russell King (Oracle) wrote:
> On Thu, Oct 03, 2024 at 11:20:03PM +0530, Kiran Kumar C.S.K wrote:
>>>>          +---------+
>>>>          |  48MHZ  |
>>>>          +----+----+
>>>>               |(clock)
>>>>               v
>>>>          +----+----+
>>>>   +------| CMN PLL |
>>>>   |      +----+----+
>>>>   |           |(clock)
>>>>   |           v
>>>>   |      +----+----+           +----+----+  clock   +----+----+
>>>>   |  +---|  NSSCC  |           |   GCC   |--------->|   MDIO  |
>>>>   |  |   +----+----+           +----+----+          +----+----+
>>>>   |  |        |(clock & reset)      |(clock & reset)
>>>>   |  |        v                     v
>>>>   |  |   +-----------------------------+----------+----------+---------+
>>>>   |  |   |       +-----+               |EDMA FIFO |          | EIP FIFO|
>>>>   |  |   |       | SCH |               +----------+          +---------+
>>>>   |  |   |       +-----+                      |               |        |
>>>>   |  |   |  +------+   +------+            +-------------------+       |
>>>>   |  |   |  |  BM  |   |  QM  |            | L2/L3 Switch Core |       |
>>>>   |  |   |  +------+   +------+            +-------------------+       |
>>>>   |  |   |                                   |                         |
>>>>   |  |   | +-------+ +-------+ +-------+ +-------+ +-------+ +-------+ |
>>>>   |  |   | |  MAC0 | |  MAC1 | |  MAC2 | |  MAC3 | | XGMAC4| |XGMAC5 | |
>>>>   |  |   | +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ +---+---+ |
>>>>   |  |   |     |         |         |         |         |         |     |
>>>>   |  |   +-----+---------+---------+---------+---------+---------+-----+
>>>>   |  |         |         |         |         |         |         |
>>>>   |  |     +---+---------+---------+---------+---+ +---+---+ +---+---+
>>>>   +--+---->|             PCS0                    | |  PCS1 | | PCS2  |
>>>>   | clock  +---+---------+---------+---------+---+ +---+---+ +---+---+
>>>>   |            |         |         |         |         |         |
>>>>   |        +---+---------+---------+---------+---+ +---+---+ +---+---+
>>>>   | clock  +----------------+                    | |       | |       |
>>>>   +------->|Clock Controller|   4-port Eth PHY   | | PHY4  | | PHY5  |
>>>>            +----------------+--------------------+ +-------+ +-------+
> ...
>>>> 3) PCS driver patch series:
>>>>         Driver for the PCS block in IPQ9574. New IPQ PCS driver will
>>>>         be enabled in drivers/net/pcs/
>>>> 	Dependent on NSS CC patch series (2).
>>>
>>> I assume this dependency is pure at runtime? So the code will build
>>> without the NSS CC patch series?
>>
>> The MII Rx/Tx clocks are supplied from the NSS clock controller to the
>> PCS's MII channels. To represent this in the DTS, the PCS node in the
>> DTS is configured with the MII Rx/Tx clock that it consumes, using
>> macros for clocks which are exported from the NSS CC driver in a header
>> file. So, there will be a compile-time dependency for the dtbindings/DTS
>> on the NSS CC patch series. We will clearly call out this dependency in
>> the cover letter of the PCS driver. Hope that this approach is ok.
> 
> Please distinguish between the clocks that are part of the connection
> between the PCS and PHY and additional clocks.
> 
> For example, RGMII has its own clocks that are part of the RGMII
> interface. Despite DT having a way to describe clocks, these clocks
> are fundamental to the RGMII interface and are outside of the scope
> of DT to describe. Their description is implicit in the relationship
> between the PHY and network driver.
> 
> Also, the PCS itself is a subset of the network driver, and we do
> not (as far as I know) ever describe any kind of connection between
> a PCS and PHY. That would be madness when we have situations where
> the PHY can change its serdes mode, causing the MAC to switch
> between several PCS - which PCS would one associate the PHY with in
> DT when the "mux" is embedded in the ethernet driver and may be
> effectively transparent?
> 

Apologies for the delay in response. I understand that the PCS<->PHY
clocks may be out of the scope of PCS DT due to the reasons you mention.
However would like to clarify that the MII clocks referred to here, are
part of the connection between the MAC and PCS and not between PCS and PHY.

Below is a diagram that shows the sub-blocks inside the 'UNIPHY' block
of IPQ9574 which houses the PCS and the serdes, along with the clock
connectivity. The MII Rx/Tx clocks are supplied from the NSS CC, to the
GMII channels between PCS and MAC. So, it seemed appropriate to have
these clocks described as part of the PCS DT node.

              +-------+ +---------+  +-------------------------+
   -----------|CMN PLL| |  GCC    |  |   NSSCC (Divider)       |
   |25/50mhz  +----+--+ +----+----+  +--+-------+--------------+
   |clk            |         |          ^       |
   |       31.25M  |  SYS/AHB|clk  RX/TX|clk    +------------+
   |       ref clk |         |          |       |            |
   |               |         v          | MII RX|TX clk   MAC| RX/TX clk
   |            +--+---------+----------+-------+---+      +-+---------+
   |            |  |   +----------------+       |   |      | |     PPE |
   v            |  |   |     UNIPHY0            V   |      | V         |
  +-------+     |  v   |       +-----------+ (X)GMII|      |           |
  |       |     |  +---+---+   |           |--------|------|-- MAC0    |
  |       |     |  |       |   |           | (X)GMII|      |           |
  |  Quad |     |  |SerDes |   |  (X)PCS   |--------|------|-- MAC1    |
  |       +<----+  |       |   |           | (X)GMII|      |           |
  |(X)GPHY|     |  |       |   |           |--------|------|-- MAC2    |
  |       |     |  |       |   |           | (X)GMII|      |           |
  |       |     |  +-------+   |           |--------|------|-- MAC3    |
  +-------+     |              |           |        |      |           |
                |              +-----------+        |      |           |
                +-----------------------------------+      |           |
                +--+---------+----------+-------+---+      |           |
  +-------+     |            UNIPHY1                |      |           |
  |       |     |              +-----------+        |      |           |
  |(X)GPHY|     | +-------+    |           | (X)GMII|      |           |
  |       +<----+ |SerDes |    |   (X)PCS  |--------|------|- MAC4     |
  |       |     | |       |    |           |        |      |           |
  +-------+     | +-------+    |           |        |      |           |
                |              +-----------+        |      |           |
                +-----------------------------------+      |           |
                +--+--------+-----------+-------+---+      |           |
  +-------+     |           UNIPHY2                 |      |           |
  |       |     |              +-----------+        |      |           |
  |(X)GPHY|     | +-------+    |           | (X)GMII|      |           |
  |       +<----+ |SerDes |    |   (X)PCS  |--------|------|- MAC5     |
  |       |     | |       |    |           |        |      |           |
  +-------+     | +-------+    |           |        |      |           |
                |              +-----------+        |      |           |
                +-----------------------------------+      +-----------+


We had one other question on the approach used in the driver for PCS
clocks, could you please provide your comments.

As we can see from the above diagram, each serdes in the UNIPHY block
provides the clocks to the NSSCC, and the PCS block consumes the MII
Rx/Tx clocks. In our current design, the PCS/UNIPHY driver registers a
provider driver for the clocks that the serdes supplies to the NSS CC.
It also enables the MII Rx/Tx clocks which are supplied to the PCS from
the NSS CC. Would this be an acceptable design to have the PCS driver
register the clock provider driver and also consume the MII Rx/Tx
clocks? It may be worth noting that the serdes and PCS are part of the
same UNIPHY block and also share same register region.

Thank you.

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Andrew Lunn]

> Apologies for the delay in response. I understand that the PCS<->PHY
> clocks may be out of the scope of PCS DT due to the reasons you mention.
> However would like to clarify that the MII clocks referred to here, are
> part of the connection between the MAC and PCS and not between PCS and PHY.
> 
> Below is a diagram that shows the sub-blocks inside the 'UNIPHY' block
> of IPQ9574 which houses the PCS and the serdes, along with the clock
> connectivity. The MII Rx/Tx clocks are supplied from the NSS CC, to the
> GMII channels between PCS and MAC. So, it seemed appropriate to have
> these clocks described as part of the PCS DT node.
> 
>               +-------+ +---------+  +-------------------------+
>    -----------|CMN PLL| |  GCC    |  |   NSSCC (Divider)       |
>    |25/50mhz  +----+--+ +----+----+  +--+-------+--------------+
>    |clk            |         |          ^       |
>    |       31.25M  |  SYS/AHB|clk  RX/TX|clk    +------------+
>    |       ref clk |         |          |       |            |
>    |               |         v          | MII RX|TX clk   MAC| RX/TX clk
>    |            +--+---------+----------+-------+---+      +-+---------+
>    |            |  |   +----------------+       |   |      | |     PPE |
>    v            |  |   |     UNIPHY0            V   |      | V         |
>   +-------+     |  v   |       +-----------+ (X)GMII|      |           |
>   |       |     |  +---+---+   |           |--------|------|-- MAC0    |
>   |       |     |  |       |   |           | (X)GMII|      |           |
>   |  Quad |     |  |SerDes |   |  (X)PCS   |--------|------|-- MAC1    |
>   |       +<----+  |       |   |           | (X)GMII|      |           |
>   |(X)GPHY|     |  |       |   |           |--------|------|-- MAC2    |
>   |       |     |  |       |   |           | (X)GMII|      |           |
>   |       |     |  +-------+   |           |--------|------|-- MAC3    |
>   +-------+     |              |           |        |      |           |
>                 |              +-----------+        |      |           |
>                 +-----------------------------------+      |           |

Thanks for the detailed diagram. That always helps get things
straight.

Im i correct in says that MII RX|TX is identical to MAC RX|TX? These
two clocks are used by the MAC and XPCS to clock data from one to the
other? If they are the exact same clocks, i would suggest you use the
same name, just to avoid confusion.

Both XPCS and PPE are clock consumers, so both will have a phandle
pointing to the NSSCC clock provider?

> We had one other question on the approach used in the driver for PCS
> clocks, could you please provide your comments.
> 
> As we can see from the above diagram, each serdes in the UNIPHY block
> provides the clocks to the NSSCC, and the PCS block consumes the MII
> Rx/Tx clocks. In our current design, the PCS/UNIPHY driver registers a
> provider driver for the clocks that the serdes supplies to the NSS CC.

That sounds reasonable.

> It also enables the MII Rx/Tx clocks which are supplied to the PCS from
> the NSS CC. Would this be an acceptable design to have the PCS driver
> register the clock provider driver and also consume the MII Rx/Tx
> clocks? It may be worth noting that the serdes and PCS are part of the
> same UNIPHY block and also share same register region.

Does the SERDES consume the MII Rx/Tx? Your diagram indicates it does
not. I'm just wondering if you have circular dependencies at runtime?

Where you will need to be careful is probe time vs runtime. Since you
have circular phandles you need to first create all the clock
providers, and only then start the clock consumers. Otherwise you
might get into an endless EPROBE_DEFER loop.

	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/22/2024 7:07 PM, Andrew Lunn wrote:
>> Apologies for the delay in response. I understand that the PCS<->PHY
>> clocks may be out of the scope of PCS DT due to the reasons you mention.
>> However would like to clarify that the MII clocks referred to here, are
>> part of the connection between the MAC and PCS and not between PCS and PHY.
>>
>> Below is a diagram that shows the sub-blocks inside the 'UNIPHY' block
>> of IPQ9574 which houses the PCS and the serdes, along with the clock
>> connectivity. The MII Rx/Tx clocks are supplied from the NSS CC, to the
>> GMII channels between PCS and MAC. So, it seemed appropriate to have
>> these clocks described as part of the PCS DT node.
>>
>>               +-------+ +---------+  +-------------------------+
>>    -----------|CMN PLL| |  GCC    |  |   NSSCC (Divider)       |
>>    |25/50mhz  +----+--+ +----+----+  +--+-------+--------------+
>>    |clk            |         |          ^       |
>>    |       31.25M  |  SYS/AHB|clk  RX/TX|clk    +------------+
>>    |       ref clk |         |          |       |            |
>>    |               |         v          | MII RX|TX clk   MAC| RX/TX clk
>>    |            +--+---------+----------+-------+---+      +-+---------+
>>    |            |  |   +----------------+       |   |      | |     PPE |
>>    v            |  |   |     UNIPHY0            V   |      | V         |
>>   +-------+     |  v   |       +-----------+ (X)GMII|      |           |
>>   |       |     |  +---+---+   |           |--------|------|-- MAC0    |
>>   |       |     |  |       |   |           | (X)GMII|      |           |
>>   |  Quad |     |  |SerDes |   |  (X)PCS   |--------|------|-- MAC1    |
>>   |       +<----+  |       |   |           | (X)GMII|      |           |
>>   |(X)GPHY|     |  |       |   |           |--------|------|-- MAC2    |
>>   |       |     |  |       |   |           | (X)GMII|      |           |
>>   |       |     |  +-------+   |           |--------|------|-- MAC3    |
>>   +-------+     |              |           |        |      |           |
>>                 |              +-----------+        |      |           |
>>                 +-----------------------------------+      |           |
> 
> Thanks for the detailed diagram. That always helps get things
> straight.
> 
> Im i correct in says that MII RX|TX is identical to MAC RX|TX? These
> two clocks are used by the MAC and XPCS to clock data from one to the
> other? If they are the exact same clocks, i would suggest you use the
> same name, just to avoid confusion.
> 

Yes, these two clocks are used by MAC and PCS to clock data to one
another. The MAC Rx/Tx clocks and MII Rx/Tx clocks are different clocks
and can be enabled/disabled independently. However their parent clock is
the same and hence their rate is same at all times. For example, the
phylink ops will set the rate of MAC Rx/Tx during a link speed change,
and this same rate is effected for the MII Rx/Tx clock.

Sure for the purpose of rest of this discussion, we can refer to them as
MII Rx/Tx clocks.

I would also like to clarify that each of the '(X)PCS' blocks shown in
the diagram, includes two PCS types, a 'PCS' type that supports 1Gbps
PCS modes, and another 'XPCS' block (Synopsys) that supports 10Gbps PCS
modes. The MII Rx/Tx clocks from the NSS CC, are supplied to the xGMII
channels of both these PCS.

> Both XPCS and PPE are clock consumers, so both will have a phandle
> pointing to the NSSCC clock provider?
> 

Yes, this is correct.

>> We had one other question on the approach used in the driver for PCS
>> clocks, could you please provide your comments.
>>
>> As we can see from the above diagram, each serdes in the UNIPHY block
>> provides the clocks to the NSSCC, and the PCS block consumes the MII
>> Rx/Tx clocks. In our current design, the PCS/UNIPHY driver registers a
>> provider driver for the clocks that the serdes supplies to the NSS CC.
> 
> That sounds reasonable>
>> It also enables the MII Rx/Tx clocks which are supplied to the PCS from
>> the NSS CC. Would this be an acceptable design to have the PCS driver
>> register the clock provider driver and also consume the MII Rx/Tx
>> clocks? It may be worth noting that the serdes and PCS are part of the
>> same UNIPHY block and also share same register region.
> 
> Does the SERDES consume the MII Rx/Tx? Your diagram indicates it does
> not. 

The SERDES in the UNIPHY does not consume the MII Rx/Tx clocks. The
consumer of these clocks is the PCS block within the UNIPHY.

The SERDES only provides the source Rx/tx clocks to the NSS CC, and the
NSS CC divides this clock and supplies the per-channel MII Rx/Tx clocks
to the PCS.

> I'm just wondering if you have circular dependencies at runtime?
> 
> Where you will need to be careful is probe time vs runtime. Since you
> have circular phandles you need to first create all the clock
> providers, and only then start the clock consumers. Otherwise you
> might get into an endless EPROBE_DEFER loop.
> 

The Rx/Tx clocks sourced from the SERDES are registered as provider
clocks by the UNIPHY/PCS driver during probe time. There is no runtime
operation needed for these clocks after this.

The MII Rx/Tx clocks are enabled later when the MAC is initialized by
the network driver. The network driver calls an API exported by the PCS
driver, which creates the PCS instance and enables the MII Rx/Tx clocks.
Ideally, the PCS driver probe should have completed and provider clocks
registered, by the time the MAC is initialized (we handle the error case
in case the probe is still not complete).

> 	Andrew
> 

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Andrew Lunn]

> > I'm just wondering if you have circular dependencies at runtime?
> > 
> > Where you will need to be careful is probe time vs runtime. Since you
> > have circular phandles you need to first create all the clock
> > providers, and only then start the clock consumers. Otherwise you
> > might get into an endless EPROBE_DEFER loop.
> > 
> 
> The Rx/Tx clocks sourced from the SERDES are registered as provider
> clocks by the UNIPHY/PCS driver during probe time. There is no runtime
> operation needed for these clocks after this.

So they are always ticking. You cannot turn them on/off? It is nice to
model them a fixed-clocks, since it describes the architecture, but i
have to question if it is worth the effort.

	Andrew

Re: RFC: Advice on adding support for Qualcomm IPQ9574 SoC Ethernet

[Kiran Kumar C.S.K]

On 10/24/2024 7:57 PM, Andrew Lunn wrote:
>>> I'm just wondering if you have circular dependencies at runtime?
>>>
>>> Where you will need to be careful is probe time vs runtime. Since you
>>> have circular phandles you need to first create all the clock
>>> providers, and only then start the clock consumers. Otherwise you
>>> might get into an endless EPROBE_DEFER loop.
>>>
>>
>> The Rx/Tx clocks sourced from the SERDES are registered as provider
>> clocks by the UNIPHY/PCS driver during probe time. There is no runtime
>> operation needed for these clocks after this.
> 
> So they are always ticking. You cannot turn them on/off? It is nice to
> model them a fixed-clocks, since it describes the architecture, but i
> have to question if it is worth the effort.
> 

Yes, we cannot turn them off. However the rates of these clocks from
SERDES to NSSCC, is not fixed. It will be either 312.5Mhz or 125Mhz,
depending on the whether the SERDES mode is USXGMII or SGMII respectively.

> 	Andrew