[v4,0/5] support ipq5332 platform

  For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
connected with one of them.

1. The Ethernet LDO needs to be enabled to make the PHY GPIO
   reset taking effect, which uses the MDIO bus level reset.

2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
   to make the ethernet PHY device accessible.

3. To provide the clock to the ethernet, the CMN clock needs
   to be initialized for selecting reference clock and enabling
   the output clock.

4. Support optional MDIO clock frequency config.

5. Update dt-bindings doc for the new added properties.

Changes in v2:
	* remove the PHY related features such as PHY address
	  program and clock initialization.
	* leverage the MDIO level GPIO reset for qca8084 PHY.

Changes in v3:
	* fix the christmas-tree format issue.
	* improve the dt-binding changes.

Changes in v4:
	* improve the CMN PLL reference clock config.
	* improve the dt-binding changes.

Luo Jie (5):
  net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
  net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
  net: mdio: ipq4019: configure CMN PLL clock for ipq5332
  net: mdio: ipq4019: support MDIO clock frequency divider
  dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

 .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
 drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
 2 files changed, 399 insertions(+), 30 deletions(-)


base-commit: 3b83fa94cf316aaf9ad9a367ac8031a06d31649b
  

Hi Luo,

I have a few questions regarding the high level design of 
implementation. I hope that clarifying these topics will help us to find 
a good model for the case and finally merge a supporting code. Please 
find the questions below.

On 25.12.2023 10:44, Luo Jie wrote:
> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
> connected with one of them.
> 
> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>     reset taking effect, which uses the MDIO bus level reset.
> 
> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>     to make the ethernet PHY device accessible.
> 
> 3. To provide the clock to the ethernet, the CMN clock needs
>     to be initialized for selecting reference clock and enabling
>     the output clock.
> 
> 4. Support optional MDIO clock frequency config.
> 
> 5. Update dt-bindings doc for the new added properties.
> 
> Changes in v2:
> 	* remove the PHY related features such as PHY address
> 	  program and clock initialization.
> 	* leverage the MDIO level GPIO reset for qca8084 PHY.
> 
> Changes in v3:
> 	* fix the christmas-tree format issue.
> 	* improve the dt-binding changes.
> 
> Changes in v4:
> 	* improve the CMN PLL reference clock config.
> 	* improve the dt-binding changes.
> 
> Luo Jie (5):
>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>    net: mdio: ipq4019: support MDIO clock frequency divider
>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
> 
>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>   drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
>   2 files changed, 399 insertions(+), 30 deletions(-)

I'm asking these questions because after checking the patches and 
following the earlier discussion, the series is looks like an 
overloading of the MDIO driver with somehow but not strictly related 
functionality.


First, let me summarize the case. Feel free to correct me if I took 
something wrong. So, we have:
- a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
- QCA8084 requires a reference clock for normal functionality;
- IPQ5332, as a chip, is able to provide a set of reference clocks for 
external devices;
- you want to configure IPQ5332 to provide the reference clock for QCA8084.


So, the high level questions are:
1. Is QCA8084 capable to consume the clock from some other generator? Is 
it possible to clock QCA8084 from external XO/PLL/whatever?
2. Is IPQ5332 capable to provide reference clock to another switch model?
3. Is the reference clock generation subsystem part of the MDIO block of 
IPQ5332?


And there are some tiny questions to make sure that we are on the same page:
a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of gate 
(or switch) that enables clock output through an IPQ5332 pin. Isn't it? 
And if it's true, then can you clarify, what exactly clock is outputted?
b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According to 
iomem addresses that was used in the example reg property, the Ethernet 
LDO is not part of MDIO.
c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according to 
iomem address, the CMN PLL is not part of MDIO.
d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are they 
need for the external reference clock generation?


Please answer these questions one by one and we will have a good basis 
to move forward.



To speed up the discussion, let me share my user's view of the reference 
clocks modeling. I would like to join the option that has already been 
suggested by the maintainers. It is better to implement reference clocks 
handling using the clocks API, and the clock subsystem will take care of 
enabling and configuring them.

And considering the expected answers to the above questions, I would 
like to suggest to implement the clock handling using a dedicated clock 
controlling driver. Or even using several of such tiny dedicated 
drivers. So DTS will become like this:

   ext_ref_clock: ext_ref_clock {
     compatible = "fixed-clock";
     clock-frequency = <48000000>;
   };

   eth_cmn_pll: clock-controller@9b000 {
     compatible = "qcom,eth-cmn-pll-ipq5223";
     reg = <0x9b000 0x800>;
     clocks = <&ext_ref_clock>; /* use external 48MHz clock */
   };

   phy0_ext_clk: clock-controller@7a00610 {
     compatible = "qcom,ipq-eth-ldo";
     reg = <0x7a00610 0x4>;
     clocks = <&eth_cmn_pll>;
   };

   mdio@90000 {
     compatible = "qcom,ipq4019-mdio";
     reg = <0x90000 0x64>;
     clocks = <&gcc GCC_MDIO_AHB_CLK>;

     ethernet-phy@1 {
       compatible = "...";
       reg = <1>;
       clocks = <&phy0_ext_clk>;
       reset-gpios = <&gcc ...>;
     };
   };

--
Sergey
  

On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
> Hi Luo,
> 
> I have a few questions regarding the high level design of 
> implementation. I hope that clarifying these topics will help us to find 
> a good model for the case and finally merge a supporting code. Please 
> find the questions below.
> 
> On 25.12.2023 10:44, Luo Jie wrote:
>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>> connected with one of them.
>>
>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>     reset taking effect, which uses the MDIO bus level reset.
>>
>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>     to make the ethernet PHY device accessible.
>>
>> 3. To provide the clock to the ethernet, the CMN clock needs
>>     to be initialized for selecting reference clock and enabling
>>     the output clock.
>>
>> 4. Support optional MDIO clock frequency config.
>>
>> 5. Update dt-bindings doc for the new added properties.
>>
>> Changes in v2:
>>     * remove the PHY related features such as PHY address
>>       program and clock initialization.
>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>
>> Changes in v3:
>>     * fix the christmas-tree format issue.
>>     * improve the dt-binding changes.
>>
>> Changes in v4:
>>     * improve the CMN PLL reference clock config.
>>     * improve the dt-binding changes.
>>
>> Luo Jie (5):
>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>
>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>   drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
>>   2 files changed, 399 insertions(+), 30 deletions(-)
> 
> I'm asking these questions because after checking the patches and 
> following the earlier discussion, the series is looks like an 
> overloading of the MDIO driver with somehow but not strictly related 
> functionality.
> 
> 
> First, let me summarize the case. Feel free to correct me if I took 
> something wrong. So, we have:
> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;

IPQ5322 SoC is currently connected with qca8386(switch that includes
QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
SoC IPQ9574.

> - QCA8084 requires a reference clock for normal functionality;

The reference clock is selected for the CMN PLL block, which outputs
the clocks to the Ethernet devices including the qca8084 PHY for normal
functionality, also for other connected Ethernet devices, the CMN PLL
block is located in SoC such as ipq5332 and ipq9574.

> - IPQ5332, as a chip, is able to provide a set of reference clocks for 
> external devices;

Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
working clocks for the external Ethernet devices such as the QCA8386
(switch chip), the reference clocks we are discussing is as the
reference clock source of the CMN PLL block.

> - you want to configure IPQ5332 to provide the reference clock for QCA8084.

The reference clocks for CMN PLL block is configurable, and the output
clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
to the external Ethernet devices.
here is the topology of clocks.
		     ---------
		    |	     |
reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
	            |        |
	            ---------
> 			   	
> 
> So, the high level questions are:
> 1. Is QCA8084 capable to consume the clock from some other generator? Is 
> it possible to clock QCA8084 from external XO/PLL/whatever?
No, the clock of qca8084/qca8386 is provided from the output clock of
CMN PLL as above.

> 2. Is IPQ5332 capable to provide reference clock to another switch model?

Yes, IPQ5332 can provide the reference clock to all connected Ethernet
devices, such as qca8386(switch), qca8081 phy and others.

> 3. Is the reference clock generation subsystem part of the MDIO block of 
> IPQ5332?

the reference clock of CMN PLL block can be from wifi and external xtal, 
the CMN PLL is integrated in the MDIO block, CMN PLL is the independent
block that generates the clocks for the connected Ethernet devices.

> 
> 
> And there are some tiny questions to make sure that we are on the same 
> page:
> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of gate 
> (or switch) that enables clock output through an IPQ5332 pin. Isn't it?

That's correct, the LDO is for enabling the output 50M clock of CMN PLL
to the connected Ethernet device, which is controlled by the hardware
register on the IPQ5332.

> And if it's true, then can you clarify, what exactly clock is outputted?

the 50M clock is outputted to the external Ethernet devices.

> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According to 
> iomem addresses that was used in the example reg property, the Ethernet 
> LDO is not part of MDIO.

LDO is not the part of MDIO block, LDO has the different register space
from MDIO, which is located in the independent Ethernet part.

> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according to 
> iomem address, the CMN PLL is not part of MDIO.

No, CMN PLL is not the part of MDIO block, which is the independent
block, but it generates the clocks to the connected Ethernet devices
managed by MDIO bus, and the CMN PLL block needs to be configured
correctly to generate the clocks to make the MDIO devices(Ethernet
devices) working.

> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are they 
> need for the external reference clock generation?

GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
consumed by the Ethernet devices, which is not for the external
reference clock generation, external reference clock of CMN PLL are the
fix clock that are from wifi or external XO.

> 
> 
> Please answer these questions one by one and we will have a good basis 
> to move forward.
OK.
> 
> 
> 
> To speed up the discussion, let me share my user's view of the reference 
> clocks modeling. I would like to join the option that has already been 
> suggested by the maintainers. It is better to implement reference clocks 
> handling using the clocks API, and the clock subsystem will take care of 
> enabling and configuring them.
> 
> And considering the expected answers to the above questions, I would 
> like to suggest to implement the clock handling using a dedicated clock 
> controlling driver. Or even using several of such tiny dedicated 
> drivers. So DTS will become like this:
> 
>    ext_ref_clock: ext_ref_clock {
>      compatible = "fixed-clock";
>      clock-frequency = <48000000>;
>    };
> 
>    eth_cmn_pll: clock-controller@9b000 {
>      compatible = "qcom,eth-cmn-pll-ipq5223";
>      reg = <0x9b000 0x800>;
>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>    };
> 
>    phy0_ext_clk: clock-controller@7a00610 {
>      compatible = "qcom,ipq-eth-ldo";
>      reg = <0x7a00610 0x4>;
>      clocks = <&eth_cmn_pll>;
>    };
> 
>    mdio@90000 {
>      compatible = "qcom,ipq4019-mdio";
>      reg = <0x90000 0x64>;
>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
> 
>      ethernet-phy@1 {
>        compatible = "...";
>        reg = <1>;
>        clocks = <&phy0_ext_clk>;
>        reset-gpios = <&gcc ...>;
>      };
>    };
> 
> -- 
> Sergey

Thanks Sergey for the reference DTS.
Since the GPIO reset of qca8084/qca8386 is needed before configuring the
Ethernet device.

The configuration of and phy0_ext_clk(LDO) should be configured
firstly, which enables the clocks to the Ethernet devices, then the GPIO
reset of the connected Ethernet devices(such as qca8386) can take
effect, currently the GPIO reset takes the MDIO bus level reset.

So phy0_ext_clk can't be put in the PHY device tree node, one LDO
controls the clock output enabled to the connected Ethernet device such
as qca8386.
  

Hi Luo,

thank you for explaining the case in such details. I also have checked 
the related DTSs in the Linaro repository to be more familiar with the 
I/O mem layout. Specifically I checked these two, hope they are relevant 
to the discussion:
https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi

Please find my comments below.

On 03.01.2024 15:31, Jie Luo wrote:
> On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
>> Hi Luo,
>>
>> I have a few questions regarding the high level design of 
>> implementation. I hope that clarifying these topics will help us to 
>> find a good model for the case and finally merge a supporting code. 
>> Please find the questions below.
>>
>> On 25.12.2023 10:44, Luo Jie wrote:
>>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>>> connected with one of them.
>>>
>>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>>     reset taking effect, which uses the MDIO bus level reset.
>>>
>>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>>     to make the ethernet PHY device accessible.
>>>
>>> 3. To provide the clock to the ethernet, the CMN clock needs
>>>     to be initialized for selecting reference clock and enabling
>>>     the output clock.
>>>
>>> 4. Support optional MDIO clock frequency config.
>>>
>>> 5. Update dt-bindings doc for the new added properties.
>>>
>>> Changes in v2:
>>>     * remove the PHY related features such as PHY address
>>>       program and clock initialization.
>>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>>
>>> Changes in v3:
>>>     * fix the christmas-tree format issue.
>>>     * improve the dt-binding changes.
>>>
>>> Changes in v4:
>>>     * improve the CMN PLL reference clock config.
>>>     * improve the dt-binding changes.
>>>
>>> Luo Jie (5):
>>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 platform
>>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>>
>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>>   drivers/net/mdio/mdio-ipq4019.c               | 288 ++++++++++++++++--
>>>   2 files changed, 399 insertions(+), 30 deletions(-)
>>
>> I'm asking these questions because after checking the patches and 
>> following the earlier discussion, the series is looks like an 
>> overloading of the MDIO driver with somehow but not strictly related 
>> functionality.
>>
>>
>> First, let me summarize the case. Feel free to correct me if I took 
>> something wrong. So, we have:
>> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
> 
> IPQ5322 SoC is currently connected with qca8386(switch that includes
> QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
> SoC IPQ9574.

As far as I understand these chips have standardized interfaces and 
QCA8386/QCA8084 can be reused with another SoC(s) in future. As well as 
IPQ5332 can be used with different phy. So now we are talking about some 
specific reference design. Isn't it?

>> - QCA8084 requires a reference clock for normal functionality;
> 
> The reference clock is selected for the CMN PLL block, which outputs
> the clocks to the Ethernet devices including the qca8084 PHY for normal
> functionality, also for other connected Ethernet devices, the CMN PLL
> block is located in SoC such as ipq5332 and ipq9574.
> 
>> - IPQ5332, as a chip, is able to provide a set of reference clocks for 
>> external devices;
> 
> Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
> working clocks for the external Ethernet devices such as the QCA8386
> (switch chip), the reference clocks we are discussing is as the
> reference clock source of the CMN PLL block.

Ok, I feel we have some ambiguity regarding the reference clock term 
here. Sure, CMN PLL needs a reference clock to functioning. And in the 
same time, the output clock provided by CMN PLL is a reference clock for 
QCA8384.

So, when I was talking about IPQ5332, I meant the whole chip including 
CMN PLL block. So, I asked about CMN PLL output clock. But you already 
clarified the SoC capabilities below.

>> - you want to configure IPQ5332 to provide the reference clock for 
>> QCA8084.
> 
> The reference clocks for CMN PLL block is configurable, and the output
> clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
> to the external Ethernet devices.
> here is the topology of clocks.
>                     ---------
>                     |        |
> reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
>                     |        |
>                     ---------
>>
>>
>> So, the high level questions are:
>> 1. Is QCA8084 capable to consume the clock from some other generator? 
>> Is it possible to clock QCA8084 from external XO/PLL/whatever?
> No, the clock of qca8084/qca8386 is provided from the output clock of
> CMN PLL as above.

Right, in case of pairing QCA8386 with IPQ5332, it is a good option to 
provide the clock from the SoC. But in general QCA8386 will be Ok with 
any 50 MHz clock. Right? I would like to say that thinking about this 
specific reference design being a single possible combination limits a 
scope of driver implementation options.

>> 2. Is IPQ5332 capable to provide reference clock to another switch model?
> 
> Yes, IPQ5332 can provide the reference clock to all connected Ethernet
> devices, such as qca8386(switch), qca8081 phy and others.

Ok. Thank you for clarifying this.

>> 3. Is the reference clock generation subsystem part of the MDIO block 
>> of IPQ5332?
> 
> the reference clock of CMN PLL block can be from wifi and external xtal, 
> the CMN PLL is integrated in the MDIO block, CMN PLL is the independent
> block that generates the clocks for the connected Ethernet devices.
> 
>>
>>
>> And there are some tiny questions to make sure that we are on the same 
>> page:
>> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of gate 
>> (or switch) that enables clock output through an IPQ5332 pin. Isn't it?
> 
> That's correct, the LDO is for enabling the output 50M clock of CMN PLL
> to the connected Ethernet device, which is controlled by the hardware
> register on the IPQ5332.
> 
>> And if it's true, then can you clarify, what exactly clock is outputted?
> 
> the 50M clock is outputted to the external Ethernet devices.
> 
>> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According to 
>> iomem addresses that was used in the example reg property, the 
>> Ethernet LDO is not part of MDIO.
> 
> LDO is not the part of MDIO block, LDO has the different register space
> from MDIO, which is located in the independent Ethernet part.

I have checked the Linaro's DTSs and noticed that mentioned LDO 
addresses belong to a node called 'ess-uniphy'. So these LDO(s) are part 
of UNIPHY block. So far, so good.

>> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according 
>> to iomem address, the CMN PLL is not part of MDIO.
> 
> No, CMN PLL is not the part of MDIO block, which is the independent
> block, but it generates the clocks to the connected Ethernet devices
> managed by MDIO bus, and the CMN PLL block needs to be configured
> correctly to generate the clocks to make the MDIO devices(Ethernet
> devices) working.

I came to the same conclusion checking Linaro's DTS. So the CMN PLL 
block looks like a small block implemented outside of any other block. 
Now I am starting to understand, why everything was putted into the MDIO 
driver. This PLL is so small that it doesn't seem to deserve a dedicated 
driver. Am I got it right?

>> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are 
>> they need for the external reference clock generation?
> 
> GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
> with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
> consumed by the Ethernet devices, which is not for the external
> reference clock generation, external reference clock of CMN PLL are the
> fix clock that are from wifi or external XO.

Again this UNIPHY block. The UNIPHY node was missed from the upstream 
DTS, so it was decided to assign AHB & SYS clocks to MDIO. Right?

What do you think about implementing this clocks handling functionality 
in a dedicated driver (e.g. uniphy) and create a dedicated DTS node for 
it? This driver could consume AHB & SYS clocks as well as consuming CMN 
PLL clock and be a clock provider for the Ethernet PHY (e.g. QCA8336).

And looks like CMN PLL should be implemented as a dedicated micro 
driver. A driver that consumes fixed reference clocks (XO or from WiFi) 
and provides the clock to UNIPHY, to be passed to the Ethernet PHY by 
means of LDO gate.

>> To speed up the discussion, let me share my user's view of the 
>> reference clocks modeling. I would like to join the option that has 
>> already been suggested by the maintainers. It is better to implement 
>> reference clocks handling using the clocks API, and the clock 
>> subsystem will take care of enabling and configuring them.
>>
>> And considering the expected answers to the above questions, I would 
>> like to suggest to implement the clock handling using a dedicated 
>> clock controlling driver. Or even using several of such tiny dedicated 
>> drivers. So DTS will become like this:
>>
>>    ext_ref_clock: ext_ref_clock {
>>      compatible = "fixed-clock";
>>      clock-frequency = <48000000>;
>>    };
>>
>>    eth_cmn_pll: clock-controller@9b000 {
>>      compatible = "qcom,eth-cmn-pll-ipq5223";
>>      reg = <0x9b000 0x800>;
>>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>>    };
>>
>>    phy0_ext_clk: clock-controller@7a00610 {
>>      compatible = "qcom,ipq-eth-ldo";
>>      reg = <0x7a00610 0x4>;
>>      clocks = <&eth_cmn_pll>;
>>    };
>>
>>    mdio@90000 {
>>      compatible = "qcom,ipq4019-mdio";
>>      reg = <0x90000 0x64>;
>>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
>>
>>      ethernet-phy@1 {
>>        compatible = "...";
>>        reg = <1>;
>>        clocks = <&phy0_ext_clk>;
>>        reset-gpios = <&gcc ...>;
>>      };
>>    };
> 
> Thanks Sergey for the reference DTS.
> Since the GPIO reset of qca8084/qca8386 is needed before configuring the
> Ethernet device.
> 
> The configuration of and phy0_ext_clk(LDO) should be configured
> firstly, which enables the clocks to the Ethernet devices, then the GPIO
> reset of the connected Ethernet devices(such as qca8386) can take
> effect, currently the GPIO reset takes the MDIO bus level reset.
> 
> So phy0_ext_clk can't be put in the PHY device tree node, one LDO
> controls the clock output enabled to the connected Ethernet device such
> as qca8386.

I still feel lost. Why it is impossible to specify clocks and resets in 
the PHY node and then implement the initialization sequence in the 
QCA8386 driver? I read the discussion of the QCA8386 driver submission. 
That driver modeling also looks a complex task. But it still puzzling 
me, why a part of the QCA8386 driver should be implemented inside the 
MDIO driver.

--
Sergey
  

On 1/5/2024 10:48 AM, Sergey Ryazanov wrote:
> Hi Luo,
> 
> thank you for explaining the case in such details. I also have checked 
> the related DTSs in the Linaro repository to be more familiar with the 
> I/O mem layout. Specifically I checked these two, hope they are relevant 
> to the discussion:
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsiThanks Sergey for looking into this driver support.
> 
> Please find my comments below.
> 
> On 03.01.2024 15:31, Jie Luo wrote:
>> On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
>>> Hi Luo,
>>>
>>> I have a few questions regarding the high level design of 
>>> implementation. I hope that clarifying these topics will help us to 
>>> find a good model for the case and finally merge a supporting code. 
>>> Please find the questions below.
>>>
>>> On 25.12.2023 10:44, Luo Jie wrote:
>>>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>>>> connected with one of them.
>>>>
>>>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>>>     reset taking effect, which uses the MDIO bus level reset.
>>>>
>>>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>>>     to make the ethernet PHY device accessible.
>>>>
>>>> 3. To provide the clock to the ethernet, the CMN clock needs
>>>>     to be initialized for selecting reference clock and enabling
>>>>     the output clock.
>>>>
>>>> 4. Support optional MDIO clock frequency config.
>>>>
>>>> 5. Update dt-bindings doc for the new added properties.
>>>>
>>>> Changes in v2:
>>>>     * remove the PHY related features such as PHY address
>>>>       program and clock initialization.
>>>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>>>
>>>> Changes in v3:
>>>>     * fix the christmas-tree format issue.
>>>>     * improve the dt-binding changes.
>>>>
>>>> Changes in v4:
>>>>     * improve the CMN PLL reference clock config.
>>>>     * improve the dt-binding changes.
>>>>
>>>> Luo Jie (5):
>>>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 
>>>> platform
>>>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>>>
>>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>>>   drivers/net/mdio/mdio-ipq4019.c               | 288 
>>>> ++++++++++++++++--
>>>>   2 files changed, 399 insertions(+), 30 deletions(-)
>>>
>>> I'm asking these questions because after checking the patches and 
>>> following the earlier discussion, the series is looks like an 
>>> overloading of the MDIO driver with somehow but not strictly related 
>>> functionality.
>>>
>>>
>>> First, let me summarize the case. Feel free to correct me if I took 
>>> something wrong. So, we have:
>>> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
>>
>> IPQ5322 SoC is currently connected with qca8386(switch that includes
>> QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
>> SoC IPQ9574.
> 
> As far as I understand these chips have standardized interfaces and 
> QCA8386/QCA8084 can be reused with another SoC(s) in future. As well as 
> IPQ5332 can be used with different phy. So now we are talking about some 
> specific reference design. Isn't it?

Not the specific reference design.
You are right, these chips can be used with other SoC(s) with the
standardized interfaces supported.

> 
>>> - QCA8084 requires a reference clock for normal functionality;
>>
>> The reference clock is selected for the CMN PLL block, which outputs
>> the clocks to the Ethernet devices including the qca8084 PHY for normal
>> functionality, also for other connected Ethernet devices, the CMN PLL
>> block is located in SoC such as ipq5332 and ipq9574.
>>
>>> - IPQ5332, as a chip, is able to provide a set of reference clocks 
>>> for external devices;
>>
>> Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
>> working clocks for the external Ethernet devices such as the QCA8386
>> (switch chip), the reference clocks we are discussing is as the
>> reference clock source of the CMN PLL block.
> 
> Ok, I feel we have some ambiguity regarding the reference clock term 
> here. Sure, CMN PLL needs a reference clock to functioning. And in the 
> same time, the output clock provided by CMN PLL is a reference clock for 
> QCA8384.
> 
> So, when I was talking about IPQ5332, I meant the whole chip including 
> CMN PLL block. So, I asked about CMN PLL output clock. But you already 
> clarified the SoC capabilities below.

Yes, Sergey.

> 
>>> - you want to configure IPQ5332 to provide the reference clock for 
>>> QCA8084.
>>
>> The reference clocks for CMN PLL block is configurable, and the output
>> clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
>> to the external Ethernet devices.
>> here is the topology of clocks.
>>                     ---------
>>                     |        |
>> reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
>>                     |        |
>>                     ---------
>>>
>>>
>>> So, the high level questions are:
>>> 1. Is QCA8084 capable to consume the clock from some other generator? 
>>> Is it possible to clock QCA8084 from external XO/PLL/whatever?
>> No, the clock of qca8084/qca8386 is provided from the output clock of
>> CMN PLL as above.
> 
> Right, in case of pairing QCA8386 with IPQ5332, it is a good option to 
> provide the clock from the SoC. But in general QCA8386 will be Ok with 
> any 50 MHz clock. Right? I would like to say that thinking about this 
> specific reference design being a single possible combination limits a 
> scope of driver implementation options.

Yes, from the view of qca8386(qca8084), any input 50M reference clock
should be fine, but normally we should keep the same clock source for
qca8386 and the connected SoC to avoid any pps offset.

For example, we also tried the crystal 50M as the reference clock of
qca8386, since the SoC connected with qca8386 has the different clock
source from qca8386, which leads to some packet drop because of the
little bit of clock frequency shift.
Normally we should keep the clock source of qca8386 same as the clock
from the connected SoC.

> 
>>> 2. Is IPQ5332 capable to provide reference clock to another switch 
>>> model?
>>
>> Yes, IPQ5332 can provide the reference clock to all connected Ethernet
>> devices, such as qca8386(switch), qca8081 phy and others.
> 
> Ok. Thank you for clarifying this.
> 
>>> 3. Is the reference clock generation subsystem part of the MDIO block 
>>> of IPQ5332?
>>
>> the reference clock of CMN PLL block can be from wifi and external 
>> xtal, the CMN PLL is integrated in the MDIO block, CMN PLL is the 
>> independent
>> block that generates the clocks for the connected Ethernet devices.
>>
>>>
>>>
>>> And there are some tiny questions to make sure that we are on the 
>>> same page:
>>> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of 
>>> gate (or switch) that enables clock output through an IPQ5332 pin. 
>>> Isn't it?
>>
>> That's correct, the LDO is for enabling the output 50M clock of CMN PLL
>> to the connected Ethernet device, which is controlled by the hardware
>> register on the IPQ5332.
>>
>>> And if it's true, then can you clarify, what exactly clock is outputted?
>>
>> the 50M clock is outputted to the external Ethernet devices.
>>
>>> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According 
>>> to iomem addresses that was used in the example reg property, the 
>>> Ethernet LDO is not part of MDIO.
>>
>> LDO is not the part of MDIO block, LDO has the different register space
>> from MDIO, which is located in the independent Ethernet part.
> 
> I have checked the Linaro's DTSs and noticed that mentioned LDO 
> addresses belong to a node called 'ess-uniphy'. So these LDO(s) are part 
> of UNIPHY block. So far, so good.

Yes, LDO is a part of uniphy block on IPQ5332.

> 
>>> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, according 
>>> to iomem address, the CMN PLL is not part of MDIO.
>>
>> No, CMN PLL is not the part of MDIO block, which is the independent
>> block, but it generates the clocks to the connected Ethernet devices
>> managed by MDIO bus, and the CMN PLL block needs to be configured
>> correctly to generate the clocks to make the MDIO devices(Ethernet
>> devices) working.
> 
> I came to the same conclusion checking Linaro's DTS. So the CMN PLL 
> block looks like a small block implemented outside of any other block. 
> Now I am starting to understand, why everything was putted into the MDIO 
> driver. This PLL is so small that it doesn't seem to deserve a dedicated 
> driver. Am I got it right?

Yes, you are right. CMN block is a independent block, we just need to
configure this block for selecting the reference clock and then do a
reset, which is a simple configuration and the related output clocks
to the Ethernet devices, so it is put in the MDIO driver currently.

> 
>>> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are 
>>> they need for the external reference clock generation?
>>
>> GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
>> with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
>> consumed by the Ethernet devices, which is not for the external
>> reference clock generation, external reference clock of CMN PLL are the
>> fix clock that are from wifi or external XO.
> 
> Again this UNIPHY block. The UNIPHY node was missed from the upstream 
> DTS, so it was decided to assign AHB & SYS clocks to MDIO. Right?

Right, currently there is no UNIPHY node defined in upstream.

> 
> What do you think about implementing this clocks handling functionality 
> in a dedicated driver (e.g. uniphy) and create a dedicated DTS node for 
> it? This driver could consume AHB & SYS clocks as well as consuming CMN 
> PLL clock and be a clock provider for the Ethernet PHY (e.g. QCA8336).

As for AHB & SYS clocks, that can be consumed by the dedicated in the
future uniphy driver, but it seem there is a sequence issue with
qca8386(qca8084) as mentioned in the reply to your comment below.

Maybe we can enable these uniphy clocks in the GCC(SoC) provider driver?
i am not sure whether it is acceptable by the GCC(SoC) provider driver.

> 
> And looks like CMN PLL should be implemented as a dedicated micro 
> driver. A driver that consumes fixed reference clocks (XO or from WiFi) 
> and provides the clock to UNIPHY, to be passed to the Ethernet PHY by 
> means of LDO gate.
the CMN PLL block can be realized as the independent driver.
maybe this CMN driver can be put in the directory drivers/clk/qcom?

> 
>>> To speed up the discussion, let me share my user's view of the 
>>> reference clocks modeling. I would like to join the option that has 
>>> already been suggested by the maintainers. It is better to implement 
>>> reference clocks handling using the clocks API, and the clock 
>>> subsystem will take care of enabling and configuring them.
>>>
>>> And considering the expected answers to the above questions, I would 
>>> like to suggest to implement the clock handling using a dedicated 
>>> clock controlling driver. Or even using several of such tiny 
>>> dedicated drivers. So DTS will become like this:
>>>
>>>    ext_ref_clock: ext_ref_clock {
>>>      compatible = "fixed-clock";
>>>      clock-frequency = <48000000>;
>>>    };
>>>
>>>    eth_cmn_pll: clock-controller@9b000 {
>>>      compatible = "qcom,eth-cmn-pll-ipq5223";
>>>      reg = <0x9b000 0x800>;
>>>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>>>    };
>>>
>>>    phy0_ext_clk: clock-controller@7a00610 {
>>>      compatible = "qcom,ipq-eth-ldo";
>>>      reg = <0x7a00610 0x4>;
>>>      clocks = <&eth_cmn_pll>;
>>>    };
>>>
>>>    mdio@90000 {
>>>      compatible = "qcom,ipq4019-mdio";
>>>      reg = <0x90000 0x64>;
>>>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
>>>
>>>      ethernet-phy@1 {
>>>        compatible = "...";
>>>        reg = <1>;
>>>        clocks = <&phy0_ext_clk>;
>>>        reset-gpios = <&gcc ...>;
>>>      };
>>>    };
>>
>> Thanks Sergey for the reference DTS.
>> Since the GPIO reset of qca8084/qca8386 is needed before configuring the
>> Ethernet device.
>>
>> The configuration of and phy0_ext_clk(LDO) should be configured
>> firstly, which enables the clocks to the Ethernet devices, then the GPIO
>> reset of the connected Ethernet devices(such as qca8386) can take
>> effect, currently the GPIO reset takes the MDIO bus level reset.
>>
>> So phy0_ext_clk can't be put in the PHY device tree node, one LDO
>> controls the clock output enabled to the connected Ethernet device such
>> as qca8386.
> 
> I still feel lost. Why it is impossible to specify clocks and resets in 
> the PHY node and then implement the initialization sequence in the 
> QCA8386 driver? I read the discussion of the QCA8386 driver submission. 
> That driver modeling also looks a complex task. But it still puzzling 
> me, why a part of the QCA8386 driver should be implemented inside the 
> MDIO driver.
> 
> -- 
> Sergey

Let me clarify the work sequence here.
1. configure CMN PLL to generate the reference clocks for qca8386(
same as qca8084).
2. enable LDO and configure the uniphy ahb & sys clocks.
3. do GPIO reset on qca8386(qca8084), the GPIO reset is for chip,
just need to do one GPIO reset on quad PHYs.
4. configure the initial clocks and resets, which are from NSSCC
clock provider driver, the NSSCC is also located in qca8386(qca8084),
these clocks and resets for all quad PHYs of qca8386(qca8084), which
just needs to be initialized one time.
5. then the qca8386(qca8084) PHY capability can be acquired correctly in
the PHY probe function.

Currently, The GPIO reset of qca8386(qca8084) takes use of the MDIO
level GPIO reset, so i put the LDO enable in the MDIO probe function
called before MDIO bus level reset.

To take your proposal, we can't use the MDIO bus level reset and MDIO
device level reset from the MDIO bus framework code, we need to do
reset in one PHY probe function, and the CMN driver and uniphy driver
needs to be initialized before PHY probe function, CMN driver is fine,
but it seems be not usual for uniphy(pcs) driver called before PHY probe
function.
  

On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
> Hi Luo,
> 
> thank you for explaining the case in such details. I also have checked the
> related DTSs in the Linaro repository to be more familiar with the I/O mem
> layout. Specifically I checked these two, hope they are relevant to the
> discussion:
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> 
> Please find my comments below.

Hi Sergey

There is a second thread going on, focused around the quad PHY. See:

https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/

Since it is very hard to get consistent information out of Luo, he has
annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
going back to baby steps, focusing on just the quad pure PHY, and
trying to get that understood and correctly described in DT.

However, does Linaro have any interest in just taking over this work,
or mentoring Luo?

	  Andrew
  

On 1/5/24 7:52 AM, Andrew Lunn wrote:
> On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
>> Hi Luo,
>>
>> thank you for explaining the case in such details. I also have checked the
>> related DTSs in the Linaro repository to be more familiar with the I/O mem
>> layout. Specifically I checked these two, hope they are relevant to the
>> discussion:
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> Please find my comments below.
> 
> Hi Sergey
> 
> There is a second thread going on, focused around the quad PHY. See:
> 
> https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/
> 
> Since it is very hard to get consistent information out of Luo, he has
> annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
> going back to baby steps, focusing on just the quad pure PHY, and
> trying to get that understood and correctly described in DT.
> 
> However, does Linaro have any interest in just taking over this work,
> or mentoring Luo?

I will reach out to Qualcomm to discuss options here.  We can
certainly offer to mentor, and I think we might even be able to
take over the work.  But I won't be able to get any resolution
on this until next week.

Jie Luo, please hold off on further posts on this for a little
while.

I will report back once I've been able to discuss this with
folks at Qualcomm.

					-Alex



> 
> 	  Andrew
>
  

Hi Andrew,

On 05.01.2024 15:52, Andrew Lunn wrote:
> On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
>> Hi Luo,
>>
>> thank you for explaining the case in such details. I also have checked the
>> related DTSs in the Linaro repository to be more familiar with the I/O mem
>> layout. Specifically I checked these two, hope they are relevant to the
>> discussion:
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> Please find my comments below.
> 
> Hi Sergey
> 
> There is a second thread going on, focused around the quad PHY. See:
> 
> https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/

Yeah. I had read your discussion yesterday before coming back to this 
clock discussion. It is a monster chip and looks like you have a hard 
time figuring out how it works and looking for a good code/DT model.

> Since it is very hard to get consistent information out of Luo, he has
> annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
> going back to baby steps, focusing on just the quad pure PHY, and
> trying to get that understood and correctly described in DT.
> 
> However, does Linaro have any interest in just taking over this work,
> or mentoring Luo?

I should clarify here a bit. I found this discussion while looking for a 
way to port one open source firmware to my router based on previous IPQ 
generation. And since I am a bit familiar with this chip family, I chose 
to put my 2c to make implementation discussion more structured. Long 
story short, I have no idea about Linaro's plans :)

If I am allowed to speak, the chosen baby steps approach to focus on 
pure PHY seems to be the only sane method in that case. Considering 
Alex's promise, we can assume that the next release will support this PHY.

--
Sergey
  

On 05.01.2024 12:34, Jie Luo wrote:
> On 1/5/2024 10:48 AM, Sergey Ryazanov wrote:
>> thank you for explaining the case in such details. I also have checked 
>> the related DTSs in the Linaro repository to be more familiar with the 
>> I/O mem layout. Specifically I checked these two, hope they are 
>> relevant to the discussion:
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>
> Thanks Sergey for looking into this driver support.
>
>> Please find my comments below.
>>
>> On 03.01.2024 15:31, Jie Luo wrote:
>>> On 1/2/2024 7:01 AM, Sergey Ryazanov wrote:
>>>> Hi Luo,
>>>>
>>>> I have a few questions regarding the high level design of 
>>>> implementation. I hope that clarifying these topics will help us to 
>>>> find a good model for the case and finally merge a supporting code. 
>>>> Please find the questions below.
>>>>
>>>> On 25.12.2023 10:44, Luo Jie wrote:
>>>>> For IPQ5332 platform, there are two MAC PCSs, and qca8084 is
>>>>> connected with one of them.
>>>>>
>>>>> 1. The Ethernet LDO needs to be enabled to make the PHY GPIO
>>>>>     reset taking effect, which uses the MDIO bus level reset.
>>>>>
>>>>> 2. The SoC GCC uniphy AHB and SYS clocks need to be enabled
>>>>>     to make the ethernet PHY device accessible.
>>>>>
>>>>> 3. To provide the clock to the ethernet, the CMN clock needs
>>>>>     to be initialized for selecting reference clock and enabling
>>>>>     the output clock.
>>>>>
>>>>> 4. Support optional MDIO clock frequency config.
>>>>>
>>>>> 5. Update dt-bindings doc for the new added properties.
>>>>>
>>>>> Changes in v2:
>>>>>     * remove the PHY related features such as PHY address
>>>>>       program and clock initialization.
>>>>>     * leverage the MDIO level GPIO reset for qca8084 PHY.
>>>>>
>>>>> Changes in v3:
>>>>>     * fix the christmas-tree format issue.
>>>>>     * improve the dt-binding changes.
>>>>>
>>>>> Changes in v4:
>>>>>     * improve the CMN PLL reference clock config.
>>>>>     * improve the dt-binding changes.
>>>>>
>>>>> Luo Jie (5):
>>>>>    net: mdio: ipq4019: move eth_ldo_rdy before MDIO bus register
>>>>>    net: mdio: ipq4019: enable the SoC uniphy clocks for ipq5332 
>>>>> platform
>>>>>    net: mdio: ipq4019: configure CMN PLL clock for ipq5332
>>>>>    net: mdio: ipq4019: support MDIO clock frequency divider
>>>>>    dt-bindings: net: ipq4019-mdio: Document ipq5332 platform
>>>>>
>>>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 141 ++++++++-
>>>>>   drivers/net/mdio/mdio-ipq4019.c               | 288 
>>>>> ++++++++++++++++--
>>>>>   2 files changed, 399 insertions(+), 30 deletions(-)
>>>>
>>>> I'm asking these questions because after checking the patches and 
>>>> following the earlier discussion, the series is looks like an 
>>>> overloading of the MDIO driver with somehow but not strictly related 
>>>> functionality.
>>>>
>>>>
>>>> First, let me summarize the case. Feel free to correct me if I took 
>>>> something wrong. So, we have:
>>>> - a reference design contains IPQ5332 SoC + QCA8084 switch/Phy;
>>>
>>> IPQ5322 SoC is currently connected with qca8386(switch that includes
>>> QCA8084 PHY), the pure PHY chip qca8084 is currently connected on the
>>> SoC IPQ9574.
>>
>> As far as I understand these chips have standardized interfaces and 
>> QCA8386/QCA8084 can be reused with another SoC(s) in future. As well 
>> as IPQ5332 can be used with different phy. So now we are talking about 
>> some specific reference design. Isn't it?
> 
> Not the specific reference design.
> You are right, these chips can be used with other SoC(s) with the
> standardized interfaces supported.
> 
>>>> - QCA8084 requires a reference clock for normal functionality;
>>>
>>> The reference clock is selected for the CMN PLL block, which outputs
>>> the clocks to the Ethernet devices including the qca8084 PHY for normal
>>> functionality, also for other connected Ethernet devices, the CMN PLL
>>> block is located in SoC such as ipq5332 and ipq9574.
>>>
>>>> - IPQ5332, as a chip, is able to provide a set of reference clocks 
>>>> for external devices;
>>>
>>> Yes, the CMN PLL block of IPQ5332 provides the output clocks as the
>>> working clocks for the external Ethernet devices such as the QCA8386
>>> (switch chip), the reference clocks we are discussing is as the
>>> reference clock source of the CMN PLL block.
>>
>> Ok, I feel we have some ambiguity regarding the reference clock term 
>> here. Sure, CMN PLL needs a reference clock to functioning. And in the 
>> same time, the output clock provided by CMN PLL is a reference clock 
>> for QCA8384.
>>
>> So, when I was talking about IPQ5332, I meant the whole chip including 
>> CMN PLL block. So, I asked about CMN PLL output clock. But you already 
>> clarified the SoC capabilities below.
> 
> Yes, Sergey.
> 
>>
>>>> - you want to configure IPQ5332 to provide the reference clock for 
>>>> QCA8084.
>>>
>>> The reference clocks for CMN PLL block is configurable, and the output
>>> clocks of CMN PLL are fixed, the output clocks are 50MHZ, which is given
>>> to the external Ethernet devices.
>>> here is the topology of clocks.
>>>                     ---------
>>>                     |        |
>>> reference clock --->| CMN PLL|--> output 50M clocks --> qca8084/qca8386
>>>                     |        |
>>>                     ---------
>>>>
>>>>
>>>> So, the high level questions are:
>>>> 1. Is QCA8084 capable to consume the clock from some other 
>>>> generator? Is it possible to clock QCA8084 from external 
>>>> XO/PLL/whatever?
>>> No, the clock of qca8084/qca8386 is provided from the output clock of
>>> CMN PLL as above.
>>
>> Right, in case of pairing QCA8386 with IPQ5332, it is a good option to 
>> provide the clock from the SoC. But in general QCA8386 will be Ok with 
>> any 50 MHz clock. Right? I would like to say that thinking about this 
>> specific reference design being a single possible combination limits a 
>> scope of driver implementation options.
> 
> Yes, from the view of qca8386(qca8084), any input 50M reference clock
> should be fine, but normally we should keep the same clock source for
> qca8386 and the connected SoC to avoid any pps offset.
> 
> For example, we also tried the crystal 50M as the reference clock of
> qca8386, since the SoC connected with qca8386 has the different clock
> source from qca8386, which leads to some packet drop because of the
> little bit of clock frequency shift.
> Normally we should keep the clock source of qca8386 same as the clock
> from the connected SoC.

Great, so we have to deal with solution that more or less following 
standards. So far, so good.

>>>> 2. Is IPQ5332 capable to provide reference clock to another switch 
>>>> model?
>>>
>>> Yes, IPQ5332 can provide the reference clock to all connected Ethernet
>>> devices, such as qca8386(switch), qca8081 phy and others.
>>
>> Ok. Thank you for clarifying this.
>>
>>>> 3. Is the reference clock generation subsystem part of the MDIO 
>>>> block of IPQ5332?
>>>
>>> the reference clock of CMN PLL block can be from wifi and external 
>>> xtal, the CMN PLL is integrated in the MDIO block, CMN PLL is the 
>>> independent
>>> block that generates the clocks for the connected Ethernet devices.
>>>
>>>>
>>>>
>>>> And there are some tiny questions to make sure that we are on the 
>>>> same page:
>>>> a. What is the mentioned Ethernet LDO? AFAIK, LDO is some kind of 
>>>> gate (or switch) that enables clock output through an IPQ5332 pin. 
>>>> Isn't it?
>>>
>>> That's correct, the LDO is for enabling the output 50M clock of CMN PLL
>>> to the connected Ethernet device, which is controlled by the hardware
>>> register on the IPQ5332.
>>>
>>>> And if it's true, then can you clarify, what exactly clock is 
>>>> outputted?
>>>
>>> the 50M clock is outputted to the external Ethernet devices.
>>>
>>>> b. Is the Ethernet LDO part of the MDIO block of IPQ5332? According 
>>>> to iomem addresses that was used in the example reg property, the 
>>>> Ethernet LDO is not part of MDIO.
>>>
>>> LDO is not the part of MDIO block, LDO has the different register space
>>> from MDIO, which is located in the independent Ethernet part.
>>
>> I have checked the Linaro's DTSs and noticed that mentioned LDO 
>> addresses belong to a node called 'ess-uniphy'. So these LDO(s) are 
>> part of UNIPHY block. So far, so good.
> 
> Yes, LDO is a part of uniphy block on IPQ5332.
> 
>>>> c. Is the CMN PLL part of the MDIO block of IPQ5332? Again, 
>>>> according to iomem address, the CMN PLL is not part of MDIO.
>>>
>>> No, CMN PLL is not the part of MDIO block, which is the independent
>>> block, but it generates the clocks to the connected Ethernet devices
>>> managed by MDIO bus, and the CMN PLL block needs to be configured
>>> correctly to generate the clocks to make the MDIO devices(Ethernet
>>> devices) working.
>>
>> I came to the same conclusion checking Linaro's DTS. So the CMN PLL 
>> block looks like a small block implemented outside of any other block. 
>> Now I am starting to understand, why everything was putted into the 
>> MDIO driver. This PLL is so small that it doesn't seem to deserve a 
>> dedicated driver. Am I got it right?
> 
> Yes, you are right. CMN block is a independent block, we just need to
> configure this block for selecting the reference clock and then do a
> reset, which is a simple configuration and the related output clocks
> to the Ethernet devices, so it is put in the MDIO driver currently.
> 
>>>> d. Are GCC AHB & SYS clocks really consumed by MDIO itself? Or are 
>>>> they need for the external reference clock generation?
>>>
>>> GCC AHB & SYS clocks are consumed by the uniphy(PCS) that is connected
>>> with the Ethernet devices, so we can say the GCC AHB & SYS clocks are
>>> consumed by the Ethernet devices, which is not for the external
>>> reference clock generation, external reference clock of CMN PLL are the
>>> fix clock that are from wifi or external XO.
>>
>> Again this UNIPHY block. The UNIPHY node was missed from the upstream 
>> DTS, so it was decided to assign AHB & SYS clocks to MDIO. Right?
> 
> Right, currently there is no UNIPHY node defined in upstream.
> 
>> What do you think about implementing this clocks handling 
>> functionality in a dedicated driver (e.g. uniphy) and create a 
>> dedicated DTS node for it? This driver could consume AHB & SYS clocks 
>> as well as consuming CMN PLL clock and be a clock provider for the 
>> Ethernet PHY (e.g. QCA8336).
> 
> As for AHB & SYS clocks, that can be consumed by the dedicated in the
> future uniphy driver, but it seem there is a sequence issue with
> qca8386(qca8084) as mentioned in the reply to your comment below.
> 
> Maybe we can enable these uniphy clocks in the GCC(SoC) provider driver?
> i am not sure whether it is acceptable by the GCC(SoC) provider driver.

I do not think that idea of putting the UNIPHY clocks into GCC is any 
better than putting its handling into the MDIO driver. UNIPHY and GCC 
look like belong to different chip blocks.

I just realized that the UNIPHY block is a MII (probably SGMII) 
controller. Isn't it? And I expect that it responsible more then just 
for clock enabling. It should also activate and perform a basic 
configuration of MII for actual data transmission. If so, then it should 
placed somewhere under drivers/net/phy or drivers/net/pcs.

>> And looks like CMN PLL should be implemented as a dedicated micro 
>> driver. A driver that consumes fixed reference clocks (XO or from 
>> WiFi) and provides the clock to UNIPHY, to be passed to the Ethernet 
>> PHY by means of LDO gate.
>
> the CMN PLL block can be realized as the independent driver.
> maybe this CMN driver can be put in the directory drivers/clk/qcom?

Yep, exactly.

As far as I understand, we basically agree that clocks configuration can 
be implemented based on the clock API using a more specialized driver(s) 
than MDIO. The only obstacle is the PHY chip initialization issue 
explained below.

>>>> To speed up the discussion, let me share my user's view of the 
>>>> reference clocks modeling. I would like to join the option that has 
>>>> already been suggested by the maintainers. It is better to implement 
>>>> reference clocks handling using the clocks API, and the clock 
>>>> subsystem will take care of enabling and configuring them.
>>>>
>>>> And considering the expected answers to the above questions, I would 
>>>> like to suggest to implement the clock handling using a dedicated 
>>>> clock controlling driver. Or even using several of such tiny 
>>>> dedicated drivers. So DTS will become like this:
>>>>
>>>>    ext_ref_clock: ext_ref_clock {
>>>>      compatible = "fixed-clock";
>>>>      clock-frequency = <48000000>;
>>>>    };
>>>>
>>>>    eth_cmn_pll: clock-controller@9b000 {
>>>>      compatible = "qcom,eth-cmn-pll-ipq5223";
>>>>      reg = <0x9b000 0x800>;
>>>>      clocks = <&ext_ref_clock>; /* use external 48MHz clock */
>>>>    };
>>>>
>>>>    phy0_ext_clk: clock-controller@7a00610 {
>>>>      compatible = "qcom,ipq-eth-ldo";
>>>>      reg = <0x7a00610 0x4>;
>>>>      clocks = <&eth_cmn_pll>;
>>>>    };
>>>>
>>>>    mdio@90000 {
>>>>      compatible = "qcom,ipq4019-mdio";
>>>>      reg = <0x90000 0x64>;
>>>>      clocks = <&gcc GCC_MDIO_AHB_CLK>;
>>>>
>>>>      ethernet-phy@1 {
>>>>        compatible = "...";
>>>>        reg = <1>;
>>>>        clocks = <&phy0_ext_clk>;
>>>>        reset-gpios = <&gcc ...>;
>>>>      };
>>>>    };
>>>
>>> Thanks Sergey for the reference DTS.
>>> Since the GPIO reset of qca8084/qca8386 is needed before configuring the
>>> Ethernet device.
>>>
>>> The configuration of and phy0_ext_clk(LDO) should be configured
>>> firstly, which enables the clocks to the Ethernet devices, then the GPIO
>>> reset of the connected Ethernet devices(such as qca8386) can take
>>> effect, currently the GPIO reset takes the MDIO bus level reset.
>>>
>>> So phy0_ext_clk can't be put in the PHY device tree node, one LDO
>>> controls the clock output enabled to the connected Ethernet device such
>>> as qca8386.
>>
>> I still feel lost. Why it is impossible to specify clocks and resets 
>> in the PHY node and then implement the initialization sequence in the 
>> QCA8386 driver? I read the discussion of the QCA8386 driver 
>> submission. That driver modeling also looks a complex task. But it 
>> still puzzling me, why a part of the QCA8386 driver should be 
>> implemented inside the MDIO driver.
> 
> Let me clarify the work sequence here.
> 1. configure CMN PLL to generate the reference clocks for qca8386(
> same as qca8084).
> 2. enable LDO and configure the uniphy ahb & sys clocks.
> 3. do GPIO reset on qca8386(qca8084), the GPIO reset is for chip,
> just need to do one GPIO reset on quad PHYs.
> 4. configure the initial clocks and resets, which are from NSSCC
> clock provider driver, the NSSCC is also located in qca8386(qca8084),
> these clocks and resets for all quad PHYs of qca8386(qca8084), which
> just needs to be initialized one time.
> 5. then the qca8386(qca8084) PHY capability can be acquired correctly in
> the PHY probe function.
> 
> Currently, The GPIO reset of qca8386(qca8084) takes use of the MDIO
> level GPIO reset, so i put the LDO enable in the MDIO probe function
> called before MDIO bus level reset.
> 
> To take your proposal, we can't use the MDIO bus level reset and MDIO
> device level reset from the MDIO bus framework code, we need to do
> reset in one PHY probe function, and the CMN driver and uniphy driver
> needs to be initialized before PHY probe function, CMN driver is fine,
> but it seems be not usual for uniphy(pcs) driver called before PHY probe
> function.

Thank you for this compact yet detailed summary. Now it much more clear, 
what this phy chip requires to be initialized.

Looks like you need to implement at least two drivers:
1. chip (package) level driver that is responsible for basic "package" 
initialization;
2. phy driver to handle actual phy capabilities.

In case of DTS, device enumeration on the MDIO bus will work like this 
(see __of_mdiobus_register() function):
1. will be probed all mdio node childs with 'reg' property;
2. will be probed all child nodes without 'reg' property;
3. in both cases 1 & 2 nodes will be probed sequentially and synchronously.

So, using two drivers (package level + phy) you can declare a node 
describing the package first, and then add per-phy nodes.

   ext_ref_clock: ext_ref_clock {
     compatible = "fixed-clock";
     clock-frequency = <48000000>;
   };

   eth_cmn_pll: clock-controller@9b000 {
     compatible = "qcom,eth-cmn-pll-ipq5223";
     reg = <0x9b000 0x800>;
     clocks = <&ext_ref_clock>; /* use external 48MHz clock */
   };

   uniphy: mii-controller@7a00000 {
     compatible = "qcom,ipq5332-uniphy";
     reg = <0x7a00000 0x20000>;
     clocks = <&eth_cmn_pll>;
     #clock-cells = <1>;
   };

   mdio@90000 {
     compatible = "qcom,ipq4019-mdio";
     reg = <0x90000 0x64>;
     clocks = <&gcc GCC_MDIO_AHB_CLK>;

     /* Package level node must be first, so it will be probed first */
     phy_pkg: multi-phy-controller@0 {
       compatible = "qcom,qca8084";  /* matches package-level driver */
       reg = <0>;                    /* value can be any */
       clocks = <&uniphy 0>, <&uniphy 1>, ...;
       reset-gpios = <&gcc 123>;
       qcom,phy-addr-fixup = <1>, <2>, <3>, <4>;
       #clock-cells = <1>;
     };

     phy1: ethernet-phy@1 {
       compatible = "ethernet-phy-id004d.d180"; /* matches phy driver */
       reg = <1>;
       clocks = <&phy_pkg 321>;
     };

     phy2: ethernet-phy@2 {
       compatible = "ethernet-phy-id004d.d180";
       reg = <2>;
       clocks = <&phy_pkg 322>;
     };

     ...
   };


And inside the package-level driver ("qcom,qca8084") you can easily 
request clocks from the kernel using the regular clock API. Request GPIO 
line, etc. And then perform any initialization sequence required by 
QCA8084. As soon as common initialization is done and PHY addresses are 
configured, return from the probe callback.

Then MDIO devices enumeration code will continue the enumeration 
process, find a next node, what will be PHY1, PHY2, etc.

I hope I understand the case correctly and this brief description and 
example DT will give some clues as to a possible solution.

--
Sergey
  

> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
> Isn't it? And I expect that it responsible more then just for clock
> enabling. It should also activate and perform a basic configuration of MII
> for actual data transmission. If so, then it should placed somewhere under
> drivers/net/phy or drivers/net/pcs.

Before we decide that, we need a description of what the UNIPHY
actually does, what registers it has, etc. Sometimes blocks like this
get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
etc. The SERDES parts go into a generic PHY driver, and the SGMII on
to of the SERDES is placed is a PCS driver.

The problem i have so far is that there is no usable description of
any of this hardware, and the developers trying to produce drivers for
this hardware don't actually seem to understand the Linux architecture
for things like this.

> As far as I understand, we basically agree that clocks configuration can be
> implemented based on the clock API using a more specialized driver(s) than
> MDIO. The only obstacle is the PHY chip initialization issue explained
> below.
> Thank you for this compact yet detailed summary. Now it much more clear,
> what this phy chip requires to be initialized.
> 
> Looks like you need to implement at least two drivers:
> 1. chip (package) level driver that is responsible for basic "package"
> initialization;
> 2. phy driver to handle actual phy capabilities.

Nope. As i keep saying, please look at the work Christian is
doing. phylib already has the concept of a PHY package, e.g. look at
the MSCC driver, and how it uses devm_phy_package_join(). What is
missing is a DT binding which allows package properties to be
expressed in DT. And this is what Christian is adding.

	  Andrew
  

On 06.01.2024 17:45, Andrew Lunn wrote:
>> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
>> Isn't it? And I expect that it responsible more then just for clock
>> enabling. It should also activate and perform a basic configuration of MII
>> for actual data transmission. If so, then it should placed somewhere under
>> drivers/net/phy or drivers/net/pcs.
> 
> Before we decide that, we need a description of what the UNIPHY
> actually does, what registers it has, etc. Sometimes blocks like this
> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
> to of the SERDES is placed is a PCS driver.

As far as I understand, UNIPHY only contains SGMII/PSGMII/whatever and a 
simple clock controller. PCIe & USB phys are implemented in other 
hardware blocks. See the lately merged USB support code for similar 
IPQ5018 SoC. But I can only speak to what I found searching online and 
checking the vendor's qca-ssdk "driver".

https://git.codelinaro.org/clo/qsdk/oss/lklm/qca-ssdk/-/tree/NHSS.QSDK.12.4.5.r3

I hope Luo can clarify with more confidence.

> The problem i have so far is that there is no usable description of
> any of this hardware, and the developers trying to produce drivers for
> this hardware don't actually seem to understand the Linux architecture
> for things like this.
>
>> As far as I understand, we basically agree that clocks configuration can be
>> implemented based on the clock API using a more specialized driver(s) than
>> MDIO. The only obstacle is the PHY chip initialization issue explained
>> below.
>> Thank you for this compact yet detailed summary. Now it much more clear,
>> what this phy chip requires to be initialized.
>>
>> Looks like you need to implement at least two drivers:
>> 1. chip (package) level driver that is responsible for basic "package"
>> initialization;
>> 2. phy driver to handle actual phy capabilities.
> 
> Nope. As i keep saying, please look at the work Christian is
> doing. phylib already has the concept of a PHY package, e.g. look at
> the MSCC driver, and how it uses devm_phy_package_join(). What is
> missing is a DT binding which allows package properties to be
> expressed in DT. And this is what Christian is adding.

Andrew, thank you so much for pointing me to that API and Christian's 
work. I have checked the DT change proposal and it fits this QCA8084 
case perfectly.

Am I right that all one has to do to solve this QCA8084 initialization 
case is wrap phys in a ethernet-phy-package node and use 
devm_phy_package_join() / phy_package_init_once() to do the basic 
initialization? So simple?

I came to put my 2c in and learnt a couple of new tricks. What a nice day :)

--
Sergey
  

> Andrew, thank you so much for pointing me to that API and Christian's work.
> I have checked the DT change proposal and it fits this QCA8084 case
> perfectly.

Not too surprising, since Christian is working on another Qualcomm PHY
which is very similar.

> Am I right that all one has to do to solve this QCA8084 initialization case
> is wrap phys in a ethernet-phy-package node and use devm_phy_package_join()
> / phy_package_init_once() to do the basic initialization? So simple?

I hope so. Once the correct kernel abstracts are used, it should be
reasonably straight forward. The clock stuff should be made into a
common clock driver, so all the consumer needs to do is enable the one
clock its needs and common clock driver core goes up the tree and
enables what ever needs enabling. It could be we need to use ID values
in the compatible get the PHY driver probed, rather than enumerate it.

Hopefully Lenaro can help get this all done correctly.

    Andrew
  

On 1/6/2024 11:45 PM, Andrew Lunn wrote:
>> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
>> Isn't it? And I expect that it responsible more then just for clock
>> enabling. It should also activate and perform a basic configuration of MII
>> for actual data transmission. If so, then it should placed somewhere under
>> drivers/net/phy or drivers/net/pcs.

UNIPHY is located in PPE, which controls the interface mode for
connecting with external PHY, the hardware register(4 bytes) of UNIPHY
is accessed by local bus(ioremap).

> 
> Before we decide that, we need a description of what the UNIPHY
> actually does, what registers it has, etc. Sometimes blocks like this
> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
> to of the SERDES is placed is a PCS driver.

Hi Andrew,
the UNIPHY is the hardware part of PPE(packet process engine) in IPQ
platform, which can't be used for USB, SATA serdes, the UNIPHY of
PPE is dedicated for connecting with external PHY CHIP.

> 
> The problem i have so far is that there is no usable description of
> any of this hardware, and the developers trying to produce drivers for
> this hardware don't actually seem to understand the Linux architecture
> for things like this.

Sorry for missing this description of UNIPHY, since the UNIPHY block is
the part of PPE, PPE driver will be posted as the independent driver for
review, so i did not give the description of UNIPHY.

The IPQ PPE includes MAC and UNIPHY integrated, the connection with
external PHY is as below.
MAC ---- UNIPHY(PCS) ---- (PCS)external PHY.

The UNIPHY here is the Ethernet dedicated SERDES for connecting with
external PHY.

> 
>> As far as I understand, we basically agree that clocks configuration can be
>> implemented based on the clock API using a more specialized driver(s) than
>> MDIO. The only obstacle is the PHY chip initialization issue explained
>> below.
>> Thank you for this compact yet detailed summary. Now it much more clear,
>> what this phy chip requires to be initialized.
>>
>> Looks like you need to implement at least two drivers:
>> 1. chip (package) level driver that is responsible for basic "package"
>> initialization;
>> 2. phy driver to handle actual phy capabilities.
> 
> Nope. As i keep saying, please look at the work Christian is
> doing. phylib already has the concept of a PHY package, e.g. look at
> the MSCC driver, and how it uses devm_phy_package_join(). What is
> missing is a DT binding which allows package properties to be
> expressed in DT. And this is what Christian is adding.
> 
> 	  Andrew

Thanks Andrew, the driver of qca8084 will be updated based on the
concept of PHY package.
  

On 1/7/2024 6:03 AM, Sergey Ryazanov wrote:
> On 06.01.2024 17:45, Andrew Lunn wrote:
>>> I just realized that the UNIPHY block is a MII (probably SGMII) 
>>> controller.
>>> Isn't it? And I expect that it responsible more then just for clock
>>> enabling. It should also activate and perform a basic configuration 
>>> of MII
>>> for actual data transmission. If so, then it should placed somewhere 
>>> under
>>> drivers/net/phy or drivers/net/pcs.
>>
>> Before we decide that, we need a description of what the UNIPHY
>> actually does, what registers it has, etc. Sometimes blocks like this
>> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
>> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
>> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
>> to of the SERDES is placed is a PCS driver.
> 
> As far as I understand, UNIPHY only contains SGMII/PSGMII/whatever and a 
> simple clock controller. PCIe & USB phys are implemented in other 
> hardware blocks. See the lately merged USB support code for similar 
> IPQ5018 SoC. But I can only speak to what I found searching online and 
> checking the vendor's qca-ssdk "driver".
> 
> https://git.codelinaro.org/clo/qsdk/oss/lklm/qca-ssdk/-/tree/NHSS.QSDK.12.4.5.r3
> 
> I hope Luo can clarify with more confidence.

Yes, Sergey. UNIPHY includes the interface mode controller(SGMII/UXGMII
PSGMII etc.) and the clock controller that provides the clocks to the
PPE(packet process engine) ports, which is the dedicated UNIPHY(PCS) for
connecting external PHY(such as qca8084 PHY) and located in the PPE
hardware block. The UNIPHY of PPE can't be used for PCIE & USB.

> 
>> The problem i have so far is that there is no usable description of
>> any of this hardware, and the developers trying to produce drivers for
>> this hardware don't actually seem to understand the Linux architecture
>> for things like this.
>>
>>> As far as I understand, we basically agree that clocks configuration 
>>> can be
>>> implemented based on the clock API using a more specialized driver(s) 
>>> than
>>> MDIO. The only obstacle is the PHY chip initialization issue explained
>>> below.
>>> Thank you for this compact yet detailed summary. Now it much more clear,
>>> what this phy chip requires to be initialized.
>>>
>>> Looks like you need to implement at least two drivers:
>>> 1. chip (package) level driver that is responsible for basic "package"
>>> initialization;
>>> 2. phy driver to handle actual phy capabilities.
>>
>> Nope. As i keep saying, please look at the work Christian is
>> doing. phylib already has the concept of a PHY package, e.g. look at
>> the MSCC driver, and how it uses devm_phy_package_join(). What is
>> missing is a DT binding which allows package properties to be
>> expressed in DT. And this is what Christian is adding.
> 
> Andrew, thank you so much for pointing me to that API and Christian's 
> work. I have checked the DT change proposal and it fits this QCA8084 
> case perfectly.
> 
> Am I right that all one has to do to solve this QCA8084 initialization 
> case is wrap phys in a ethernet-phy-package node and use 
> devm_phy_package_join() / phy_package_init_once() to do the basic 
> initialization? So simple?
> 
> I came to put my 2c in and learnt a couple of new tricks. What a nice 
> day :)
> 
> -- 
> Sergey
  

On 1/6/2024 4:14 AM, Sergey Ryazanov wrote:
> Hi Andrew,
> 
> On 05.01.2024 15:52, Andrew Lunn wrote:
>> On Fri, Jan 05, 2024 at 04:48:31AM +0200, Sergey Ryazanov wrote:
>>> Hi Luo,
>>>
>>> thank you for explaining the case in such details. I also have 
>>> checked the
>>> related DTSs in the Linaro repository to be more familiar with the 
>>> I/O mem
>>> layout. Specifically I checked these two, hope they are relevant to the
>>> discussion:
>>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4.r3/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>
>>> Please find my comments below.
>>
>> Hi Sergey
>>
>> There is a second thread going on, focused around the quad PHY. See:
>>
>> https://lore.kernel.org/netdev/60b9081c-76fa-4122-b7ae-5c3dcf7229f9@lunn.ch/
> 
> Yeah. I had read your discussion yesterday before coming back to this 
> clock discussion. It is a monster chip and looks like you have a hard 
> time figuring out how it works and looking for a good code/DT model.

qca8084 is indeed a little complex, unlike other qcom PHY chips, qca8084
also includes the integrated clock controller that generates the
different clocks for the link of quad PHYs, which leads to some
misunderstanding of the clocks and resets used by qca8084.

i will refer to Christian's code and base on that to propose the DT
model of qca8084 for the review.

I am really sorry for the annoyance and misunderstanding caused by my
patches and replies.

> 
>> Since it is very hard to get consistent information out of Luo, he has
>> annoyed nearly all the PHY maintainers and all the DT maintainers, i'm
>> going back to baby steps, focusing on just the quad pure PHY, and
>> trying to get that understood and correctly described in DT.
>>
>> However, does Linaro have any interest in just taking over this work,
>> or mentoring Luo?
> 
> I should clarify here a bit. I found this discussion while looking for a 
> way to port one open source firmware to my router based on previous IPQ 
> generation. And since I am a bit familiar with this chip family, I chose 
> to put my 2c to make implementation discussion more structured. Long 
> story short, I have no idea about Linaro's plans :)
> 
> If I am allowed to speak, the chosen baby steps approach to focus on 
> pure PHY seems to be the only sane method in that case. Considering 
> Alex's promise, we can assume that the next release will support this PHY.
> 
> -- 
> Sergey

Thanks for help and guidance.
  

> The IPQ PPE includes MAC and UNIPHY integrated, the connection with
> external PHY is as below.
> MAC ---- UNIPHY(PCS) ---- (PCS)external PHY.
> 
> The UNIPHY here is the Ethernet dedicated SERDES for connecting with
> external PHY.

You call it a PCS here. So does it implement clause 37 or 73 of the
802.3 standard? If it does, the driver for it belongs in
drivers/net/pcs.

	Andrew
  

On Sat, Jan 06, 2024 at 04:45:08PM +0100, Andrew Lunn wrote:
> > I just realized that the UNIPHY block is a MII (probably SGMII) controller.
> > Isn't it? And I expect that it responsible more then just for clock
> > enabling. It should also activate and perform a basic configuration of MII
> > for actual data transmission. If so, then it should placed somewhere under
> > drivers/net/phy or drivers/net/pcs.
> 
> Before we decide that, we need a description of what the UNIPHY
> actually does, what registers it has, etc. Sometimes blocks like this
> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
> to of the SERDES is placed is a PCS driver.
> 
> The problem i have so far is that there is no usable description of
> any of this hardware, and the developers trying to produce drivers for
> this hardware don't actually seem to understand the Linux architecture
> for things like this.

+1. I think it's now more convoluted than ever, and someone needs to
take a step back, look at the hardware, look at the kernel model, and
work out how to implement this. It needs to be explained in a clear
and concise way in _one_ go, not spread over multiple emails. Probably
with ASCII art diagrams showing the structure.

If that isn't possible, then someone needs to provide a detailed
description of the hardware so that the subsystem maintainers get a
proper view of what this hardware is so they can advise. This is the
least preferable option due to the maintainer time it takes.

If neither of these two things happen, then I'm afraid all bets are
off for getting this into the kernel.
  

On 1/8/2024 9:27 PM, Andrew Lunn wrote:
>> The IPQ PPE includes MAC and UNIPHY integrated, the connection with
>> external PHY is as below.
>> MAC ---- UNIPHY(PCS) ---- (PCS)external PHY.
>>
>> The UNIPHY here is the Ethernet dedicated SERDES for connecting with
>> external PHY.
> 
> You call it a PCS here. So does it implement clause 37 or 73 of the
> 802.3 standard? If it does, the driver for it belongs in
> drivers/net/pcs.
> 
> 	Andrew

Hi Andrew,
The PPE integrated PCSes support multiple interface modes such as SGMII,
UXSGMII, QSGMII, PSGMII and 10g-baser etc. which is configurable for
connecting the different PHY devices.

the SGMII and UXSGMII follows Cisco standard, which does not implement
the 802.3 standard in this interface modes.
PSGMII includes the qcom private protocol.

The PPE driver also including the integrated PCS driver will be
posted for the review in the near future, we can discuss in detail
based on that patch series raised.

Thanks for the suggestions.
  

On 1/8/2024 11:53 PM, Russell King (Oracle) wrote:
> On Sat, Jan 06, 2024 at 04:45:08PM +0100, Andrew Lunn wrote:
>>> I just realized that the UNIPHY block is a MII (probably SGMII) controller.
>>> Isn't it? And I expect that it responsible more then just for clock
>>> enabling. It should also activate and perform a basic configuration of MII
>>> for actual data transmission. If so, then it should placed somewhere under
>>> drivers/net/phy or drivers/net/pcs.
>>
>> Before we decide that, we need a description of what the UNIPHY
>> actually does, what registers it has, etc. Sometimes blocks like this
>> get split into a generic PHY, aka drivers/phy/ and a PCS driver. This
>> would be true if the UNIPHY is also used for USB SERDES, SATA SERDES
>> etc. The SERDES parts go into a generic PHY driver, and the SGMII on
>> to of the SERDES is placed is a PCS driver.
>>
>> The problem i have so far is that there is no usable description of
>> any of this hardware, and the developers trying to produce drivers for
>> this hardware don't actually seem to understand the Linux architecture
>> for things like this.
> 
> +1. I think it's now more convoluted than ever, and someone needs to
> take a step back, look at the hardware, look at the kernel model, and
> work out how to implement this. It needs to be explained in a clear
> and concise way in _one_ go, not spread over multiple emails. Probably
> with ASCII art diagrams showing the structure.
> 
> If that isn't possible, then someone needs to provide a detailed
> description of the hardware so that the subsystem maintainers get a
> proper view of what this hardware is so they can advise. This is the
> least preferable option due to the maintainer time it takes.
> 
> If neither of these two things happen, then I'm afraid all bets are
> off for getting this into the kernel.
> 

Thanks Russell for suggestions.
I will provide the diagram of the hardware and the description in the
cover letter of new patch set, i know it is really important to for the
review smoothly.

Christian's work is designing the PHY package level DT, which is very
suitable for the complex clock configs of qca8084 PHY, i also provides
some descriptions of the qca8084 PHY, it will be very welcome to
review the clock DT model of qca8084 PHY.