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

From: Kiran Kumar C . S . K
Date: Fri Oct 04 2024 - 10:18:13 EST




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@xxxxxxxxxxx/
>>>>
>>>>
>>>> 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@xxxxxxxxxxx/
>>>>
>>>>
>>>> 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@xxxxxxxxxxx/


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