On Mon, Jun 05, 2023 at 09:06:54PM +0200, Konrad Dybcio wrote:
>
>
> On 5.06.2023 09:08, Stephan Gerhold wrote:
> > Add a simple driver for the qcom,rpm-proc compatible that registers the
> > "smd-edge" and populates other children defined in the device tree.
> >
> > Note that the DT schema belongs to the remoteproc subsystem while this
> > driver is added inside soc/qcom. I argue that the RPM *is* a remoteproc,
> > but as an implementation detail in Linux it can currently not benefit
> > from anything provided by the remoteproc subsystem. The RPM firmware is
> > usually already loaded and started by earlier components in the boot
> > chain and is not meant to be ever restarted.
> >
> > To avoid breaking existing kernel configurations the driver is always
> > built when smd-rpm.c is also built. They belong closely together anyway.
> >
> > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > ---
> > drivers/soc/qcom/Makefile | 2 +-
> > drivers/soc/qcom/rpm-proc.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index 99114c71092b..113b9ff2ad43 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -18,7 +18,7 @@ obj-$(CONFIG_QCOM_RPM_MASTER_STATS) += rpm_master_stats.o
> > obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o
> > qcom_rpmh-y += rpmh-rsc.o
> > qcom_rpmh-y += rpmh.o
> > -obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
> > +obj-$(CONFIG_QCOM_SMD_RPM) += rpm-proc.o smd-rpm.o
> > obj-$(CONFIG_QCOM_SMEM) += smem.o
> > obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> > obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> > diff --git a/drivers/soc/qcom/rpm-proc.c b/drivers/soc/qcom/rpm-proc.c
> > new file mode 100644
> > index 000000000000..0652be7f7895
> > --- /dev/null
> > +++ b/drivers/soc/qcom/rpm-proc.c
> > @@ -0,0 +1,76 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2021-2023, Stephan Gerhold <stephan@xxxxxxxxxxx> */
> > +
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/rpmsg/qcom_smd.h>
> > +
> > +static int rpm_proc_probe(struct platform_device *pdev)
> > +{
> > + struct qcom_smd_edge *edge = NULL;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *edge_node;
> > + int ret;
> > +
> > + edge_node = of_get_child_by_name(dev->of_node, "smd-edge");
> > + if (edge_node) {
> > + edge = qcom_smd_register_edge(dev, edge_node);
> > + if (IS_ERR(edge))
> > + return dev_err_probe(dev, PTR_ERR(edge),
> > + "Failed to register smd-edge\n");
> Need of_node_put in both success and IS_ERR paths
>
Oops. :/
> > + }
> > +
> > + ret = devm_of_platform_populate(dev);
> > + if (ret) {
> > + dev_err(dev, "Failed to populate children devices: %d\n", ret);
> I may be having a brain lag moment but I think it should be "child"
> singular, otherwise it sounds like they're devices for children!
>
Uh somehow both sounds fine to me so I'll just change it in v2. :)
> > + goto err;
> > + }
> > +
> > + platform_set_drvdata(pdev, edge);
> > + return 0;
> > +err:
> > + if (edge)
> > + qcom_smd_unregister_edge(edge);
> > + return ret;
> > +}
> > +
> > +static void rpm_proc_remove(struct platform_device *pdev)
> > +{
> > + struct qcom_smd_edge *edge = platform_get_drvdata(pdev);
> > +
> > + if (edge)
> > + qcom_smd_unregister_edge(edge);
> > +}
> > +
> > +static const struct of_device_id rpm_proc_of_match[] = {
> > + { .compatible = "qcom,rpm-proc", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rpm_proc_of_match);
> > +
> > +static struct platform_driver rpm_proc_driver = {
> > + .probe = rpm_proc_probe,
> > + .remove_new = rpm_proc_remove,
> > + .driver = {
> > + .name = "qcom-rpm-proc",
> > + .of_match_table = rpm_proc_of_match,
> > + },
> > +};
> > +
> > +static int __init rpm_proc_init(void)
> > +{
> > + return platform_driver_register(&rpm_proc_driver);
> > +}
> > +arch_initcall(rpm_proc_init);
> Maybe we can go as early as core...
>
SMEM is arch_initcall() so at least for the SMD case it can never
succeed probing in core_initcall() and would likely just cause
unnecessary probe deferrals. That's why I chose arch_initcall().
Are you sure anything will really benefit from core_initcall() here?
I'd just like to avoid making things worse by using a random way too
early initcall level. We have some really weird examples in the tree
currently, e.g.:
- rpmpd: core_initcall()
- smd-rpm: arch_initcall()
- glink-rpm: subsys_initcall()
But they actually need to be loaded in opposite order...
Thanks,
Stephan
