Re: [PATCH v7 5/6] soc: qcom: add pd-mapper implementation

2024-04-26 Thread Chris Lew




On 4/24/2024 2:28 AM, Dmitry Baryshkov wrote:


+static int qcom_pdm_start(void)
+{
+   const struct of_device_id *match;
+   const struct qcom_pdm_domain_data * const *domains;
+   struct device_node *root;
+   int ret, i;
+
+   root = of_find_node_by_path("/");
+   if (!root)
+   return -ENODEV;
+
+   match = of_match_node(qcom_pdm_domains, root);
+   of_node_put(root);
+   if (!match) {
+   pr_notice("PDM: no support for the platform, userspace daemon might 
be required.\n");
+   return -ENODEV;
+   }
+
+   domains = match->data;
+   if (!domains) {
+   pr_debug("PDM: no domains\n");
+   return -ENODEV;
+   }
+
+   mutex_lock(_pdm_mutex);
+   for (i = 0; domains[i]; i++) {
+   ret = qcom_pdm_add_domain(domains[i]);
+   if (ret)
+   goto free_domains;
+   }
+
+   ret = qmi_handle_init(_pdm_handle, 1024,
+ NULL, qcom_pdm_msg_handlers);


1024 here seems arbitrary, I think most other usage of qmi_handle_init 
has a macro defined for the max message length of the qmi service.



+   if (ret)
+   goto free_domains;
+
+   ret = qmi_add_server(_pdm_handle, SERVREG_LOCATOR_SERVICE,
+SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
+   if (ret) {
+   pr_err("PDM: error adding server %d\n", ret);
+   goto release_handle;
+   }
+   mutex_unlock(_pdm_mutex);
+
+   return 0;
+
+release_handle:
+   qmi_handle_release(_pdm_handle);
+
+free_domains:
+   qcom_pdm_free_domains();
+   mutex_unlock(_pdm_mutex);
+
+   return ret;
+}
+
+static void qcom_pdm_stop(void)
+{
+   qmi_del_server(_pdm_handle, SERVREG_LOCATOR_SERVICE,
+  SERVREG_QMI_VERSION, SERVREG_QMI_INSTANCE);
+
+   qmi_handle_release(_pdm_handle);
+


I don't think doing an explicit qmi_del_server() is necessary. As part 
of the qmi_handle_release(), the qrtr socket will be closed and the qrtr 
ns will broadcast a DEL_SERVER and DEL_CLIENT notification as part of 
the cleanup.



+   qcom_pdm_free_domains();
+}
+




Re: [PATCH v7 5/6] soc: qcom: add pd-mapper implementation

2024-04-26 Thread Alexey Minnekhanov

On 24.04.2024 12:28, Dmitry Baryshkov wrote:

Existing userspace protection domain mapper implementation has several
issue. It doesn't play well with CONFIG_EXTRA_FIRMWARE, it doesn't
reread JSON files if firmware location is changed (or if firmware was
not available at the time pd-mapper was started but the corresponding
directory is mounted later), etc.

Provide in-kernel service implementing protection domain mapping
required to work with several services, which are provided by the DSP
firmware.

This module is loaded automatically by the remoteproc drivers when
necessary via the symbol dependency. It uses a root node to match a
protection domains map for a particular board. It is not possible to
implement it as a 'driver' as there is no corresponding device.

Signed-off-by: Dmitry Baryshkov 




diff --git a/drivers/soc/qcom/qcom_pd_mapper.c 
b/drivers/soc/qcom/qcom_pd_mapper.c

...

+
+static const struct qcom_pdm_domain_data *sdm660_domains[] = {
+   _audio_pd,
+   _root_pd,
+   _wlan_pd,
+   NULL,
+};
+


On my SDM660 device (xiaomi-lavender) I see also the following files on 
modem partition:

 - adsps.jsn with sensor_pd (instance id 74)
 - cdspr.jsn with cdsp root_pd (instance id 76)
 - modemr.jsn with root_pd (instance id 180)

I see these numbers match those you already have defined above, so 
perhaps sdm660_domains should also have adsp_sensor_pd, cdsp_root_pd, 
and mpss_root_pd?


I'm not sure how useful they are currently, as AFAIK cdsp is not added 
to sdm660 DT at all; and ADSP sensors are very hard to use/test, needs 
very special userspace...


--
Regards,
Alexey Minnekhanov
postmarketOS developer