Re: [RFC] ACPI table HID/CID allocation

[Udit Kumar <udit.kumar@nxp.com>]

Thanks Ard, 
For internal SOC devices, this is perfectly ok to drop PRP0001 from CID. 

> This could be a valid reason to use PRP0001 + compatible, for things like I2C
> slaves that are external to the SoC

For external devices (for which HID is not available), you suggest to go
with PRP0001 + compatible or that device driver needs add ACPI HID support.

As you pointed out, are external devices to SOC such exception to use PRP0001 + compatible be it 
i2c slave or SPI slave ?


Regards
Udit

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, November 21, 2017 7:34 PM
> To: Udit Kumar <udit.kumar@nxp.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-devel@lists.01.org; Varun
> Sethi <V.Sethi@nxp.com>; Daniel Thompson <daniel.thompson@linaro.org>;
> Graeme Gregory <graeme.gregory@linaro.org>
> Subject: Re: [RFC] ACPI table HID/CID allocation
> 
> On 21 November 2017 at 13:24, Udit Kumar <udit.kumar@nxp.com> wrote:
> > Thanks Ard,
> >
> > My intend of this email to know, what is right way to define HID and
> > CID in ACPI firmware i.e
> >
> > Device(XYZ) {
> >                 Name(_HID, "NXP0001")
> >                 Name(_CID, "PRP0001")
> >           Device(Slave1) {
> >                                 Name(_CID, "PRP0001")
> >                  }
> > }
> > is ok or
> >
> >
> > Device(XYZ) {
> >                 Name(_HID, "NXP0001")
> >                 Name(_CID, " NXP0001")
> >           Device(Slave1) {
> >                                 Name(_CID, " NXP0002")
> >                  }
> > }
> > Seems good
> >
> 
> I don't think it makes a lot of sense to use the same value for _HID and _CID, so
> you can just drop the latter.

Sure, 
 
> > For sure, AML methods (as needed _ON/OFF/RST/STA etc) /_DSD will to be
> coded in table using either of.
> >
> > Please see more in line
> >
> > Regards
> > Udit
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: Tuesday, November 21, 2017 5:59 PM
> >> To: Udit Kumar <udit.kumar@nxp.com>
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>;
> >> edk2-devel@lists.01.org; Varun Sethi <V.Sethi@nxp.com>; Daniel
> >> Thompson <daniel.thompson@linaro.org>; Graeme Gregory
> >> <graeme.gregory@linaro.org>
> >> Subject: Re: [RFC] ACPI table HID/CID allocation
> >>
> >> On 21 November 2017 at 11:32, Udit Kumar <udit.kumar@nxp.com> wrote:
> >> >
> >> >> On 21 November 2017 at 09:59, Udit Kumar <udit.kumar@nxp.com>
> wrote:
> >> >> > Thanks Ard.
> >> >> > Below table was for example. I am not converting whole DT to
> >> >> > ACPI tables :) My idea is to reduce Linux patches for probing as possible.
> >> >> > Also keeping firmware and OS separately then Let firmware expose
> >> >> > both way (HID and PRP00001) and Linux to decide binding
> >> >>
> >> >> No.
> >> >>
> >> >> You are still assuming ACPI and DT device drivers bind at the same
> >> >> level, and they don't.
> >> >
> >> > No, my above comments was just limited to binding.
> >>
> >> Yes, but if you leave it to the OS to decide which binding it uses,
> >> you will have to support all of them into eternity. And the more
> >> detailed binding you need to support may limit you in the available
> >> choices when it comes to making hardware changes, something ACPI allows
> you to do.
> >
> > Thanks,
> > Is this ok to say , we can provide max same properties in driver as of
> > DT (with _DSD) , But prefer to use AML methods.
> > With note, clocking regulators are out of scope here.
> >
> 
> Yes. _DSD may be used to describe device specific data that goes beyond what
> ACPI can express natively. Using _DSD to describe clocks and regulators is an
> absolute no-go. Putting things like "status" or "dma-coherent" in _DSD
> properties is absolutely unacceptable as well.
> But even things like initialization data that the driver programs into the device a
> single time really do not belong in _DSD. Instead, it should be the firmware that
> initializes the device, and presents it to the OS in its initialized state.
>

Agreed, I never meant something to add in DSD which was prohibited in ACPI spces.

> >
> >> > Right, here device driver should know that device is present in
> >> > system, I see probe function (device-driver binding) is way to know this.
> >> > Then driver can execute AML methods exposed by firmware.
> >> >
> >>
> >> Yes, declaring the presence of the device is the main purpose of
> >> describing it both in ACPI and in DT.
> >>
> >> >> An ACPI device has AML methods to manage power state and perform
> >> >> other device related low-level tasks. The device driver has no
> >> >> knowledge of the hardware beyond what it needs to invoke those
> >> >> abstract
> >> methods.
> >> >
> >> > You meant, If we need to update driver for AML methods then why not
> >> > to
> >> update binding as well . ?
> >> >
> >>
> >> No. What I am saying is that you should not expose two different
> >> bindings, and let the OS choose.
> >
> > Ok, got it , then PRP00001 stuff should be used only at urgent or say
> > no other choice left , Right ?
> >
> 
> Yes.


> >> > On side track,
> >> >  With limited search,  I got many each driver is having (acpi_id
> >> > and of_id), looks, acpi support is added on the top of DT flavored drivers.
> >> > and therefore acpi tables are following the same.
> >> > Sorry to say, reference I am looking at (edk2-platform) JunoPkg and
> >> > VExpressPkg, Has following devices has description similar to Device tree
> >> >     Device (NET0) {
> >> >     Device (SREG) {
> >> >     Device (VIRT) {
> >>
> >> These were taken from the ACPI tables for an emulator
> >>
> >> >    Device(KMI0) {
> >>
> >> I have no clue what kind of device this is
> >>
> >> >    Device(ETH0) {
> >>
> >> This one uses _DSD as intended, to set device properties in a DT
> >> compatible fashion, but note that this does *not* include the
> >> 'compatible' property, nor anything else that ACPI defines itself
> >> (status, dma-coherent, etc)
> >
> > Let me put in simple way,
> > A simple driver can survive only with _DSD in acpi world.
> > (clocking/regulators are used as said in UEFI/ACPI)
> >
> 
> Why can a simple driver only survive with _DSD? That statement does not make
> any sense to me.

Why so, please see below one for example 
 
> > Copying below from Juno,
> > Are below kind of tables are acceptable ?
> >
> >     Device(ETH0) {
> >       Name(_HID, "ARMH9118")
> >       Name(_UID, Zero)
> >       Name(_CRS, ResourceTemplate() {
> >               Memory32Fixed(ReadWrite, 0x18000000, 0x1000)
> >               Interrupt(ResourceConsumer, Level, ActiveHigh, Exclusive) { 192 }
> >       })
> >       Name(_DSD, Package() {
> >                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >                        Package() {
> >                                Package(2) {"phy-mode", "mii"},
> >                                Package(2) {"reg-io-width", 4 },
> >                                Package(2) {"smsc,irq-active-high",1},
> >                                Package(2) {"smsc,irq-push-pull",1}
> >                       }
> >       }) // _DSD()
> >     }
> >
> 
> Yes. But please be aware that you should not simply invent your own properties
> here. The _DSD namespace was intended to be managed, and not free for all

Agreed, I didn’t meant to add something new, which is not available at present, 

 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.k
> ernel.org%2Fdoc%2FDocumentation%2Facpi%2FDSD-properties-
> rules.txt&data=02%7C01%7Cudit.kumar%40nxp.com%7C164c1ff7350a4f6373e
> e08d530e8b591%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63646
> 8698397705869&sdata=O78k8r6tcK9fwpuTuQ82ZXGiWkBtLduf4bqrM6D6L1U%
> 3D&reserved=0
> 
> >> > Where no AML method is exposed. Then I expect OS driver to manage
> >> > this
> >> device.
> >> > While grepping over few other edk2-platforms.  Looks some of
> >> > methods are abstracted, not whole device.
> >> >
> >>
> >> So what is your point? Why does this argue in favor of allowing
> >> PRP0001 + compatible?
> >
> > I am seeking your help here to define HID and CID,  please see above
> > Also for non-NXP devices, how to define HID (if PRP0001 + compatible
> > not to be used)
> >
> 
> This could be a valid reason to use PRP0001 + compatible, for things like I2C
> slaves that are external to the SoC

Well,  for internal SOC devices, I am in agreement to use NXP specific HIDs 
But for external devices (for which HID is not available), you suggest to go
With PRP0001 + compatible
 
> >> >> A DT device describes everything in detail, and requires clock and
> >> >> regulator drivers and other bits and pieces that are tightly
> >> >> coupled to details of the hardware.
> >> >>
> >> >> So now, you have the worst of both worlds:
> >> >>
> >> >> - you need to implement all of this in firmware so ACPI can
> >> >> support it,
> >> >> - you have to expose the internals to the OS so DT can support it.
> >> >
> >> > Yes, for time being or may be longer, DT support needs to be there
> >> > along with ACPI introduction.
> >> >
> >> > Are you suggesting here to abstract whole device details from OS
> >> > and expose AML methods to be used by device driver.
> >> > And maintain two drivers instead of fitting DT style driver into ACPI world ?
> >> >
> >>
> >> No. You should update the driver so it can support both ACPI and DT bindings.
> >> That way, the driver can use the abstractions offered by ACPI when it
> >> can, and can invoke the clock and regulator frameworks and other low
> >> level infrastructure only when it needs to.
> >
> > Ok, I am align on this, to have one driver which supports both.
> >
> >> Let me try to illustrate this a bit better: imagine a NXP customer
> >> that runs a datacenter that has 10,000 NXP servers, and is using RHEL
> >> x.y. The business is going well, and at some point, he wants to order another
> 2,000 servers.
> >> Unfortunately, the vendor cannot supply the exact same revision of
> >> the hardware, and the latest revision uses some component that is not
> >> supported in RHEL x.y.
> >>
> >> You now have made your customer very unhappy. He invested in RHEL and
> >> ACPI based servers precisely to avoid this situation. The cost of
> >> adding 2,000 servers now includes the cost of upgrading the other
> >> 10,000 servers to a new OS version or the cost of supporting two
> >> different OS versions at the same time, for a reason that is not justifiable.
> >
> > Do you mean here with PRP0001 HID/CID, we cannot use AML methods.
> 
> You cannot use the abstractions ACPI provides when using PRP0001 +
> compatible.
Oh, thx