On Mon 20 Jun 07:48 PDT 2016, Srinivas Kandagatla wrote: > Thanks Bjorn for this patch, > > I will start playing with patch soon, but here are few comments. > > On 17/06/16 18:17, Bjorn Andersson wrote: > >From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx> > > > >This initial hack powers the q6v5, boots and authenticate the mba and > >use that to load the mdt and subsequent bXX files. > > > >Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx> > >Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> > >--- > > > >Changes since v1: > >- Corrected boot address in relocation case > >- Using rproc_da_to_va() to clean up mdt loader api > >- Dynamically allocating scratch space for mdt verification > > > > drivers/remoteproc/Kconfig | 12 + > > drivers/remoteproc/Makefile | 2 + > > drivers/remoteproc/qcom_mdt_loader.c | 166 +++++++ > > drivers/remoteproc/qcom_mdt_loader.h | 13 + > > drivers/remoteproc/qcom_q6v5_pil.c | 914 +++++++++++++++++++++++++++++++++++ > > We should probably split this patch into two one for mdt loader and other > for pil. > I did consider it, as it's currently out for review in two different patch sets, but I don't want to add dead code so it shouldn't be merged on its own. > Also checkpatch reports: > > total: 1 errors, 28 warnings, 1117 lines checked > I'll review that again. [..] > >diff --git a/drivers/remoteproc/qcom_mdt_loader.c b/drivers/remoteproc/qcom_mdt_loader.c > >new file mode 100644 > >index 000000000000..4efeda908d9a > >--- /dev/null > >+++ b/drivers/remoteproc/qcom_mdt_loader.c > >@@ -0,0 +1,166 @@ > >+/* > >+ * Qualcomm Peripheral Image Loader > >+ * > >+ * Copyright (C) 2016 Linaro Ltd > >+ * Copyright (C) 2015 Sony Mobile Communications Inc > >+ * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > >+ * > >+ * This program is free software; you can redistribute it and/or > >+ * modify it under the terms of the GNU General Public License > >+ * version 2 as published by the Free Software Foundation. > >+ * > >+ * This program is distributed in the hope that it will be useful, > >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of > >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >+ * GNU General Public License for more details. > >+ */ > >+ > >+#include <linux/elf.h> > >+#include <linux/firmware.h> > >+#include <linux/kernel.h> > >+#include <linux/module.h> > >+#include <linux/qcom_scm.h> > > ?? > Leftover from being TZ-only. > >+#include <linux/remoteproc.h> > >+#include <linux/slab.h> > ?? > For kfree() ? > >+ > >+#include "remoteproc_internal.h" > >+#include "qcom_mdt_loader.h" > >+ > >+/** > >+ * qcom_mdt_find_rsc_table() - provide dummy resource table for remoteproc > >+ * @rproc: remoteproc handle > >+ * @fw: firmware header > >+ * @tablesz: outgoing size of the table > >+ * > >+ * Returns a dummy table. > >+ */ > >+struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc, > >+ const struct firmware *fw, > >+ int *tablesz) > >+{ > >+ static struct resource_table table = { .ver = 1, }; > >+ > >+ *tablesz = sizeof(table); > >+ return &table; > >+} > >+EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table); > >+ > >+int qcom_mdt_parse(const struct firmware *fw, phys_addr_t *fw_addr, size_t *fw_size, bool *fw_relocate) > Missing doc for this function? > Yes, that should be added. > >+{ [..] > >+} > >+EXPORT_SYMBOL_GPL(qcom_mdt_parse); > >+ > >+/** > >+ * qcom_mdt_load() - load the firmware which header is defined in fw > >+ * @rproc: rproc handle > >+ * @pas_id: PAS identifier to load this firmware into > > ?? > Sorry, another leftover. Thanks for spotting that. > >+ * @fw: frimware object for the header > > s/frimware/firmware > Thanks. > >+ * > >+ * Returns 0 on success, negative errno otherwise. > >+ */ > >+int qcom_mdt_load(struct rproc *rproc, > >+ const struct firmware *fw, > >+ const char *firmware) > >+{ [..] > >+} > >+EXPORT_SYMBOL_GPL(qcom_mdt_load); > > Module Licence info? > Didn't consider it a module on it's own, but you're right. [..] > >diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c [..] > >+ > >+static void q6v5_regulator_disable(struct q6v5 *qproc) > >+{ > >+ regulator_bulk_disable(ARRAY_SIZE(qproc->supply), qproc->supply); > >+ regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_CX].consumer, 0, INT_MAX); > >+ regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_MX].consumer, 0, INT_MAX); > > we disable the regulators and then set voltage why? > I think these should be moved to q6v5_regulator_enable() unless am missing > something here. > Because during enable we reduce the valid range of voltages on the SMPS, regardless of it being enabled or not. So I need to broaden that vote for the application CPU to be allowed to vote for the lower voltages again. cx is however supposed to be a corner, so not sure if I should touch that here at all. I'll replace that line with a TODO comment. > >+ regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_MSS].consumer, 0, 1150000); > >+} > >+ [..] > >+ > >+static int q6v5_rmb_pbl_wait(struct q6v5 *qproc, int ms) > >+{ > >+ unsigned long timeout; > >+ s32 val; > >+ > >+ timeout = jiffies + msecs_to_jiffies(ms); > >+ for (;;) { > >+ val = readl(qproc->rmb_base + RMB_PBL_STATUS_REG); > >+ if (val) > > I think making an explicit check for a bits of interest would be much > readable. > Or a comment would be good. > All we do here is wait for the register to become non-zero; I can add a short comment on the function if you would like. > > >+ break; > >+ > >+ if (time_after(jiffies, timeout)) > >+ return -ETIMEDOUT; > >+ > >+ msleep(1); > >+ } > >+ > >+ return val; > >+} > >+ [..] > >+ > >+static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev) > >+{ > >+ struct device_node *halt_np; > >+ struct resource *res; > >+ int ret; > >+ > >+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "qdsp6"); > >+ qproc->reg_base = devm_ioremap_resource(&pdev->dev, res); > >+ if (IS_ERR(qproc->reg_base)) { > >+ dev_err(qproc->dev, "failed to get qdsp6_base\n"); > >+ return PTR_ERR(qproc->reg_base); > >+ } > >+ > >+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rmb"); > >+ qproc->rmb_base = devm_ioremap_resource(&pdev->dev, res); > >+ if (IS_ERR(qproc->rmb_base)) { > >+ dev_err(qproc->dev, "failed to get rmb_base\n"); > >+ return PTR_ERR(qproc->rmb_base); > >+ } > >+ > > >+ halt_np = of_parse_phandle(pdev->dev.of_node, "qcom,halt-regs", 0); > >+ if (!halt_np) { > >+ dev_err(&pdev->dev, "no qcom,halt-regs node\n"); > >+ return -ENODEV; > >+ } > >+ > >+ qproc->halt_map = syscon_node_to_regmap(halt_np); > >+ if (IS_ERR(qproc->halt_map)) > >+ return PTR_ERR(qproc->halt_map); > >+ > >+ ret = of_property_read_u32_index(pdev->dev.of_node, "qcom,halt-regs", > >+ 1, &qproc->halt_q6); > >+ if (ret < 0) { > >+ dev_err(&pdev->dev, "no q6 halt offset\n"); > >+ return -EINVAL; > >+ } > >+ > >+ ret = of_property_read_u32_index(pdev->dev.of_node, "qcom,halt-regs", > >+ 2, &qproc->halt_modem); > >+ if (ret < 0) { > >+ dev_err(&pdev->dev, "no modem halt offset\n"); > >+ return -EINVAL; > >+ } > >+ > >+ ret = of_property_read_u32_index(pdev->dev.of_node, "qcom,halt-regs", > >+ 3, &qproc->halt_nc); > >+ if (ret < 0) { > >+ dev_err(&pdev->dev, "no nc halt offset\n"); > >+ return -EINVAL; > >+ } > We could used of_parse_phandle_with_fixed_args() here. > Ahh that's better. Thanks! > >+ > >+ return 0; > >+} > >+ [..] > >+ > >+static int q6v5_alloc_memory_region(struct q6v5 *qproc) > >+{ > >+ struct device_node *child; > >+ struct device_node *node; > >+ struct resource r; > >+ int ret; > >+ > <--- > >+ child = of_get_child_by_name(qproc->dev->of_node, "mba"); > >+ node = of_parse_phandle(child, "memory-region", 0); > >+ ret = of_address_to_resource(node, 0, &r); > >+ if (ret) { > >+ dev_err(qproc->dev, "unable to resolve mba region\n"); > >+ return ret; > >+ } > >+ > >+ qproc->mba_phys = r.start; > >+ qproc->mba_size = resource_size(&r); > >+ qproc->mba_region = devm_ioremap_wc(qproc->dev, qproc->mba_phys, qproc->mba_size); > >+ if (!qproc->mba_region) { > >+ dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", > >+ &r.start, qproc->mba_size); > >+ return -EBUSY; > >+ } > >+ > --> > > Looks like same code below, we could add a function to do the same. > I'll give it some thought; in the end I'm hoping to hand this off to the remoteproc core as "carveouts" and have it deal with the mapping and whatnot. > >+ child = of_get_child_by_name(qproc->dev->of_node, "mpss"); > >+ node = of_parse_phandle(child, "memory-region", 0); > >+ ret = of_address_to_resource(node, 0, &r); > >+ if (ret) { > >+ dev_err(qproc->dev, "unable to resolve mpss region\n"); > >+ return ret; > >+ } > >+ > >+ qproc->mpss_phys = qproc->mpss_reloc = r.start; > >+ qproc->mpss_size = resource_size(&r); > >+ qproc->mpss_region = devm_ioremap_wc(qproc->dev, qproc->mpss_phys, qproc->mpss_size); > >+ if (!qproc->mpss_region) { > >+ dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", > >+ &r.start, qproc->mpss_size); > >+ return -EBUSY; > >+ } > >+ > >+ return 0; > >+} > >+ [..] > >+ > >+static int q6v5_remove(struct platform_device *pdev) > >+{ > >+ struct q6v5 *qproc = platform_get_drvdata(pdev); > >+ > >+ rproc_del(qproc->rproc); > >+ rproc_put(qproc->rproc); > clk_uprepare_disable of the used clks here? > resets? > We should not end up here without passing q6v5_stop(), which handles all resources but rom_clk; so I'll update this. > >+ > >+ return 0; > >+} > >+ [..] > >+ > >+module_platform_driver(q6v5_driver); > > Module licence? > Sure. Thanks for the review Srinivas! Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html