Re: [RFC] ACPI table HID/CID allocation

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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.

> 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.

>
>> > 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.

> 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

https://www.kernel.org/doc/Documentation/acpi/DSD-properties-rules.txt

>> > 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

>> >> 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.