Re: [RFC] ACPI table HID/CID allocation

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

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