public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Andrew Fish <afish@apple.com>
To: Udit Kumar <udit.kumar@nxp.com>
Cc: Daniel Thompson <daniel.thompson@linaro.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [RFC] ACPI table HID/CID allocation
Date: Wed, 22 Nov 2017 09:34:31 -0800	[thread overview]
Message-ID: <45939FDA-1DA8-424C-BBD1-4A46480DC6DD@apple.com> (raw)
In-Reply-To: <AM6PR0402MB3334AB886BCF9B53B3DE645791200@AM6PR0402MB3334.eurprd04.prod.outlook.com>

FYI now that the UEFI Forum owns the ACPI Spec here is the location to register a new PNP ID or ACPI ID: https://stash.sd.apple.com/projects/COREOSMACFW/repos/macefifirmware/pull-requests/630/overview <https://stash.sd.apple.com/projects/COREOSMACFW/repos/macefifirmware/pull-requests/630/overview>. Anyone can make a request. 

PNP ID Registry: http://www.uefi.org/pnp_id_list <http://www.uefi.org/pnp_id_list>
ACPI ID Registry: http://www.uefi.org/acpi_id_list <http://www.uefi.org/acpi_id_list>

Thanks,

Andrew Fish

> On Nov 22, 2017, at 5:39 AM, Udit Kumar <udit.kumar@nxp.com> wrote:
> 
> Hi Daniel 
> 
> 
>>> 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.
>> 
>> I don't think internal or external to the SoC would be any kind of deciding factor
>> in how to best to bind, simply because I don't understand why there is no HID
>> available.
> 
> This is more a choice/rule between allocating HID or using PRP0001.
> HID could be assigned to external devices, and getting them reviewed 
> by maintainers. 
> 
>> Large OEMs and board manufacturers usually have their own vendor IDs and
>> sometimes have to use these to describe hardware (IIRC the SMC LAN9xxx on
>> the ARM Juno uses an ARM HID).
> 
> Thanks,  for this example. 
> This is good example for me, where HID allocation is not limited to Vendor devices.
> 
> 
>> Admittedly the part you are describing follows a JEDEC standard so it would be
>> nice to have more widely agreed bindings... however making SPI NOR FLASH
>> available as raw MTD device to the OS is sufficiently unusual in ACPI systems
>> that there may not be any prior art to follow.
> 
> Please take this as an example. ( Main point was to use HID or PRP0001)
> Could be possible, if such device is exposed then this may not be used at all.
> Thanks for help. 
> 
> Thanks
> Udit 
> 
>> 
>> Daniel.
>> 
>> 
>>> 
>>> 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%2Fww
>>>> w.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
>>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel



  reply	other threads:[~2017-11-22 17:30 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-21  9:19 [RFC] ACPI table HID/CID allocation Udit Kumar
2017-11-21  9:38 ` Ard Biesheuvel
2017-11-21  9:59   ` Udit Kumar
2017-11-21 10:13     ` Ard Biesheuvel
2017-11-21 11:32       ` Udit Kumar
2017-11-21 12:29         ` Ard Biesheuvel
2017-11-21 13:24           ` Udit Kumar
2017-11-21 14:03             ` Ard Biesheuvel
2017-11-21 18:10               ` Udit Kumar
2017-11-22 11:30                 ` Daniel Thompson
2017-11-22 13:39                   ` Udit Kumar
2017-11-22 17:34                     ` Andrew Fish [this message]
2017-11-25 12:40                       ` Udit Kumar
2017-11-22 19:39                   ` Ard Biesheuvel
2017-11-22 20:11                     ` Daniel Thompson
2017-11-25 12:56                       ` Udit Kumar
2017-11-25 19:41                         ` Andrew Fish
2017-11-26  8:35                           ` Udit Kumar
2017-11-27 12:13                         ` Daniel Thompson
2017-11-27 13:31                           ` Udit Kumar
2017-11-25 12:47                     ` Udit Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45939FDA-1DA8-424C-BBD1-4A46480DC6DD@apple.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox