diff mbox series

[v8,14/14] dt-bindings: net: ar803x: add qca8084 PHY properties

Message ID 20231215074005.26976-15-quic_luoj@quicinc.com
State New
Headers
Series add qca8084 ethernet phy driver |

Commit Message

Jie Luo Dec. 15, 2023, 7:40 a.m. UTC 
  The following properties are added for qca8084 PHY.

1. add the compatible string "ethernet-phy-id004d.d180" since
   the PHY device is not accessible during MDIO bus register.
2. add property "qcom,phy-addr-fixup" for customizing MDIO address.
3. add property "qcom,phy-work-mode" for specifying qca8084 PHY
   work mode.
4. add the initial clocks and resets.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 .../devicetree/bindings/net/qca,ar803x.yaml   | 158 +++++++++++++++++-
 1 file changed, 155 insertions(+), 3 deletions(-)

Comments

Krzysztof Kozlowski Dec. 15, 2023, 8:22 a.m. UTC | #1 
On 15/12/2023 08:40, Luo Jie wrote:
> The following properties are added for qca8084 PHY.
> 
> 1. add the compatible string "ethernet-phy-id004d.d180" since
>    the PHY device is not accessible during MDIO bus register.
> 2. add property "qcom,phy-addr-fixup" for customizing MDIO address.
> 3. add property "qcom,phy-work-mode" for specifying qca8084 PHY
>    work mode.
> 4. add the initial clocks and resets.

All my previous comments (sent one minute before this patchset :) )
apply. Please respond to them or implement them in v5 (not earlier than
after 24h).


Best regards,
Krzysztof
Jie Luo Dec. 15, 2023, 10:16 a.m. UTC | #2 
On 12/15/2023 4:22 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 08:40, Luo Jie wrote:
>> The following properties are added for qca8084 PHY.
>>
>> 1. add the compatible string "ethernet-phy-id004d.d180" since
>>     the PHY device is not accessible during MDIO bus register.
>> 2. add property "qcom,phy-addr-fixup" for customizing MDIO address.
>> 3. add property "qcom,phy-work-mode" for specifying qca8084 PHY
>>     work mode.
>> 4. add the initial clocks and resets.
> 
> All my previous comments (sent one minute before this patchset :) )
> apply. Please respond to them or implement them in v5 (not earlier than
> after 24h).
> 
> 
> Best regards,
> Krzysztof
> 

Sure, will update the new version after the discussion completed.
actually i have query about the dt-bindings doc.

The "maxItems: 1" of the property resets is defined in ethernet-phy.yaml
that is referenced by qca,ar803x.yaml, but i have 11 reset instances
used for qca8084 PHY, it seems i can't overwrite maxItems of reset
property to 11 from 1 in qca,ar803x.yaml.

is there any method to overwrite the maxItems value?

Thanks,
Jie
Andrew Lunn Dec. 15, 2023, 11:25 a.m. UTC | #3 
> The "maxItems: 1" of the property resets is defined in ethernet-phy.yaml
> that is referenced by qca,ar803x.yaml, but i have 11 reset instances
> used for qca8084 PHY

11!?!?? Really? Why?

I assume the order and timer matters, otherwise why would you need
11? So the PHY driver needs to handle this, not phylib framework. So
you will be adding vendor properties to describe all 11 of them. So
ethernet-phy.yaml does not matter.

	Andrew
Andrew Lunn Dec. 15, 2023, 12:12 p.m. UTC | #4 
> +  clocks:
> +    items:
> +      - description: APB bridge clock
> +      - description: AHB clock
> +      - description: Security control clock
> +      - description: TLMM clock
> +      - description: TLMM AHB clock
> +      - description: CNOC AHB clock
> +      - description: MDIO AHB clock
> +      - description: MDIO master AHB clock
> +      - description: PCS0 system clock
> +      - description: PCS1 system clock
> +      - description: EPHY0 system clock
> +      - description: EPHY1 system clock
> +      - description: EPHY2 system clock
> +      - description: EPHY3 system clock

What exactly are you describing here? A PHY, or a PHY package?

The ethernet-phy.yaml describes a PHY. So does each of your 4 PHYs
have 14 clocks? The PHY package as a whole has 14*4 clocks?

This seems unlikely. You have some clocks used by the package as a
whole, and you have some clocks used by one specific PHY within the
package. So you need a hierarchical description of the hardware in DT,
to match the actual hierarchical of the hardware.

This is exactly what Christian has been working on, and you have
persistently ignored what he is doing. You need to work with him.
Nothing is going to be merged until you and Christian have one
consistent design for the two PHYs you are working on.


    Andrew

---
pw-bot: cr
Jie Luo Dec. 15, 2023, 12:16 p.m. UTC | #5 
On 12/15/2023 7:25 PM, Andrew Lunn wrote:
>> The "maxItems: 1" of the property resets is defined in ethernet-phy.yaml
>> that is referenced by qca,ar803x.yaml, but i have 11 reset instances
>> used for qca8084 PHY
> 
> 11!?!?? Really? Why?
> 
> I assume the order and timer matters, otherwise why would you need
> 11? So the PHY driver needs to handle this, not phylib framework. So
> you will be adding vendor properties to describe all 11 of them. So
> ethernet-phy.yaml does not matter.
> 
> 	Andrew

Since these resets need to be configured in the special sequence, and
these clocks need to be configured with different clock rate.

But the clock instance get, the property name is fixed to "clock-names"
according to the function of_parse_clkspec, and the reset property name
is also fixed to "reset-names" from function __of_reset_control_get.
Jie Luo Dec. 15, 2023, 12:33 p.m. UTC | #6 
On 12/15/2023 8:12 PM, Andrew Lunn wrote:
>> +  clocks:
>> +    items:
>> +      - description: APB bridge clock
>> +      - description: AHB clock
>> +      - description: Security control clock
>> +      - description: TLMM clock
>> +      - description: TLMM AHB clock
>> +      - description: CNOC AHB clock
>> +      - description: MDIO AHB clock
>> +      - description: MDIO master AHB clock
>> +      - description: PCS0 system clock
>> +      - description: PCS1 system clock
>> +      - description: EPHY0 system clock
>> +      - description: EPHY1 system clock
>> +      - description: EPHY2 system clock
>> +      - description: EPHY3 system clock
> 
> What exactly are you describing here? A PHY, or a PHY package?
> 
> The ethernet-phy.yaml describes a PHY. So does each of your 4 PHYs
> have 14 clocks? The PHY package as a whole has 14*4 clocks?
> 
> This seems unlikely. You have some clocks used by the package as a
> whole, and you have some clocks used by one specific PHY within the
> package. So you need a hierarchical description of the hardware in DT,
> to match the actual hierarchical of the hardware.
> 
> This is exactly what Christian has been working on, and you have
> persistently ignored what he is doing. You need to work with him.
> Nothing is going to be merged until you and Christian have one
> consistent design for the two PHYs you are working on.
> 
> 
>      Andrew
> 
> ---
> pw-bot: cr

Hi Andrew,
These clocks are for the whole PHY package including quad PHYs, since
these clocks & resets need to be initialized at one point, i put it
the previous MDIO driver code, these clocks & resets are configured
after GPIO hardware reset, after these clocks and resets sequences
configured, each PHY capabilities can be acquired correctly in the PHY
probe function.

Sorry for missing Christian's patches, i will look his patches and
update qca8084 PHY driver correspondingly.
Andrew Lunn Dec. 15, 2023, 1:31 p.m. UTC | #7 
On Fri, Dec 15, 2023 at 08:33:00PM +0800, Jie Luo wrote:
> 
> 
> On 12/15/2023 8:12 PM, Andrew Lunn wrote:
> > > +  clocks:
> > > +    items:
> > > +      - description: APB bridge clock
> > > +      - description: AHB clock
> > > +      - description: Security control clock
> > > +      - description: TLMM clock
> > > +      - description: TLMM AHB clock
> > > +      - description: CNOC AHB clock
> > > +      - description: MDIO AHB clock
> > > +      - description: MDIO master AHB clock
> > > +      - description: PCS0 system clock
> > > +      - description: PCS1 system clock
> > > +      - description: EPHY0 system clock
> > > +      - description: EPHY1 system clock
> > > +      - description: EPHY2 system clock
> > > +      - description: EPHY3 system clock

> Hi Andrew,
> These clocks are for the whole PHY package including quad PHYs, since
> these clocks & resets need to be initialized at one point, i put it
> the previous MDIO driver code, these clocks & resets are configured
> after GPIO hardware reset, after these clocks and resets sequences
> configured, each PHY capabilities can be acquired correctly in the PHY
> probe function.

I really expect the hardware is hierarchical. Its unlikely that EPHY0
is connected to all four PHYs in the package. Its specific to one
PHY. So it should be in the DT properties for that one specific PHY. I
expect the resets are the same. It seems there is a soft and hard
reset per PHY, so i would expect these to be in the node for one PHY.

Do the two PCS instances take up two MDIO address? They can be
considered devices on the bus, so could have a DT node, and hence you
can place the PCS clocks on that node?

What exactly do the two MDIO clocks do? I assume these are not for the
MDIO bus master, but the MDIO slave block within the PHY package?
There is one MDIO slave block shared by the four PHYs. So these are
package properties and should be in the package node in DT.

Look at all the other clocks and decide, are they package clocks, or
specific to one block on the MDIO bus? Do the properties go in the
package node, or the per PHY node?

	Andrew
Russell King (Oracle) Dec. 15, 2023, 1:42 p.m. UTC | #8 
On Fri, Dec 15, 2023 at 08:16:53PM +0800, Jie Luo wrote:
> On 12/15/2023 7:25 PM, Andrew Lunn wrote:
> > > The "maxItems: 1" of the property resets is defined in ethernet-phy.yaml
> > > that is referenced by qca,ar803x.yaml, but i have 11 reset instances
> > > used for qca8084 PHY
> > 
> > 11!?!?? Really? Why?
> > 
> > I assume the order and timer matters, otherwise why would you need
> > 11? So the PHY driver needs to handle this, not phylib framework. So
> > you will be adding vendor properties to describe all 11 of them. So
> > ethernet-phy.yaml does not matter.
> > 
> > 	Andrew
> 
> Since these resets need to be configured in the special sequence, and
> these clocks need to be configured with different clock rate.
> 
> But the clock instance get, the property name is fixed to "clock-names"
> according to the function of_parse_clkspec, and the reset property name
> is also fixed to "reset-names" from function __of_reset_control_get.

I think you need to give more details about this.

Where are these 11 resets located? What is the sequence? Why does the
PHY driver need to deal with each individual reset?

IMHO, a PHY driver should _not_ be dealing with the resets outside of
the PHY device itself, and I find it hard to imagine that qca8084
would have 11 external resets.

If these are 11 internal resets (to qca8084) then why are you using the
reset subsystem, and why do you need to describe them in DT? Surely if
they are internal to the PHY, that can be encapsulated within the PHY
driver?

This is an example of why it is useful to have an _example_ of the use
of this binding, because it would answer some of the above questions.
Jie Luo Dec. 16, 2023, 7:37 a.m. UTC | #9 
On 12/15/2023 9:31 PM, Andrew Lunn wrote:
> On Fri, Dec 15, 2023 at 08:33:00PM +0800, Jie Luo wrote:
>>
>>
>> On 12/15/2023 8:12 PM, Andrew Lunn wrote:
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: APB bridge clock
>>>> +      - description: AHB clock
>>>> +      - description: Security control clock
>>>> +      - description: TLMM clock
>>>> +      - description: TLMM AHB clock
>>>> +      - description: CNOC AHB clock
>>>> +      - description: MDIO AHB clock
>>>> +      - description: MDIO master AHB clock
>>>> +      - description: PCS0 system clock
>>>> +      - description: PCS1 system clock
>>>> +      - description: EPHY0 system clock
>>>> +      - description: EPHY1 system clock
>>>> +      - description: EPHY2 system clock
>>>> +      - description: EPHY3 system clock
> 
>> Hi Andrew,
>> These clocks are for the whole PHY package including quad PHYs, since
>> these clocks & resets need to be initialized at one point, i put it
>> the previous MDIO driver code, these clocks & resets are configured
>> after GPIO hardware reset, after these clocks and resets sequences
>> configured, each PHY capabilities can be acquired correctly in the PHY
>> probe function.
> 
> I really expect the hardware is hierarchical. Its unlikely that EPHY0
> is connected to all four PHYs in the package. Its specific to one
> PHY. So it should be in the DT properties for that one specific PHY. I
> expect the resets are the same. It seems there is a soft and hard
> reset per PHY, so i would expect these to be in the node for one PHY.be 

Hi Andrew,
i understand your point, i tried putting the related clocks and resets
into each device node per PHY, which does not work, since these clocks
ans resets need to be initialized at one function pointer after GPIO 
reset on the qca8084 package.

Sorry for these confusions here, let me explain the qca8084 chip more
detail here, and i will also update this info on the cover letter.

The following is the chip package, the chip can work on the switch mode
like the existed upstream code qca8k, where PHY1-PHY4 is connected with
MAC1-MAC4 directly; The chip can also work on the PHY mode, where PHY1-
PHY4 is connected with PCS1 by 10g-qxgmii; Either switch mode or PHY 
mode, the PHY4 is optionally connected with PCS0 by SGMII, PCS0 and PCS1
are connected with the SoC(IPQ platform) PCSes.
+----------------------------------------------+
|          PCS1           PCS0                 |
|                                              |
|          MAC0           MAC5                 |
|                                              |
|                                              |
|                                              |
|      MAC1      MAC2      MAC3      MAC4      |
|                                              |
|      PHY1      PHY2      PHY3      PHY4      |
+----------------------------------------------+

After the GPIO reset on this package from the MDIO bus level, the chip
is in the cold hardware reset status, the clocks and resets mentioned
here need to be initialized before the capabilities of PHY can be
acquired correctly in the PHY probe function, that is why i put these
clocks and resets in the first PHY device tree node.
When the chip works on the PHY mode, the MAC listed in the block is
not used, the MAC is only used in the switch mode.

i know we can also put the related clocks and resets into the each
PHY and PCS device node to make the hardware hierarchical, then i can
read all initial clocks and resets in the first PHY probe function,
but there are still some package level clocks such as APB/AHB/CNOC
clocks needed for these initial configuration, so i put these clocks
and resets at one PHY device node and only needed to be called by one
time.

 From the clocks and resets name, EPHY0 is specific to the first PHY1,
which is providing clock and reset on the first PHY1, other EPHY clock
and reset is same for the corresponding PHY.

> 
> Do the two PCS instances take up two MDIO address? They can be
> considered devices on the bus, so could have a DT node, and hence you
> can place the PCS clocks on that node?

Yes, two PCS instances take up two MDIO device with different MDIO
address, which also have the DT nodes as the child node of MDIO bus node
there is also some specific clocks and resets defined in the PCS DT
node for the PCS driver.

> 
> What exactly do the two MDIO clocks do? I assume these are not for the
> MDIO bus master, but the MDIO slave block within the PHY package?
> There is one MDIO slave block shared by the four PHYs. So these are
> package properties and should be in the package node in DT.

The MDIO clocks are the qca8084 package level clock, since there
are other modules such as GCC and security control located in the
qca8084 chip, these module register is also accessed by the MDIO bus,
the MDIO AHB clock is this modules access.
there is also a MDIO master for the back pressure function in qca8084
chip, the MDIO master AHB clock is for this function, actually this
function is for the switch mode, but it is the package level clock,
so i put it together here.

No, these clocks are not for the IPQ4019 SoC MDIO bus master. Four PHYs 
are the independent MDIO slave devices.

For the switch mode, we can define these package level clocks and resets
in the DSA device node.

But for the PHY mode, All 4 PHYs is connected with PCS1 by 10g-qxgmii,
there is no package level device tree node defined.

> 
> Look at all the other clocks and decide, are they package clocks, or
> specific to one block on the MDIO bus? Do the properties go in the
> package node, or the per PHY node?
> 
> 	Andrew

The clocks and resets with prefix PCS or EPHY is per PHY/PCS node, other
clocks and resets are the qca8084 package level node.
Jie Luo Dec. 16, 2023, 7:57 a.m. UTC | #10 
On 12/15/2023 9:42 PM, Russell King (Oracle) wrote:
> On Fri, Dec 15, 2023 at 08:16:53PM +0800, Jie Luo wrote:
>> On 12/15/2023 7:25 PM, Andrew Lunn wrote:
>>>> The "maxItems: 1" of the property resets is defined in ethernet-phy.yaml
>>>> that is referenced by qca,ar803x.yaml, but i have 11 reset instances
>>>> used for qca8084 PHY
>>>
>>> 11!?!?? Really? Why?
>>>
>>> I assume the order and timer matters, otherwise why would you need
>>> 11? So the PHY driver needs to handle this, not phylib framework. So
>>> you will be adding vendor properties to describe all 11 of them. So
>>> ethernet-phy.yaml does not matter.
>>>
>>> 	Andrew
>>
>> Since these resets need to be configured in the special sequence, and
>> these clocks need to be configured with different clock rate.
>>
>> But the clock instance get, the property name is fixed to "clock-names"
>> according to the function of_parse_clkspec, and the reset property name
>> is also fixed to "reset-names" from function __of_reset_control_get.
> 
> I think you need to give more details about this.
> 
> Where are these 11 resets located? What is the sequence? Why does the
> PHY driver need to deal with each individual reset?

All these resets and clocks are located in qca8084 chip that includes
the GCC module registered as the clock provider, all these clocks and
resets are from qca8084 clock provider driver, the clock and reset name
prefix with PCS or EPHY is for the individual MDIO device, others are
the qca8084 level, but these clocks and resets need to be initialized
completely, then the PHY probe function can be initialized correctly
with the PHY capabilities acquired after the qca8084 chip level GPIO
reset.

The sequence code is realized in the function qca8084_clock_config of
the patch <net: phy: at803x: add qca808x initial config sequence>, this
function is doing as below.
1. set the Ethernet system clock tree as 25M clock rate.
2. reset and enable PCS system clocks.
3. reset and enable the PHY system clocks.
4. loading the efuse, which is not related the clock and reset.

> 
> IMHO, a PHY driver should _not_ be dealing with the resets outside of
> the PHY device itself, and I find it hard to imagine that qca8084
> would have 11 external resets.

Indeed, these resets are not for a PHY device, some are the chip level
resets, and each PHY has the individual resets.

> 
> If these are 11 internal resets (to qca8084) then why are you using the
> reset subsystem, and why do you need to describe them in DT? Surely if
> they are internal to the PHY, that can be encapsulated within the PHY
> driver?

These resets and clocks are realized by the qca8k clock controller
driver, and i use the clock consumer APIs to initialize these clocks and
resets,
when qca8084 works on the PHY mode, there are only PHY driver
and PCS driver needed to be enabled to make PHY working, and these
clocks and resets need to be initialized before the PHY probe function
finished correctly, where the phy capabilities is read during the probe
function.

> 
> This is an example of why it is useful to have an _example_ of the use
> of this binding, because it would answer some of the above questions.
> 

Yes, Russell, i will add an example in the DT doc in the next patch set.
The following is the device node used for the current qca8084 PHY
code design.

mdio: mdio@90000 {
     ethernet-phy@0 { 

               compatible = "ethernet-phy-id004d.d180"; 

               reg = <1>; 

               qcom,phy-addr-fixup = <1 2 3 4 5 6 7>; 

               qcom,phy-work-mode = <2>; 

               clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>, 

                      <&qca8k_nsscc NSS_CC_AHB_CLK>, 

                      <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>, 

                      <&qca8k_nsscc NSS_CC_TLMM_CLK>, 

                      <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>, 

                      <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>, 

                      <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>, 

                      <&qca8k_nsscc NSS_CC_MDIO_MASTER_AHB_CLK>, 

                      <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>, 

                      <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>, 

                      <&qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>, 

                      <&qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>, 

                      <&qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>, 

                      <&qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>; 

 

               clock-names = "apb_bridge", 

                       "ahb", 

                       "sec_ctrl_ahb", 

                       "tlmm", 

                       "tlmm_ahb", 

                       "cnoc_ahb", 

                       "mdio_ahb", 

                       "mdio_master_ahb", 

                       "srds0_sys", 

                       "srds1_sys", 

                       "gephy0_sys", 

                       "gephy1_sys", 

                       "gephy2_sys", 

                       "gephy3_sys";
               resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>, 

                      <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY3_SYS_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY0_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY1_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY2_ARES>, 

                      <&qca8k_nsscc NSS_CC_GEPHY3_ARES>, 

                      <&qca8k_nsscc NSS_CC_DSP_ARES>; 

 

               reset-names = "srds0_sys", 

                       "srds1_sys", 

                       "gephy0_sys", 

                       "gephy1_sys", 

                       "gephy2_sys", 

                       "gephy3_sys", 

                       "gephy0_soft", 

                       "gephy1_soft", 

                       "gephy2_soft", 

                       "gephy3_soft", 

                       "gephy_dsp"; 

 

       }; 

       ethernet-phy@1 { 

               compatible = "ethernet-phy-id004d.d180"; 

               reg = <2>; 

       }; 

       ethernet-phy@2 { 

               compatible = "ethernet-phy-id004d.d180"; 

               reg = <3>; 

       }; 

 

       ethernet-phy@3 { 

               compatible = "ethernet-phy-id004d.d180"; 

               reg = <4>; 

       };
    };

Thanks.
Andrew Lunn Dec. 16, 2023, 10:21 a.m. UTC | #11 
> The following is the chip package, the chip can work on the switch mode
> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> MAC1-MAC4 directly;

Ah, that is new information, and has a big effect on the design.

Can the qca8K driver be extended to drive this hardware in switch
mode?

	Andrew
Jie Luo Dec. 16, 2023, 1:25 p.m. UTC | #12 
On 12/16/2023 6:21 PM, Andrew Lunn wrote:
>> The following is the chip package, the chip can work on the switch mode
>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>> MAC1-MAC4 directly;
> 
> Ah, that is new information, and has a big effect on the design.
> 
> Can the qca8K driver be extended to drive this hardware in switch
> mode?
> 
> 	Andrew

Yes, Andrew, the qca8k driver can be extended to drive this hardware in
the switch mode, we will push it to the upstream in the near future.
Russell King (Oracle) Dec. 16, 2023, 1:51 p.m. UTC | #13 
On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
> > The following is the chip package, the chip can work on the switch mode
> > like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> > MAC1-MAC4 directly;
> 
> Ah, that is new information, and has a big effect on the design.

This QCA8084 that's being proposed in these patches is not a PHY in
itself, but is a SoC. I came across this:

 https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/

It's sounding like what we have here is some PHY IP that is integrated
into a larger SoC, and the larger SoC needs to be configured so the
PHY IP can work correctly.

Given that this package of four PHYs seems to be rather unique, I think
we need Jie Luo to provide sufficient information so we can understand:

1) this package of four PHYs itself
2) how this package is integrated into the SoC

Specifically, what resets and clocks are controlled from within the
package's register space, which are external to the package
register space (and thus are provided by other IPs in the SoC).

As I've said previously, the lack of DT example doesn't help to further
our understanding. The lack of details of what the package encompases
also doesn't help us understand the hardware.

Unless we can gain that understanding, I feel that Jie Luo's patches
are effectively unreviewable and can't be accepted into mainline.
Jie Luo Dec. 16, 2023, 2:41 p.m. UTC | #14 
On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
> On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
>>> The following is the chip package, the chip can work on the switch mode
>>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>>> MAC1-MAC4 directly;
>>
>> Ah, that is new information, and has a big effect on the design.
> 
> This QCA8084 that's being proposed in these patches is not a PHY in
> itself, but is a SoC. I came across this:
> 
>   https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/

The chip mentioned in the link you mentioned is SoC, which is not the
chip that the qca8084 driver work for.

qca8084/qca8386 is just the Ethernet CHIP, not SoC, for the switch mode 
qca8386, which is most like qca8337 the dsa drive qca8k.c is already in 
upstream.

i qca8084 chip package includes 4 PHYs, 2 PCSs and the common chip level
modules such as GCC and security control modules, all these modules are
located in the qca8084 chip package, since qca8084 works on PHY mode, so
the MACs are not used.

qca8084 is connected with the SoC CHIP such as IPQ platform by PCS1
working on 10g-qxgmii mode and the fourth PHY can also optionally
be connected with the IPQ SoC PCS by sgmii mode, there is no more
interface on qca8084 to connect the external chips.

> It's sounding like what we have here is some PHY IP that is integrated
> into a larger SoC, and the larger SoC needs to be configured so the
> PHY IP can work correctly.

qca8084 is not a SoC, it is the Ethernet chip, in this qca8084 package,
there are GCC that is driving the PHY working on the various link speed.
that is the reason we need to do these package level common clocks and
resets initialization before probing PHY correctly.

> 
> Given that this package of four PHYs seems to be rather unique, I think
> we need Jie Luo to provide sufficient information so we can understand:
> 
> 1) this package of four PHYs itself

Yes, this chip package for all 4 PHYs itself, also including the PCSes
and common package level modules such as GCC.

> 2) how this package is integrated into the SoC

the qca8084 is connected with SoC by PCSes.

> 
> Specifically, what resets and clocks are controlled from within the
> package's register space, which are external to the package
> register space (and thus are provided by other IPs in the SoC).

All clocks and resets mentioned for qca8084 drive including package
level and PCS & PHY clocks and resets from the qca8084 internal GCC
modules register space,

> 
> As I've said previously, the lack of DT example doesn't help to further
> our understanding. The lack of details of what the package encompases
> also doesn't help us understand the hardware.

Indeed, i will add the qca8084 DT example in the next patch set.
BTW, i also replied your earlier comments by providing the DTS defined
for the current qca8084 drive code.

hope you can have a better understanding with the provided DTS code in
earlier reply of this email thread.
> 
> Unless we can gain that understanding, I feel that Jie Luo's patches
> are effectively unreviewable and can't be accepted into mainline.
>
Christian Marangi Dec. 16, 2023, 4:01 p.m. UTC | #15 
On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
> 
> 
> On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
> > On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
> > > > The following is the chip package, the chip can work on the switch mode
> > > > like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> > > > MAC1-MAC4 directly;
> > > 
> > > Ah, that is new information, and has a big effect on the design.
> > 
> > This QCA8084 that's being proposed in these patches is not a PHY in
> > itself, but is a SoC. I came across this:
> > 
> >   https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
> 
> The chip mentioned in the link you mentioned is SoC, which is not the
> chip that the qca8084 driver work for.
> 
> qca8084/qca8386 is just the Ethernet CHIP, not SoC, for the switch mode
> qca8386, which is most like qca8337 the dsa drive qca8k.c is already in
> upstream.

Hi,
sorry for stepping in. I guess here there is a massive confusion with
naming and using qca8k.

Since it seems the same name is used for PHY and for Switch stuff, I
would add PHY and MAC prefix when referring to qca8064.

With the previous message I was a bit confused by the use of qca8k and
didn't know you were actually referring to the DSA driver.
Interesting... this is for the upcoming WiFi 7 platoform right? (ipq9574)

All these discussion comes for the problem of using this PHY as an
integrated PHY in the qca8386 switch and trying to select the mode in
the PHY driver.

Considering you would use the same logic of the current DSA qca8k driver
with the integrated PHY, the problem doesn't apply or a different
implementation should be used (and actually handled later when the
actual DSA code will come)

I would expect in the integrated mode, the switch to handle the PHY (as
it's done by qca8337) with the PHY defined in the switch node and qca8k
handling the PHY registration. With the following implementation flags
can be passed and PHY can be configured to integrated mode. (or virtual
PHY ID can be used for such scope with dedicated functions in the PHY
driver)

With this in mind the entire integrated problem can put on hold and
dropped to be later reimplemented when it's time. (assuming that all the
prereq are already here and the very same implementation of qca8k will
be used)

Anyway, I'm more or less the maintainer of the qca8k.c DSA driver and I
would be more than happy to help you guys internally or externally on
pushing and make this proceed further. (again assuming this is ipq9574
stuff, it would be good to finally have proper DSA driver instead of
leaving the thing unusable as it's the current situation with ipq8074)

> 
> i qca8084 chip package includes 4 PHYs, 2 PCSs and the common chip level
> modules such as GCC and security control modules, all these modules are
> located in the qca8084 chip package, since qca8084 works on PHY mode, so
> the MACs are not used.
> 
> qca8084 is connected with the SoC CHIP such as IPQ platform by PCS1
> working on 10g-qxgmii mode and the fourth PHY can also optionally
> be connected with the IPQ SoC PCS by sgmii mode, there is no more
> interface on qca8084 to connect the external chips.
> 
> > It's sounding like what we have here is some PHY IP that is integrated
> > into a larger SoC, and the larger SoC needs to be configured so the
> > PHY IP can work correctly.
> 
> qca8084 is not a SoC, it is the Ethernet chip, in this qca8084 package,
> there are GCC that is driving the PHY working on the various link speed.
> that is the reason we need to do these package level common clocks and
> resets initialization before probing PHY correctly.
> 
> > 
> > Given that this package of four PHYs seems to be rather unique, I think
> > we need Jie Luo to provide sufficient information so we can understand:
> > 
> > 1) this package of four PHYs itself
> 
> Yes, this chip package for all 4 PHYs itself, also including the PCSes
> and common package level modules such as GCC.
> 
> > 2) how this package is integrated into the SoC
> 
> the qca8084 is connected with SoC by PCSes.
> 
> > 
> > Specifically, what resets and clocks are controlled from within the
> > package's register space, which are external to the package
> > register space (and thus are provided by other IPs in the SoC).
> 
> All clocks and resets mentioned for qca8084 drive including package
> level and PCS & PHY clocks and resets from the qca8084 internal GCC
> modules register space,
> 
> > 
> > As I've said previously, the lack of DT example doesn't help to further
> > our understanding. The lack of details of what the package encompases
> > also doesn't help us understand the hardware.
> 
> Indeed, i will add the qca8084 DT example in the next patch set.
> BTW, i also replied your earlier comments by providing the DTS defined
> for the current qca8084 drive code.
> 
> hope you can have a better understanding with the provided DTS code in
> earlier reply of this email thread.
> > 
> > Unless we can gain that understanding, I feel that Jie Luo's patches
> > are effectively unreviewable and can't be accepted into mainline.
> >
Russell King (Oracle) Dec. 16, 2023, 4:09 p.m. UTC | #16 
On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
> 
> 
> On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
> > On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
> > > > The following is the chip package, the chip can work on the switch mode
> > > > like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> > > > MAC1-MAC4 directly;
> > > 
> > > Ah, that is new information, and has a big effect on the design.
> > 
> > This QCA8084 that's being proposed in these patches is not a PHY in
> > itself, but is a SoC. I came across this:
> > 
> >   https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
> 
> The chip mentioned in the link you mentioned is SoC, which is not the
> chip that the qca8084 driver work for.

So there's two chips called QCA8084 both produced by Qualcomm? I find
that hard to believe.
Andrew Lunn Dec. 16, 2023, 5:17 p.m. UTC | #17 
> Yes, Russell, i will add an example in the DT doc in the next patch set.
> The following is the device node used for the current qca8084 PHY
> code design.

If you look at Christians work, this would be expressed differently:

> mdio: mdio@90000 {
>     ethernet-phy-package@1 {
> 
>         compatible = "qca,qca8084-package";
> 
>         qcom,phy-work-mode = <2>;
>         clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
>                <&qca8k_nsscc NSS_CC_AHB_CLK>,
>                <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
>                <&qca8k_nsscc NSS_CC_TLMM_CLK>,
>                <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
>                <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
>                <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>,
>                <&qca8k_nsscc NSS_CC_MDIO_MASTER_AHB_CLK>,
>                <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>,
>                <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
>         clock-names = "apb_bridge",
>                 "ahb",
>                 "sec_ctrl_ahb",
>                 "tlmm",
>                 "tlmm_ahb",
>                 "cnoc_ahb",
>                 "mdio_ahb",
>                 "mdio_master_ahb",
>                 "srds0_sys",
>                 "srds1_sys";
>         resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>,
>                <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>,
>                <&qca8k_nsscc NSS_CC_DSP_ARES>;
>         reset-names = "srds0_sys",
>                       "srds1_sys";
>

All the properties above are common to the package as a whole.

Then follow the four individual PHYs, and the properties which are
specific to each one.

> 
>         ethernet-phy@0 {
>             compatible = "ethernet-phy-id004d.d180";
>             reg = <0>;
>             clocks = <qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>,
>             clock-names = <"gephy_sys">;
>             resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
>                      <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
>             reset-names = "gephy_sys", "gephy_soft";
>         };
>     
>     
>         ethernet-phy@1 {
>             compatible = "ethernet-phy-id004d.d180";
>             reg = <1>;
>             clocks = <qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>,
>             clock-names = <"gephy_sys">;
>             resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
>                      <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
>             reset-names = "gephy_sys", "gephy_soft";
>     
>         };
>     
>         ethernet-phy@2 {
>             compatible = "ethernet-phy-id004d.d180";
>             reg = <2>;
>             clocks = <qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>,
>             clock-names = <"gephy_sys">;
>             resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
>                      <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
>             reset-names = "gephy_sys", "gephy_soft";
>     
>         };
>     
>         ethernet-phy@3 {
>             compatible = "ethernet-phy-id004d.d180";
>             reg = <3>;
>             clocks = <qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>,
>             clock-names = <"gephy_sys">;
>             reset-names = "gephy_sys", "gephy_soft";
>         };
> };

	Andrew
Andrew Lunn Dec. 16, 2023, 5:30 p.m. UTC | #18 
> The following is the chip package, the chip can work on the switch mode
> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> MAC1-MAC4 directly; The chip can also work on the PHY mode, where PHY1-
> PHY4 is connected with PCS1 by 10g-qxgmii; Either switch mode or PHY mode,
> the PHY4 is optionally connected with PCS0 by SGMII, PCS0 and PCS1
> are connected with the SoC(IPQ platform) PCSes.

I don't really understand. Are you saying the hardware is actually :


+----------------------------------------------+
|          PCS1           PCS0                 |
|                                              |
|          MAC0           MAC5                 |
|           |              |                   |
|     +-----+--------------+-------------+     |
|     |                                  |     |
|     |                Switch            |     |
|     |                                  |     |
|     +-+---------+---------+---------+--+     |
|       |         |         |         |        |
|      MAC1      MAC2      MAC3      MAC4      |
|                                              |
|      PHY1      PHY2      PHY3      PHY4      |
+----------------------------------------------+

When in PHY mode, the switch is hard coded to map the 4 PCS1 channels
straight to MAC1-MAC4 and all switch functionality is disabled. But
then in switch mode, the switch can be controlled as a DSA switch? The
10G PCS1 is then a single 10G port, not 4x 2.5G?

Is there a product brief for this PHY? That might help us understand
this hardware?

    Andrew
Russell King (Oracle) Dec. 16, 2023, 7:08 p.m. UTC | #19 
On Sat, Dec 16, 2023 at 06:30:00PM +0100, Andrew Lunn wrote:
> > The following is the chip package, the chip can work on the switch mode
> > like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> > MAC1-MAC4 directly; The chip can also work on the PHY mode, where PHY1-
> > PHY4 is connected with PCS1 by 10g-qxgmii; Either switch mode or PHY mode,
> > the PHY4 is optionally connected with PCS0 by SGMII, PCS0 and PCS1
> > are connected with the SoC(IPQ platform) PCSes.
> 
> I don't really understand. Are you saying the hardware is actually :
> 
> 
> +----------------------------------------------+
> |          PCS1           PCS0                 |
> |                                              |
> |          MAC0           MAC5                 |
> |           |              |                   |
> |     +-----+--------------+-------------+     |
> |     |                                  |     |
> |     |                Switch            |     |
> |     |                                  |     |
> |     +-+---------+---------+---------+--+     |
> |       |         |         |         |        |
> |      MAC1      MAC2      MAC3      MAC4      |
> |                                              |
> |      PHY1      PHY2      PHY3      PHY4      |
> +----------------------------------------------+
> 
> When in PHY mode, the switch is hard coded to map the 4 PCS1 channels
> straight to MAC1-MAC4 and all switch functionality is disabled. But
> then in switch mode, the switch can be controlled as a DSA switch? The
> 10G PCS1 is then a single 10G port, not 4x 2.5G?
> 
> Is there a product brief for this PHY? That might help us understand
> this hardware?

Not even digikey give any clues what "QCA8084" is - they list it as
"unclassified" and give no documentation and no photo. Basically it
seems to be a super secret device.
Jie Luo Dec. 18, 2023, 3:01 a.m. UTC | #20 
On 12/17/2023 12:09 AM, Russell King (Oracle) wrote:
> On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
>>
>>
>> On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
>>> On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
>>>>> The following is the chip package, the chip can work on the switch mode
>>>>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>>>>> MAC1-MAC4 directly;
>>>>
>>>> Ah, that is new information, and has a big effect on the design.
>>>
>>> This QCA8084 that's being proposed in these patches is not a PHY in
>>> itself, but is a SoC. I came across this:
>>>
>>>    https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
>>
>> The chip mentioned in the link you mentioned is SoC, which is not the
>> chip that the qca8084 driver work for.
> 
> So there's two chips called QCA8084 both produced by Qualcomm? I find
> that hard to believe.
> 

The SoC mentioned in the link you provided is the APQ8084 that is 
introduced in the link below:
https://www.qualcomm.com/products/mobile/snapdragon/smartphones/snapdragon-8-series-mobile-platforms/snapdragon-processors-805

https://hwbot.org/hardware/processor/snapdragon_805_apq8084_pro__(8084_fusion_4.5)_2700mhz/

The driver here is for qca8084, which has the different prefix,
and qca8084 is the Ethernet CHIP like qca8081, but qca8084 is
multiple ports(quad-phy).
Jie Luo Dec. 18, 2023, 3:27 a.m. UTC | #21 
On 12/17/2023 1:30 AM, Andrew Lunn wrote:
>> The following is the chip package, the chip can work on the switch mode
>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>> MAC1-MAC4 directly; The chip can also work on the PHY mode, where PHY1-
>> PHY4 is connected with PCS1 by 10g-qxgmii; Either switch mode or PHY mode,
>> the PHY4 is optionally connected with PCS0 by SGMII, PCS0 and PCS1
>> are connected with the SoC(IPQ platform) PCSes.
> 
> I don't really understand. Are you saying the hardware is actually :
> 
> 
> +----------------------------------------------+
> |          PCS1           PCS0                 |
> |                                              |
> |          MAC0           MAC5                 |
> |           |              |                   |
> |     +-----+--------------+-------------+     |
> |     |                                  |     |
> |     |                Switch            |     |
> |     |                                  |     |
> |     +-+---------+---------+---------+--+     |
> |       |         |         |         |        |
> |      MAC1      MAC2      MAC3      MAC4      |
> |                                              |
> |      PHY1      PHY2      PHY3      PHY4      |
> +----------------------------------------------+
> 

Actually there are two CHIP types, ,let me explain to be more clear.

1. The diagram you describe is actually the switch work mode, which has
the different chip name called qca8386, the DSA driver and PHY driver
are used, since the general PHY driver can't work for the PHY here.

  +----------------------------------------------+
  | +-----+
  | | GCC |
  | +-----+  PCS1           PCS0                 |
  |                                              |
  |          MAC0           MAC5                 |
  |           |              |                   |
  |     +-----+--------------+-------------+     |
  |     |                                  |     |
  |     |                Switch            |     |
  |     |                                  |     |
  |     +-+---------+---------+---------+--+     |
  |       |         |         |         |        |
  |      MAC1      MAC2      MAC3      MAC4      |
  |                                              |
  |      PHY1      PHY2      PHY3      PHY4      |
  +----------------------------------------------+

2. The pure PHY chip called by qca8084 works on the PHY mode 10-qxgmii
on quad-phy, or the sgmii mode can be configured on PHY4 optionally.
The qca8084 is below, there is no MAC involved on qca8084.

  +----------------------------------------------+
  |          PCS1           PCS0                 |
  |                                              |
  |   +-----+
  |   | GCC |
  |   +-----+
  |                                              |
  |      PHY1      PHY2      PHY3      PHY4      |
  +----------------------------------------------+

On qca8386, the same qca8084 PHY is used, but the qca8084 PHY is
connected with internal MAC directly same as qca8337(qca8k dsa driver).

On both Ethernet chips qca8386 and qca8084, GCC block is same and
with the same clock controller driver that provides the clocks and
resets used by the qca8084 PHY driver and qca8386 DSA driver(leverage
the existed DSA driver qca8k.c).

> When in PHY mode, the switch is hard coded to map the 4 PCS1 channels
> straight to MAC1-MAC4 and all switch functionality is disabled. But
> then in switch mode, the switch can be controlled as a DSA switch? The
> 10G PCS1 is then a single 10G port, not 4x 2.5G?

For the qca8084 PHY chip, there is no MAC involved, the PHY is connected 
with the PCS with 10g-qxgmii, PHY4 is optional connected with sgmii.

For the qca8386 switch chip, it is controlled as DSA, the PCS is
connected with the SOC(such as IPQ5332) PCS.
> 
> Is there a product brief for this PHY? That might help us understand
> this hardware?
> 
Sorry, i also searched it on the internet and Qualcomm website, there is
no Doc found, the CHIP is developed recently 1-2 year before, the Doc is
not updated to the website.

>      Andrew
Jie Luo Dec. 18, 2023, 3:31 a.m. UTC | #22 
On 12/17/2023 3:08 AM, Russell King (Oracle) wrote:
> On Sat, Dec 16, 2023 at 06:30:00PM +0100, Andrew Lunn wrote:
>>> The following is the chip package, the chip can work on the switch mode
>>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>>> MAC1-MAC4 directly; The chip can also work on the PHY mode, where PHY1-
>>> PHY4 is connected with PCS1 by 10g-qxgmii; Either switch mode or PHY mode,
>>> the PHY4 is optionally connected with PCS0 by SGMII, PCS0 and PCS1
>>> are connected with the SoC(IPQ platform) PCSes.
>>
>> I don't really understand. Are you saying the hardware is actually :
>>
>>
>> +----------------------------------------------+
>> |          PCS1           PCS0                 |
>> |                                              |
>> |          MAC0           MAC5                 |
>> |           |              |                   |
>> |     +-----+--------------+-------------+     |
>> |     |                                  |     |
>> |     |                Switch            |     |
>> |     |                                  |     |
>> |     +-+---------+---------+---------+--+     |
>> |       |         |         |         |        |
>> |      MAC1      MAC2      MAC3      MAC4      |
>> |                                              |
>> |      PHY1      PHY2      PHY3      PHY4      |
>> +----------------------------------------------+
>>
>> When in PHY mode, the switch is hard coded to map the 4 PCS1 channels
>> straight to MAC1-MAC4 and all switch functionality is disabled. But
>> then in switch mode, the switch can be controlled as a DSA switch? The
>> 10G PCS1 is then a single 10G port, not 4x 2.5G?
>>
>> Is there a product brief for this PHY? That might help us understand
>> this hardware?
> 
> Not even digikey give any clues what "QCA8084" is - they list it as
> "unclassified" and give no documentation and no photo. Basically it
> seems to be a super secret device.
> 
Sorry for the confusion here, maybe the chip is developed recently,
which leads to the Doc or introduction is not released in time.
Jie Luo Dec. 18, 2023, 4:53 a.m. UTC | #23 
On 12/17/2023 1:17 AM, Andrew Lunn wrote:
>> Yes, Russell, i will add an example in the DT doc in the next patch set.
>> The following is the device node used for the current qca8084 PHY
>> code design.
> 
> If you look at Christians work, this would be expressed differently:
> 
>> mdio: mdio@90000 {
>>      ethernet-phy-package@1 {
>>
>>          compatible = "qca,qca8084-package";
>>
>>          qcom,phy-work-mode = <2>;
>>          clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>,
>>                 <&qca8k_nsscc NSS_CC_AHB_CLK>,
>>                 <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>,
>>                 <&qca8k_nsscc NSS_CC_TLMM_CLK>,
>>                 <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>,
>>                 <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>,
>>                 <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>,
>>                 <&qca8k_nsscc NSS_CC_MDIO_MASTER_AHB_CLK>,
>>                 <&qca8k_nsscc NSS_CC_SRDS0_SYS_CLK>,
>>                 <&qca8k_nsscc NSS_CC_SRDS1_SYS_CLK>;
>>          clock-names = "apb_bridge",
>>                  "ahb",
>>                  "sec_ctrl_ahb",
>>                  "tlmm",
>>                  "tlmm_ahb",
>>                  "cnoc_ahb",
>>                  "mdio_ahb",
>>                  "mdio_master_ahb",
>>                  "srds0_sys",
>>                  "srds1_sys";
>>          resets = <&qca8k_nsscc NSS_CC_SRDS0_SYS_ARES>,
>>                 <&qca8k_nsscc NSS_CC_SRDS1_SYS_ARES>,
>>                 <&qca8k_nsscc NSS_CC_DSP_ARES>;
>>          reset-names = "srds0_sys",
>>                        "srds1_sys";
>>
> 
> All the properties above are common to the package as a whole.
> 
> Then follow the four individual PHYs, and the properties which are
> specific to each one.

Thanks Andrew for the proposal.
For the pure PHY chip qca8084, there is no driver to parse the package
level device tree node for common clocks and resets.

For the common clocks and resets above, whether we can add a qca8084
common device tree node as the child node of MDIO bus node, and then
parse these common properties in the PHY probe function? since the DSA
driver is not enabled for the pure PHY chip.

> 
>>
>>          ethernet-phy@0 {
>>              compatible = "ethernet-phy-id004d.d180";
>>              reg = <0>;
>>              clocks = <qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>,
>>              clock-names = <"gephy_sys">;
>>              resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
>>                       <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
>>              reset-names = "gephy_sys", "gephy_soft";
>>          };
>>      
>>      
>>          ethernet-phy@1 {
>>              compatible = "ethernet-phy-id004d.d180";
>>              reg = <1>;
>>              clocks = <qca8k_nsscc NSS_CC_GEPHY1_SYS_CLK>,
>>              clock-names = <"gephy_sys">;
>>              resets = <&qca8k_nsscc NSS_CC_GEPHY1_SYS_ARES>,
>>                       <&qca8k_nsscc NSS_CC_GEPHY1_ARES>;
>>              reset-names = "gephy_sys", "gephy_soft";
>>      
>>          };
>>      
>>          ethernet-phy@2 {
>>              compatible = "ethernet-phy-id004d.d180";
>>              reg = <2>;
>>              clocks = <qca8k_nsscc NSS_CC_GEPHY2_SYS_CLK>,
>>              clock-names = <"gephy_sys">;
>>              resets = <&qca8k_nsscc NSS_CC_GEPHY2_SYS_ARES>,
>>                       <&qca8k_nsscc NSS_CC_GEPHY2_ARES>;
>>              reset-names = "gephy_sys", "gephy_soft";
>>      
>>          };
>>      
>>          ethernet-phy@3 {
>>              compatible = "ethernet-phy-id004d.d180";
>>              reg = <3>;
>>              clocks = <qca8k_nsscc NSS_CC_GEPHY3_SYS_CLK>,
>>              clock-names = <"gephy_sys">;
>>              reset-names = "gephy_sys", "gephy_soft";
>>          };
>> };
> 
> 	Andrew
Jie Luo Dec. 18, 2023, 5:22 a.m. UTC | #24 
On 12/17/2023 12:01 AM, Christian Marangi wrote:
> On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
>>
>>
>> On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
>>> On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
>>>>> The following is the chip package, the chip can work on the switch mode
>>>>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>>>>> MAC1-MAC4 directly;
>>>>
>>>> Ah, that is new information, and has a big effect on the design.
>>>
>>> This QCA8084 that's being proposed in these patches is not a PHY in
>>> itself, but is a SoC. I came across this:
>>>
>>>    https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
>>
>> The chip mentioned in the link you mentioned is SoC, which is not the
>> chip that the qca8084 driver work for.
>>
>> qca8084/qca8386 is just the Ethernet CHIP, not SoC, for the switch mode
>> qca8386, which is most like qca8337 the dsa drive qca8k.c is already in
>> upstream.
> 
> Hi,
> sorry for stepping in. I guess here there is a massive confusion with
> naming and using qca8k.
> 
> Since it seems the same name is used for PHY and for Switch stuff, I
> would add PHY and MAC prefix when referring to qca8064.

Hi Christian,
Welcome to join the discussion.

Sorry for the confusion, since the switch and PHY are the different
chips, the switch chip is named as qca8386, the pure PHY chip is named
as qca8084, the pure PHY chip qca8084 is connected with the SoC by PCS
working on 10g-qxgmii, the switch chip qca8386 is connected with SoC by
PCS working on sgmii.

> 
> With the previous message I was a bit confused by the use of qca8k and
> didn't know you were actually referring to the DSA driver.
> Interesting... this is for the upcoming WiFi 7 platoform right? (ipq9574)

For qca8386, it is the switch chip and leverages the qca8k DSA driver,
it is connected on the ipq5332 platform currently.

But for qca8084, it is the pure PHY chip, which works on 10g-qxgmii for
quad-phy, which is connected on the ipq9574.

> 
> All these discussion comes for the problem of using this PHY as an
> integrated PHY in the qca8386 switch and trying to select the mode in
> the PHY driver.

Yes, for qca8386, the PHY is integrated to switch, since same PHY needs
to work with qca8386 and qca8084, so the work mode needs to be
configured to distinguish the different work mode as the patch
<[PATCH v8 13/14] net: phy: at803x: configure qca8084 work mode>.

> 
> Considering you would use the same logic of the current DSA qca8k driver
> with the integrated PHY, the problem doesn't apply or a different
> implementation should be used (and actually handled later when the
> actual DSA code will come)

qca8386 has some differences from qca8337(qca8k dsa driver), qca8337
integrated PHY can work by the general PHY, but the PHY in qca8386 needs
the extra PHY driver, and the GCC clocks introduced.

> 
> I would expect in the integrated mode, the switch to handle the PHY (as
> it's done by qca8337) with the PHY defined in the switch node and qca8k
> handling the PHY registration. With the following implementation flags
> can be passed and PHY can be configured to integrated mode. (or virtual
> PHY ID can be used for such scope with dedicated functions in the PHY
> driver)
> 
Since there is the pure PHY chip qca8084 using the same PHY, which does
not enable the DSA driver.

So the PHY driver should be standalone, and the common clocks and resets
is better to put in the PHY driver, since these are the common configs
for the qca8386 and qca8084.

> With this in mind the entire integrated problem can put on hold and
> dropped to be later reimplemented when it's time. (assuming that all the
> prereq are already here and the very same implementation of qca8k will
> be used)

For the qca8386, it is true very same implementation of qca8k, besides
the 2.5G capability and GCC clock involved.

> 
> Anyway, I'm more or less the maintainer of the qca8k.c DSA driver and I
> would be more than happy to help you guys internally or externally on
> pushing and make this proceed further. (again assuming this is ipq9574
> stuff, it would be good to finally have proper DSA driver instead of
> leaving the thing unusable as it's the current situation with ipq8074)

Yes, the pure PHY chip qca8084 is connected on ipq9574 platform,
and the switch chip qca8386 is connected on ipq5332 platform.

Many thanks Christian again, will add you when pushing the qca8386 DSA
patches for upstream review.

> 
>>
>> i qca8084 chip package includes 4 PHYs, 2 PCSs and the common chip level
>> modules such as GCC and security control modules, all these modules are
>> located in the qca8084 chip package, since qca8084 works on PHY mode, so
>> the MACs are not used.
>>
>> qca8084 is connected with the SoC CHIP such as IPQ platform by PCS1
>> working on 10g-qxgmii mode and the fourth PHY can also optionally
>> be connected with the IPQ SoC PCS by sgmii mode, there is no more
>> interface on qca8084 to connect the external chips.
>>
>>> It's sounding like what we have here is some PHY IP that is integrated
>>> into a larger SoC, and the larger SoC needs to be configured so the
>>> PHY IP can work correctly.
>>
>> qca8084 is not a SoC, it is the Ethernet chip, in this qca8084 package,
>> there are GCC that is driving the PHY working on the various link speed.
>> that is the reason we need to do these package level common clocks and
>> resets initialization before probing PHY correctly.
>>
>>>
>>> Given that this package of four PHYs seems to be rather unique, I think
>>> we need Jie Luo to provide sufficient information so we can understand:
>>>
>>> 1) this package of four PHYs itself
>>
>> Yes, this chip package for all 4 PHYs itself, also including the PCSes
>> and common package level modules such as GCC.
>>
>>> 2) how this package is integrated into the SoC
>>
>> the qca8084 is connected with SoC by PCSes.
>>
>>>
>>> Specifically, what resets and clocks are controlled from within the
>>> package's register space, which are external to the package
>>> register space (and thus are provided by other IPs in the SoC).
>>
>> All clocks and resets mentioned for qca8084 drive including package
>> level and PCS & PHY clocks and resets from the qca8084 internal GCC
>> modules register space,
>>
>>>
>>> As I've said previously, the lack of DT example doesn't help to further
>>> our understanding. The lack of details of what the package encompases
>>> also doesn't help us understand the hardware.
>>
>> Indeed, i will add the qca8084 DT example in the next patch set.
>> BTW, i also replied your earlier comments by providing the DTS defined
>> for the current qca8084 drive code.
>>
>> hope you can have a better understanding with the provided DTS code in
>> earlier reply of this email thread.
>>>
>>> Unless we can gain that understanding, I feel that Jie Luo's patches
>>> are effectively unreviewable and can't be accepted into mainline.
>>>
>
Andrew Lunn Dec. 18, 2023, 9:34 a.m. UTC | #25 
> Thanks Andrew for the proposal.
> For the pure PHY chip qca8084, there is no driver to parse the package
> level device tree node for common clocks and resets.

So you still have not look at the work Christian is doing. You must
work together with Christian. This driver is not going to be accepted
unless you do.

> > >          ethernet-phy@0 {
> > >              compatible = "ethernet-phy-id004d.d180";
> > >              reg = <0>;
> > >              clocks = <qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>,
> > >              clock-names = <"gephy_sys">;
> > >              resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
> > >                       <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
> > >              reset-names = "gephy_sys", "gephy_soft";

Which of these properties exist for the Pure PHY device? Which exist
for the integrated switch? And by that, i mean which are actual pins
on the PHY device? We need the device tree binding to list which
properties are required for each use case.

	   Andrew
Jie Luo Dec. 19, 2023, 8:52 a.m. UTC | #26 
On 12/18/2023 5:34 PM, Andrew Lunn wrote:
>> Thanks Andrew for the proposal.
>> For the pure PHY chip qca8084, there is no driver to parse the package
>> level device tree node for common clocks and resets.
> 
> So you still have not look at the work Christian is doing. You must
> work together with Christian. This driver is not going to be accepted
> unless you do.
OK, Andrew, i am looking at Christian's patches on at803x.c, and i will
update qca8084 patches based on Christian's patch set.

> 
>>>>           ethernet-phy@0 {
>>>>               compatible = "ethernet-phy-id004d.d180";
>>>>               reg = <0>;
>>>>               clocks = <qca8k_nsscc NSS_CC_GEPHY0_SYS_CLK>,
>>>>               clock-names = <"gephy_sys">;
>>>>               resets = <&qca8k_nsscc NSS_CC_GEPHY0_SYS_ARES>,
>>>>                        <&qca8k_nsscc NSS_CC_GEPHY0_ARES>;
>>>>               reset-names = "gephy_sys", "gephy_soft";
> 
> Which of these properties exist for the Pure PHY device? Which exist
> for the integrated switch? And by that, i mean which are actual pins
> on the PHY device? We need the device tree binding to list which
> properties are required for each use case.
> 
> 	   Andrew

Hi Andrew,
For the clocks and resets listed here, only the clock "mdio_master_ahb"
is dedicated in qca8386, others are needed on the both chips qca8386
and qca8084.

Here is the DTS example for the clocks and resets working on the
devices, from the example below, we can get the dedicated clocks
and resets for each MDIO device and package level device.

The DTS properties in the "qcom,phy-common" should be initialized by
the first PHY probe function, and only being initialized one time.

phy0: ethernet-phy@0 { 

         compatible = "ethernet-phy-id004d.d180"; 

         reg = <1>;

         /* Package level configs, applicable on qca8386 and qca8081. */ 

         phy-common-config { 

                 qcom,phy-addr-fixup = <1 2 3 4 5 6 7>; 

                 qcom,phy-work-mode = <2>; 

                 clocks = <&qca8k_nsscc NSS_CC_APB_BRIDGE_CLK>, 

                        <&qca8k_nsscc NSS_CC_AHB_CLK>, 

                        <&qca8k_nsscc NSS_CC_SEC_CTRL_AHB_CLK>, 

                        <&qca8k_nsscc NSS_CC_TLMM_CLK>, 

                        <&qca8k_nsscc NSS_CC_TLMM_AHB_CLK>, 

                        <&qca8k_nsscc NSS_CC_CNOC_AHB_CLK>, 

                        <&qca8k_nsscc NSS_CC_MDIO_AHB_CLK>; 

 

                 clock-names = "apb_bridge", 

                         "ahb", 

                         "sec_ctrl_ahb", 

                         "tlmm", 

                         "tlmm_ahb", 

                         "cnoc_ahb", 

                         "mdio_ahb"; 

 

                 resets = <&qca8k_nsscc NSS_CC_DSP_ARES>; 

                 reset-names = "gephy_dsp"; 

 

                 ethernet-ports { 

                         /* clocks and resets for pcs0. */ 

                         pcs0 {
                                 clocks = <&qca8k_nsscc 
NSS_CC_SRDS0_SYS_CLK>;
                                 clock-names = "srds0_sys"; 

                                 resets = <&qca8k_nsscc 
NSS_CC_SRDS0_SYS_ARES>;
                                 reset-names = "srds0_sys"; 

                         };

                         /* clocks and resets for pcs1. */ 

                         pcs1 {
                                 clocks = <&qca8k_nsscc 
NSS_CC_SRDS1_SYS_CLK>;
                                 clock-names = "srds1_sys"; 

                                 resets = <&qca8k_nsscc 
NSS_CC_SRDS1_SYS_ARES>;
                                 reset-names = "srds1_sys"; 

                         };

			/* clocks and resets for first phy */
                         phy0 {
                                 clocks = <&qca8k_nsscc 
NSS_CC_GEPHY0_SYS_CLK>;
                                 clock-names = "gephy0_sys"; 

                                 resets = <&qca8k_nsscc 
NSS_CC_GEPHY0_SYS_ARES>,
                                        <&qca8k_nsscc 
NSS_CC_GEPHY0_ARES>;
                                 reset-names = "gephy0_sys", 

                                         "gephy0_soft"; 

                         }; 


			/* clocks and resets for second phy */ 

                         phy1 {
                                 clocks = <&qca8k_nsscc 
NSS_CC_GEPHY1_SYS_CLK>;
                                 clock-names = "gephy1_sys"; 

                                 resets = <&qca8k_nsscc 
NSS_CC_GEPHY1_SYS_ARES>,
                                        <&qca8k_nsscc 
NSS_CC_GEPHY1_ARES>;
                                 reset-names = "gephy1_sys", 

                                         "gephy1_soft"; 

                         }; 


			/* clocks and resets for third phy */ 

                         phy2 {
                                 clocks = <&qca8k_nsscc 
NSS_CC_GEPHY2_SYS_CLK>;
                                 clock-names = "gephy2_sys"; 

                                 resets = <&qca8k_nsscc 
NSS_CC_GEPHY2_SYS_ARES>,
                                        <&qca8k_nsscc 
NSS_CC_GEPHY2_ARES>;
                                 reset-names = "gephy2_sys", 

                                         "gephy2_soft"; 

                         }; 


			/* clocks and resets for fourth phy */ 

                         phy3 {
                                 clocks = <&qca8k_nsscc 
NSS_CC_GEPHY3_SYS_CLK>;
                                 clock-names = "gephy3_sys"; 

                                 resets = <&qca8k_nsscc 
NSS_CC_GEPHY3_SYS_ARES>,
                                        <&qca8k_nsscc 
NSS_CC_GEPHY3_ARES>;
                                 reset-names = "gephy3_sys", 

                                         "gephy3_soft"; 

                         }; 

 

                 }; 

         };

phy1: ethernet-phy@1 { 

         compatible = "ethernet-phy-id004d.d180"; 

         reg = <2>; 

};
 

phy2: ethernet-phy@2 { 

         compatible = "ethernet-phy-id004d.d180"; 

         reg = <3>; 

}; 

 

phy3: ethernet-phy@3 { 

         compatible = "ethernet-phy-id004d.d180"; 

         reg = <4>; 

};
Russell King (Oracle) Jan. 2, 2024, 9:57 a.m. UTC | #27 
On Mon, Dec 18, 2023 at 11:01:03AM +0800, Jie Luo wrote:
> 
> 
> On 12/17/2023 12:09 AM, Russell King (Oracle) wrote:
> > On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
> > > 
> > > 
> > > On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
> > > > On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
> > > > > > The following is the chip package, the chip can work on the switch mode
> > > > > > like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> > > > > > MAC1-MAC4 directly;
> > > > > 
> > > > > Ah, that is new information, and has a big effect on the design.
> > > > 
> > > > This QCA8084 that's being proposed in these patches is not a PHY in
> > > > itself, but is a SoC. I came across this:
> > > > 
> > > >    https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
> > > 
> > > The chip mentioned in the link you mentioned is SoC, which is not the
> > > chip that the qca8084 driver work for.
> > 
> > So there's two chips called QCA8084 both produced by Qualcomm? I find
> > that hard to believe.
> > 
> 
> The SoC mentioned in the link you provided is the APQ8084 that is introduced
> in the link below:
> https://www.qualcomm.com/products/mobile/snapdragon/smartphones/snapdragon-8-series-mobile-platforms/snapdragon-processors-805

So the one mentioned in the rt-rk article and a load of CVEs is _not_
QCA8084 but is APQ8084. Sounds like a lot of people are getting stuff
wrong - which is hardly surprising as there are people that seem to
_enjoy_ getting the technical details wrong. I haven't worked out if
it's intentional malace, or they're just fundamentally lazy individuals
who just like to screw with other people.

Sigh.
Christian Marangi Jan. 2, 2024, 10:08 a.m. UTC | #28 
On Tue, Jan 02, 2024 at 09:57:48AM +0000, Russell King (Oracle) wrote:
> On Mon, Dec 18, 2023 at 11:01:03AM +0800, Jie Luo wrote:
> > 
> > 
> > On 12/17/2023 12:09 AM, Russell King (Oracle) wrote:
> > > On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
> > > > 
> > > > 
> > > > On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
> > > > > On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
> > > > > > > The following is the chip package, the chip can work on the switch mode
> > > > > > > like the existed upstream code qca8k, where PHY1-PHY4 is connected with
> > > > > > > MAC1-MAC4 directly;
> > > > > > 
> > > > > > Ah, that is new information, and has a big effect on the design.
> > > > > 
> > > > > This QCA8084 that's being proposed in these patches is not a PHY in
> > > > > itself, but is a SoC. I came across this:
> > > > > 
> > > > >    https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
> > > > 
> > > > The chip mentioned in the link you mentioned is SoC, which is not the
> > > > chip that the qca8084 driver work for.
> > > 
> > > So there's two chips called QCA8084 both produced by Qualcomm? I find
> > > that hard to believe.
> > > 
> > 
> > The SoC mentioned in the link you provided is the APQ8084 that is introduced
> > in the link below:
> > https://www.qualcomm.com/products/mobile/snapdragon/smartphones/snapdragon-8-series-mobile-platforms/snapdragon-processors-805
> 
> So the one mentioned in the rt-rk article and a load of CVEs is _not_
> QCA8084 but is APQ8084. Sounds like a lot of people are getting stuff
> wrong - which is hardly surprising as there are people that seem to
> _enjoy_ getting the technical details wrong. I haven't worked out if
> it's intentional malace, or they're just fundamentally lazy individuals
> who just like to screw with other people.
> 
> Sigh.
>

Hoping to give some clarification with the naming.
- APQ8084 ("Application" SoC for 8084 family)
- IPQ8084 ("Internet" SoC version of APQ8084)
- QCA8084 (Integrated PHYs in the IPQ8084 SoC)

I guess?

Considering QCA8084 is only in in IPQ8084 SoC, the confusion with
referring to it is in the fact that it's all the same thing, and
everything related to APQ is also related to IPQ since they are the same
SoC with minor difference (different DSP, presence of NSS cores)

I can totally see sencente like "The IPQ8084 PHYs..." referencing the
QCA8084 PHY.

(Just to put how the naming is confusing there are PMIC with the
same exact naming)
Jie Luo Jan. 3, 2024, 1:27 p.m. UTC | #29 
On 1/2/2024 6:08 PM, Christian Marangi wrote:
> On Tue, Jan 02, 2024 at 09:57:48AM +0000, Russell King (Oracle) wrote:
>> On Mon, Dec 18, 2023 at 11:01:03AM +0800, Jie Luo wrote:
>>>
>>>
>>> On 12/17/2023 12:09 AM, Russell King (Oracle) wrote:
>>>> On Sat, Dec 16, 2023 at 10:41:28PM +0800, Jie Luo wrote:
>>>>>
>>>>>
>>>>> On 12/16/2023 9:51 PM, Russell King (Oracle) wrote:
>>>>>> On Sat, Dec 16, 2023 at 11:21:53AM +0100, Andrew Lunn wrote:
>>>>>>>> The following is the chip package, the chip can work on the switch mode
>>>>>>>> like the existed upstream code qca8k, where PHY1-PHY4 is connected with
>>>>>>>> MAC1-MAC4 directly;
>>>>>>>
>>>>>>> Ah, that is new information, and has a big effect on the design.
>>>>>>
>>>>>> This QCA8084 that's being proposed in these patches is not a PHY in
>>>>>> itself, but is a SoC. I came across this:
>>>>>>
>>>>>>     https://www.rt-rk.com/android-tv-solution-tv-in-smartphone-pantsstb-based-on-qualcomm-soc-design/
>>>>>
>>>>> The chip mentioned in the link you mentioned is SoC, which is not the
>>>>> chip that the qca8084 driver work for.
>>>>
>>>> So there's two chips called QCA8084 both produced by Qualcomm? I find
>>>> that hard to believe.
>>>>
>>>
>>> The SoC mentioned in the link you provided is the APQ8084 that is introduced
>>> in the link below:
>>> https://www.qualcomm.com/products/mobile/snapdragon/smartphones/snapdragon-8-series-mobile-platforms/snapdragon-processors-805
>>
>> So the one mentioned in the rt-rk article and a load of CVEs is _not_
>> QCA8084 but is APQ8084. Sounds like a lot of people are getting stuff
>> wrong - which is hardly surprising as there are people that seem to
>> _enjoy_ getting the technical details wrong. I haven't worked out if
>> it's intentional malace, or they're just fundamentally lazy individuals
>> who just like to screw with other people.
>>
>> Sigh.
>>
> 
> Hoping to give some clarification with the naming.
> - APQ8084 ("Application" SoC for 8084 family)
> - IPQ8084 ("Internet" SoC version of APQ8084)
> - QCA8084 (Integrated PHYs in the IPQ8084 SoC)
> 
> I guess? >
> Considering QCA8084 is only in in IPQ8084 SoC, the confusion with
> referring to it is in the fact that it's all the same thing, and
> everything related to APQ is also related to IPQ since they are the same
> SoC with minor difference (different DSP, presence of NSS cores)
> 
> I can totally see sencente like "The IPQ8084 PHYs..." referencing the
> QCA8084 PHY.
> 
> (Just to put how the naming is confusing there are PMIC with the
> same exact naming)
> 

There should be NO IPQ8084.
Yes, APQ8084 is the application SoC.
QCA8084 is the pure PHY chip which has quad-phy.

The prefix QCA is the Ethernet device, like qca8081(PHY chip), qca8386(
switch chip) and qca8084(PHY chip).
The prefix IPQ is the internet processor SoC, like ipq5332.
Andrew Lunn Jan. 3, 2024, 2:22 p.m. UTC | #30 
> Yes, APQ8084 is the application SoC.
> QCA8084 is the pure PHY chip which has quad-phy.

I think everybody agrees these are terrible names, being so close
together but being very different devices.

You have the issues of not giving clear explanations of your
hardware. This is resulting in a lot of wasted tome for everybody. So
please make your explanations very clear. I personally would avoid
using APQ8084 or QCA8084 on there own. Always say the application SoC
APQ8084, or the PHY chip QCA8084, or the switch embedded within the
application processor APQ8084, or the PHYs embedded within the
Application processor etc. This is particularly important when talking
about clocks and resets, since the PHYs embedded within the
application processor are likely to have different clocks and reset
controllers to the PHY chip QCA8084.

	Andrew
Jie Luo Jan. 4, 2024, 9:53 a.m. UTC | #31 
On 1/3/2024 10:22 PM, Andrew Lunn wrote:
>> Yes, APQ8084 is the application SoC.
>> QCA8084 is the pure PHY chip which has quad-phy.
> 
> I think everybody agrees these are terrible names, being so close
> together but being very different devices.
> 
> You have the issues of not giving clear explanations of your
> hardware. This is resulting in a lot of wasted tome for everybody. S
> please make your explanations very clear. I personally would avoid
> using APQ8084 or QCA8084 on there own. Always say the application SoC
> APQ8084, or the PHY chip QCA8084, or the switch embedded within the
> application processor APQ8084, or the PHYs embedded within the
> Application processor etc. This is particularly important when talking
> about clocks and resets, since the PHYs embedded within the
> application processor are likely to have different clocks and reset
> controllers to the PHY chip QCA8084.
> 
> 	Andrew

Let me explain it more.
APQ8084 is the Snapdragon SoC(for smart phone or other applicaiton)
according to the link below.
https://www.qualcomm.com/products/mobile/snapdragon/smartphones/snapdragon-8-series-mobile-platforms/snapdragon-processors-805.
which has nothing to do with QCA8084 or IPQ SoC we are discussing here.
let's remove out the APQ SoC from the discussion here.

1. For IPQ SoC series, there are only ipq4019, ipq5018, ipq6018,
ipq8074 documented in the current dt-bindings doc qcom,ipq4019-mdio.yaml
and ipq9574, ipq5332 that are being added by the MDIO patch, and one
more ipq8064 whose MDIO driver is mdio-ipq8064.c, on more others.

2. For qca8084(pure PHY chip), which is the quad-phy chip, which is just
    like qca8081 PHY(single port PHY), each port can be linked to maximum
    speed 2.5G.

    For qca8386(switch chip), which includes the same PHY CHIP as qca8084
    (4 physical ports and two CPU ports), qca8386 switch can work with
    the current qca8k.c DSA driver with the supplement patches.

    Both qca8084 and qca8386 includes same network clock controller(let's
    call it NSSCC, since this clock controller is located in the
    Ethernet chip qca8084 and qca8386), they have the same clock initial
    configuration sequence to initialize the Ethernet chip.

   The Ethernet chip qca8084 and qca8386 are only connected with IPQ SoC,
   Currently qca8084 is connected with IPQ SoC by 10G-QXGMII mode.
   the 4 PHYs of qca8386 are connected with the internal MAC of qca8386
   by GMII, the maximum speed is also 2.5G.
   The port4 of qca8084 or qca8386 is optionally be able to connected
   with IPQ SoC by sgmii.
Andrew Lunn Jan. 4, 2024, 1:57 p.m. UTC | #32 
> 1. For IPQ SoC series, there are only ipq4019, ipq5018, ipq6018,
> ipq8074 documented in the current dt-bindings doc qcom,ipq4019-mdio.yaml
> and ipq9574, ipq5332 that are being added by the MDIO patch, and one
> more ipq8064 whose MDIO driver is mdio-ipq8064.c, on more others.
> 
> 2. For qca8084(pure PHY chip), which is the quad-phy chip, which is just
>    like qca8081 PHY(single port PHY), each port can be linked to maximum
>    speed 2.5G.
> 
>    For qca8386(switch chip), which includes the same PHY CHIP as qca8084
>    (4 physical ports and two CPU ports), qca8386 switch can work with
>    the current qca8k.c DSA driver with the supplement patches.

Is the qca8386 purely a switch plus integrated PHYs? There is no CPU
on it? What is the management path? MDIO?

> 
>    Both qca8084 and qca8386 includes same network clock controller(let's
>    call it NSSCC, since this clock controller is located in the
>    Ethernet chip qca8084 and qca8386), they have the same clock initial
>    configuration sequence to initialize the Ethernet chip.

You said For "qca8084(pure PHY chip)". Here you just called it an
Ethernet chip? To me, and Ethernet chip is a MAC, Intel e1000e etc.
Do you now see how your explanations are confusing. Is it s pure PHY,
or is it an Ethernet chip?

O.K. Since we are getting nowhere at the moment, lets take just the
pure PHY chip, and ignore the rest for the moment.

For any pure PHY, there is generally one clock input, which might be a
crystal, or an actual clock. If you look at other DT bindings for
PHYs, it is only listed if the clock is expected to come from
somewhere else, like a SoC, and it needs to be turned on before the
PHY will work. And generally, a pure PHY has one defined clock
frequency input. If that is true, there is no need to specify the
clock. If multiple clock input frequencies are supported, then you do
need to specify the clock, so its possible to work out what frequency
it is using. How that clock input is then used internally in the PHY
is not described in DT, but the driver can set any dividers, PLLs
needed etc.

So, for the pure PHY chip, what is the pinout? Is there one clock
input? Or 4 clock inputs, one per PHY in the quad package? Typically,
where does this/these clocks come from? Is the frequency fixed by the
design, or are a number of input frequencies supported?

>   The Ethernet chip qca8084 and qca8386 are only connected with IPQ SoC,
>   Currently qca8084 is connected with IPQ SoC by 10G-QXGMII mode.
>   the 4 PHYs of qca8386 are connected with the internal MAC of qca8386
>   by GMII, the maximum speed is also 2.5G.
>   The port4 of qca8084 or qca8386 is optionally be able to connected
>   with IPQ SoC by sgmii.

To some extent, this does not matter. The DT binding and the driver
should not care what the pure PHY is connected to. It has standardised
ports, so in theory it could be connected to any vendors MAC.

Please be very careful with your wording. Because computers
instructions should be unambiguous, it does what it is told, we also
expect computer scientists to be unambiguous. Wording is very
important.

       Andrew
Jie Luo Jan. 5, 2024, 10:26 a.m. UTC | #33 
On 1/4/2024 9:57 PM, Andrew Lunn wrote:
>> 1. For IPQ SoC series, there are only ipq4019, ipq5018, ipq6018,
>> ipq8074 documented in the current dt-bindings doc qcom,ipq4019-mdio.yaml
>> and ipq9574, ipq5332 that are being added by the MDIO patch, and one
>> more ipq8064 whose MDIO driver is mdio-ipq8064.c, on more others.
>>
>> 2. For qca8084(pure PHY chip), which is the quad-phy chip, which is just
>>     like qca8081 PHY(single port PHY), each port can be linked to maximum
>>     speed 2.5G.
>>
>>     For qca8386(switch chip), which includes the same PHY CHIP as qca8084
>>     (4 physical ports and two CPU ports), qca8386 switch can work with
>>     the current qca8k.c DSA driver with the supplement patches.
> 
> Is the qca8386 purely a switch plus integrated PHYs? There is no CPU
> on it? What is the management path? MDIO?

Yes, qca8386 is a pure switch plus integrated PHYs(same PHY type as
qca8084), there is no CPU on qca8386, the management path is MDIO.
the access of switch register is by the multiple MDIO operations.

> 
>>
>>     Both qca8084 and qca8386 includes same network clock controller(let's
>>     call it NSSCC, since this clock controller is located in the
>>     Ethernet chip qca8084 and qca8386), they have the same clock initial
>>     configuration sequence to initialize the Ethernet chip.
> 
> You said For "qca8084(pure PHY chip)". Here you just called it an
> Ethernet chip? To me, and Ethernet chip is a MAC, Intel e1000e etc.
> Do you now see how your explanations are confusing. Is it s pure PHY,
> or is it an Ethernet chip?

My bad, sorry for this confusion.
qca8084 is a pure PHY, there is no MAC in qca8084.

> 
> O.K. Since we are getting nowhere at the moment, lets take just the
> pure PHY chip, and ignore the rest for the moment.
> 
> For any pure PHY, there is generally one clock input, which might be a
> crystal, or an actual clock. If you look at other DT bindings for
> PHYs, it is only listed if the clock is expected to come from
> somewhere else, like a SoC, and it needs to be turned on before the
> PHY will work. And generally, a pure PHY has one defined clock
> frequency input. If that is true, there is no need to specify the
> clock. If multiple clock input frequencies are supported, then you do
> need to specify the clock, so its possible to work out what frequency
> it is using. How that clock input is then used internally in the PHY
> is not described in DT, but the driver can set any dividers, PLLs
> needed etc.

Yes, Andrew, there is only one clock input to qca8084(same as qca8386),
this input clock rate is 50MHZ, which is from the output clock of CMN
PLL block that is configured by the MDIO bus driver patch under review.

In qca8084(same as qca8386), there is a clock controller, let's call it
as NSSCC, the logic of NSSCC is same as qualcomm GCC(located in SoC),
the NSSCC provides the clocks to the quad PHYs, the initial clocks for
quad PHYs need to be configured before PHY to work.

These clocks and resets are provided by the NSSCC provider driver,
i need to define these clocks and resets in DT to use it.

> 
> So, for the pure PHY chip, what is the pinout? Is there one clock
> input? Or 4 clock inputs, one per PHY in the quad package? Typically,
> where does this/these clocks come from? Is the frequency fixed by the
> design, or are a number of input frequencies supported?

There is one 50M clock input for qca8084(same as qca8386), the input
clock is generated from the CMN PLL block that is configured by MDIO
driver patch of mdio-ipq4019.c.
The frequency of input clock is fixed to 50MHZ.

> 
>>    The Ethernet chip qca8084 and qca8386 are only connected with IPQ SoC,
>>    Currently qca8084 is connected with IPQ SoC by 10G-QXGMII mode.
>>    the 4 PHYs of qca8386 are connected with the internal MAC of qca8386
>>    by GMII, the maximum speed is also 2.5G.
>>    The port4 of qca8084 or qca8386 is optionally be able to connected
>>    with IPQ SoC by sgmii.
> 
> To some extent, this does not matter. The DT binding and the driver
> should not care what the pure PHY is connected to. It has standardised
> ports, so in theory it could be connected to any vendors MAC.

Yes, it can be connected with any vendors MAC with the interface mode
supported.

> 
> Please be very careful with your wording. Because computers
> instructions should be unambiguous, it does what it is told, we also
> expect computer scientists to be unambiguous. Wording is very
> important.
> 
>         Andrew
Got it. Thanks Andrew for the comments and suggestions.
Andrew Lunn Jan. 5, 2024, 1:37 p.m. UTC | #34 
> > O.K. Since we are getting nowhere at the moment, lets take just the
> > pure PHY chip, and ignore the rest for the moment.
> > 
> > For any pure PHY, there is generally one clock input, which might be a
> > crystal, or an actual clock. If you look at other DT bindings for
> > PHYs, it is only listed if the clock is expected to come from
> > somewhere else, like a SoC, and it needs to be turned on before the
> > PHY will work. And generally, a pure PHY has one defined clock
> > frequency input. If that is true, there is no need to specify the
> > clock. If multiple clock input frequencies are supported, then you do
> > need to specify the clock, so its possible to work out what frequency
> > it is using. How that clock input is then used internally in the PHY
> > is not described in DT, but the driver can set any dividers, PLLs
> > needed etc.
> 
> Yes, Andrew, there is only one clock input to qca8084(same as qca8386),
> this input clock rate is 50MHZ, which is from the output clock of CMN
> PLL block that is configured by the MDIO bus driver patch under review.

Lets concentrate on the pure PHY. All it sees is a clock. It does not
care where it come from. All you need in the device tree for the pure
PHY is a clock consumer.

There is one clock input, so its shared by all four instances in the
pure PHY package. So you need to use Christians code which extends the
PHY DT bindings to allow DT properties for a package of PHYs.

What about resets. Is there one reset pin for the pure PHY package, or
one per PHY?

Go find Christians code, understand it, and propose a DT binding for
the pure PHY. Include the clock provider and the reset
provider. Forget about the MDIO controller, and the PHY integrated
into the switch, etc. Baby steps...

> In qca8084(same as qca8386), there is a clock controller, let's call it
> as NSSCC, the logic of NSSCC is same as qualcomm GCC(located in SoC),
> the NSSCC provides the clocks to the quad PHYs, the initial clocks for
> quad PHYs need to be configured before PHY to work.

You said above, there is one clock input to the qca8084. Here you use
the word clocks, plural. Is there one clock, or multiple clocks?

    Andrew
Jie Luo Jan. 8, 2024, 8:27 a.m. UTC | #35 
On 1/5/2024 9:37 PM, Andrew Lunn wrote:
>>> O.K. Since we are getting nowhere at the moment, lets take just the
>>> pure PHY chip, and ignore the rest for the moment.
>>>
>>> For any pure PHY, there is generally one clock input, which might be a
>>> crystal, or an actual clock. If you look at other DT bindings for
>>> PHYs, it is only listed if the clock is expected to come from
>>> somewhere else, like a SoC, and it needs to be turned on before the
>>> PHY will work. And generally, a pure PHY has one defined clock
>>> frequency input. If that is true, there is no need to specify the
>>> clock. If multiple clock input frequencies are supported, then you do
>>> need to specify the clock, so its possible to work out what frequency
>>> it is using. How that clock input is then used internally in the PHY
>>> is not described in DT, but the driver can set any dividers, PLLs
>>> needed etc.
>>
>> Yes, Andrew, there is only one clock input to qca8084(same as qca8386),
>> this input clock rate is 50MHZ, which is from the output clock of CMN
>> PLL block that is configured by the MDIO bus driver patch under review.
> 
> Lets concentrate on the pure PHY. All it sees is a clock. It does not
> care where it come from. All you need in the device tree for the pure
> PHY is a clock consumer.
Yes.

> 
> There is one clock input, so its shared by all four instances in the
> pure PHY package. So you need to use Christians code which extends the
> PHY DT bindings to allow DT properties for a package of PHYs.

OK, will

> 
> What about resets. Is there one reset pin for the pure PHY package, or
> one per PHY?

There is only one GPIO hardware reset PIN for the chip qca8084 including
all 4 PHYs.

> 
> Go find Christians code, understand it, and propose a DT binding for
> the pure PHY. Include the clock provider and the reset
> provider. Forget about the MDIO controller, and the PHY integrated
> into the switch, etc. Baby steps...

Thanks Andrew for pointing me the Christians code, i will keep the
driver of qca8084 synced with Christian's code before pushing the
next patch set.

> 
>> In qca8084(same as qca8386), there is a clock controller, let's call it
>> as NSSCC, the logic of NSSCC is same as qualcomm GCC(located in SoC),
>> the NSSCC provides the clocks to the quad PHYs, the initial clocks for
>> quad PHYs need to be configured before PHY to work.
> 
> You said above, there is one clock input to the qca8084. Here you use
> the word clocks, plural. Is there one clock, or multiple clocks?
> 
>      Andrew

Yes, Andrew, it is multiple clocks.
These multiple clocks are generated(PLL, divider) and used internally by
qca8084 CHIP, these clocks are generated by the clock controller of
qca8084, let's call the clock controller of qca8084 as NSSCC provider,
which generates the clocks to the PHYs, this NSSCC is located in
qca8084.

The only one input clock of qca8084 is the clock source of the chip
qca8084, which is fixed to 50MHZ.

The NSSCC of qca8084 generates different clock rates for the different
link speed of the PHY, which is the internal block of qca8084.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/qca,ar803x.yaml b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
index 3acd09f0da86..febff039a44f 100644
--- a/Documentation/devicetree/bindings/net/qca,ar803x.yaml
+++ b/Documentation/devicetree/bindings/net/qca,ar803x.yaml
@@ -14,9 +14,6 @@  maintainers:
 description: |
   Bindings for Qualcomm Atheros AR803x PHYs
 
-allOf:
-  - $ref: ethernet-phy.yaml#
-
 properties:
   qca,clk-out-frequency:
     description: Clock output frequency in Hertz.
@@ -85,6 +82,161 @@  properties:
     $ref: /schemas/regulator/regulator.yaml
     unevaluatedProperties: false
 
+  qcom,phy-addr-fixup:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description:
+      MDIO address for 4 PHY devices and 3 PCS devices
+
+  qcom,phy-work-mode:
+    description: PHY device work mode.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+
+  clocks:
+    items:
+      - description: APB bridge clock
+      - description: AHB clock
+      - description: Security control clock
+      - description: TLMM clock
+      - description: TLMM AHB clock
+      - description: CNOC AHB clock
+      - description: MDIO AHB clock
+      - description: MDIO master AHB clock
+      - description: PCS0 system clock
+      - description: PCS1 system clock
+      - description: EPHY0 system clock
+      - description: EPHY1 system clock
+      - description: EPHY2 system clock
+      - description: EPHY3 system clock
+    description: PHY initial common clock configs
+
+  clock-names:
+    items:
+      - const: apb_bridge
+      - const: ahb
+      - const: sec_ctrl_ahb
+      - const: tlmm
+      - const: tlmm_ahb
+      - const: cnoc_ahb
+      - const: mdio_ahb
+      - const: mdio_master_ahb
+      - const: srds0_sys
+      - const: srds1_sys
+      - const: gephy0_sys
+      - const: gephy1_sys
+      - const: gephy2_sys
+      - const: gephy3_sys
+
+  resets:
+    items:
+      - description: PCS0 system reset
+      - description: PCS1 system reset
+      - description: EPHY0 system reset
+      - description: EPHY1 system reset
+      - description: EPHY2 system reset
+      - description: EPHY3 system reset
+      - description: EPHY0 software reset
+      - description: EPHY1 software reset
+      - description: EPHY2 software reset
+      - description: EPHY3 software reset
+      - description: Ethernet DSP reset
+    description: PHY initial common reset configs
+
+  reset-names:
+    items:
+      - const: srds0_sys
+      - const: srds1_sys
+      - const: gephy0_sys
+      - const: gephy1_sys
+      - const: gephy2_sys
+      - const: gephy3_sys
+      - const: gephy0_soft
+      - const: gephy1_soft
+      - const: gephy2_soft
+      - const: gephy3_soft
+      - const: gephy_dsp
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - ethernet-phy-id004d.d180
+    then:
+      properties:
+        clocks:
+          items:
+            - description: APB bridge clock
+            - description: AHB clock
+            - description: Security control clock
+            - description: TLMM clock
+            - description: TLMM AHB clock
+            - description: CNOC AHB clock
+            - description: MDIO AHB clock
+            - description: MDIO master AHB clock
+            - description: PCS0 system clock
+            - description: PCS1 system clock
+            - description: EPHY0 system clock
+            - description: EPHY1 system clock
+            - description: EPHY2 system clock
+            - description: EPHY3 system clock
+        clock-names:
+          items:
+            - const: apb_bridge
+            - const: ahb
+            - const: sec_ctrl_ahb
+            - const: tlmm
+            - const: tlmm_ahb
+            - const: cnoc_ahb
+            - const: mdio_ahb
+            - const: mdio_master_ahb
+            - const: srds0_sys
+            - const: srds1_sys
+            - const: gephy0_sys
+            - const: gephy1_sys
+            - const: gephy2_sys
+            - const: gephy3_sys
+        resets:
+          items:
+            - description: PCS0 system reset
+            - description: PCS1 system reset
+            - description: EPHY0 system reset
+            - description: EPHY1 system reset
+            - description: EPHY2 system reset
+            - description: EPHY3 system reset
+            - description: EPHY0 software reset
+            - description: EPHY1 software reset
+            - description: EPHY2 software reset
+            - description: EPHY3 software reset
+            - description: Ethernet DSP reset
+        reset-names:
+          items:
+            - const: srds0_sys
+            - const: srds1_sys
+            - const: gephy0_sys
+            - const: gephy1_sys
+            - const: gephy2_sys
+            - const: gephy3_sys
+            - const: gephy0_soft
+            - const: gephy1_soft
+            - const: gephy2_soft
+            - const: gephy3_soft
+            - const: gephy_dsp
+      required:
+        - qcom,phy-addr-fixup
+        - qcom,phy-work-mode
+        - clocks
+        - clock-names
+        - resets
+        - reset-names
+    else:
+      properties:
+        qcom,phy-addr-fixup: false
+        qcom,phy-work-mode: false
+
 unevaluatedProperties: false
 
 examples: