Message-ID:
 <PAXPR04MB8510BE30C31D55831BB276B2884F2@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Fri, 25 Oct 2024 08:22:57 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Krzysztof Kozlowski <krzk@...nel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>, Claudiu
 Manoil <claudiu.manoil@....com>, Clark Wang <xiaoning.wang@....com>, Frank Li
	<frank.li@....com>, "christophe.leroy@...roup.eu"
	<christophe.leroy@...roup.eu>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"horms@...nel.org" <horms@...nel.org>, "imx@...ts.linux.dev"
	<imx@...ts.linux.dev>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"alexander.stein@...tq-group.com" <alexander.stein@...tq-group.com>
Subject: RE: [PATCH v4 net-next 03/13] dt-bindings: net: add bindings for NETC
 blocks control

> On Wed, Oct 23, 2024 at 11:18:43AM +0300, Wei Fang wrote:
> > > > +maintainers:
> > > > +  - Wei Fang <wei.fang@....com>
> > > > +  - Clark Wang <xiaoning.wang@....com>
> > > > +
> > > > +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@...00000
> a child of system-controller@...e0000, 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@...e0000" address space, and it's not
> even clear what the "system-controller@...e0000" node _represents_:
> 
> examples:
>   - |
>     bus {
>         #address-cells = <2>;
>         #size-cells = <2>;
> 
>         system-controller@...e0000 {
>             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@...00000 {
>                 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@...e0000" 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.

Powered by blists - more mailing lists