Re: [RESEND PATCH 1/2] dt-bindings: dma: Add reg-names to nvidia,tegra210-adma

[Rob Herring]

On Fri, May 24, 2024 at 09:36:08AM +0200, 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@xxxxxxxxxx>
> > >>>>>
> > >>>>> For Non-Hypervisor mode, Tegra ADMA driver requires the register
> > >>>>> resource range to include both global and channel page in the reg
> > >>>>> 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@xxxxxxxxxx>
> > >>>>> Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx>
> > >>>>> ---
> > >>>>>    .../devicetree/bindings/dma/nvidia,tegra210-adma.yaml  | 10 ++++++++++
> > >>>>>    1 file changed, 10 insertions(+)
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml b/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml
> > >>>>> index 877147e95ecc..ede47f4a3eec 100644
> > >>>>> --- a/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.yaml
> > >>>>> +++ b/Documentation/devicetree/bindings/dma/nvidia,tegra210-adma.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 Channel
> > >>>>> +      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-hypervisor mode
> > >>>> and what? You claim it is virtualized?
> > >>>>
> > >>>> Drop property.
> > >>> With 'vm' entry added for hypervisor mode, the 'reg' address range needs
> > >>> to be updated to use channel specific region only. This is used to
> > >>> inform driver to skip global regions which is taken care by hypervisor.
> > >>> This is expected to be used in the scenario where Linux acts as a
> > >>> virtual machine (VM). May be the hypervisor mode gives a different
> > >>> impression here? Sorry, I did not understand what dropping the property
> > >>> 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 
> > > hypervisor config based on the 'reg' address range alone. So 'vm' entry 
> > > with restricted 'reg' range is used to differentiate here for the 
> > > hypervisor config. Just adding 'vm' entry won't be enough, the 'reg' 
> > > region must be updated as well to have expected behavior. Not sure how 
> > > this dependency can be enforced in the schema.
> >
> > That's not a unusual problem, so please come with a solution for your
> > 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 mailing
> > lists about it), which is not that far away from the problem here. Other
> > 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 one
> > 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 entire
> > 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.
> 
> One of these chunks, usually the first one, is a global region that
> contains registers that configure the device as a whole. This is usually
> 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 same
> register block and control that separate slice of the hardware.
> 
> What makes this a bit confusing is that for the sake of simplicity (and,
> I guess, lack of foresight) the original bindings were written in a way
> to encompass all registers without making that distinction. This worked
> 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 works.
> 
> Given the above, we can't read any registers in order to probe whether
> we run as a guest or not. Trying to access any of the global registers
> from a VM simply won't work and may crash the system. None of the
> "channel" registers contain information indicating host vs. guest
> either.
> 
> In order to make this work we need to more fine-grainedly specify the
> register layout. I think the binding changes here aren't sufficient to
> do that, though.
> 
> Currently we have this for the ADMA controller:
> 
> 	dma-controller@2930000 {
> 		reg = <0x0 0x02930000 0x0 0x20000>;
> 	};
> 
> This contains the global registers (0x2930000-0x293ffff) and the first
> page/channel registers (0x2940000-0x294ffff) in one "reg" entry. Instead
> I think what we need is this:
> 
> 	dma-controller@2930000 {
> 		reg = <0x0 0x02930000 0x0 0x10000>,
> 		      <0x0 0x02940000 0x0 0x10000>,
> 		      <0x0 0x02950000 0x0 0x10000>,
> 		      <0x0 0x02960000 0x0 0x10000>,
> 		      <0x0 0x02970000 0x0 0x10000>;
> 		reg-names = "global", "page0", "page1", "page2",
> 		            "page3";
> 	};
> 
> That describes the device fully, but each of these entries is optional.
> If "global" is present it means we are a hypervisor (or host OS). If an
> additional "page" entry is present, we can also use those resources to
> stream audio data.
> 
> If "global" is not present, we know we are not a hypervisor and those
> registers cannot be accessed. This would be the typical case for a guest
> OS which has access only to the listed "page" entries.
> 
> For backwards-compatibility with the existing bindings we should be able
> to fallback to the singular register region and partition it up in the
> driver as necessary.
> 
> This is an approach that we've already implemented for certain devices
> such as host1x and Ethernet where a similar split exists. I suspect that
> we'll need to do this kind of split in a number of other bindings as
> well.

In a VM is a different (being a subset) programming model, so why not 
just a new compatible for virtualized case. That's what we'd do if 
actual h/w registers changed from one device to the next.

Rob