| Message ID | 20180521184559.20864-1-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sibi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v4.17-rc6]
[also build test ERROR on next-20180517]
[cannot apply to remoteproc/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sibi-Sankar/remoteproc-Introduce-prepare-unprepare-ops-for-rproc-coredump/20180522-133348
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
drivers/remoteproc/qcom_q6v5_pil.c: In function 'q6v5_start':
>> drivers/remoteproc/qcom_q6v5_pil.c:796:3: error: implicit declaration of function 'q6v5_enable_irqs'; did you mean 'enable_irq'? [-Werror=implicit-function-declaration]
q6v5_enable_irqs(qproc);
^~~~~~~~~~~~~~~~
enable_irq
cc1: some warnings being treated as errors
vim +796 drivers/remoteproc/qcom_q6v5_pil.c
725
726 static int q6v5_start(struct rproc *rproc)
727 {
728 struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
729 int xfermemop_ret;
730 int ret;
731
732 ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
733 qproc->proxy_reg_count);
734 if (ret) {
735 dev_err(qproc->dev, "failed to enable proxy supplies\n");
736 goto clear_coredump_pending;
737 }
738
739 ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
740 qproc->proxy_clk_count);
741 if (ret) {
742 dev_err(qproc->dev, "failed to enable proxy clocks\n");
743 goto disable_proxy_reg;
744 }
745
746 ret = q6v5_regulator_enable(qproc, qproc->active_regs,
747 qproc->active_reg_count);
748 if (ret) {
749 dev_err(qproc->dev, "failed to enable supplies\n");
750 goto disable_proxy_clk;
751 }
752 ret = reset_control_deassert(qproc->mss_restart);
753 if (ret) {
754 dev_err(qproc->dev, "failed to deassert mss restart\n");
755 goto disable_vdd;
756 }
757
758 ret = q6v5_clk_enable(qproc->dev, qproc->active_clks,
759 qproc->active_clk_count);
760 if (ret) {
761 dev_err(qproc->dev, "failed to enable clocks\n");
762 goto assert_reset;
763 }
764
765 /* Assign MBA image access in DDR to q6 */
766 xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true,
767 qproc->mba_phys,
768 qproc->mba_size);
769 if (xfermemop_ret) {
770 dev_err(qproc->dev,
771 "assigning Q6 access to mba memory failed: %d\n",
772 xfermemop_ret);
773 goto disable_active_clks;
774 }
775
776 writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
777
778 ret = q6v5proc_reset(qproc);
779 if (ret)
780 goto reclaim_mba;
781
782 ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
783 if (ret == -ETIMEDOUT) {
784 dev_err(qproc->dev, "MBA boot timed out\n");
785 goto halt_axi_ports;
786 } else if (ret != RMB_MBA_XPU_UNLOCKED &&
787 ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
788 dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
789 ret = -EINVAL;
790 goto halt_axi_ports;
791 }
792
793 if (qproc->coredump_pending) {
794 dev_info(qproc->dev, "MBA booted, skipping mpss for coredump\n");
795 qproc->coredump_pending = false;
> 796 q6v5_enable_irqs(qproc);
797 xfermemop_ret = q6v5_xfer_mem_ownership(qproc,
798 &qproc->mba_perm, false,
799 qproc->mba_phys,
800 qproc->mba_size);
801 if (xfermemop_ret)
802 dev_err(qproc->dev, "Failed to reclaim mba buffer\n");
803 return 0;
804 }
805
806 dev_info(qproc->dev, "MBA booted, loading mpss\n");
807
808 ret = q6v5_mpss_load(qproc);
809 if (ret)
810 goto reclaim_mpss;
811
812 ret = wait_for_completion_timeout(&qproc->start_done,
813 msecs_to_jiffies(5000));
814 if (ret == 0) {
815 dev_err(qproc->dev, "start timed out\n");
816 ret = -ETIMEDOUT;
817 goto reclaim_mpss;
818 }
819
820 xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
821 qproc->mba_phys,
822 qproc->mba_size);
823 if (xfermemop_ret)
824 dev_err(qproc->dev,
825 "Failed to reclaim mba buffer system may become unstable\n");
826 qproc->running = true;
827
828 q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
829 qproc->proxy_clk_count);
830 q6v5_regulator_disable(qproc, qproc->proxy_regs,
831 qproc->proxy_reg_count);
832
833 return 0;
834
835 reclaim_mpss:
836 xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
837 false, qproc->mpss_phys,
838 qproc->mpss_size);
839 WARN_ON(xfermemop_ret);
840
841 halt_axi_ports:
842 q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
843 q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
844 q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
845
846 reclaim_mba:
847 xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false,
848 qproc->mba_phys,
849 qproc->mba_size);
850 if (xfermemop_ret) {
851 dev_err(qproc->dev,
852 "Failed to reclaim mba buffer, system may become unstable\n");
853 }
854
855 disable_active_clks:
856 q6v5_clk_disable(qproc->dev, qproc->active_clks,
857 qproc->active_clk_count);
858
859 assert_reset:
860 reset_control_assert(qproc->mss_restart);
861 disable_vdd:
862 q6v5_regulator_disable(qproc, qproc->active_regs,
863 qproc->active_reg_count);
864 disable_proxy_clk:
865 q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
866 qproc->proxy_clk_count);
867 disable_proxy_reg:
868 q6v5_regulator_disable(qproc, qproc->proxy_regs,
869 qproc->proxy_reg_count);
870 clear_coredump_pending:
871 qproc->coredump_pending = false;
872
873 return ret;
874 }
875
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon 21 May 11:45 PDT 2018, Sibi Sankar wrote: > In some occasions the remoteproc device might need to > prepare some hardware before the coredump can be performed > and cleanup the state afterwards. > > Q6V5 modem requires the mba to be loaded before the > coredump and some cleanup of the resources afterwards. > This describes two different changes, so please put it in two+ patches. [..] > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index dfdaede9139e..010819e01279 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -333,6 +333,8 @@ struct firmware; > * @kick: kick a virtqueue (virtqueue id given as a parameter) > * @da_to_va: optional platform hook to perform address translations > * @load_rsc_table: load resource table from firmware image > + * @prepare_coredump: prepare function, called before coredump > + * @unprepare_coredump: unprepare function, called post coredump I believe there will be other cases where we will need driver-specific logic to extract the memory content of the segments, e.g. through custom hardware sequences or non-mmio reads. To support this I think we should extend the struct rproc_dump_segment to carry an optional "dump" function that if specified will be used instead of the memcpy in rproc_coredump(). Drivers can then for each segment specify this function, if needed. Through some restructuring in the msa driver and your patch you should be able to implement this using such a mechanism instead - and it would be useful to these other cases as well. PS. I hope we can get away from some of the conditionals in your patch through some restructuring of the code. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thanks for the review. On 05/29/2018 10:35 AM, Bjorn Andersson wrote: > On Mon 21 May 11:45 PDT 2018, Sibi Sankar wrote: > >> In some occasions the remoteproc device might need to >> prepare some hardware before the coredump can be performed >> and cleanup the state afterwards. >> >> Q6V5 modem requires the mba to be loaded before the >> coredump and some cleanup of the resources afterwards. >> > > This describes two different changes, so please put it in two+ patches. > The second change just describes an example of a remoteproc device that requires remoteproc coredump prepare and unprepare but sure with restructuring required in msa it definitely will require 2+ patches. > [..] >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index dfdaede9139e..010819e01279 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -333,6 +333,8 @@ struct firmware; >> * @kick: kick a virtqueue (virtqueue id given as a parameter) >> * @da_to_va: optional platform hook to perform address translations >> * @load_rsc_table: load resource table from firmware image >> + * @prepare_coredump: prepare function, called before coredump >> + * @unprepare_coredump: unprepare function, called post coredump > > I believe there will be other cases where we will need driver-specific > logic to extract the memory content of the segments, e.g. through custom > hardware sequences or non-mmio reads. > > To support this I think we should extend the struct rproc_dump_segment > to carry an optional "dump" function that if specified will be used > instead of the memcpy in rproc_coredump(). Drivers can then for each > segment specify this function, if needed. > In q6 modem mba needs to unlocked just once for all the segments to be dumped but as you say other remote processors may need it for each segment. This logic should be internal to the dump function anyway. So will use the dump function approach. What about the cleanup path, can we still reserve it till the coredump is done? > Through some restructuring in the msa driver and your patch you should > be able to implement this using such a mechanism instead - and it would > be useful to these other cases as well. > > > PS. I hope we can get away from some of the conditionals in your patch > through some restructuring of the code. > Thought you might preferred the conditionals with the least amount of code addition/change but having prepare and unprepare coredump again calling q6v5_start/stop respectively seemed a little hacky anyway. Sure will restructure msa as needed :) > Regards, > Bjorn >
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index 7656731a6c51..4a33e50b6aeb 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -173,6 +173,7 @@ struct q6v5 { struct completion start_done; struct completion stop_done; bool running; + bool coredump_pending; phys_addr_t mba_phys; void *mba_region; @@ -742,6 +743,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } mpss_reloc = relocate ? min_addr : qproc->mpss_phys; + qproc->mpss_reloc = mpss_reloc; /* Load firmware segments */ for (i = 0; i < ehdr->e_phnum; i++) { phdr = &phdrs[i]; @@ -816,7 +818,7 @@ static int q6v5_start(struct rproc *rproc) qproc->proxy_reg_count); if (ret) { dev_err(qproc->dev, "failed to enable proxy supplies\n"); - return ret; + goto clear_coredump_pending; } ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks, @@ -880,6 +882,19 @@ static int q6v5_start(struct rproc *rproc) goto halt_axi_ports; } + if (qproc->coredump_pending) { + dev_info(qproc->dev, "MBA booted, skipping mpss for coredump\n"); + qproc->coredump_pending = false; + q6v5_enable_irqs(qproc); + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, + &qproc->mba_perm, false, + qproc->mba_phys, + qproc->mba_size); + if (xfermemop_ret) + dev_err(qproc->dev, "Failed to reclaim mba buffer\n"); + return 0; + } + dev_info(qproc->dev, "MBA booted, loading mpss\n"); ret = q6v5_mpss_load(qproc); @@ -945,6 +960,8 @@ static int q6v5_start(struct rproc *rproc) disable_proxy_reg: q6v5_regulator_disable(qproc, qproc->proxy_regs, qproc->proxy_reg_count); +clear_coredump_pending: + qproc->coredump_pending = false; return ret; } @@ -955,17 +972,19 @@ static int q6v5_stop(struct rproc *rproc) int ret; u32 val; - qproc->running = false; - - qcom_smem_state_update_bits(qproc->state, - BIT(qproc->stop_bit), BIT(qproc->stop_bit)); + if (qproc->running) { + qproc->running = false; + qcom_smem_state_update_bits(qproc->state, + BIT(qproc->stop_bit), BIT(qproc->stop_bit)); - ret = wait_for_completion_timeout(&qproc->stop_done, - msecs_to_jiffies(5000)); - if (ret == 0) - dev_err(qproc->dev, "timed out on wait\n"); + ret = wait_for_completion_timeout(&qproc->stop_done, + msecs_to_jiffies(5000)); + if (ret == 0) + dev_err(qproc->dev, "timed out on wait\n"); - qcom_smem_state_update_bits(qproc->state, BIT(qproc->stop_bit), 0); + qcom_smem_state_update_bits(qproc->state, + BIT(qproc->stop_bit), 0); + } q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6); q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); @@ -1017,10 +1036,31 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len) return qproc->mpss_region + offset; } +static int qcom_mpss_register_dump_segments(struct rproc *rproc, + const struct firmware *fw_unused) +{ + const struct firmware *fw; + struct q6v5 *qproc = (struct q6v5 *)rproc->priv; + int ret; + + ret = request_firmware(&fw, "modem.mdt", qproc->dev); + if (ret < 0) { + dev_err(qproc->dev, "unable to load modem.mdt\n"); + return ret; + } + ret = qcom_register_dump_segments(rproc, fw); + + release_firmware(fw); + return ret; +} + static const struct rproc_ops q6v5_ops = { .start = q6v5_start, .stop = q6v5_stop, .da_to_va = q6v5_da_to_va, + .parse_fw = qcom_mpss_register_dump_segments, + .prepare_coredump = q6v5_start, + .unprepare_coredump = q6v5_stop, .load = q6v5_load, }; @@ -1036,6 +1076,7 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *dev) return IRQ_HANDLED; } + qproc->coredump_pending = true; msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, MPSS_CRASH_REASON_SMEM, &len); if (!IS_ERR(msg) && len > 0 && msg[0]) dev_err(qproc->dev, "watchdog received: %s\n", msg); @@ -1053,6 +1094,7 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *dev) size_t len; char *msg; + qproc->coredump_pending = true; msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, MPSS_CRASH_REASON_SMEM, &len); if (!IS_ERR(msg) && len > 0 && msg[0]) dev_err(qproc->dev, "fatal error received: %s\n", msg); diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index a9609d971f7f..8c254d9b5c67 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1083,6 +1083,9 @@ static void rproc_coredump(struct rproc *rproc) if (list_empty(&rproc->dump_segments)) return; + if (rproc_prepare_coredump(rproc)) + return; + data_size = sizeof(*ehdr); list_for_each_entry(segment, &rproc->dump_segments, node) { data_size += sizeof(*phdr) + segment->size; @@ -1139,6 +1142,8 @@ static void rproc_coredump(struct rproc *rproc) } dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL); + + rproc_unprepare_coredump(rproc); } /** diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h index 7570beb035b5..22a1b276e110 100644 --- a/drivers/remoteproc/remoteproc_internal.h +++ b/drivers/remoteproc/remoteproc_internal.h @@ -96,6 +96,22 @@ static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw) return 0; } +static inline int rproc_prepare_coredump(struct rproc *rproc) +{ + if (rproc->ops->prepare_coredump) + return rproc->ops->prepare_coredump(rproc); + + return 0; +} + +static inline int rproc_unprepare_coredump(struct rproc *rproc) +{ + if (rproc->ops->unprepare_coredump) + return rproc->ops->unprepare_coredump(rproc); + + return 0; +} + static inline struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw) diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index dfdaede9139e..010819e01279 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -333,6 +333,8 @@ struct firmware; * @kick: kick a virtqueue (virtqueue id given as a parameter) * @da_to_va: optional platform hook to perform address translations * @load_rsc_table: load resource table from firmware image + * @prepare_coredump: prepare function, called before coredump + * @unprepare_coredump: unprepare function, called post coredump * @find_loaded_rsc_table: find the loaded resouce table * @load: load firmeware to memory, where the remote processor * expects to find it @@ -345,6 +347,8 @@ struct rproc_ops { void (*kick)(struct rproc *rproc, int vqid); void * (*da_to_va)(struct rproc *rproc, u64 da, int len); int (*parse_fw)(struct rproc *rproc, const struct firmware *fw); + int (*prepare_coredump)(struct rproc *rproc); + int (*unprepare_coredump)(struct rproc *rproc); struct resource_table *(*find_loaded_rsc_table)( struct rproc *rproc, const struct firmware *fw); int (*load)(struct rproc *rproc, const struct firmware *fw);
In some occasions the remoteproc device might need to prepare some hardware before the coredump can be performed and cleanup the state afterwards. Q6V5 modem requires the mba to be loaded before the coredump and some cleanup of the resources afterwards. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- This patch depends on the following patches: https://patchwork.kernel.org/patch/10416047/ https://patchwork.kernel.org/patch/10416031/ drivers/remoteproc/qcom_q6v5_pil.c | 62 ++++++++++++++++++++---- drivers/remoteproc/remoteproc_core.c | 5 ++ drivers/remoteproc/remoteproc_internal.h | 16 ++++++ include/linux/remoteproc.h | 4 ++ 4 files changed, 77 insertions(+), 10 deletions(-)