Received-SPF: pass (google.com: domain of linux-kernel+bounces-196547-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Content-Type: multipart/signed;
 boundary=5756bc5e83a6bad3a9f5eb4a93427ad8fb0c0c23d1457925c7710f966752;
 micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: bulk
Mime-Version: 1.0
Date: Fri, 31 May 2024 11:06:29 +0200
Message-Id: <D1NPLD5J2YE2.3H93K6QYATI49@gmail.com>
Cc: <linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
 <ldewangan@nvidia.com>, <mkumard@nvidia.com>
Subject: Re: [RESEND PATCH 1/2] dt-bindings: dma: Add reg-names to
 nvidia,tegra210-adma
From: "Thierry Reding" <thierry.reding@gmail.com>
To: "Krzysztof Kozlowski" <krzk@kernel.org>, "Sameer Pujar"
 <spujar@nvidia.com>, <vkoul@kernel.org>, <robh@kernel.org>,
 <krzk+dt@kernel.org>, <conor+dt@kernel.org>, <jonathanh@nvidia.com>,
 <dmaengine@vger.kernel.org>, <devicetree@vger.kernel.org>
References: <20240521110801.1692582-1-spujar@nvidia.com>
 <20240521110801.1692582-2-spujar@nvidia.com>
 <80b6e6e6-9805-4a85-97d5-38e1b2bf2dd0@kernel.org>
 <e6fab314-8d1e-4ed7-bb5a-025fd65e1494@nvidia.com>
 <56bf93ac-6c1e-48aa-89d0-7542ea707848@kernel.org>
 <f785f699-be50-4547-9411-d41a4e66a225@nvidia.com>
 <774df64c-56a1-461a-82fa-a0340732b779@kernel.org>
 <D1HPADDIQNIK.2F4AL70NLHQCY@gmail.com>
 <819d7180-db8c-438c-afed-463fe495bfc5@kernel.org>
 <D1MZOIQR9R4G.16O4LIDN6AN5Q@gmail.com>
 <18946269-8529-434e-bcef-85c74b7a81b7@kernel.org>
In-Reply-To: <18946269-8529-434e-bcef-85c74b7a81b7@kernel.org>

--5756bc5e83a6bad3a9f5eb4a93427ad8fb0c0c23d1457925c7710f966752
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

On Fri May 31, 2024 at 9:43 AM CEST, Krzysztof Kozlowski wrote:
> On 30/05/2024 14:48, Thierry Reding wrote:
> > On Tue May 28, 2024 at 8:48 AM CEST, Krzysztof Kozlowski wrote:
> >> On 24/05/2024 09:36, Thierry Reding wrote:
> >>> On Wed May 22, 2024 at 1:29 PM CEST, Krzysztof Kozlowski wrote:
> >>>> On 22/05/2024 09:43, Sameer Pujar wrote:
> >>>>>
> >>>>>
> >>>>> On 22-05-2024 12:17, Krzysztof Kozlowski wrote:
> >>>>>> On 22/05/2024 07:35, Sameer Pujar wrote:
> >>>>>>> On 21-05-2024 17:23, Krzysztof Kozlowski wrote:
> >>>>>>>> On 21/05/2024 13:08, Sameer Pujar wrote:
> >>>>>>>>> From: Mohan Kumar <mkumard@nvidia.com>
> >>>>>>>>>
> >>>>>>>>> For Non-Hypervisor mode, Tegra ADMA driver requires the registe=
r
> >>>>>>>>> resource range to include both global and channel page in the r=
eg
> >>>>>>>>> entry. For Hypervisor more, Tegra ADMA driver requires only the
> >>>>>>>>> channel page and global page range is not allowed for access.
> >>>>>>>>>
> >>>>>>>>> Add reg-names DT binding for Hypervisor mode to help driver to
> >>>>>>>>> differentiate the config between Hypervisor and Non-Hypervisor
> >>>>>>>>> mode of execution.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
> >>>>>>>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> >>>>>>>>> ---
> >>>>>>>>>    .../devicetree/bindings/dma/nvidia,tegra210-adma.yaml  | 10 =
++++++++++
> >>>>>>>>>    1 file changed, 10 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra=
210-adma.yaml b/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.=
yaml
> >>>>>>>>> index 877147e95ecc..ede47f4a3eec 100644
> >>>>>>>>> --- a/Documentation/devicetree/bindings/dma/nvidia,tegra210-adm=
a.yaml
> >>>>>>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra210-adm=
a.yaml
> >>>>>>>>> @@ -29,8 +29,18 @@ properties:
> >>>>>>>>>              - const: nvidia,tegra186-adma
> >>>>>>>>>
> >>>>>>>>>      reg:
> >>>>>>>>> +    description: |
> >>>>>>>>> +      For hypervisor mode, the address range should include a
> >>>>>>>>> +      ADMA channel page address range, for non-hypervisor mode
> >>>>>>>>> +      it starts with ADMA base address covering Global and Cha=
nnel
> >>>>>>>>> +      page address range.
> >>>>>>>>>        maxItems: 1
> >>>>>>>>>
> >>>>>>>>> +  reg-names:
> >>>>>>>>> +    description: only required for Hypervisor mode.
> >>>>>>>> This does not work like that. I provide vm entry for non-hypervi=
sor mode
> >>>>>>>> and what? You claim it is virtualized?
> >>>>>>>>
> >>>>>>>> Drop property.
> >>>>>>> With 'vm' entry added for hypervisor mode, the 'reg' address rang=
e needs
> >>>>>>> to be updated to use channel specific region only. This is used t=
o
> >>>>>>> inform driver to skip global regions which is taken care by hyper=
visor.
> >>>>>>> This is expected to be used in the scenario where Linux acts as a
> >>>>>>> virtual machine (VM). May be the hypervisor mode gives a differen=
t
> >>>>>>> impression here? Sorry, I did not understand what dropping the pr=
operty
> >>>>>>> exactly means here.
> >>>>>> It was imperative. Drop it. Remove it. I provided explanation why.
> >>>>>
> >>>>> The driver doesn't know if it is operated in a native config or in =
the=20
> >>>>> hypervisor config based on the 'reg' address range alone. So 'vm' e=
ntry=20
> >>>>> with restricted 'reg' range is used to differentiate here for the=
=20
> >>>>> hypervisor config. Just adding 'vm' entry won't be enough, the 'reg=
'=20
> >>>>> region must be updated as well to have expected behavior. Not sure =
how=20
> >>>>> this dependency can be enforced in the schema.
> >>>>
> >>>> That's not a unusual problem, so please come with a solution for you=
r
> >>>> entire subarch. We've been discussing similar topic in terms of SCMI
> >>>> controlled resources (see talk on Linaro Connect a week ago:
> >>>> https://www.kitefor.events/events/linaro-connect-24/submissions/161 =
I
> >>>> don't know where is recording or slides, see also discussions on mai=
ling
> >>>> lists about it), which is not that far away from the problem here. O=
ther
> >>>> platforms and maybe nvidia had as well changes in IO space for
> >>>> virtualized configuration.
> >>>>
> >>>> Come with unified approach FOR ALL your devices, not only this one
> >>>> (that's kind of basic thing we keep repeating... don't solve only on=
e
> >>>> your problem), do not abuse the regular property, because as I said:
> >>>> reg-names will be provided as well in non-vm case and then your enti=
re
> >>>> logic is wrong. The purpose of reg-names is not to tell whether you =
have
> >>>> or have not virtualized environment.
> >>>
> >>> This isn't strictly about telling whether this is a virtualized
> >>> environment or not. Unfortunately the bindings don't make that very
> >>> clear, so let me try to give a bit more background.
> >>>
> >>> On Tegra devices the register regions associated with a device are
> >>> usually split up into 64 KiB chunks.
> >>
> >> So describing it as one IO region was incorrect from the start and you
> >> want to fix it by adding one more incorrect description: making first
> >> item meaning two different things. Sorry, that's not a correct way to
> >> fix things.
> >=20
> > Yes, describing this as one I/O region was incorrect, and in hindsight
> > it should have been done differently.
> >=20
> > However, I don't think it's correct to describe this as adding one more
> > incorrect description. Instead, what this does is add reg-names to
> > provide additional context so that the operating system can make the
> > necessary decisions as to what is allowed and what isn't.
> >=20
> > In the absence of a reg-names property the current definition of the DT
> > bindings applies, so it means the region represents the entirety of the
> > device's I/O register space. That's one particular use-case for this
> > device.
> >=20
> > For additional use-cases we can then use reg-names to differentiate
> > between what separate regions are and use them accordingly.
> >=20
> >> Items are defined, thus first item is always expected to be what the
> >> binding already said. Adding reg-names changes nothing, because (as
> >> repeated many times) xxx-names is just a helper. Items are already def=
ined.
> >=20
> > I don't understand what you're trying to say here. I suppose adding
> > reg-names alone indeed doesn't change anything. But the point is that
> > once added we can now use these properties, at which point of course
> > things change.
> >=20
> >>> One of these chunks, usually the first one, is a global region that
> >>> contains registers that configure the device as a whole. This is usua=
lly
> >>> privileged and accessible only to the hypervisor.
> >>>
> >>> Subsequent regions are meant to be assigned to individual VMs. Often =
the
> >>> regions take the form of "channels", so they are instances of the sam=
e
> >>> register block and control that separate slice of the hardware.
> >>>
> >>> What makes this a bit confusing is that for the sake of simplicity (a=
nd,
> >>> I guess, lack of foresight) the original bindings were written in a w=
ay
> >>> to encompass all registers without making that distinction. This work=
ed
> >>> fine because we've only ever run Linux as host OS where it has access=
 to
> >>> all those registers.
> >>>
> >>> However, when we move to virtualized environments that no longer work=
s.
> >>>
> >>> Given the above, we can't read any registers in order to probe whethe=
r
> >>> we run as a guest or not. Trying to access any of the global register=
s
> >>> from a VM simply won't work and may crash the system. None of the
> >>> "channel" registers contain information indicating host vs. guest
> >>> either.
> >>
> >> I don't understand how it differs from what I said - you want to
> >> indicate that you run in virtualized environment and not all resources
> >> are accessible.
> >>
> >> The device still has the first (global) address, just it is not
> >> available due to hypervisor.
> >=20
> > Yes, and that's a bad thing because there's no way for the device to
> > know that it can't access the registers. So it will just assume that it
> > can and try to access them, which would then result in a crash/error.
>
> Different compatible could note that or the global address would be
> removed from IO space, although then you need to rely on names and order
> is not fixed. I think Rob already proposed different compatible.
>
> This is also the way new Qcom platforms are going (older were using
> properties).
>
> However my earlier comment stays on: you will have for sure more cases
> like this, so please think upfront and pick unified approach for all
> future devices.

We already have. In fact we already have a few devices (host1x[0] and
MGBE[1]) where a similar path was chosen. Unification with those is why
we're proposing this.

This also applies to the memory controller SID bindings update that we
proposed a little while ago.

Thierry

[0]:
Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.yaml
[1]: Documentation/devicetree/bindings/net/nvidia,tegra234-mgbe.yaml

--5756bc5e83a6bad3a9f5eb4a93427ad8fb0c0c23d1457925c7710f966752
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmZZkxYACgkQ3SOs138+
s6Gxsg/9FpCyMeDV4FlwtV47MwNWpVc45/xh+Jlu8zR6fnOUsBIIkDuCE6mQDOVg
O8bBg2SK2ja2ZgAYZ2++fuq78YI58aYFZdWZRaReVZmn9y+qs1lMhjomvkQh9M52
zAtAAfs0qEQlJQSFPeA3p/ZibxjvxVxEqR9aQod9mBfm8+6ll1BPl2nTGfnXhDnY
cgI/J/6rvmD/wq2Zf5M/JS2BedOcA1NZpmb8Kdxu/G4WPvBOiQccKpzUBfaDOCSV
J/F2MIOjgNwpR0fnuZZxY3J5hxACfemnR2w9QIq1u3ykALgm4bKXD3vj85NXPCsM
7RC9ehjzSBeolhcEiX7DwKMYgD19lYQQEicwyeHJ8WSm03B8wXQxqPP49KD0vOTC
I0140x6cqmlEyD0eUVe6PEdtbURhZUrKfh9V3DO7sMt/BUkpt+NDtyWs2prclJsx
efdNUl+IJ5bZrCU/TBTVdAQ6HlXTWtRot7jlPlgSbuooow1rfZWczA72WHg5DF4o
RXVRM5bmiqwpd7f3lXyI+lQNVl8/TOhAeJlgV4IrDXYkIH53j/80rJrhptF4Z0Nv
/ydOjcpOXMnXbTbdQYtw6BVs2Hfh9HHLE/5/coxMCOB+5L75ExisFAXuQB5I6I9B
BfM6y8DW5jNXIt39g97TdSkSr/ORk1vfbKMBV8ZgiZIIu7pcGyE=
=ViI5
-----END PGP SIGNATURE-----

--5756bc5e83a6bad3a9f5eb4a93427ad8fb0c0c23d1457925c7710f966752--