AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C
From: werner.zeh@xxxxxxxxxxx
Date: Wed Nov 18 2020 - 05:58:44 EST
Hi Henning.
We had a short discussion with Andy on coreboot Gerrit (https://review.coreboot.org/c/coreboot/+/47235) regarding this issue.
We have agreed that we will give Epson a period of two weeks for reaction. If we will not have a valid HID by then, I will push a patch which will use a non-valid HID instead (something like XXXX0000 as proposed by Andy).
I will clarify in the meantime when the next coreboot release will happen and prevent this wrong ID from getting part of the release.
Werner
-----Ursprüngliche Nachricht-----
Von: Henning Schild <henning.schild@xxxxxxxxxxx>
Gesendet: Mittwoch, 18. November 2020 11:04
An: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Cc: Hahn, Johannes (DI FA CTR PLC PRC3) <johannes-hahn@xxxxxxxxxxx>; Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>; ACPI Devel Maling List <linux-acpi@xxxxxxxxxxxxxxx>; Brown, Len <len.brown@xxxxxxxxx>; Simon Glass <sjg@xxxxxxxxxxxx>; Simon Glass <sjg@xxxxxxxxxx>; val.krutov@xxxxxxxxxxxxx; Claudius Heine <ch@xxxxxxx>; Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>; linux-rtc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Zeh, Werner (DI MC MTS R&D HW 1) <werner.zeh@xxxxxxxxxxx>; Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>; Mantel, Martin (DI FA CTR PLC PO) <martin.mantel@xxxxxxxxxxx>
Betreff: Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C
We are trying to reach out to Epson to find valid IDs, but will do that off-list for now.
In the meantime we will propose just the I2C patches, without ACPI.
We have Werner Zeh here, who pushed the coreboot change. In coreboot nobody objected, maybe they have not been aware of the processes or it was missed in their review.
Werner i think the coreboot change should be reverted so it does not enter a release and will become legacy that needs to be supported. Let us revisit it once we have the proper IDs. If that fails we have to look into the other options that Andy proposed. There is no point having the coreboot support without the Linux-user ...
regards,
Henning
Am Tue, 17 Nov 2020 13:41:08 +0200
schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
> +Cc: Simon
>
> Simon, this is an issue with ACPI IDs and I think JFYI would be an
> interesting topic since this may happen in the future in U-Boot or
> other projects. Also, you may know people from coreboot to figure out
> what to do with this case and how to prevent something similar from
> happening in the future.
>
> On Tue, Nov 17, 2020 at 1:33 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> >
> > On Tue, Nov 17, 2020 at 11:51 AM johannes-hahn@xxxxxxxxxxx
> > <johannes-hahn@xxxxxxxxxxx> wrote:
> > >
> > > Hello Val,
> > >
> > > my name is Johannes Hahn from Siemens AG in Germany.
> > > Our product Open Controller II (OCII)[1] uses the Realtime Clock
> > > RX6110SA from SEIKO EPSON.
> >
> > Nice to hear from you!
> >
> > > Currently there is a merge request ongoing for the Linux Kernel
> > > master branch[2] which adds I²C and ACPI support to your original
> > > driver implementation.
> > >
> > > Simultaneously there is an already merged patch-set for
> > > coreboot[3] available creating the ACPI (SSDT) table entries for
> > > the RX6110SA.
> >
> > Thanks for pointers, I commented there. The ACPI ID change must be
> > reverted!
> > > The OCII uses coreboot for firmware initialization.
> > >
> > > During the merge request the eligible objection arose that the
> > > ACPI ID used in the Linux driver patch is not conforming the ACPI
> > > Specification. Indeed it does not. But when searching for a
> > > product identifier of RX6110SA I was not able to find a sufficient
> > > one with respect to the ACPI Specification (see [4] chapter 6.1.5
> > > _HID (Hardware ID),[5]).
> >
> > Unfortunately many vendors, even being registered in the ACPI/PNP
> > registry, are still neglecting the process.
> >
> > > According to the fact that there are other Linux RTC drivers on
> > > the Kernel mainline[6] which support ACPI matching that also do
> > > not have ACPI Specification compatible IDs we used that as an
> > > example for our first patch attempt.
> >
> > I answered this in previous mail.
> >
> > > A PNP ID for SEIKO EPSON is already registered at UEFI
> > > database[7].
> > >
> > > What I kindly ask your for is an ACPI Specification conforming
> > > Product Identifier for the RX6110SA RTC ? According to [5] this
> > > Product Identifier should be "... always four-character
> > > hexadecimal numbers (0-9 and A-F)".
> > >
> > > In case you do not know it our can not acquire/create one could
> > > you please redirect me to someone from SEIKO EPSON who can help me
> > > with that demand ?
> >
> > So, to be on the constructive page (I thought initially you are from
> > G company, but anyway) you may do the following:
> >
> > - (for prototyping only) you may use the PRP0001 approach, described
> > in [8]
> > - you may issue an ID under your (Siemens) vendor ID
> > - you may insist G company to issue the ID under their vendor space
> > (thru coreboot)
> > - (the best option) to communicate to Seiko Epson to get official ID
> > from them for this component (and ID mustn't abuse 6.1.5)
> >
> > Unfortunately I have no contacts there, but I think the best effort
> > is to contact their support and at the same time ask ASWG [9] how to
> > proceed. I Cc'ed this to ACPI people in Linux kernel, maybe they can
> > help.
> >
> > Of course you have choice to push bad ID forward and use precedence
> > (like many other companies, even Intel in past, do with firmwares
> > and Linux kernel is full of badly formed IDs), but since the change
> > is not existed in read devices I would really like to see proper
> > process to be followed.
> >
> > In the Linux kernel I'm in principle trying to prevent bad IDs from
> > happening as much as I can.
> >
> > > [1]:
> > > (https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7
> > > 677-2DB42-0GB0)
> > > [2]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F11%2F12%2F561&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=mc2cCsxmX6cjx6HP1yEY7BYd2UmDayTh%2FAqvfA5%2BG20%3D&reserved=0 [3]:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IPt%2B2j%2Fb7TCIwnQpTh0YQfRhAk3gG34xgs9j5J1p0IA%3D&reserved=0 [4]:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > uefi.org%2Fsites%2Fdefault%2Ffiles%2Fresources%2FACPI_6_3_final_Ja
> > > n30.pdf&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e
> > > 4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C
> > > 637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=9DXsd
> > > gRozFKHm%2F3B1i1ls2cwtzip1rsFuJ9qnwFvXOY%3D&reserved=0
> > > [5]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.uefi.org%2FPNP_ACPI_Registry&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tFOTF40yzDdfVWGKatL56IYo87aCAABHoxPefmLAtPU%3D&reserved=0 [6]:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > elixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Frtc%2Frtc
> > > -ds1307.c%23L1142&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57
> > > e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%
> > > 7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> > > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&s
> > > data=fn6HCTcy1YYOH02dd3uiXymz87jXzTzLO3rWX6S1AeI%3D&reserved=0
> > > [7]:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > www.uefi.org%2FPNP_ID_List%3Fsearch%3DSEIKO%2BEPSON&data=04%7C
> > > 01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7
> > > C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CU
> > > nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> > > k1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NDHUqVKgnhNASqTOEydkG%2BocR
> > > eOauHMeo1uCrGCvOyk%3D&reserved=0
> >
> > [8]:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fel
> > ixir.bootlin.com%2Flinux%2Flatest%2Fsource%2FDocumentation%2Ffirmwar
> > e-guide%2Facpi%2Fenumeration.rst&data=04%7C01%7Cwerner.zeh%40sie
> > mens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab4
> > 2e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJW
> > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100
> > 0&sdata=y7mB4fH7d98r4VOSeTM2qQ%2BraUvwRrmsznK7qj7W6bM%3D&res
> > erved=0
> > [9]:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > w.uefi.org%2Fworkinggroups&data=04%7C01%7Cwerner.zeh%40siemens.c
> > om%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495
> > d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&
> > sdata=cCwPr1JYgrP7Rou%2FdR0y5DOL%2FUg9MRgnu7QBoOGURZQ%3D&reserve
> > d=0
> > > > Before adding new ACPI ID, can you provide an evidence (either
> > > > from vendor of the component, or a real snapshot of DSDT from
> > > > device on market) that this is real ID?
> > > >
> > > > Before that happens, NAK.
> > > >
> > > > P.S. Seems to me that this is kinda cargo cult patch because
> > > > proposed ID is against ACPI and PNP registry and ACPI
> > > > specification.
> > >
> > > In fact we pushed it in coreboot and Linux at the same time.
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > review.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%
> > > 7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38
> > > ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnkn
> > > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > > aWwiLCJXVCI6Mn0%3D%7C1000&sdata=IPt%2B2j%2Fb7TCIwnQpTh0YQfRhAk
> > > 3gG34xgs9j5J1p0IA%3D&reserved=0
> > >
> > > That is the evidence. But in case this is wrong we can probably
> > > still change coreboot, even though the patches have been merged
> > > there already.
> > >
> > > Maybe you can go into detail where you see the violations and
> > > maybe even suggest fixes that come to mind.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
>
>