> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> Sent: 2024年10月25日 21:06
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; Claudiu Manoil
> <claudiu.manoil@xxxxxxx>; Clark Wang <xiaoning.wang@xxxxxxx>; Frank Li
> <frank.li@xxxxxxx>; christophe.leroy@xxxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; horms@xxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> alexander.stein@xxxxxxxxxxxxxxx
> 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 <wei.fang@xxxxxxx>
> > > > > > > + - Clark Wang <xiaoning.wang@xxxxxxx>
> > > > > > > +
> > > > > > > +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.
