[v4,net-next,03/13] dt-bindings: net: add bindings for NETC blocks control

Commit Message

Wei Fang Oct. 22, 2024, 5:52 a.m. UTC

Add bindings for NXP NETC blocks control. Usually, NETC has 2 blocks of
64KB registers, integrated endpoint register block (IERB) and privileged
register block (PRB). IERB is used for pre-boot initialization for all
NETC devices, such as ENETC, Timer, EMDIO and so on. And PRB controls
global reset and global error handling for NETC. Moreover, for the i.MX
platform, there is also a NETCMIX block for link configuration, such as
MII protocol, PCS protocol, etc.

Signed-off-by: Wei Fang <[e-mail]>
---
v2 changes:
1. Rephrase the commit message.
2. Change unevaluatedProperties to additionalProperties.
3. Remove the useless lables from examples.
v3 changes:
1. Remove the items from clocks and clock-names, add maxItems to clocks
and rename the clock.
v4 changes:
1. Reorder the required properties.
2. Add assigned-clocks, assigned-clock-parents and assigned-clock-rates.
---
 .../bindings/net/nxp,netc-blk-ctrl.yaml       | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml

Comments

Frank Li Oct. 22, 2024, 4:17 p.m. UTC | #1

On Tue, Oct 22, 2024 at 01:52:13PM +0800, Wei Fang wrote:
> Add bindings for NXP NETC blocks control. Usually, NETC has 2 blocks of
> 64KB registers, integrated endpoint register block (IERB) and privileged
> register block (PRB). IERB is used for pre-boot initialization for all
> NETC devices, such as ENETC, Timer, EMDIO and so on. And PRB controls
> global reset and global error handling for NETC. Moreover, for the i.MX
> platform, there is also a NETCMIX block for link configuration, such as
> MII protocol, PCS protocol, etc.
>
> Signed-off-by: Wei Fang <[e-mail]>
> ---
> v2 changes:
> 1. Rephrase the commit message.
> 2. Change unevaluatedProperties to additionalProperties.
> 3. Remove the useless lables from examples.
> v3 changes:
> 1. Remove the items from clocks and clock-names, add maxItems to clocks
> and rename the clock.
> v4 changes:
> 1. Reorder the required properties.
> 2. Add assigned-clocks, assigned-clock-parents and assigned-clock-rates.
> ---
>  .../bindings/net/nxp,netc-blk-ctrl.yaml       | 111 ++++++++++++++++++
>  1 file changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> new file mode 100644
> index 000000000000..0b7fd2c5e0d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,netc-blk-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NETC Blocks Control
> +
> +description:
> +  Usually, NETC has 2 blocks of 64KB registers, integrated endpoint register
> +  block (IERB) and privileged register block (PRB). IERB is used for pre-boot
> +  initialization for all NETC devices, such as ENETC, Timer, EMIDO and so on.
> +  And PRB controls global reset and global error handling for NETC. Moreover,
> +  for the i.MX platform, there is also a NETCMIX block for link configuration,
> +  such as MII protocol, PCS protocol, etc.
> +
> +maintainers:
> +  - Wei Fang <[e-mail]>
> +  - Clark Wang <[e-mail]>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,imx95-netc-blk-ctrl
> +
> +  reg:
> +    minItems: 2
> +    maxItems: 3
> +
> +  reg-names:
> +    minItems: 2
> +    items:
> +      - const: ierb
> +      - const: prb
> +      - const: netcmix
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 2
> +
> +  ranges: true
> +  assigned-clocks: true
> +  assigned-clock-parents: true
> +  assigned-clock-rates: true

I am not sure if it necessary. But if add restriction, it should be

assigned-clocks:
  maxItems: 2

> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: ipg
> +
> +  power-domains:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^pcie@[0-9a-f]+$":
> +    $ref: /schemas/pci/host-generic-pci.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        netc-blk-ctrl@4cde0000 {
> +            compatible = "nxp,imx95-netc-blk-ctrl";
> +            reg = <0x0 0x4cde0000 0x0 0x10000>,
> +                  <0x0 0x4cdf0000 0x0 0x10000>,
> +                  <0x0 0x4c81000c 0x0 0x18>;
> +            reg-names = "ierb", "prb", "netcmix";
> +            #address-cells = <2>;
> +            #size-cells = <2>;
> +            ranges;
> +            assigned-clocks = <&scmi_clk 98>, <&scmi_clk 102>;
> +            assigned-clock-parents = <&scmi_clk 12>, <&scmi_clk 6>;
> +            assigned-clock-rates = <666666666>, <250000000>;
> +            clocks = <&scmi_clk 98>;
> +            clock-names = "ipg";
> +            power-domains = <&scmi_devpd 18>;
> +
> +            pcie@4cb00000 {
> +                compatible = "pci-host-ecam-generic";
> +                reg = <0x0 0x4cb00000 0x0 0x100000>;
> +                #address-cells = <3>;
> +                #size-cells = <2>;
> +                device_type = "pci";
> +                bus-range = <0x1 0x1>;
> +                ranges = <0x82000000 0x0 0x4cce0000  0x0 0x4cce0000  0x0 0x20000
> +                          0xc2000000 0x0 0x4cd10000  0x0 0x4cd10000  0x0 0x10000>;
> +
> +                mdio@0,0 {
> +                    compatible = "pci1131,ee00";
> +                    reg = <0x010000 0 0 0 0>;
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                };
> +            };
> +        };
> +    };
> --
> 2.34.1
>

Wei Fang Oct. 23, 2024, 1:46 a.m. UTC | #2

> -----Original Message-----
> From: Frank Li <[e-mail]>
> Sent: 2024年10月23日 0:18
> To: Wei Fang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Vladimir Oltean <[e-mail]>; Claudiu
> Manoil <[e-mail]>; Clark Wang <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 net-next 03/13] dt-bindings: net: add bindings for NETC
> blocks control
> 
> On Tue, Oct 22, 2024 at 01:52:13PM +0800, Wei Fang wrote:
> > Add bindings for NXP NETC blocks control. Usually, NETC has 2 blocks
> > of 64KB registers, integrated endpoint register block (IERB) and
> > privileged register block (PRB). IERB is used for pre-boot
> > initialization for all NETC devices, such as ENETC, Timer, EMDIO and
> > so on. And PRB controls global reset and global error handling for
> > NETC. Moreover, for the i.MX platform, there is also a NETCMIX block
> > for link configuration, such as MII protocol, PCS protocol, etc.
> >
> > Signed-off-by: Wei Fang <[e-mail]>
> > ---
> > v2 changes:
> > 1. Rephrase the commit message.
> > 2. Change unevaluatedProperties to additionalProperties.
> > 3. Remove the useless lables from examples.
> > v3 changes:
> > 1. Remove the items from clocks and clock-names, add maxItems to
> > clocks and rename the clock.
> > v4 changes:
> > 1. Reorder the required properties.
> > 2. Add assigned-clocks, assigned-clock-parents and assigned-clock-rates.
> > ---
> >  .../bindings/net/nxp,netc-blk-ctrl.yaml       | 111 ++++++++++++++++++
> >  1 file changed, 111 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> > b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> > new file mode 100644
> > index 000000000000..0b7fd2c5e0d8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> > @@ -0,0 +1,111 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,netc-blk-ctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NETC Blocks Control
> > +
> > +description:
> > +  Usually, NETC has 2 blocks of 64KB registers, integrated endpoint
> > +register
> > +  block (IERB) and privileged register block (PRB). IERB is used for
> > +pre-boot
> > +  initialization for all NETC devices, such as ENETC, Timer, EMIDO and so on.
> > +  And PRB controls global reset and global error handling for NETC.
> > +Moreover,
> > +  for the i.MX platform, there is also a NETCMIX block for link
> > +configuration,
> > +  such as MII protocol, PCS protocol, etc.
> > +
> > +maintainers:
> > +  - Wei Fang <[e-mail]>
> > +  - Clark Wang <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nxp,imx95-netc-blk-ctrl
> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 3
> > +
> > +  reg-names:
> > +    minItems: 2
> > +    items:
> > +      - const: ierb
> > +      - const: prb
> > +      - const: netcmix
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 2
> > +
> > +  ranges: true
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
> 
> I am not sure if it necessary. But if add restriction, it should be
> 
> assigned-clocks:
>   maxItems: 2
> 

There is no need to add restrictions here, different SoCs have different
clocks that need to be configured. For example, the upcoming SoC has
more clocks to be configured.

> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: ipg
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +patternProperties:
> > +  "^pcie@[0-9a-f]+$":
> > +    $ref: /schemas/pci/host-generic-pci.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        netc-blk-ctrl@4cde0000 {
> > +            compatible = "nxp,imx95-netc-blk-ctrl";
> > +            reg = <0x0 0x4cde0000 0x0 0x10000>,
> > +                  <0x0 0x4cdf0000 0x0 0x10000>,
> > +                  <0x0 0x4c81000c 0x0 0x18>;
> > +            reg-names = "ierb", "prb", "netcmix";
> > +            #address-cells = <2>;
> > +            #size-cells = <2>;
> > +            ranges;
> > +            assigned-clocks = <&scmi_clk 98>, <&scmi_clk 102>;
> > +            assigned-clock-parents = <&scmi_clk 12>, <&scmi_clk 6>;
> > +            assigned-clock-rates = <666666666>, <250000000>;
> > +            clocks = <&scmi_clk 98>;
> > +            clock-names = "ipg";
> > +            power-domains = <&scmi_devpd 18>;
> > +
> > +            pcie@4cb00000 {
> > +                compatible = "pci-host-ecam-generic";
> > +                reg = <0x0 0x4cb00000 0x0 0x100000>;
> > +                #address-cells = <3>;
> > +                #size-cells = <2>;
> > +                device_type = "pci";
> > +                bus-range = <0x1 0x1>;
> > +                ranges = <0x82000000 0x0 0x4cce0000  0x0
> 0x4cce0000  0x0 0x20000
> > +                          0xc2000000 0x0 0x4cd10000  0x0
> 0x4cd10000
> > + 0x0 0x10000>;
> > +
> > +                mdio@0,0 {
> > +                    compatible = "pci1131,ee00";
> > +                    reg = <0x010000 0 0 0 0>;
> > +                    #address-cells = <1>;
> > +                    #size-cells = <0>;
> > +                };
> > +            };
> > +        };
> > +    };
> > --
> > 2.34.1
> >

Krzysztof Kozlowski Oct. 23, 2024, 6:56 a.m. UTC | #3

On Tue, Oct 22, 2024 at 01:52:13PM +0800, Wei Fang wrote:
> Add bindings for NXP NETC blocks control. Usually, NETC has 2 blocks of
> 64KB registers, integrated endpoint register block (IERB) and privileged
> register block (PRB). IERB is used for pre-boot initialization for all
> NETC devices, such as ENETC, Timer, EMDIO and so on. And PRB controls
> global reset and global error handling for NETC. Moreover, for the i.MX
> platform, there is also a NETCMIX block for link configuration, such as
> MII protocol, PCS protocol, etc.
> 
> Signed-off-by: Wei Fang <[e-mail]>
> ---
> v2 changes:
> 1. Rephrase the commit message.
> 2. Change unevaluatedProperties to additionalProperties.
> 3. Remove the useless lables from examples.
> v3 changes:
> 1. Remove the items from clocks and clock-names, add maxItems to clocks
> and rename the clock.
> v4 changes:
> 1. Reorder the required properties.
> 2. Add assigned-clocks, assigned-clock-parents and assigned-clock-rates.
> ---
>  .../bindings/net/nxp,netc-blk-ctrl.yaml       | 111 ++++++++++++++++++
>  1 file changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> new file mode 100644
> index 000000000000..0b7fd2c5e0d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/nxp,netc-blk-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NETC Blocks Control
> +
> +description:
> +  Usually, NETC has 2 blocks of 64KB registers, integrated endpoint register
> +  block (IERB) and privileged register block (PRB). IERB is used for pre-boot
> +  initialization for all NETC devices, such as ENETC, Timer, EMIDO and so on.
> +  And PRB controls global reset and global error handling for NETC. Moreover,
> +  for the i.MX platform, there is also a NETCMIX block for link configuration,
> +  such as MII protocol, PCS protocol, etc.
> +
> +maintainers:
> +  - Wei Fang <[e-mail]>
> +  - Clark Wang <[e-mail]>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,imx95-netc-blk-ctrl
> +
> +  reg:
> +    minItems: 2
> +    maxItems: 3

You have one device, why this is flexible? Device either has exactly 2
or exactly 3 IO spaces, not both depending on the context.

> +
> +  reg-names:
> +    minItems: 2
> +    items:
> +      - const: ierb
> +      - const: prb
> +      - const: netcmix
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 2
> +
> +  ranges: true
> +  assigned-clocks: true
> +  assigned-clock-parents: true
> +  assigned-clock-rates: true

Drop these three.

> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: ipg
> +
> +  power-domains:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^pcie@[0-9a-f]+$":
> +    $ref: /schemas/pci/host-generic-pci.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        netc-blk-ctrl@4cde0000 {

system-controller? Don't use compatible as node name.

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Best regards,
Krzysztof

Wei Fang Oct. 23, 2024, 8:18 a.m. UTC | #4

> > +maintainers:
> > +  - Wei Fang <[e-mail]>
> > +  - Clark Wang <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nxp,imx95-netc-blk-ctrl
> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 3
> 
> You have one device, why this is flexible? Device either has exactly 2
> or exactly 3 IO spaces, not both depending on the context.
> 

There are three register blocks, IERB and PRB are inside NETC IP, but NETCMIX
is outside NETC. There are dependencies between these three blocks, so it is
better to configure them in one driver. But for other platforms like S32, it does
not have NETCMIX, so NETCMIX is optional.

> > +
> > +  reg-names:
> > +    minItems: 2
> > +    items:
> > +      - const: ierb
> > +      - const: prb
> > +      - const: netcmix
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 2
> > +
> > +  ranges: true
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
> 
> Drop these three.
> 

Okay, I will drop them. Thanks.

> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: ipg
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +patternProperties:
> > +  "^pcie@[0-9a-f]+$":
> > +    $ref: /schemas/pci/host-generic-pci.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        netc-blk-ctrl@4cde0000 {
> 
> system-controller? Don't use compatible as node name.
> 

netc-blk-ctrl provides pre-configuration and warm reset services for the entire NETC
IP, so system-controller sounds good.

> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevicetr
> ee-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.
> html%23generic-names-recommendation&data=05%7C02%7Cwei.fang%40nx
> p.com%7C35715ef05b824c5d479f08dcf32fd256%7C686ea1d3bc2b4c6fa92cd
> 99c5c301635%7C0%7C0%7C638652633944352532%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C0%7C%7C%7C&sdata=unWcE1OaH2Id%2FEny9UFAUH%2F5Xablg
> PM0Yj4Br2jfQuI%3D&reserved=0
> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski Oct. 23, 2024, 8:55 a.m. UTC | #5

On 23/10/2024 10:18, Wei Fang wrote:
>>> +maintainers:
>>> +  - Wei Fang <[e-mail]>
>>> +  - Clark Wang <[e-mail]>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - nxp,imx95-netc-blk-ctrl
>>> +
>>> +  reg:
>>> +    minItems: 2
>>> +    maxItems: 3
>>
>> You have one device, why this is flexible? Device either has exactly 2
>> or exactly 3 IO spaces, not both depending on the context.
>>
> 
> There are three register blocks, IERB and PRB are inside NETC IP, but NETCMIX
> is outside NETC. There are dependencies between these three blocks, so it is
> better to configure them in one driver. But for other platforms like S32, it does
> not have NETCMIX, so NETCMIX is optional.

But how s32 is related here? That's a different device.

Best regards,
Krzysztof

Wei Fang Oct. 23, 2024, 10:03 a.m. UTC | #6

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 2024年10月23日 16:56
> To: Wei Fang <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Vladimir Oltean <[e-mail]>; Claudiu
> Manoil <[e-mail]>; Clark Wang <[e-mail]>;
> Frank Li <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v4 net-next 03/13] dt-bindings: net: add bindings for NETC
> blocks control
> 
> On 23/10/2024 10:18, Wei Fang wrote:
> >>> +maintainers:
> >>> +  - Wei Fang <[e-mail]>
> >>> +  - Clark Wang <[e-mail]>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - nxp,imx95-netc-blk-ctrl
> >>> +
> >>> +  reg:
> >>> +    minItems: 2
> >>> +    maxItems: 3
> >>
> >> You have one device, why this is flexible? Device either has exactly
> >> 2 or exactly 3 IO spaces, not both depending on the context.
> >>
> >
> > There are three register blocks, IERB and PRB are inside NETC IP, but
> > NETCMIX is outside NETC. There are dependencies between these three
> > blocks, so it is better to configure them in one driver. But for other
> > platforms like S32, it does not have NETCMIX, so NETCMIX is optional.
> 
> But how s32 is related here? That's a different device.
> 

The S32 SoC also uses the NETC IP, so this YAML should be compatible with
S32 SoC. Or do you mean when S32 NETC is supported, we then add restrictions
to the reg property for S32?

Krzysztof Kozlowski Oct. 24, 2024, 1:27 p.m. UTC | #7

On 23/10/2024 12:03, Wei Fang wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: 2024年10月23日 16:56
>> To: Wei Fang <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; Vladimir Oltean <[e-mail]>; Claudiu
>> Manoil <[e-mail]>; Clark Wang <[e-mail]>;
>> Frank Li <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH v4 net-next 03/13] dt-bindings: net: add bindings for NETC
>> blocks control
>>
>> On 23/10/2024 10:18, Wei Fang wrote:
>>>>> +maintainers:
>>>>> +  - Wei Fang <[e-mail]>
>>>>> +  - Clark Wang <[e-mail]>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - nxp,imx95-netc-blk-ctrl
>>>>> +
>>>>> +  reg:
>>>>> +    minItems: 2
>>>>> +    maxItems: 3
>>>>
>>>> You have one device, why this is flexible? Device either has exactly
>>>> 2 or exactly 3 IO spaces, not both depending on the context.
>>>>
>>>
>>> There are three register blocks, IERB and PRB are inside NETC IP, but
>>> NETCMIX is outside NETC. There are dependencies between these three
>>> blocks, so it is better to configure them in one driver. But for other
>>> platforms like S32, it does not have NETCMIX, so NETCMIX is optional.
>>
>> But how s32 is related here? That's a different device.
>>
> 
> The S32 SoC also uses the NETC IP, so this YAML should be compatible with
> S32 SoC.

What? How? Where is this compatible documented?

> Or do you mean when S32 NETC is supported, we then add restrictions
> to the reg property for S32?

I don't know what you are creating here. That's a binding for one
specific device (see writing bindings guideline).

Best regards,
Krzysztof

Vladimir Oltean Oct. 24, 2024, 2:32 p.m. UTC | #8

On Wed, Oct 23, 2024 at 11:18:43AM +0300, Wei Fang wrote:
> > > +maintainers:
> > > +  - Wei Fang <[e-mail]>
> > > +  - Clark Wang <[e-mail]>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - nxp,imx95-netc-blk-ctrl
> > > +
> > > +  reg:
> > > +    minItems: 2
> > > +    maxItems: 3
> > 
> > You have one device, why this is flexible? Device either has exactly 2
> > or exactly 3 IO spaces, not both depending on the context.
> > 
> 
> There are three register blocks, IERB and PRB are inside NETC IP, but NETCMIX
> is outside NETC. There are dependencies between these three blocks, so it is
> better to configure them in one driver. But for other platforms like S32, it does
> not have NETCMIX, so NETCMIX is optional.

Looking at this patch (in v5), I was confused as to why you've made pcie@4cb00000
a child of system-controller@4cde0000, when there's no obvious parent/child
relationship between them (the ECAM node is not even within the same address
space as the "system-controller@4cde0000" address space, and it's not
even clear what the "system-controller@4cde0000" node _represents_:

examples:
  - |
    bus {
        #address-cells = <2>;
        #size-cells = <2>;

        system-controller@4cde0000 {
            compatible = "nxp,imx95-netc-blk-ctrl";
            reg = <0x0 0x4cde0000 0x0 0x10000>,
                  <0x0 0x4cdf0000 0x0 0x10000>,
                  <0x0 0x4c81000c 0x0 0x18>;
            reg-names = "ierb", "prb", "netcmix";
            #address-cells = <2>;
            #size-cells = <2>;
            ranges;
            clocks = <&scmi_clk 98>;
            clock-names = "ipg";
            power-domains = <&scmi_devpd 18>;

            pcie@4cb00000 {
                compatible = "pci-host-ecam-generic";
                reg = <0x0 0x4cb00000 0x0 0x100000>;
                #address-cells = <3>;
                #size-cells = <2>;
                device_type = "pci";
                bus-range = <0x1 0x1>;
                ranges = <0x82000000 0x0 0x4cce0000  0x0 0x4cce0000  0x0 0x20000
                          0xc2000000 0x0 0x4cd10000  0x0 0x4cd10000  0x0 0x10000>;

But then I saw your response, and I think your response answers my confusion.
The "system-controller@4cde0000" node doesn't represent anything in and
of itself, it is just a container to make the implementation easier.

The Linux driver treatment should not have a definitive say in the device tree bindings.
To solve the dependencies problem, you have options such as the component API at
your disposal to have a "component master" driver which waits until all its
components have probed.

But if the IERB, PRB and NETCMIX are separate register blocks, they should have
separate OF nodes under their respective buses, and the ECAM should be on the same
level. You should describe the hierarchy from the perspective of the SoC address
space, and not abuse the "ranges" property here.

Wei Fang Oct. 25, 2024, 7:15 a.m. UTC | #9

> On 23/10/2024 12:03, Wei Fang wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: 2024年10月23日 16:56
> >> To: Wei Fang <[e-mail]>
> >> Cc: [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; Vladimir Oltean <[e-mail]>;
> >> conor+Claudiu
> >> Manoil <[e-mail]>; Clark Wang
> <[e-mail]>;
> >> Frank Li <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v4 net-next 03/13] dt-bindings: net: add bindings
> >> for NETC blocks control
> >>
> >> On 23/10/2024 10:18, Wei Fang wrote:
> >>>>> +maintainers:
> >>>>> +  - Wei Fang <[e-mail]>
> >>>>> +  - Clark Wang <[e-mail]>
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    enum:
> >>>>> +      - nxp,imx95-netc-blk-ctrl
> >>>>> +
> >>>>> +  reg:
> >>>>> +    minItems: 2
> >>>>> +    maxItems: 3
> >>>>
> >>>> You have one device, why this is flexible? Device either has
> >>>> exactly
> >>>> 2 or exactly 3 IO spaces, not both depending on the context.
> >>>>
> >>>
> >>> There are three register blocks, IERB and PRB are inside NETC IP,
> >>> but NETCMIX is outside NETC. There are dependencies between these
> >>> three blocks, so it is better to configure them in one driver. But
> >>> for other platforms like S32, it does not have NETCMIX, so NETCMIX is
> optional.
> >>
> >> But how s32 is related here? That's a different device.
> >>
> >
> > The S32 SoC also uses the NETC IP, so this YAML should be compatible
> > with
> > S32 SoC.
> 
> What? How? Where is this compatible documented?
> 

Yes, it is not added yet, so that is why I asked the below question. I should
only focus on i.MX95.


> > Or do you mean when S32 NETC is supported, we then add restrictions to
> > the reg property for S32?
> 
> I don't know what you are creating here. That's a binding for one specific device
> (see writing bindings guideline).
>

Wei Fang Oct. 25, 2024, 8:22 a.m. UTC | #10

> On Wed, Oct 23, 2024 at 11:18:43AM +0300, Wei Fang wrote:
> > > > +maintainers:
> > > > +  - Wei Fang <[e-mail]>
> > > > +  - Clark Wang <[e-mail]>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - nxp,imx95-netc-blk-ctrl
> > > > +
> > > > +  reg:
> > > > +    minItems: 2
> > > > +    maxItems: 3
> > >
> > > You have one device, why this is flexible? Device either has exactly 2
> > > or exactly 3 IO spaces, not both depending on the context.
> > >
> >
> > There are three register blocks, IERB and PRB are inside NETC IP, but NETCMIX
> > is outside NETC. There are dependencies between these three blocks, so it is
> > better to configure them in one driver. But for other platforms like S32, it
> does
> > not have NETCMIX, so NETCMIX is optional.
> 
> Looking at this patch (in v5), I was confused as to why you've made
> pcie@4cb00000
> a child of system-controller@4cde0000, when there's no obvious parent/child
> relationship between them (the ECAM node is not even within the same
> address
> space as the "system-controller@4cde0000" address space, and it's not
> even clear what the "system-controller@4cde0000" node _represents_:
> 
> examples:
>   - |
>     bus {
>         #address-cells = <2>;
>         #size-cells = <2>;
> 
>         system-controller@4cde0000 {
>             compatible = "nxp,imx95-netc-blk-ctrl";
>             reg = <0x0 0x4cde0000 0x0 0x10000>,
>                   <0x0 0x4cdf0000 0x0 0x10000>,
>                   <0x0 0x4c81000c 0x0 0x18>;
>             reg-names = "ierb", "prb", "netcmix";
>             #address-cells = <2>;
>             #size-cells = <2>;
>             ranges;
>             clocks = <&scmi_clk 98>;
>             clock-names = "ipg";
>             power-domains = <&scmi_devpd 18>;
> 
>             pcie@4cb00000 {
>                 compatible = "pci-host-ecam-generic";
>                 reg = <0x0 0x4cb00000 0x0 0x100000>;
>                 #address-cells = <3>;
>                 #size-cells = <2>;
>                 device_type = "pci";
>                 bus-range = <0x1 0x1>;
>                 ranges = <0x82000000 0x0 0x4cce0000  0x0 0x4cce0000
> 0x0 0x20000
>                           0xc2000000 0x0 0x4cd10000  0x0
> 0x4cd10000  0x0 0x10000>;
> 
> But then I saw your response, and I think your response answers my confusion.
> The "system-controller@4cde0000" node doesn't represent anything in and
> of itself, it is just a container to make the implementation easier.
> 
> The Linux driver treatment should not have a definitive say in the device tree
> bindings.
> To solve the dependencies problem, you have options such as the component
> API at
> your disposal to have a "component master" driver which waits until all its
> components have probed.
> 
> But if the IERB, PRB and NETCMIX are separate register blocks, they should
> have
> separate OF nodes under their respective buses, and the ECAM should be on
> the same
> level. You should describe the hierarchy from the perspective of the SoC
> address
> space, and not abuse the "ranges" property here.

I don't know much about component API. Today I spent some time to learn
about the component API framework. In my opinion, the framework is also
implemented based on DTS. For example, the master device specifies the
slave devices through a port child node or a property of phandle-array type. 

For i.MX95 NETC, according to your suggestion, the probe sequence is as
follows:

--> netxmix_probe() # NETCMIX
		--> netc_prb_ierb_probe() # IERB and PRB
				--> enetc4_probe() # ENETC 0/1/2
				--> netc_timer_probe() #PTP Timer
				--> enetc_pci_mdio_probe() # NETC EMDIO
						

From this sequence, there are two levels. The first level is IERB&PRB is
the master device, NETCMIX is the slave device. The second level is
IERB&PRB is the slave device, and ENETC, TIMER and EMDIO are the master
devices. First of all, I am not sure whether the component API supports
mapping a slave device to multiple master devices, I only know that
multiple slave devices can be mapped to one master device. Secondly,
the two levels will make the driver more complicated, which is a greater
challenge for us to support suspend/resume in the future. As far as I
know, the component helper also doesn't solve runtime dependencies, e.g.
for system suspend and resume operations.

I don't think there is anything wrong with the current approach. First,
as you said, it makes implementation easier. Second, establishing this
parent-child relationship in DTS can solve the suspend/resume operation
order problem, which we have verified locally. Why do we need each register
block to has a separated node? These are obviously different register
blocks in the NETC system.

Wei Fang Oct. 25, 2024, 8:48 a.m. UTC | #11

> > On Wed, Oct 23, 2024 at 11:18:43AM +0300, Wei Fang wrote:
> > > > > +maintainers:
> > > > > +  - Wei Fang <[e-mail]>
> > > > > +  - Clark Wang <[e-mail]>
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - nxp,imx95-netc-blk-ctrl
> > > > > +
> > > > > +  reg:
> > > > > +    minItems: 2
> > > > > +    maxItems: 3
> > > >
> > > > You have one device, why this is flexible? Device either has
> > > > exactly 2 or exactly 3 IO spaces, not both depending on the context.
> > > >
> > >
> > > There are three register blocks, IERB and PRB are inside NETC IP,
> > > but NETCMIX is outside NETC. There are dependencies between these
> > > three blocks, so it is better to configure them in one driver. But
> > > for other platforms like S32, it
> > does
> > > not have NETCMIX, so NETCMIX is optional.
> >
> > Looking at this patch (in v5), I was confused as to why you've made
> > pcie@4cb00000
> > a child of system-controller@4cde0000, when there's no obvious
> > parent/child relationship between them (the ECAM node is not even
> > within the same address space as the "system-controller@4cde0000"
> > address space, and it's not even clear what the
> > "system-controller@4cde0000" node _represents_:
> >
> > examples:
> >   - |
> >     bus {
> >         #address-cells = <2>;
> >         #size-cells = <2>;
> >
> >         system-controller@4cde0000 {
> >             compatible = "nxp,imx95-netc-blk-ctrl";
> >             reg = <0x0 0x4cde0000 0x0 0x10000>,
> >                   <0x0 0x4cdf0000 0x0 0x10000>,
> >                   <0x0 0x4c81000c 0x0 0x18>;
> >             reg-names = "ierb", "prb", "netcmix";
> >             #address-cells = <2>;
> >             #size-cells = <2>;
> >             ranges;
> >             clocks = <&scmi_clk 98>;
> >             clock-names = "ipg";
> >             power-domains = <&scmi_devpd 18>;
> >
> >             pcie@4cb00000 {
> >                 compatible = "pci-host-ecam-generic";
> >                 reg = <0x0 0x4cb00000 0x0 0x100000>;
> >                 #address-cells = <3>;
> >                 #size-cells = <2>;
> >                 device_type = "pci";
> >                 bus-range = <0x1 0x1>;
> >                 ranges = <0x82000000 0x0 0x4cce0000  0x0 0x4cce0000
> > 0x0 0x20000
> >                           0xc2000000 0x0 0x4cd10000  0x0
> > 0x4cd10000  0x0 0x10000>;
> >
> > But then I saw your response, and I think your response answers my confusion.
> > The "system-controller@4cde0000" node doesn't represent anything in
> > and of itself, it is just a container to make the implementation easier.
> >
> > The Linux driver treatment should not have a definitive say in the
> > device tree bindings.
> > To solve the dependencies problem, you have options such as the
> > component API at your disposal to have a "component master" driver
> > which waits until all its components have probed.
> >
> > But if the IERB, PRB and NETCMIX are separate register blocks, they
> > should have separate OF nodes under their respective buses, and the
> > ECAM should be on the same level. You should describe the hierarchy
> > from the perspective of the SoC address space, and not abuse the
> > "ranges" property here.
> 
> I don't know much about component API. Today I spent some time to learn
> about the component API framework. In my opinion, the framework is also
> implemented based on DTS. For example, the master device specifies the slave
> devices through a port child node or a property of phandle-array type.
> 
> For i.MX95 NETC, according to your suggestion, the probe sequence is as
> follows:
> 
> --> netxmix_probe() # NETCMIX
> 		--> netc_prb_ierb_probe() # IERB and PRB
> 				--> enetc4_probe() # ENETC 0/1/2
> 				--> netc_timer_probe() #PTP Timer
> 				--> enetc_pci_mdio_probe() # NETC EMDIO
> 
> 
> From this sequence, there are two levels. The first level is IERB&PRB is the
> master device, NETCMIX is the slave device. The second level is IERB&PRB is the
> slave device, and ENETC, TIMER and EMDIO are the master devices. First of all, I
> am not sure whether the component API supports mapping a slave device to
> multiple master devices, I only know that multiple slave devices can be mapped
> to one master device. Secondly, the two levels will make the driver more
> complicated, which is a greater challenge for us to support suspend/resume in
> the future. As far as I know, the component helper also doesn't solve runtime
> dependencies, e.g.
> for system suspend and resume operations.
> 
> I don't think there is anything wrong with the current approach. First, as you
> said, it makes implementation easier. Second, establishing this parent-child
> relationship in DTS can solve the suspend/resume operation order problem,
> which we have verified locally. Why do we need each register block to has a
> separated node? These are obviously different register blocks in the NETC
> system.

Another reason as you know, many customers require Ethernet to work as soon
as possible after Linux boots up. If the component API is used, this may delay the
ENETC probe time, which may be unacceptable to customers.

Vladimir Oltean Oct. 25, 2024, 1:06 p.m. UTC | #12

On Fri, Oct 25, 2024 at 11:48:18AM +0300, Wei Fang wrote:
> > > On Wed, Oct 23, 2024 at 11:18:43AM +0300, Wei Fang wrote:
> > > > > > +maintainers:
> > > > > > +  - Wei Fang <[e-mail]>
> > > > > > +  - Clark Wang <[e-mail]>
> > > > > > +
> > > > > > +properties:
> > > > > > +  compatible:
> > > > > > +    enum:
> > > > > > +      - nxp,imx95-netc-blk-ctrl
> > > > > > +
> > > > > > +  reg:
> > > > > > +    minItems: 2
> > > > > > +    maxItems: 3
> > > > >
> > > > > You have one device, why this is flexible? Device either has
> > > > > exactly 2 or exactly 3 IO spaces, not both depending on the context.
> > > > >
> > > >
> > > > There are three register blocks, IERB and PRB are inside NETC IP,
> > > > but NETCMIX is outside NETC. There are dependencies between these
> > > > three blocks, so it is better to configure them in one driver. But
> > > > for other platforms like S32, it does
> > > > not have NETCMIX, so NETCMIX is optional.
> > >
> > > Looking at this patch (in v5), I was confused as to why you've made
> > > pcie@4cb00000
> > > a child of system-controller@4cde0000, when there's no obvious
> > > parent/child relationship between them (the ECAM node is not even
> > > within the same address space as the "system-controller@4cde0000"
> > > address space, and it's not even clear what the
> > > "system-controller@4cde0000" node _represents_:
> > >
> > > examples:
> > >   - |
> > >     bus {
> > >         #address-cells = <2>;
> > >         #size-cells = <2>;
> > >
> > >         system-controller@4cde0000 {
> > >             compatible = "nxp,imx95-netc-blk-ctrl";
> > >             reg = <0x0 0x4cde0000 0x0 0x10000>,
> > >                   <0x0 0x4cdf0000 0x0 0x10000>,
> > >                   <0x0 0x4c81000c 0x0 0x18>;
> > >             reg-names = "ierb", "prb", "netcmix";
> > >             #address-cells = <2>;
> > >             #size-cells = <2>;
> > >             ranges;
> > >             clocks = <&scmi_clk 98>;
> > >             clock-names = "ipg";
> > >             power-domains = <&scmi_devpd 18>;
> > >
> > >             pcie@4cb00000 {
> > >                 compatible = "pci-host-ecam-generic";
> > >                 reg = <0x0 0x4cb00000 0x0 0x100000>;
> > >                 #address-cells = <3>;
> > >                 #size-cells = <2>;
> > >                 device_type = "pci";
> > >                 bus-range = <0x1 0x1>;
> > >                 ranges = <0x82000000 0x0 0x4cce0000  0x0 0x4cce0000 0x0 0x20000
> > >                           0xc2000000 0x0 0x4cd10000  0x0 0x4cd10000  0x0 0x10000>;
> > >
> > > But then I saw your response, and I think your response answers my confusion.
> > > The "system-controller@4cde0000" node doesn't represent anything in
> > > and of itself, it is just a container to make the implementation easier.
> > >
> > > The Linux driver treatment should not have a definitive say in the
> > > device tree bindings.
> > > To solve the dependencies problem, you have options such as the
> > > component API at your disposal to have a "component master" driver
> > > which waits until all its components have probed.
> > >
> > > But if the IERB, PRB and NETCMIX are separate register blocks, they
> > > should have separate OF nodes under their respective buses, and the
> > > ECAM should be on the same level. You should describe the hierarchy
> > > from the perspective of the SoC address space, and not abuse the
> > > "ranges" property here.
> > 
> > I don't know much about component API. Today I spent some time to learn
> > about the component API framework. In my opinion, the framework is also
> > implemented based on DTS. For example, the master device specifies the slave
> > devices through a port child node or a property of phandle-array type.
> > 
> > For i.MX95 NETC, according to your suggestion, the probe sequence is as
> > follows:
> > 
> > --> netxmix_probe() # NETCMIX
> > 		--> netc_prb_ierb_probe() # IERB and PRB
> > 				--> enetc4_probe() # ENETC 0/1/2
> > 				--> netc_timer_probe() #PTP Timer
> > 				--> enetc_pci_mdio_probe() # NETC EMDIO
> > 
> > From this sequence, there are two levels. The first level is IERB&PRB is the
> > master device, NETCMIX is the slave device. The second level is IERB&PRB is the
> > slave device, and ENETC, TIMER and EMDIO are the master devices. First of all, I
> > am not sure whether the component API supports mapping a slave device to
> > multiple master devices, I only know that multiple slave devices can be mapped
> > to one master device.

I meant that the component master would be an aggregate driver for the
IERB and PRB, not the NETC, PTP, MDIO (PCIe function) drivers as you
seem to have understood. The component master driver could be an
abstract entity which is not necessarily represented in OF. It can
simply be a platform driver instantiated with platform_device_add().
Only its components (IERB etc) can be represented in OF.

The PCIe function drivers would be outside of the component API scheme.
They would all get a reference to the aggregate driver through some
other mechanism - such as a function call to netcmix_get() from their
probe function. If a platform device for the aggregate driver doesn't
exist, it is created using platform_device_add(). If it does exist
already, just get_device() on it.

Anyway, I wasn't suggesting you _have_ to use the component API, and I
don't have enough knowledge about this SoC to make a concrete design
suggestion. Just suggesting to not model the dt-bindings after the
driver implementation.

> > Secondly, the two levels will make the driver more complicated, which is a
> > greater challenge for us to support suspend/resume in the future. As far as
> > I know, the component helper also doesn't solve runtime dependencies, e.g.
> > for system suspend and resume operations.

Indeed it doesn't. Device links should take care of that (saying in
general, not necessarily applied to this context).

> > I don't think there is anything wrong with the current approach. First, as you
> > said, it makes implementation easier. Second, establishing this parent-child
> > relationship in DTS can solve the suspend/resume operation order problem,
> > which we have verified locally. Why do we need each register block to has a
> > separated node? These are obviously different register blocks in the NETC
> > system.

Let's concentrate on getting the device tree representation of the hardware
accurate first, then figure out driver implementation issues later.

I'm concerned that there is no parent/child relationship from an address
space perspective between system-controller@4cde0000 and pcie@4cb00000.
I don't see a strong reason to not place these 2 nodes on the same
hierarchical level. If the dt-binding maintainers do not share this
concern, I will drop it.

> Another reason as you know, many customers require Ethernet to work as soon
> as possible after Linux boots up. If the component API is used, this may delay the
> ENETC probe time, which may be unacceptable to customers.

I mean, if I were to look at the big picture here, the huge problem is
the SoC suspend/resume flow, where the NETC requires Linux to reconfigure
the NETCMIX in the first place, before it becomes operational.
The use (or not) of the component API to achieve that (avoidable
in principle) purpose seems like splitting hairs at this point.

Wei Fang Oct. 26, 2024, 3:01 a.m. UTC | #13

> -----Original Message-----
> From: Vladimir Oltean <[e-mail]>
> Sent: 2024年10月25日 21:06
> To: Wei Fang <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Claudiu Manoil
> <[e-mail]>; Clark Wang <[e-mail]>; Frank Li
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v4 net-next 03/13] dt-bindings: net: add bindings for NETC
> blocks control
> 
> On Fri, Oct 25, 2024 at 11:48:18AM +0300, Wei Fang wrote:
> > > > On Wed, Oct 23, 2024 at 11:18:43AM +0300, Wei Fang wrote:
> > > > > > > +maintainers:
> > > > > > > +  - Wei Fang <[e-mail]>
> > > > > > > +  - Clark Wang <[e-mail]>
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    enum:
> > > > > > > +      - nxp,imx95-netc-blk-ctrl
> > > > > > > +
> > > > > > > +  reg:
> > > > > > > +    minItems: 2
> > > > > > > +    maxItems: 3
> > > > > >
> > > > > > You have one device, why this is flexible? Device either has
> > > > > > exactly 2 or exactly 3 IO spaces, not both depending on the context.
> > > > > >
> > > > >
> > > > > There are three register blocks, IERB and PRB are inside NETC IP,
> > > > > but NETCMIX is outside NETC. There are dependencies between these
> > > > > three blocks, so it is better to configure them in one driver. But
> > > > > for other platforms like S32, it does
> > > > > not have NETCMIX, so NETCMIX is optional.
> > > >
> > > > Looking at this patch (in v5), I was confused as to why you've made
> > > > pcie@4cb00000
> > > > a child of system-controller@4cde0000, when there's no obvious
> > > > parent/child relationship between them (the ECAM node is not even
> > > > within the same address space as the "system-controller@4cde0000"
> > > > address space, and it's not even clear what the
> > > > "system-controller@4cde0000" node _represents_:
> > > >
> > > > examples:
> > > >   - |
> > > >     bus {
> > > >         #address-cells = <2>;
> > > >         #size-cells = <2>;
> > > >
> > > >         system-controller@4cde0000 {
> > > >             compatible = "nxp,imx95-netc-blk-ctrl";
> > > >             reg = <0x0 0x4cde0000 0x0 0x10000>,
> > > >                   <0x0 0x4cdf0000 0x0 0x10000>,
> > > >                   <0x0 0x4c81000c 0x0 0x18>;
> > > >             reg-names = "ierb", "prb", "netcmix";
> > > >             #address-cells = <2>;
> > > >             #size-cells = <2>;
> > > >             ranges;
> > > >             clocks = <&scmi_clk 98>;
> > > >             clock-names = "ipg";
> > > >             power-domains = <&scmi_devpd 18>;
> > > >
> > > >             pcie@4cb00000 {
> > > >                 compatible = "pci-host-ecam-generic";
> > > >                 reg = <0x0 0x4cb00000 0x0 0x100000>;
> > > >                 #address-cells = <3>;
> > > >                 #size-cells = <2>;
> > > >                 device_type = "pci";
> > > >                 bus-range = <0x1 0x1>;
> > > >                 ranges = <0x82000000 0x0 0x4cce0000  0x0
> 0x4cce0000 0x0 0x20000
> > > >                           0xc2000000 0x0 0x4cd10000  0x0
> 0x4cd10000  0x0 0x10000>;
> > > >
> > > > But then I saw your response, and I think your response answers my
> confusion.
> > > > The "system-controller@4cde0000" node doesn't represent anything in
> > > > and of itself, it is just a container to make the implementation easier.
> > > >
> > > > The Linux driver treatment should not have a definitive say in the
> > > > device tree bindings.
> > > > To solve the dependencies problem, you have options such as the
> > > > component API at your disposal to have a "component master" driver
> > > > which waits until all its components have probed.
> > > >
> > > > But if the IERB, PRB and NETCMIX are separate register blocks, they
> > > > should have separate OF nodes under their respective buses, and the
> > > > ECAM should be on the same level. You should describe the hierarchy
> > > > from the perspective of the SoC address space, and not abuse the
> > > > "ranges" property here.
> > >
> > > I don't know much about component API. Today I spent some time to learn
> > > about the component API framework. In my opinion, the framework is also
> > > implemented based on DTS. For example, the master device specifies the
> slave
> > > devices through a port child node or a property of phandle-array type.
> > >
> > > For i.MX95 NETC, according to your suggestion, the probe sequence is as
> > > follows:
> > >
> > > --> netxmix_probe() # NETCMIX
> > > 		--> netc_prb_ierb_probe() # IERB and PRB
> > > 				--> enetc4_probe() # ENETC 0/1/2
> > > 				--> netc_timer_probe() #PTP Timer
> > > 				--> enetc_pci_mdio_probe() # NETC EMDIO
> > >
> > > From this sequence, there are two levels. The first level is IERB&PRB is the
> > > master device, NETCMIX is the slave device. The second level is IERB&PRB is
> the
> > > slave device, and ENETC, TIMER and EMDIO are the master devices. First of
> all, I
> > > am not sure whether the component API supports mapping a slave device
> to
> > > multiple master devices, I only know that multiple slave devices can be
> mapped
> > > to one master device.
> 
> I meant that the component master would be an aggregate driver for the
> IERB and PRB, not the NETC, PTP, MDIO (PCIe function) drivers as you
> seem to have understood. The component master driver could be an
> abstract entity which is not necessarily represented in OF. It can
> simply be a platform driver instantiated with platform_device_add().
> Only its components (IERB etc) can be represented in OF.
> 
> The PCIe function drivers would be outside of the component API scheme.
> They would all get a reference to the aggregate driver through some
> other mechanism - such as a function call to netcmix_get() from their
> probe function. If a platform device for the aggregate driver doesn't
> exist, it is created using platform_device_add(). If it does exist
> already, just get_device() on it.
> 
> Anyway, I wasn't suggesting you _have_ to use the component API, and I
> don't have enough knowledge about this SoC to make a concrete design
> suggestion. Just suggesting to not model the dt-bindings after the
> driver implementation.
> 
> > > Secondly, the two levels will make the driver more complicated, which is a
> > > greater challenge for us to support suspend/resume in the future. As far as
> > > I know, the component helper also doesn't solve runtime dependencies,
> e.g.
> > > for system suspend and resume operations.
> 
> Indeed it doesn't. Device links should take care of that (saying in
> general, not necessarily applied to this context).
> 
> > > I don't think there is anything wrong with the current approach. First, as
> you
> > > said, it makes implementation easier. Second, establishing this parent-child
> > > relationship in DTS can solve the suspend/resume operation order problem,
> > > which we have verified locally. Why do we need each register block to has a
> > > separated node? These are obviously different register blocks in the NETC
> > > system.
> 
> Let's concentrate on getting the device tree representation of the hardware
> accurate first, then figure out driver implementation issues later.
> 
> I'm concerned that there is no parent/child relationship from an address
> space perspective between system-controller@4cde0000 and pcie@4cb00000.
> I don't see a strong reason to not place these 2 nodes on the same
> hierarchical level. If the dt-binding maintainers do not share this
> concern, I will drop it.

system-controller not only configure the endpoints of the NETC, but also
can configure the ECAM space, such as the vendor ID, device ID, the RID
of endpoint, VF stride and so on. For this perspective, I don't think the
ECAM space should placed at the same hierarchical level with system-controller.

If they are placed at the same level, then before pci_host_common_probe() is
called, we need to ensure that IERB completes probe(), which means we need
to modify the PCI host common driver, component API or add a callback function
or something else, which I don't think is a good idea.

> 
> > Another reason as you know, many customers require Ethernet to work as
> soon
> > as possible after Linux boots up. If the component API is used, this may delay
> the
> > ENETC probe time, which may be unacceptable to customers.
> 
> I mean, if I were to look at the big picture here, the huge problem is
> the SoC suspend/resume flow, where the NETC requires Linux to reconfigure
> the NETCMIX in the first place, before it becomes operational.
> The use (or not) of the component API to achieve that (avoidable
> in principle) purpose seems like splitting hairs at this point.

Vladimir Oltean Oct. 31, 2024, 12:45 p.m. UTC | #14

On Sat, Oct 26, 2024 at 06:01:37AM +0300, Wei Fang wrote:
> system-controller not only configure the endpoints of the NETC, but also
> can configure the ECAM space, such as the vendor ID, device ID, the RID
> of endpoint, VF stride and so on. For this perspective, I don't think the
> ECAM space should placed at the same hierarchical level with system-controller.
> 
> If they are placed at the same level, then before pci_host_common_probe() is
> called, we need to ensure that IERB completes probe(), which means we need
> to modify the PCI host common driver, component API or add a callback function
> or something else, which I don't think is a good idea.

Ok, that does sound important. If the NETCMIX block were to actually
modify the ECAM space, what would be the primary source of information
for how the ECAM device descriptions should look like?

I remember a use case being discussed internally a while ago was that
where the Cortex-A cores are only guests which only have ownership of
some Ethernet ports discovered through the ECAM, but not of the entire
NETCMIX block and not of physical Ethernet ports. How would that be
described in the device tree? The ECAM node would no longer be placed
under system-controller?

At what point does it simply just make more sense to have a different
PCIe ECAM driver than pcie-host-ecam-generic, which just handles
internally the entire NETCMIX?

Wei Fang Nov. 1, 2024, 2:18 a.m. UTC | #15

> 
> On Sat, Oct 26, 2024 at 06:01:37AM +0300, Wei Fang wrote:
> > system-controller not only configure the endpoints of the NETC, but also
> > can configure the ECAM space, such as the vendor ID, device ID, the RID
> > of endpoint, VF stride and so on. For this perspective, I don't think the
> > ECAM space should placed at the same hierarchical level with
> system-controller.
> >
> > If they are placed at the same level, then before pci_host_common_probe() is
> > called, we need to ensure that IERB completes probe(), which means we need
> > to modify the PCI host common driver, component API or add a callback
> function
> > or something else, which I don't think is a good idea.
> 
> Ok, that does sound important. If the NETCMIX block were to actually
> modify the ECAM space, what would be the primary source of information
> for how the ECAM device descriptions should look like?
> 

I think the related info should be provided by DTS, but currently, we do not
have such requirement that needs Linux to change the ECAM space, this may
be supported in the future if we have the requirement.

> I remember a use case being discussed internally a while ago was that
> where the Cortex-A cores are only guests which only have ownership of
> some Ethernet ports discovered through the ECAM, but not of the entire
> NETCMIX block and not of physical Ethernet ports. How would that be
> described in the device tree? The ECAM node would no longer be placed
> under system-controller?

Yes, we indeed have this use case on i.MX95, only the VFs of 10G ENETC
are owned by Cortex-A, the entire ECAM space and other NETC devices
are all owned by Cortex-M. In this case, the system-controller is no needed
in DTS, because Linux have no permission to access these resources.

> 
> At what point does it simply just make more sense to have a different
> PCIe ECAM driver than pcie-host-ecam-generic, which just handles
> internally the entire NETCMIX?

Currently, I have not idea in what use case we need a different ECAM driver
to handle internally the entire system-controller.

For the use case I mentioned above, we use a different ECAM driver, which
is implemented by RPMSG, because the entire ECAM space is owned by
Cortex-M. So we use the ECAM driver to notify the Cortex-M to enable/disable
VFs or do FLR for VFs and so on. But this ECAM driver does not need to
configure the system-controller.

Vladimir Oltean Nov. 1, 2024, 10:34 a.m. UTC | #16

On Fri, Nov 01, 2024 at 04:18:55AM +0200, Wei Fang wrote:
> > On Sat, Oct 26, 2024 at 06:01:37AM +0300, Wei Fang wrote:
> > > system-controller not only configure the endpoints of the NETC, but also
> > > can configure the ECAM space, such as the vendor ID, device ID, the RID
> > > of endpoint, VF stride and so on. For this perspective, I don't think the
> > > ECAM space should placed at the same hierarchical level with system-controller.
> > >
> > > If they are placed at the same level, then before pci_host_common_probe() is
> > > called, we need to ensure that IERB completes probe(), which means we need
> > > to modify the PCI host common driver, component API or add a callback function
> > > or something else, which I don't think is a good idea.
> > 
> > Ok, that does sound important. If the NETCMIX block were to actually
> > modify the ECAM space, what would be the primary source of information
> > for how the ECAM device descriptions should look like?
> > 
> 
> I think the related info should be provided by DTS, but currently, we do not
> have such requirement that needs Linux to change the ECAM space, this may
> be supported in the future if we have the requirement.
> 
> > I remember a use case being discussed internally a while ago was that
> > where the Cortex-A cores are only guests which only have ownership of
> > some Ethernet ports discovered through the ECAM, but not of the entire
> > NETCMIX block and not of physical Ethernet ports. How would that be
> > described in the device tree? The ECAM node would no longer be placed
> > under system-controller?
> 
> Yes, we indeed have this use case on i.MX95, only the VFs of 10G ENETC
> are owned by Cortex-A, the entire ECAM space and other NETC devices
> are all owned by Cortex-M. In this case, the system-controller is no needed
> in DTS, because Linux have no permission to access these resources.
> 
> > 
> > At what point does it simply just make more sense to have a different
> > PCIe ECAM driver than pcie-host-ecam-generic, which just handles
> > internally the entire NETCMIX?
> 
> Currently, I have not idea in what use case we need a different ECAM driver
> to handle internally the entire system-controller.
> 
> For the use case I mentioned above, we use a different ECAM driver, which
> is implemented by RPMSG, because the entire ECAM space is owned by
> Cortex-M. So we use the ECAM driver to notify the Cortex-M to enable/disable
> VFs or do FLR for VFs and so on. But this ECAM driver does not need to
> configure the system-controller.

Ok, I was actually wondering if it makes sense for the the parent bus of
the NETC PCIe functions to be described through a unified binding that
covers all of the above use cases, so that major device tree modifications
aren't necessary to adapt between the 'Linux as host' and 'Linux as guest'
use cases. But you're saying it doesn't make much sense, because the
device tree in the guest case would contain descriptions of inaccessible
resources (the NETCMIX block). Oh well, this is just another case where
"device tree should describe hardware" actually means "device tree describes
what software wants to know about the hardware".

Anyway, I am now convinced by your design choices at least to the extent
that they appear self-consistent to me (I still don't really have an
independent opinion). If somebody has a different idea on how the PCIe
bus should be described, feel free to chime in.

Patch

diff --git a/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
new file mode 100644
index 000000000000..0b7fd2c5e0d8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nxp,netc-blk-ctrl.yaml
@@ -0,0 +1,111 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/nxp,netc-blk-ctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NETC Blocks Control
+
+description:
+  Usually, NETC has 2 blocks of 64KB registers, integrated endpoint register
+  block (IERB) and privileged register block (PRB). IERB is used for pre-boot
+  initialization for all NETC devices, such as ENETC, Timer, EMIDO and so on.
+  And PRB controls global reset and global error handling for NETC. Moreover,
+  for the i.MX platform, there is also a NETCMIX block for link configuration,
+  such as MII protocol, PCS protocol, etc.
+
+maintainers:
+  - Wei Fang <[e-mail]>
+  - Clark Wang <[e-mail]>
+
+properties:
+  compatible:
+    enum:
+      - nxp,imx95-netc-blk-ctrl
+
+  reg:
+    minItems: 2
+    maxItems: 3
+
+  reg-names:
+    minItems: 2
+    items:
+      - const: ierb
+      - const: prb
+      - const: netcmix
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+  ranges: true
+  assigned-clocks: true
+  assigned-clock-parents: true
+  assigned-clock-rates: true
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: ipg
+
+  power-domains:
+    maxItems: 1
+
+patternProperties:
+  "^pcie@[0-9a-f]+$":
+    $ref: /schemas/pci/host-generic-pci.yaml#
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        netc-blk-ctrl@4cde0000 {
+            compatible = "nxp,imx95-netc-blk-ctrl";
+            reg = <0x0 0x4cde0000 0x0 0x10000>,
+                  <0x0 0x4cdf0000 0x0 0x10000>,
+                  <0x0 0x4c81000c 0x0 0x18>;
+            reg-names = "ierb", "prb", "netcmix";
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges;
+            assigned-clocks = <&scmi_clk 98>, <&scmi_clk 102>;
+            assigned-clock-parents = <&scmi_clk 12>, <&scmi_clk 6>;
+            assigned-clock-rates = <666666666>, <250000000>;
+            clocks = <&scmi_clk 98>;
+            clock-names = "ipg";
+            power-domains = <&scmi_devpd 18>;
+
+            pcie@4cb00000 {
+                compatible = "pci-host-ecam-generic";
+                reg = <0x0 0x4cb00000 0x0 0x100000>;
+                #address-cells = <3>;
+                #size-cells = <2>;
+                device_type = "pci";
+                bus-range = <0x1 0x1>;
+                ranges = <0x82000000 0x0 0x4cce0000  0x0 0x4cce0000  0x0 0x20000
+                          0xc2000000 0x0 0x4cd10000  0x0 0x4cd10000  0x0 0x10000>;
+
+                mdio@0,0 {
+                    compatible = "pci1131,ee00";
+                    reg = <0x010000 0 0 0 0>;
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+                };
+            };
+        };
+    };