Re: [PATCH V5 2/7] soc: qcom-geni-se: Add interconnect support to fix earlycon crash

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Matthias,

On 5/8/2020 11:29 PM, Matthias Kaehlcke wrote:
Hi Akash,

overall this looks good to me, a few comments inline

On Fri, May 08, 2020 at 12:03:34PM +0530, Akash Asthana wrote:
QUP core clock is shared among all the SE drivers present on particular
QUP wrapper, the system will reset(unclocked access) if earlycon used after
QUP core clock is put to 0 from other SE drivers before real console comes
up.

As earlycon can't vote for it's QUP core need, to fix this add ICC
support to common/QUP wrapper driver and put vote for QUP core from
probe on behalf of earlycon and remove vote during earlycon exit call.

Signed-off-by: Akash Asthana <akashast@xxxxxxxxxxxxxx>
Reported-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
---
Change in V3:
  - Add geni_remove_earlycon_icc_vote API that will be used by earlycon
    exit function to remove ICC vote for earlyconsole.
  - Remove suspend/resume hook for geni-se driver as we are no longer
    removing earlyconsole ICC vote from system suspend, we are removing
    from earlycon exit.

Change in V4:
  - As per Matthias comment make 'earlycon_wrapper' as static structure.

Changes in V5:
  - Vote for core path only after checking whether "qcom_geni" earlycon is
    actually present or not by traversing over structure "console_drivers".

  drivers/soc/qcom/qcom-geni-se.c       | 63 +++++++++++++++++++++++++++++++++++
  drivers/tty/serial/qcom_geni_serial.c |  7 ++++
  include/linux/qcom-geni-se.h          |  2 ++
  3 files changed, 72 insertions(+)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 63403bf..66fe6f2 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -3,6 +3,7 @@
#include <linux/acpi.h>
  #include <linux/clk.h>
+#include <linux/console.h>
  #include <linux/slab.h>
  #include <linux/dma-mapping.h>
  #include <linux/io.h>
@@ -90,11 +91,14 @@ struct geni_wrapper {
  	struct device *dev;
  	void __iomem *base;
  	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
+	struct geni_icc_path to_core;
  };
static const char * const icc_path_names[] = {"qup-core", "qup-config",
  								"qup-memory"};
+static struct geni_wrapper *earlycon_wrapper;
+
  #define QUP_HW_VER_REG			0x4
/* Common SE registers */
@@ -812,11 +816,33 @@ int geni_icc_disable(struct geni_se *se)
  }
  EXPORT_SYMBOL(geni_icc_disable);
+void geni_remove_earlycon_icc_vote(void)
+{
+	struct geni_wrapper *wrapper = earlycon_wrapper;
+	struct device_node *parent = of_get_next_parent(wrapper->dev->of_node);
Do we need to check that earlycon_wrapper != NULL before dereferencing it?
In theory this should not happen, but better be safe.
Ok, I will add NULL check.

+	struct device_node *child;
+
+	for_each_child_of_node(parent, child) {
+		if (of_device_is_compatible(child, "qcom,geni-se-qup")) {
+			wrapper = platform_get_drvdata(of_find_device_by_node(
+					child));
+			icc_put(wrapper->to_core.path);
+			wrapper->to_core.path = NULL;
nit: setting the path to NULL isn't really needed IIUC.
icc_put just free the path and don't reinitialize the path to NULL, if the path is used after it is put target will crash. So just for safety I am setting this path to NULL.

+		}
+	}
+	of_node_put(parent);
+
+	earlycon_wrapper = NULL;
+}
+EXPORT_SYMBOL(geni_remove_earlycon_icc_vote);
+
  static int geni_se_probe(struct platform_device *pdev)
  {
  	struct device *dev = &pdev->dev;
  	struct resource *res;
  	struct geni_wrapper *wrapper;
+	struct console *bcon;
+	int earlycon_present = 0;
use bool & true/false

The variable is only used when CONFIG_SERIAL_EARLYCON is set, I think
you need to add '__maybe_unused' to avoid a compiler warning then earlycon
support is disabled.

bikeshed: 'has_earlycon' would be slightly more concise (feel free to ignore).
Ok

  	int ret;
wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
@@ -839,6 +865,43 @@ static int geni_se_probe(struct platform_device *pdev)
  		}
  	}
+#ifdef CONFIG_SERIAL_EARLYCON
+	if (console_drivers)
The loop should have curly braces ("use braces when a loop contains more than
a single simple statement"), even though the compiler doesn't need them in
this case. This is not a loop, but I was told by a maintainer that it equally
applies, which makes sense.

You could avoid one level of indentation through:
Ok

if (!console_drivers)
	goto exit;

I think I can omit this extra check because "for_each_console" will take care of this.


+		for_each_console(bcon)
ditto (braces)

+			if (!strcmp(bcon->name, "qcom_geni")) {
+				earlycon_present = 1;
+				break;
+			}
+	if (!earlycon_present)
+		goto exit;
+
+	wrapper->to_core.path = devm_of_icc_get(dev, "qup-core");
+	if (IS_ERR(wrapper->to_core.path))
+		return PTR_ERR(wrapper->to_core.path);
+	/*
+	 * Put minmal BW request on core clocks on behalf of early console.
+	 * The vote will be removed earlycon exit function.
+	 *
+	 * Note: We are putting vote on each QUP wrapper instead only to which
+	 * earlycon is connected because QUP core clock of different wrapper
+	 * share same voltage domain. If core1 is put to 0, then core2 will
+	 * also run at 0, if not voted. Default ICC vote will be removed ASA
+	 * we touch any of the core clock.
+	 * core1 = core2 = max(core1, core2)
+	 */
+	ret = icc_set_bw(wrapper->to_core.path, GENI_DEFAULT_BW,
+							GENI_DEFAULT_BW);
nit: the indentation is a bit odd. Align with 'wrapper->to_core.path' or a
nearby tab stop?

ok


Thanks for providing the feedback.

Regards,

Akash

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux