Received-SPF: pass (google.com: domain of linux-kernel+bounces-56489-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 7 Feb 2024 15:37:30 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Frank Li <Frank.li@nxp.com>, Rob Herring <robh@kernel.org>, 
	Bjorn Helgaas <bhelgaas@google.com>
Cc: Jingoo Han <jingoohan1@gmail.com>, 
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, 
	Lorenzo Pieralisi <lpieralisi@kernel.org>, Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, imx@lists.linux.dev, 
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v3 5/6] dt-bindings: PCI: dwc: Add 'msg' register region
Message-ID: <luk5hswq4wnk5p7axml73qih35hio3y3pfnklctbn6rwres62s@mumnvygjh5ch>
References: <20240202-pme_msg-v3-0-ff2af57a02ad@nxp.com>
 <20240202-pme_msg-v3-5-ff2af57a02ad@nxp.com>
 <eg7wrjp5ebz43g37fvebr44nwkoh4rptbtyu76nalbmgbbnqke@4zugpgwesyqd>
 <20240205183048.GA3818249-robh@kernel.org>
 <ZcEzYdZKotBJlR5i@lizhi-Precision-Tower-5810>
 <ZcK2/tmLG9O7CBEH@lizhi-Precision-Tower-5810>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcK2/tmLG9O7CBEH@lizhi-Precision-Tower-5810>

On Tue, Feb 06, 2024 at 05:47:26PM -0500, Frank Li wrote:
> On Mon, Feb 05, 2024 at 02:13:37PM -0500, Frank Li wrote:
> > On Mon, Feb 05, 2024 at 06:30:48PM +0000, Rob Herring wrote:
> > > On Sat, Feb 03, 2024 at 01:44:31AM +0300, Serge Semin wrote:
> > > > On Fri, Feb 02, 2024 at 10:11:27AM -0500, Frank Li wrote:
> > > > > Add an outbound iATU-capable memory-region which will be used to send PCIe
> > > > > message (such as PME_Turn_Off) to peripheral. So all platforms can use
> > > > > common method to send out PME_Turn_Off message by using one outbound iATU.
> > > > > 
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > index 022055edbf9e6..25a5420a9ce1e 100644
> > > > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > > > @@ -101,6 +101,10 @@ properties:
> > > > 
> > > > >              Outbound iATU-capable memory-region which will be used to access
> > > > >              the peripheral PCIe devices configuration space.
> > > > >            const: config
> > > > > +        - description:
> > > > > +            Outbound iATU-capable memory-region which will be used to send
> > > > > +            PCIe message (such as PME_Turn_Off) to peripheral.
> > > > > +          const: msg
> > > > 
> > > > Note there is a good chance Rob won't like this change. AFAIR he
> > > > already expressed a concern regarding having the "config" reg-name
> > > > describing a memory space within the outbound iATU memory which is
> > > > normally defined by the "ranges" property. Adding a new reg-entry with
> > > > similar semantics I guess won't receive warm welcome.
> > > 
> > > I do think it is a bit questionable. Ideally, the driver could 
> > > just configure this on its own. However, since we don't describe all of 
> > > the CPU address space (that's input to the iATU) already, that's not 
> > > going to be possible. I suppose we could fix that, but then config space 
> > > would have to be handled differently too.
> > 
> > Sorry, I have not understand what your means. Do you means, you want
> > a "cpu-space", for example, 0x8000000 - 0x9000000 for all ATU. 
> > 
> > Then allocated some space to 'config', 'io', 'memory' and this 'msg'.
> 
> @rob:
> 
>     So far, I think "msg" is feasilbe solution. Or give me some little
> detail direction?

Found the Rob' note about the iATU-space chunks utilized in the reg
property:
https://lore.kernel.org/linux-pci/CAL_JsqLp7QVgxrAZkW=z38iB7SV5VeWH1O6s+DVCm9p338Czdw@mail.gmail.com/

So basically Rob meant back then that
either originally we should have defined a new reg-name like "atu-out"
with the entire outbound iATU CPU-space specified and unpin the
regions like "config"/"ecam"/"msg"/etc from there in the driver
or, well, stick to the chunking further. The later path was chosen
after the patch with the "ecam" reg-name was accepted (see the link
above).

Really ECAM/config space access, custom TLP messages, legacy interrupt
TLPs, etc are all application-specific features. Each of them is
implemented based on a bit specific but basically the same outbound
iATU engine setup. Thus from the "DT is a hardware description" point
of view it would have been enough to describe the entire outbound iATU
CPU address space and then let the software do the space
reconfiguration in runtime based on it' application needs.

* Rob, correct me if am wrong.

On the other hand it's possible to have more than one disjoint CPU
address region handled by the outbound iATU (especially if there is no
AXI-bridge enabled, see XALI - application transmit client interfaces
in HW manual). Thus having a single reg-property might get to be
inapplicable in some cases. Thinking about that got me to an idea.
What about just extending the PCIe "ranges" property flags
(IORESOURCE_TYPE_BITS) with the new ones in this case indicating the
TLP Msg mapping? Thus we can avoid creating app-specific reg-names and
use the flag to define a custom memory range for the TLP messages
generation. At some point it can be also utilized for the config-space
mapping. What do you think?

-Serge(y)

> 
> Frank
> 
> > 
> > Frank
> > 
> > > 
> > > Rob