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.