| Message ID | 20240731-imx95-bbm-misc-v2-v7-0-a41394365602@nxp.com |
|---|---|
| Headers | show |
| Series | firmware: support i.MX95 SCMI BBM/MISC Extenstion | expand |
Hi Dmitry, > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 BBM > module > > Hi Peng, > > On Wed, Jul 31, 2024 at 03:37:18PM +0000, Peng Fan wrote: > > Hi Cristian, > > > > > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 BBM > > > module > > > > > > On Wed, Jul 31, 2024 at 08:56:11PM +0800, Peng Fan (OSS) wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > The BBM module provides BUTTON feature. To i.MX95, this > module is > > > > managed by System Manager and exported using System > > > Management Control > > > > Interface(SCMI). Linux could use i.MX SCMI BBM Extension > protocol > > > to > > > > use BUTTON feature. > > > > > > > > This driver is to use SCMI interface to enable pwrkey. > > > > > > > > +} > > > > + > > > > +static void scmi_imx_bbm_key_remove(struct scmi_device > *sdev) { > > > > + struct device *dev = &sdev->dev; > > > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > > > + > > > > + device_init_wakeup(dev, false); > > I do not believe you need to reset the wakeup flag on driver unbind, as > well as in the error handling path of probe(). If this is needed then > driver core should do this cleanup (maybe it already does?). I just check the driver core code, you are right, there is no need do this. DevAttrError: device_pm_remove-> device_wakeup_disable(dev); dpm_sysfs_remove > > > > > + > > > > + cancel_delayed_work_sync(&bbnsm->check_work); > > > > +} > > > > + > > > > > > ..so in v6 I asked you to add a cancel_delayed_work_sync() on the > > > removal path, BUT I missed, my bad, that indeed above there was > > > already a call to cancel_delayed_work_sync() associated to a > > > devm_add_action_or_reset....so now we have 2....also you should > try > > > not to mix devm_add_action_or_reset and plain .remove > methods..use > > > one or the other. > > > > Thanks for your detailed reviewing on this. I will wait to see if > > Sudeep has any comments to patch 1-4. If no comments, I will not do > a > > new version to this patchset. > > > > If v7 patch 1-4 are good for Sudeep to pick up, I will separate this > > patch out as a standalone one for input subsystem maintainer. > > If you remove the duplicated cancel_delayed_work_sync() in remove() > and unneded device_init_wakeup(dev, false); then you can merge the > input patch with the rest of them with my: > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thanks for your Ack. But I think patch 1-4 needs go to arm-scmi tree, Patch 5 to arm imx tree, patch 6 to rtc tree, patch 7 to input tree. I put the patches together in a patchset is to let reviewers could get a full picture how the whole stuff work. For patch 7, I will send out it as a separate patch with fix and tag after patch 1-4 is ready in arm-scmi tree. Thanks, Peng. > > Thanks. > > -- > Dmitry
On Thu, Aug 01, 2024 at 01:36:10AM +0000, Peng Fan wrote: > Hi Dmitry, > > > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 BBM > > module > > > > Hi Peng, > > > > On Wed, Jul 31, 2024 at 03:37:18PM +0000, Peng Fan wrote: > > > Hi Cristian, > > > > > > > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 BBM > > > > module > > > > > > > > On Wed, Jul 31, 2024 at 08:56:11PM +0800, Peng Fan (OSS) wrote: > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > The BBM module provides BUTTON feature. To i.MX95, this > > module is > > > > > managed by System Manager and exported using System > > > > Management Control > > > > > Interface(SCMI). Linux could use i.MX SCMI BBM Extension > > protocol > > > > to > > > > > use BUTTON feature. > > > > > > > > > > This driver is to use SCMI interface to enable pwrkey. > > > > > > > > > > +} > > > > > + > > > > > +static void scmi_imx_bbm_key_remove(struct scmi_device > > *sdev) { > > > > > + struct device *dev = &sdev->dev; > > > > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > > > > + > > > > > + device_init_wakeup(dev, false); > > > > I do not believe you need to reset the wakeup flag on driver unbind, as > > well as in the error handling path of probe(). If this is needed then > > driver core should do this cleanup (maybe it already does?). > > I just check the driver core code, you are right, there is > no need do this. > > DevAttrError: > device_pm_remove-> device_wakeup_disable(dev); > dpm_sysfs_remove > > > > > > > > + > > > > > + cancel_delayed_work_sync(&bbnsm->check_work); > > > > > +} > > > > > + > > > > > > > > ..so in v6 I asked you to add a cancel_delayed_work_sync() on the > > > > removal path, BUT I missed, my bad, that indeed above there was > > > > already a call to cancel_delayed_work_sync() associated to a > > > > devm_add_action_or_reset....so now we have 2....also you should > > try > > > > not to mix devm_add_action_or_reset and plain .remove > > methods..use > > > > one or the other. > > > > > > Thanks for your detailed reviewing on this. I will wait to see if > > > Sudeep has any comments to patch 1-4. If no comments, I will not do > > a > > > new version to this patchset. > > > > > > If v7 patch 1-4 are good for Sudeep to pick up, I will separate this > > > patch out as a standalone one for input subsystem maintainer. > > > > If you remove the duplicated cancel_delayed_work_sync() in remove() > > and unneded device_init_wakeup(dev, false); then you can merge the > > input patch with the rest of them with my: > > > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Thanks for your Ack. But I think patch 1-4 needs go to arm-scmi tree, > Patch 5 to arm imx tree, patch 6 to rtc tree, patch 7 to input tree. > > I put the patches together in a patchset is to let reviewers could > get a full picture how the whole stuff work. > > For patch 7, I will send out it as a separate patch with fix and tag > after patch 1-4 is ready in arm-scmi tree. Right, but to accelerate getting support for your part into the mainline I am OK with input piece not going through the input tree but together with the rest of the patches through some other tree, probably through arm-scmi. If they are not willing to take it then we will have to wait till core support lands in mainline and then I can pick up the input piece and move it through my tree. Thanks.
Hi Dmitry, > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 BBM > module > > On Thu, Aug 01, 2024 at 01:36:10AM +0000, Peng Fan wrote: > > Hi Dmitry, > > > > > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 BBM > > > module > > > > > > Hi Peng, > > > > > > On Wed, Jul 31, 2024 at 03:37:18PM +0000, Peng Fan wrote: > > > > Hi Cristian, > > > > > > > > > Subject: Re: [PATCH v7 7/7] input: keyboard: support i.MX95 > BBM > > > > > module > > > > > > > > > > On Wed, Jul 31, 2024 at 08:56:11PM +0800, Peng Fan (OSS) > wrote: > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > > > The BBM module provides BUTTON feature. To i.MX95, this > > > module is > > > > > > managed by System Manager and exported using System > > > > > Management Control > > > > > > Interface(SCMI). Linux could use i.MX SCMI BBM Extension > > > protocol > > > > > to > > > > > > use BUTTON feature. > > > > > > > > > > > > This driver is to use SCMI interface to enable pwrkey. > > > > > > > > > > > > +} > > > > > > + > > > > > > +static void scmi_imx_bbm_key_remove(struct scmi_device > > > *sdev) { > > > > > > + struct device *dev = &sdev->dev; > > > > > > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > > > > > > + > > > > > > + device_init_wakeup(dev, false); > > > > > > I do not believe you need to reset the wakeup flag on driver unbind, > > > as well as in the error handling path of probe(). If this is needed > > > then driver core should do this cleanup (maybe it already does?). > > > > I just check the driver core code, you are right, there is no need do > > this. > > > > DevAttrError: > > device_pm_remove-> device_wakeup_disable(dev); > dpm_sysfs_remove > > > > > > > > > > > + > > > > > > + cancel_delayed_work_sync(&bbnsm->check_work); > > > > > > +} > > > > > > + > > > > > > > > > > ..so in v6 I asked you to add a cancel_delayed_work_sync() on > > > > > the removal path, BUT I missed, my bad, that indeed above > there > > > > > was already a call to cancel_delayed_work_sync() associated to > a > > > > > devm_add_action_or_reset....so now we have 2....also you > should > > > try > > > > > not to mix devm_add_action_or_reset and plain .remove > > > methods..use > > > > > one or the other. > > > > > > > > Thanks for your detailed reviewing on this. I will wait to see if > > > > Sudeep has any comments to patch 1-4. If no comments, I will not > > > > do > > > a > > > > new version to this patchset. > > > > > > > > If v7 patch 1-4 are good for Sudeep to pick up, I will separate > > > > this patch out as a standalone one for input subsystem maintainer. > > > > > > If you remove the duplicated cancel_delayed_work_sync() in > remove() > > > and unneded device_init_wakeup(dev, false); then you can merge > the > > > input patch with the rest of them with my: > > > > > > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > Thanks for your Ack. But I think patch 1-4 needs go to arm-scmi tree, > > Patch 5 to arm imx tree, patch 6 to rtc tree, patch 7 to input tree. > > > > I put the patches together in a patchset is to let reviewers could get > > a full picture how the whole stuff work. > > > > For patch 7, I will send out it as a separate patch with fix and tag > > after patch 1-4 is ready in arm-scmi tree. > > Right, but to accelerate getting support for your part into the mainline I > am OK with input piece not going through the input tree but together > with the rest of the patches through some other tree, probably through > arm-scmi. Thanks for your supporting on this patchset. I appreciate! If they are not willing to take it then we will have to wait till > core support lands in mainline and then I can pick up the input piece > and move it through my tree. There is no rush here, I still need to wait Sudeep's comments on the scmi parts. So this patch probably needs go through your tree in the end. Thanks, Peng. > > Thanks. > > -- > Dmitry
Hi Sudeep, > Subject: [PATCH v7 0/7] firmware: support i.MX95 SCMI BBM/MISC > Extension Would you please give a look on patch [1-4]/7? Cristian has gave his R-b, are the patches good for you to pick up? Thanks, Peng. > > i.MX95 System Manager Firmware source: https://github.com/nxp- > imx/imx-sm To generate html from the repo: make html > > i.MX95 System Manager Firmware support vendor extension protocol: > - Battery Backed Module(BBM) Protocol > This protocol is intended provide access to the battery-backed module. > This contains persistent storage (GPR), an RTC, and the ON/OFF > button. > The protocol can also provide access to similar functions implemented > via > external board components. The BBM protocol provides functions to: > > - Describe the protocol version. > - Discover implementation attributes. > - Read/write GPR > - Discover the RTCs available in the system. > - Read/write the RTC time in seconds and ticks > - Set an alarm (per LM) in seconds > - Get notifications on RTC update, alarm, or rollover. > - Get notification on ON/OFF button activity. > > - MISC Protocol for misc settings > This includes controls that are misc settings/actions that must be > exposed from the SM to agents. They are device specific and are > usually > define to access bit fields in various mix block control modules, > IOMUX_GPR, and other GPR/CSR owned by the SM. > This protocol supports the following functions: > > - Describe the protocol version. > - Discover implementation attributes. > - Set/Get a control. > - Initiate an action on a control. > - Obtain platform (i.e. SM) build information. > - Obtain ROM passover data. > - Read boot/shutdown/reset information for the LM or the system. > > This patchset is to support the two protocols and users that use the > protocols. The upper protocol infomation is also included in patch 1 > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > Changes in v7: > - Just correct R-b tag from Rob to drop quotes "", and rebased > - Link to v6: https://lore.kernel.org/r/20240718-imx95-bbm-misc-v2- > v6-0-18f008e16e9d@nxp.com > > Changes in v6: > - Add R-b from Cristian for patch 2,3,4,5,6 > - Add a new function parameter 'bool enable' to rtc_alarm_set in patch > 2 > - Drop dev_err per RTC maintainer, move devm_rtc_register to function > end in patch 6 > - Address Cristian's comment to documentation. Also moved the > documentation to patch 3, which adds the imx.rst under > drivers/firmware/arm_scmi/imx > - Add remove hook to cancel_delayed_work_sync in patch 7 > - Link to v5: https://lore.kernel.org/r/20240621-imx95-bbm-misc-v2- > v5-0-b85a6bf778cb@nxp.com > > Changes in v5: > - Collected missing comments in v1, I not intend to miss any, and sorry > if I make something wrong. > - Update the documentation per Cristian's comments. Not sure we > need a new directory for firmware stuff, not firmware-guide direcotyr. > - Added R-b for patch 3 "firmware: arm_scmi: add initial support for > i.MX BBM protocol" > - For patch 4, added comments in scmi_imx_misc_ctrl_validate_id, use > num_sources in scmi_protocol_events, move > scmi_imx_misc_protocol_init > near init, use get_max_msg_size and drop MISC_MAX_VAL. > - Separate the sm-bbm.c into rtc and key drivers with > each has its own notifiy callback, put the driver in rtc and input > directory, handle error return, add kconfig for each driver, use > to_delayed_work, use READ/WRITE_ONCE, still keep ops as private, > device_init_wakeup set false if failure. > - For patch 5, Add kconfig for sm-misc.c. Only support one instance, so > add a check > ops in probe. > - Link to v4: https://lore.kernel.org/r/20240524-imx95-bbm-misc-v2- > v4-0-dc456995d590@nxp.com > > Changes in v4: > - Rebased to next-20240520 > - Added vendor/sub-vendor, currently the sub-vendor is "i.MX95 EVK", > this may not be proper, I will check with firmware owner on this to > seen any update. please still help review other parts of the patchset. > - Added constrain value in binding doc, change the property name from > nxp,wakeup-sources to nxp,ctrl-ids to match firmware definition. > - Put i.MX code under new directory imx/ > - Change the misc event from three to one, the code in previous > patchset > was wrong. > - Link to v3: https://lore.kernel.org/r/20240412-imx95-bbm-misc-v2- > v3-0-4380a4070980@nxp.com > > Changes in v3: > - Update cover letter and patch commit log to include more > information. > - Add documentation for BBM and MISC protocols under > Documentation/firmware-guide/nxp. Not sure if this is a good place. > - Fix the bindings, hope I have addressed the issues. > Drop imx,scmi.yaml. > Add nxp,imx95-scmi.yaml for NXP vendor protocol properties. > Add constraints, add nxp prefix for NXP vendor properties. > Use anyOf in arm,scmi.yaml to ref vendor yaml. > - Use cpu_to_le32 per Cristian > - Link to v2: https://lore.kernel.org/r/20240405-imx95-bbm-misc-v2- > v2-0-9fc9186856c2@nxp.com > > Changes in v2: > - Sorry for late update since v1. > - Add a new patch 1 > - Address imx,scmi.yaml issues > - Address comments for imx-sm-bbm.c and imx-sm-misc.c > - I not add vendor id since related patches not landed in linux-next. > - Link to v1: https://lore.kernel.org/r/20240202-imx95-bbm-misc-v1-0- > 3cb743020933@nxp.com > > --- > Peng Fan (7): > dt-bindings: firmware: add i.MX95 SCMI Extension protocol > firmware: arm_scmi: add initial support for i.MX BBM protocol > firmware: arm_scmi: add initial support for i.MX MISC protocol > firmware: arm_scmi: add NXP i.MX95 SCMI documentation > firmware: imx: add i.MX95 MISC driver > rtc: support i.MX95 BBM RTC > input: keyboard: support i.MX95 BBM module > > .../devicetree/bindings/firmware/arm,scmi.yaml | 5 +- > .../bindings/firmware/nxp,imx95-scmi.yaml | 43 + > drivers/firmware/arm_scmi/Kconfig | 2 + > drivers/firmware/arm_scmi/Makefile | 1 + > drivers/firmware/arm_scmi/imx/Kconfig | 23 + > drivers/firmware/arm_scmi/imx/Makefile | 3 + > drivers/firmware/arm_scmi/imx/imx-sm-bbm.c | 379 +++++++++ > drivers/firmware/arm_scmi/imx/imx-sm-misc.c | 315 ++++++++ > drivers/firmware/arm_scmi/imx/imx95.rst | 886 > +++++++++++++++++++++ > drivers/firmware/imx/Kconfig | 11 + > drivers/firmware/imx/Makefile | 1 + > drivers/firmware/imx/sm-misc.c | 119 +++ > drivers/input/keyboard/Kconfig | 11 + > drivers/input/keyboard/Makefile | 1 + > drivers/input/keyboard/imx-sm-bbm-key.c | 236 ++++++ > drivers/rtc/Kconfig | 8 + > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-imx-sm-bbm.c | 162 ++++ > include/linux/firmware/imx/sm.h | 33 + > include/linux/scmi_imx_protocol.h | 59 ++ > 20 files changed, 2298 insertions(+), 1 deletion(-) > --- > base-commit: 668d33c9ff922c4590c58754ab064aaf53c387dd > change-id: 20240405-imx95-bbm-misc-v2-b5e9d24adc42 > > Best regards, > -- > Peng Fan <peng.fan@nxp.com>
i.MX95 System Manager Firmware source: https://github.com/nxp-imx/imx-sm To generate html from the repo: make html i.MX95 System Manager Firmware support vendor extension protocol: - Battery Backed Module(BBM) Protocol This protocol is intended provide access to the battery-backed module. This contains persistent storage (GPR), an RTC, and the ON/OFF button. The protocol can also provide access to similar functions implemented via external board components. The BBM protocol provides functions to: - Describe the protocol version. - Discover implementation attributes. - Read/write GPR - Discover the RTCs available in the system. - Read/write the RTC time in seconds and ticks - Set an alarm (per LM) in seconds - Get notifications on RTC update, alarm, or rollover. - Get notification on ON/OFF button activity. - MISC Protocol for misc settings This includes controls that are misc settings/actions that must be exposed from the SM to agents. They are device specific and are usually define to access bit fields in various mix block control modules, IOMUX_GPR, and other GPR/CSR owned by the SM. This protocol supports the following functions: - Describe the protocol version. - Discover implementation attributes. - Set/Get a control. - Initiate an action on a control. - Obtain platform (i.e. SM) build information. - Obtain ROM passover data. - Read boot/shutdown/reset information for the LM or the system. This patchset is to support the two protocols and users that use the protocols. The upper protocol infomation is also included in patch 1 Signed-off-by: Peng Fan <peng.fan@nxp.com> Changes in v7: - Just correct R-b tag from Rob to drop quotes "", and rebased - Link to v6: https://lore.kernel.org/r/20240718-imx95-bbm-misc-v2-v6-0-18f008e16e9d@nxp.com Changes in v6: - Add R-b from Cristian for patch 2,3,4,5,6 - Add a new function parameter 'bool enable' to rtc_alarm_set in patch 2 - Drop dev_err per RTC maintainer, move devm_rtc_register to function end in patch 6 - Address Cristian's comment to documentation. Also moved the documentation to patch 3, which adds the imx.rst under drivers/firmware/arm_scmi/imx - Add remove hook to cancel_delayed_work_sync in patch 7 - Link to v5: https://lore.kernel.org/r/20240621-imx95-bbm-misc-v2-v5-0-b85a6bf778cb@nxp.com Changes in v5: - Collected missing comments in v1, I not intend to miss any, and sorry if I make something wrong. - Update the documentation per Cristian's comments. Not sure we need a new directory for firmware stuff, not firmware-guide direcotyr. - Added R-b for patch 3 "firmware: arm_scmi: add initial support for i.MX BBM protocol" - For patch 4, added comments in scmi_imx_misc_ctrl_validate_id, use num_sources in scmi_protocol_events, move scmi_imx_misc_protocol_init near init, use get_max_msg_size and drop MISC_MAX_VAL. - Separate the sm-bbm.c into rtc and key drivers with each has its own notifiy callback, put the driver in rtc and input directory, handle error return, add kconfig for each driver, use to_delayed_work, use READ/WRITE_ONCE, still keep ops as private, device_init_wakeup set false if failure. - For patch 5, Add kconfig for sm-misc.c. Only support one instance, so add a check ops in probe. - Link to v4: https://lore.kernel.org/r/20240524-imx95-bbm-misc-v2-v4-0-dc456995d590@nxp.com Changes in v4: - Rebased to next-20240520 - Added vendor/sub-vendor, currently the sub-vendor is "i.MX95 EVK", this may not be proper, I will check with firmware owner on this to seen any update. please still help review other parts of the patchset. - Added constrain value in binding doc, change the property name from nxp,wakeup-sources to nxp,ctrl-ids to match firmware definition. - Put i.MX code under new directory imx/ - Change the misc event from three to one, the code in previous patchset was wrong. - Link to v3: https://lore.kernel.org/r/20240412-imx95-bbm-misc-v2-v3-0-4380a4070980@nxp.com Changes in v3: - Update cover letter and patch commit log to include more information. - Add documentation for BBM and MISC protocols under Documentation/firmware-guide/nxp. Not sure if this is a good place. - Fix the bindings, hope I have addressed the issues. Drop imx,scmi.yaml. Add nxp,imx95-scmi.yaml for NXP vendor protocol properties. Add constraints, add nxp prefix for NXP vendor properties. Use anyOf in arm,scmi.yaml to ref vendor yaml. - Use cpu_to_le32 per Cristian - Link to v2: https://lore.kernel.org/r/20240405-imx95-bbm-misc-v2-v2-0-9fc9186856c2@nxp.com Changes in v2: - Sorry for late update since v1. - Add a new patch 1 - Address imx,scmi.yaml issues - Address comments for imx-sm-bbm.c and imx-sm-misc.c - I not add vendor id since related patches not landed in linux-next. - Link to v1: https://lore.kernel.org/r/20240202-imx95-bbm-misc-v1-0-3cb743020933@nxp.com --- Peng Fan (7): dt-bindings: firmware: add i.MX95 SCMI Extension protocol firmware: arm_scmi: add initial support for i.MX BBM protocol firmware: arm_scmi: add initial support for i.MX MISC protocol firmware: arm_scmi: add NXP i.MX95 SCMI documentation firmware: imx: add i.MX95 MISC driver rtc: support i.MX95 BBM RTC input: keyboard: support i.MX95 BBM module .../devicetree/bindings/firmware/arm,scmi.yaml | 5 +- .../bindings/firmware/nxp,imx95-scmi.yaml | 43 + drivers/firmware/arm_scmi/Kconfig | 2 + drivers/firmware/arm_scmi/Makefile | 1 + drivers/firmware/arm_scmi/imx/Kconfig | 23 + drivers/firmware/arm_scmi/imx/Makefile | 3 + drivers/firmware/arm_scmi/imx/imx-sm-bbm.c | 379 +++++++++ drivers/firmware/arm_scmi/imx/imx-sm-misc.c | 315 ++++++++ drivers/firmware/arm_scmi/imx/imx95.rst | 886 +++++++++++++++++++++ drivers/firmware/imx/Kconfig | 11 + drivers/firmware/imx/Makefile | 1 + drivers/firmware/imx/sm-misc.c | 119 +++ drivers/input/keyboard/Kconfig | 11 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/imx-sm-bbm-key.c | 236 ++++++ drivers/rtc/Kconfig | 8 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-imx-sm-bbm.c | 162 ++++ include/linux/firmware/imx/sm.h | 33 + include/linux/scmi_imx_protocol.h | 59 ++ 20 files changed, 2298 insertions(+), 1 deletion(-) --- base-commit: 668d33c9ff922c4590c58754ab064aaf53c387dd change-id: 20240405-imx95-bbm-misc-v2-b5e9d24adc42 Best regards,