[PATCH 0/5] Self authenticating hexagon driver for q6v55

[PATCH 0/5] Self authenticating hexagon driver for q6v55

[Avaneesh Kumar Dwivedi]

This patchset series modifies existing hexagon v5 driver to work with
hexagon v55 module. hexagon v5 driver need modification because of 
difference in resources as well as their handling wrt to v55. Resources
required such as clock and regulator are different for v55 than v5,
hence separate set of routines are implemented to initialize and enable 
hexagon v55 resource list, these routines are invoked based on 
differentiation through compatible string matches.

These patches are compiled and tested with kernel tip for hexagon v5 boot
functionality.

Avaneesh Kumar Dwivedi (5):
  remoteproc: Add q6v55 specific parameters and enable probing for q6v55
	This patch modifies device private data structure to incorporate
	aditional v55 device specific information, add certain parameters to 
	be used with hexagon v55, add v55 specific compatible string to 
	enable probe for hexagon v55 and add compatible string in dtbinding
	of devicetree.
  remoteproc: Adding q6v55 specific regulator, clk, reset interface.
	This patch implement routines to initialize and enable regulator
	and clock resources for hexagon v55, it also change reset register 
	programming method from secure to non secure.
  remoteproc: Adding reset sequence and halt seq changes for q6v55
	This patch add hexagon v55 specific reset sequence.
	This also remove IDLE check before asserting halt to qdsp axi ports.
  remoteproc: Add start and shutdown interface for q6v55
	This patch add start and shutdown interface of hexagon v55 and plug them
	into new common routine which invoke v5 or v55 specific start or shutdown.
  remoteproc: Modifying probe for initializing q6v55 specific resources
	This patch identify qdsp version and based on that invoke individual
	initialization routines.

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   3 +-
 drivers/remoteproc/qcom_q6v5_pil.c                 | 616 ++++++++++++++++++++-
 2 files changed, 595 insertions(+), 24 deletions(-)

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

[PATCH 1/5] remoteproc: Add q6v55 specific parameters and enable probing for q6v55

[Avaneesh Kumar Dwivedi]

Adding required definition of parameters along with new structure
fields specific to q6v55 and enabling probe for q6v55 mss remote-
proc driver.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  3 +-
 drivers/remoteproc/qcom_q6v5_pil.c                 | 33 ++++++++++++++++++++--
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 57cb49e..0986f8b 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -7,7 +7,8 @@ on the Qualcomm Hexagon core.
 	Usage: required
 	Value type: <string>
 	Definition: must be one of:
-		    "qcom,q6v5-pil"
+		    "qcom,q6v5-pil",
+			"qcom,q6v55-pil"
 
 - reg:
 	Usage: required
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 2e0caaa..8df95a0 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -30,13 +30,14 @@
 #include <linux/reset.h>
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
+#include <linux/mutex.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_mdt_loader.h"
 
 #include <linux/qcom_scm.h>
 
-#define MBA_FIRMWARE_NAME		"mba.b00"
+#define MBA_FIRMWARE_NAME		"mba.mbn"
 #define MPSS_FIRMWARE_NAME		"modem.mdt"
 
 #define MPSS_CRASH_REASON_SMEM		421
@@ -65,6 +66,8 @@
 #define QDSP6SS_RESET_REG		0x014
 #define QDSP6SS_GFMUX_CTL_REG		0x020
 #define QDSP6SS_PWR_CTL_REG		0x030
+#define QDSP6SS_MEM_PWR_CTL		0x0B0
+#define QDSP6SS_STRAP_ACC		0x110
 
 /* AXI Halt Register Offsets */
 #define AXI_HALTREQ_REG			0x0
@@ -93,13 +96,24 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+/* QDSP6v55 parameters */
+#define QDSP6v55_LDO_BYP                BIT(25)
+#define QDSP6v55_BHS_ON                 BIT(24)
+#define QDSP6v55_CLAMP_WL               BIT(21)
+#define QDSP6v55_CLAMP_QMC_MEM          BIT(22)
+
+#define HALT_CHECK_MAX_LOOPS            (200)
+#define QDSP6SS_XO_CBCR                 (0x0038)
+
+#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
+
 struct q6v5 {
 	struct device *dev;
 	struct rproc *rproc;
 
 	void __iomem *reg_base;
 	void __iomem *rmb_base;
-
+	void __iomem *restart_reg;
 	struct regmap *halt_map;
 	u32 halt_q6;
 	u32 halt_modem;
@@ -115,6 +129,13 @@ struct q6v5 {
 	struct clk *ahb_clk;
 	struct clk *axi_clk;
 	struct clk *rom_clk;
+	struct clk *gpll0_mss_clk;
+	struct clk *snoc_axi_clk;
+	struct clk *mnoc_axi_clk;
+
+	struct clk *xo;
+	struct clk *pnoc_clk;
+	struct clk *qdss_clk;
 
 	struct completion start_done;
 	struct completion stop_done;
@@ -128,13 +149,18 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+	struct mutex q6_lock;
+	u32 boot_count;
+	bool unvote_flag;
+	bool is_q6v55;
+	bool ahb_clk_vote;
 };
 
 enum {
 	Q6V5_SUPPLY_CX,
 	Q6V5_SUPPLY_MX,
-	Q6V5_SUPPLY_MSS,
 	Q6V5_SUPPLY_PLL,
+	Q6V5_SUPPLY_MSS,
 };
 
 static int q6v5_regulator_init(struct q6v5 *qproc)
@@ -892,6 +918,7 @@ static int q6v5_remove(struct platform_device *pdev)
 
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", },
+	{ .compatible = "qcom,q6v55-pil", },
 	{ },
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/5] remoteproc: Add q6v55 specific parameters and enable probing for q6v55

[Bjorn Andersson]

On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> Adding required definition of parameters along with new structure
> fields specific to q6v55 and enabling probe for q6v55 mss remote-
> proc driver.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  3 +-
>  drivers/remoteproc/qcom_q6v5_pil.c                 | 33 ++++++++++++++++++++--
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 57cb49e..0986f8b 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -7,7 +7,8 @@ on the Qualcomm Hexagon core.
>  	Usage: required
>  	Value type: <string>
>  	Definition: must be one of:
> -		    "qcom,q6v5-pil"
> +		    "qcom,q6v5-pil",
> +			"qcom,q6v55-pil"
>  
>  - reg:
>  	Usage: required
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 2e0caaa..8df95a0 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -30,13 +30,14 @@
>  #include <linux/reset.h>
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
> +#include <linux/mutex.h>
>  
>  #include "remoteproc_internal.h"
>  #include "qcom_mdt_loader.h"
>  
>  #include <linux/qcom_scm.h>
>  
> -#define MBA_FIRMWARE_NAME		"mba.b00"
> +#define MBA_FIRMWARE_NAME		"mba.mbn"

On 8974 we must load the mba.b00, on 8916 and 8996 we must load mba.mbn.
But looking at downstream we seem to have:

8974: q6v5 -> mba.b00
8916: q6v56 -> mba.mbn
8996: q6v55 -> mba.mbn

So we should be able to pick this based on compatible then.

>  #define MPSS_FIRMWARE_NAME		"modem.mdt"
>  
>  #define MPSS_CRASH_REASON_SMEM		421
> @@ -65,6 +66,8 @@
>  #define QDSP6SS_RESET_REG		0x014
>  #define QDSP6SS_GFMUX_CTL_REG		0x020
>  #define QDSP6SS_PWR_CTL_REG		0x030
> +#define QDSP6SS_MEM_PWR_CTL		0x0B0
> +#define QDSP6SS_STRAP_ACC		0x110
>  
>  /* AXI Halt Register Offsets */
>  #define AXI_HALTREQ_REG			0x0
> @@ -93,13 +96,24 @@
>  #define QDSS_BHS_ON			BIT(21)
>  #define QDSS_LDO_BYP			BIT(22)
>  
> +/* QDSP6v55 parameters */
> +#define QDSP6v55_LDO_BYP                BIT(25)
> +#define QDSP6v55_BHS_ON                 BIT(24)
> +#define QDSP6v55_CLAMP_WL               BIT(21)
> +#define QDSP6v55_CLAMP_QMC_MEM          BIT(22)
> +
> +#define HALT_CHECK_MAX_LOOPS            (200)
> +#define QDSP6SS_XO_CBCR                 (0x0038)
> +
> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
> +
>  struct q6v5 {
>  	struct device *dev;
>  	struct rproc *rproc;
>  
>  	void __iomem *reg_base;
>  	void __iomem *rmb_base;
> -
> +	void __iomem *restart_reg;

The restart_reg is a register in the gcc, on 8974 this is exposed as a
reset by gcc. Please add the GCC_MSS_RESTART to the list of resets in
gcc-msm8996 rather than remapping and poking the register directly from
this driver.

>  	struct regmap *halt_map;
>  	u32 halt_q6;
>  	u32 halt_modem;
> @@ -115,6 +129,13 @@ struct q6v5 {
>  	struct clk *ahb_clk;
>  	struct clk *axi_clk;
>  	struct clk *rom_clk;
> +	struct clk *gpll0_mss_clk;
> +	struct clk *snoc_axi_clk;
> +	struct clk *mnoc_axi_clk;
> +
> +	struct clk *xo;
> +	struct clk *pnoc_clk;
> +	struct clk *qdss_clk;

Please introduce these changes as they are needed by the implementation,
rather than in a separate patch. It makes it much much easier to review.

>  
>  	struct completion start_done;
>  	struct completion stop_done;
> @@ -128,13 +149,18 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +	struct mutex q6_lock;
> +	u32 boot_count;
> +	bool unvote_flag;
> +	bool is_q6v55;
> +	bool ahb_clk_vote;

Dito

>  };
>  
>  enum {
>  	Q6V5_SUPPLY_CX,
>  	Q6V5_SUPPLY_MX,
> -	Q6V5_SUPPLY_MSS,
>  	Q6V5_SUPPLY_PLL,
> +	Q6V5_SUPPLY_MSS,
>  };
>  
>  static int q6v5_regulator_init(struct q6v5 *qproc)
> @@ -892,6 +918,7 @@ static int q6v5_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id q6v5_of_match[] = {
>  	{ .compatible = "qcom,q6v5-pil", },
> +	{ .compatible = "qcom,q6v55-pil", },

Just out of curiosity, is the version 55 or 5.5?

>  	{ },
>  };

Regards,
Bjorn

Re: [PATCH 1/5] remoteproc: Add q6v55 specific parameters and enable probing for q6v55

[Avaneesh Kumar Dwivedi]

On 10/26/2016 12:17 AM, Bjorn Andersson wrote:
> On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Adding required definition of parameters along with new structure
>> fields specific to q6v55 and enabling probe for q6v55 mss remote-
>> proc driver.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  3 +-
>>   drivers/remoteproc/qcom_q6v5_pil.c                 | 33 ++++++++++++++++++++--
>>   2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> index 57cb49e..0986f8b 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
>> @@ -7,7 +7,8 @@ on the Qualcomm Hexagon core.
>>   	Usage: required
>>   	Value type: <string>
>>   	Definition: must be one of:
>> -		    "qcom,q6v5-pil"
>> +		    "qcom,q6v5-pil",
>> +			"qcom,q6v55-pil"
>>   
>>   - reg:
>>   	Usage: required
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 2e0caaa..8df95a0 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -30,13 +30,14 @@
>>   #include <linux/reset.h>
>>   #include <linux/soc/qcom/smem.h>
>>   #include <linux/soc/qcom/smem_state.h>
>> +#include <linux/mutex.h>
>>   
>>   #include "remoteproc_internal.h"
>>   #include "qcom_mdt_loader.h"
>>   
>>   #include <linux/qcom_scm.h>
>>   
>> -#define MBA_FIRMWARE_NAME		"mba.b00"
>> +#define MBA_FIRMWARE_NAME		"mba.mbn"
> On 8974 we must load the mba.b00, on 8916 and 8996 we must load mba.mbn.
> But looking at downstream we seem to have:
>
> 8974: q6v5 -> mba.b00
> 8916: q6v56 -> mba.mbn
> 8996: q6v55 -> mba.mbn
>
> So we should be able to pick this based on compatible then.
OK, have take care of in patch set v2
>
>>   #define MPSS_FIRMWARE_NAME		"modem.mdt"
>>   
>>   #define MPSS_CRASH_REASON_SMEM		421
>> @@ -65,6 +66,8 @@
>>   #define QDSP6SS_RESET_REG		0x014
>>   #define QDSP6SS_GFMUX_CTL_REG		0x020
>>   #define QDSP6SS_PWR_CTL_REG		0x030
>> +#define QDSP6SS_MEM_PWR_CTL		0x0B0
>> +#define QDSP6SS_STRAP_ACC		0x110
>>   
>>   /* AXI Halt Register Offsets */
>>   #define AXI_HALTREQ_REG			0x0
>> @@ -93,13 +96,24 @@
>>   #define QDSS_BHS_ON			BIT(21)
>>   #define QDSS_LDO_BYP			BIT(22)
>>   
>> +/* QDSP6v55 parameters */
>> +#define QDSP6v55_LDO_BYP                BIT(25)
>> +#define QDSP6v55_BHS_ON                 BIT(24)
>> +#define QDSP6v55_CLAMP_WL               BIT(21)
>> +#define QDSP6v55_CLAMP_QMC_MEM          BIT(22)
>> +
>> +#define HALT_CHECK_MAX_LOOPS            (200)
>> +#define QDSP6SS_XO_CBCR                 (0x0038)
>> +
>> +#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
>> +
>>   struct q6v5 {
>>   	struct device *dev;
>>   	struct rproc *rproc;
>>   
>>   	void __iomem *reg_base;
>>   	void __iomem *rmb_base;
>> -
>> +	void __iomem *restart_reg;
> The restart_reg is a register in the gcc, on 8974 this is exposed as a
> reset by gcc. Please add the GCC_MSS_RESTART to the list of resets in
> gcc-msm8996 rather than remapping and poking the register directly from
> this driver.
         Infact i had done it the way suggested above before i sent out 
initial
         patchset, but then when i discussed with internal clock team, 
they said they will
         not support MSS RESET to be done via GCC because

         "MSS_RESET is not a block reset, GCC reset controller is used 
only when we need a BCR to be reset,
         and which has a special purpose. MSS RESET doesn't fall under 
block resets, it is not a clock and
         cannot be associated with clock."

         Downstream also MSS RESET is programmed through dev_ioremap.
>
>>   	struct regmap *halt_map;
>>   	u32 halt_q6;
>>   	u32 halt_modem;
>> @@ -115,6 +129,13 @@ struct q6v5 {
>>   	struct clk *ahb_clk;
>>   	struct clk *axi_clk;
>>   	struct clk *rom_clk;
>> +	struct clk *gpll0_mss_clk;
>> +	struct clk *snoc_axi_clk;
>> +	struct clk *mnoc_axi_clk;
>> +
>> +	struct clk *xo;
>> +	struct clk *pnoc_clk;
>> +	struct clk *qdss_clk;
> Please introduce these changes as they are needed by the implementation,
> rather than in a separate patch. It makes it much much easier to review.
OK, have take care of in patch set v2
>
>>   
>>   	struct completion start_done;
>>   	struct completion stop_done;
>> @@ -128,13 +149,18 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +	struct mutex q6_lock;
>> +	u32 boot_count;
>> +	bool unvote_flag;
>> +	bool is_q6v55;
>> +	bool ahb_clk_vote;
> Dito
>
>>   };
>>   
>>   enum {
>>   	Q6V5_SUPPLY_CX,
>>   	Q6V5_SUPPLY_MX,
>> -	Q6V5_SUPPLY_MSS,
>>   	Q6V5_SUPPLY_PLL,
>> +	Q6V5_SUPPLY_MSS,
>>   };
>>   
>>   static int q6v5_regulator_init(struct q6v5 *qproc)
>> @@ -892,6 +918,7 @@ static int q6v5_remove(struct platform_device *pdev)
>>   
>>   static const struct of_device_id q6v5_of_match[] = {
>>   	{ .compatible = "qcom,q6v5-pil", },
>> +	{ .compatible = "qcom,q6v55-pil", },
> Just out of curiosity, is the version 55 or 5.5?
Version is q6v56 1.3
>
>>   	{ },
>>   };
> Regards,
> Bjorn

Re: [PATCH 1/5] remoteproc: Add q6v55 specific parameters and enable probing for q6v55

[Bjorn Andersson]

On Fri 04 Nov 06:27 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> 
> 
> On 10/26/2016 12:17 AM, Bjorn Andersson wrote:
> >On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:
> >
> >>Adding required definition of parameters along with new structure
> >>fields specific to q6v55 and enabling probe for q6v55 mss remote-
> >>proc driver.
> >>
> >>Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> >>---
> >>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  3 +-
> >>  drivers/remoteproc/qcom_q6v5_pil.c                 | 33 ++++++++++++++++++++--
> >>  2 files changed, 32 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> >>index 57cb49e..0986f8b 100644
> >>--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> >>+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> >>@@ -7,7 +7,8 @@ on the Qualcomm Hexagon core.
> >>  	Usage: required
> >>  	Value type: <string>
> >>  	Definition: must be one of:
> >>-		    "qcom,q6v5-pil"
> >>+		    "qcom,q6v5-pil",
> >>+			"qcom,q6v55-pil"
> >>  - reg:
> >>  	Usage: required
> >>diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> >>index 2e0caaa..8df95a0 100644
> >>--- a/drivers/remoteproc/qcom_q6v5_pil.c
> >>+++ b/drivers/remoteproc/qcom_q6v5_pil.c
> >>@@ -30,13 +30,14 @@
> >>  #include <linux/reset.h>
> >>  #include <linux/soc/qcom/smem.h>
> >>  #include <linux/soc/qcom/smem_state.h>
> >>+#include <linux/mutex.h>
> >>  #include "remoteproc_internal.h"
> >>  #include "qcom_mdt_loader.h"
> >>  #include <linux/qcom_scm.h>
> >>-#define MBA_FIRMWARE_NAME		"mba.b00"
> >>+#define MBA_FIRMWARE_NAME		"mba.mbn"
> >On 8974 we must load the mba.b00, on 8916 and 8996 we must load mba.mbn.
> >But looking at downstream we seem to have:
> >
> >8974: q6v5 -> mba.b00
> >8916: q6v56 -> mba.mbn
> >8996: q6v55 -> mba.mbn
> >
> >So we should be able to pick this based on compatible then.
> OK, have take care of in patch set v2
> >
> >>  #define MPSS_FIRMWARE_NAME		"modem.mdt"
> >>  #define MPSS_CRASH_REASON_SMEM		421
> >>@@ -65,6 +66,8 @@
> >>  #define QDSP6SS_RESET_REG		0x014
> >>  #define QDSP6SS_GFMUX_CTL_REG		0x020
> >>  #define QDSP6SS_PWR_CTL_REG		0x030
> >>+#define QDSP6SS_MEM_PWR_CTL		0x0B0
> >>+#define QDSP6SS_STRAP_ACC		0x110
> >>  /* AXI Halt Register Offsets */
> >>  #define AXI_HALTREQ_REG			0x0
> >>@@ -93,13 +96,24 @@
> >>  #define QDSS_BHS_ON			BIT(21)
> >>  #define QDSS_LDO_BYP			BIT(22)
> >>+/* QDSP6v55 parameters */
> >>+#define QDSP6v55_LDO_BYP                BIT(25)
> >>+#define QDSP6v55_BHS_ON                 BIT(24)
> >>+#define QDSP6v55_CLAMP_WL               BIT(21)
> >>+#define QDSP6v55_CLAMP_QMC_MEM          BIT(22)
> >>+
> >>+#define HALT_CHECK_MAX_LOOPS            (200)
> >>+#define QDSP6SS_XO_CBCR                 (0x0038)
> >>+
> >>+#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
> >>+
> >>  struct q6v5 {
> >>  	struct device *dev;
> >>  	struct rproc *rproc;
> >>  	void __iomem *reg_base;
> >>  	void __iomem *rmb_base;
> >>-
> >>+	void __iomem *restart_reg;
> >The restart_reg is a register in the gcc, on 8974 this is exposed as a
> >reset by gcc. Please add the GCC_MSS_RESTART to the list of resets in
> >gcc-msm8996 rather than remapping and poking the register directly from
> >this driver.
> Infact i had done it the way suggested above before i sent out initial
> patchset, but then when i discussed with internal clock team, they
> said they will not support MSS RESET to be done via GCC because
> "MSS_RESET is not a block reset, GCC reset controller is used only
> when we need a BCR to be reset, and which has a special purpose. MSS
> RESET doesn't fall under block resets, it is not a clock and cannot be
> associated with clock."

The mss reset is a "reset" and we've shown that modelling it through a
reset-controller in DT and Linux makes the remoteproc driver cleaner.

We could implement an additional reset-controller, for the
non-block-reset resets of GCC, but I can't see any technical reason for
doing so

Can you please help me understand the possible technical reasons for not
having mss reset handled through the same reset-controller as the other
resets in gcc?

For prior platforms the upstream driver does expose these resets through
the same reset-controller as the block resets.

> 
> Downstream also MSS RESET is programmed through dev_ioremap.

A lot of the downstream code was designed and written before the reset
controller framework was invented, so that's not a good argument to
stick with ioremap.

Regards,
Bjorn

[PATCH 2/5] remoteproc: Adding q6v55 specific regulator, clk, reset interface.

[Avaneesh Kumar Dwivedi]

q6v55 use different regulator and clock resource than q6v5, hence
using separate resource handling for same. Also reset register
programming in q6v55 is non secure so resource controller init-
ilization is not required for q6v55.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 271 +++++++++++++++++++++++++++++++++++++
 1 file changed, 271 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8df95a0..c7dca40 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -215,6 +215,203 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
 	regulator_set_voltage(mss, 0, 1150000);
 }
 
+static int q6v55_regulator_init(struct q6v5 *qproc)
+{
+	int ret;
+
+	qproc->supply[Q6V5_SUPPLY_CX].supply = "vdd_cx";
+	qproc->supply[Q6V5_SUPPLY_MX].supply = "vdd_mx";
+	qproc->supply[Q6V5_SUPPLY_PLL].supply = "vdd_pll";
+
+	ret = devm_regulator_bulk_get(qproc->dev,
+			(ARRAY_SIZE(qproc->supply) - 1), qproc->supply);
+	if (ret < 0) {
+		dev_err(qproc->dev, "failed to get supplies\n");
+		return ret;
+	}
+
+
+	return 0;
+}
+
+static int q6v55_clk_enable(struct q6v5 *qproc)
+{
+	int ret;
+
+	ret = clk_prepare_enable(qproc->ahb_clk);
+	if (ret)
+		goto out;
+
+	ret = clk_prepare_enable(qproc->axi_clk);
+	if (ret)
+		goto err_axi_clk;
+
+	ret = clk_prepare_enable(qproc->rom_clk);
+	if (ret)
+		goto err_rom_clk;
+
+	ret = clk_prepare_enable(qproc->gpll0_mss_clk);
+	if (ret)
+		goto err_gpll0_mss_clk;
+
+	ret = clk_prepare_enable(qproc->snoc_axi_clk);
+	if (ret)
+		goto err_snoc_axi_clk;
+
+	ret = clk_prepare_enable(qproc->mnoc_axi_clk);
+	if (ret)
+		goto err_mnoc_axi_clk;
+
+	return 0;
+err_mnoc_axi_clk:
+	clk_disable_unprepare(qproc->snoc_axi_clk);
+err_snoc_axi_clk:
+	clk_disable_unprepare(qproc->gpll0_mss_clk);
+err_gpll0_mss_clk:
+	clk_disable_unprepare(qproc->rom_clk);
+err_rom_clk:
+	clk_disable_unprepare(qproc->axi_clk);
+err_axi_clk:
+	clk_disable_unprepare(qproc->ahb_clk);
+out:
+	dev_err(qproc->dev, "Clk Vote Failed\n");
+	return ret;
+}
+
+static void q6v55_clk_disable(struct q6v5 *qproc)
+{
+
+	clk_disable_unprepare(qproc->mnoc_axi_clk);
+	clk_disable_unprepare(qproc->snoc_axi_clk);
+	clk_disable_unprepare(qproc->gpll0_mss_clk);
+	clk_disable_unprepare(qproc->rom_clk);
+	clk_disable_unprepare(qproc->axi_clk);
+	if (!qproc->ahb_clk_vote)
+		clk_disable_unprepare(qproc->ahb_clk);
+}
+
+static int q6v55_proxy_vote(struct q6v5 *qproc)
+{
+	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
+	struct regulator *cx = qproc->supply[Q6V5_SUPPLY_CX].consumer;
+	struct regulator *vdd_pll = qproc->supply[Q6V5_SUPPLY_PLL].consumer;
+	int ret;
+
+	ret = regulator_set_voltage(mx, INT_MAX, INT_MAX);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to set voltage for vreg_mx\n");
+		return ret;
+	}
+
+	ret = regulator_enable(mx);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to enable vreg_mx\n");
+		goto err_mx_enable;
+	}
+
+	ret = regulator_set_voltage(cx, INT_MAX, INT_MAX);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to request vdd_cx voltage.\n");
+		goto err_cx_voltage;
+	}
+
+	ret = regulator_set_load(cx, 100000);
+	if (ret < 0) {
+		dev_err(qproc->dev, "Failed to set vdd_cx mode.\n");
+		goto err_cx_set_load;
+	}
+
+	ret = regulator_enable(cx);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to vote for vdd_cx\n");
+		goto err_cx_enable;
+	}
+
+	ret = regulator_set_voltage(vdd_pll, 1800000, 1800000);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to set voltage for  vdd_pll\n");
+		goto err_vreg_pll_set_vol;
+	}
+
+	ret = regulator_set_load(vdd_pll, 10000);
+	if (ret < 0) {
+		dev_err(qproc->dev, "Failed to set vdd_pll mode.\n");
+		goto err_vreg_pll_load;
+	}
+
+	ret = regulator_enable(vdd_pll);
+	if (ret) {
+		dev_err(qproc->dev, "Failed to vote for vdd_pll\n");
+		goto err_vreg_pll_enable;
+	}
+
+	ret = clk_prepare_enable(qproc->xo);
+	if (ret)
+		goto err_xo_vote;
+
+	ret = clk_prepare_enable(qproc->pnoc_clk);
+	if (ret)
+		goto err_pnoc_vote;
+
+	ret = clk_prepare_enable(qproc->qdss_clk);
+	if (ret)
+		goto err_qdss_vote;
+	qproc->unvote_flag = false;
+	return 0;
+
+err_qdss_vote:
+	clk_disable_unprepare(qproc->pnoc_clk);
+err_pnoc_vote:
+	clk_disable_unprepare(qproc->xo);
+err_xo_vote:
+	regulator_disable(vdd_pll);
+err_vreg_pll_enable:
+	regulator_set_load(vdd_pll, 0);
+err_vreg_pll_load:
+	regulator_set_voltage(vdd_pll, 0, INT_MAX);
+err_vreg_pll_set_vol:
+	regulator_disable(cx);
+err_cx_enable:
+	regulator_set_load(cx, 0);
+err_cx_set_load:
+	regulator_set_voltage(cx, 0, INT_MAX);
+err_cx_voltage:
+	regulator_disable(mx);
+err_mx_enable:
+	regulator_set_voltage(mx, 0, INT_MAX);
+
+	return ret;
+}
+
+static void q6v55_proxy_unvote(struct q6v5 *qproc)
+{
+	if (!qproc->unvote_flag) {
+		regulator_disable(qproc->supply[Q6V5_SUPPLY_PLL].consumer);
+		regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 0);
+		regulator_disable(qproc->supply[Q6V5_SUPPLY_CX].consumer);
+		regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 0);
+		regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_CX].consumer,
+			0, INT_MAX);
+
+		clk_disable_unprepare(qproc->qdss_clk);
+		clk_disable_unprepare(qproc->pnoc_clk);
+		clk_disable_unprepare(qproc->xo);
+
+		regulator_disable(qproc->supply[Q6V5_SUPPLY_MX].consumer);
+		regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_MX].consumer,
+			0, INT_MAX);
+	}
+	qproc->unvote_flag = true;
+}
+
+static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
+{
+	if (qproc->restart_reg) {
+		writel_relaxed(mss_restart, qproc->restart_reg);
+		udelay(2);
+	}
+}
+
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
@@ -751,6 +948,65 @@ static int q6v5_init_clocks(struct q6v5 *qproc)
 	return 0;
 }
 
+static int q6v55_init_clocks(struct q6v5 *qproc)
+{
+	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
+	if (IS_ERR(qproc->ahb_clk)) {
+		dev_err(qproc->dev, "failed to get iface clock\n");
+		return PTR_ERR(qproc->ahb_clk);
+	}
+
+	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
+	if (IS_ERR(qproc->axi_clk)) {
+		dev_err(qproc->dev, "failed to get bus clock\n");
+		return PTR_ERR(qproc->axi_clk);
+	}
+
+	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
+	if (IS_ERR(qproc->rom_clk)) {
+		dev_err(qproc->dev, "failed to get mem clock\n");
+		return PTR_ERR(qproc->rom_clk);
+	}
+
+	qproc->snoc_axi_clk = devm_clk_get(qproc->dev, "snoc_axi_clk");
+	if (IS_ERR(qproc->snoc_axi_clk)) {
+		dev_err(qproc->dev, "failed to get snoc_axi_clk\n");
+		return PTR_ERR(qproc->snoc_axi_clk);
+	}
+
+	qproc->mnoc_axi_clk = devm_clk_get(qproc->dev, "mnoc_axi_clk");
+	if (IS_ERR(qproc->mnoc_axi_clk)) {
+		dev_err(qproc->dev, "failed to get mnoc_axi_clk clock\n");
+		return PTR_ERR(qproc->mnoc_axi_clk);
+	}
+
+	qproc->gpll0_mss_clk = devm_clk_get(qproc->dev, "gpll0_mss_clk");
+	if (IS_ERR(qproc->gpll0_mss_clk)) {
+		dev_err(qproc->dev, "failed to get gpll0_mss_clk clock\n");
+		return PTR_ERR(qproc->gpll0_mss_clk);
+	}
+
+	qproc->xo = devm_clk_get(qproc->dev, "xo");
+	if (IS_ERR(qproc->xo)) {
+		dev_err(qproc->dev, "failed to get xo\n");
+		return PTR_ERR(qproc->xo);
+	}
+
+	qproc->pnoc_clk = devm_clk_get(qproc->dev, "pnoc");
+	if (IS_ERR(qproc->pnoc_clk)) {
+		dev_err(qproc->dev, "failed to get pnoc_clk clock\n");
+		return PTR_ERR(qproc->pnoc_clk);
+	}
+
+	qproc->qdss_clk = devm_clk_get(qproc->dev, "qdss");
+	if (IS_ERR(qproc->qdss_clk)) {
+		dev_err(qproc->dev, "failed to get qdss_clk clock\n");
+		return PTR_ERR(qproc->qdss_clk);
+	}
+
+	return 0;
+}
+
 static int q6v5_init_reset(struct q6v5 *qproc)
 {
 	qproc->mss_restart = devm_reset_control_get(qproc->dev, NULL);
@@ -762,6 +1018,21 @@ static int q6v5_init_reset(struct q6v5 *qproc)
 	return 0;
 }
 
+static int q6v55_init_reset(struct q6v5 *qproc, struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "restart_reg");
+	qproc->restart_reg = devm_ioremap(qproc->dev, res->start,
+							resource_size(res));
+	if (IS_ERR(qproc->restart_reg)) {
+		dev_err(qproc->dev, "failed to get restart_reg\n");
+		return PTR_ERR(qproc->restart_reg);
+	}
+
+	return 0;
+}
+
 static int q6v5_request_irq(struct q6v5 *qproc,
 			     struct platform_device *pdev,
 			     const char *name,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 2/5] remoteproc: Adding q6v55 specific regulator, clk, reset interface.

[Bjorn Andersson]

On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> q6v55 use different regulator and clock resource than q6v5, hence
> using separate resource handling for same.

> Also reset register
> programming in q6v55 is non secure so resource controller init-
> ilization is not required for q6v55.

The reset comes from GCC, so please use the fact that gcc already is a
reset-controller.

> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 271 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 271 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 8df95a0..c7dca40 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -215,6 +215,203 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
>  	regulator_set_voltage(mss, 0, 1150000);
>  }
>  
> +static int q6v55_regulator_init(struct q6v5 *qproc)
> +{
> +	int ret;
> +
> +	qproc->supply[Q6V5_SUPPLY_CX].supply = "vdd_cx";
> +	qproc->supply[Q6V5_SUPPLY_MX].supply = "vdd_mx";
> +	qproc->supply[Q6V5_SUPPLY_PLL].supply = "vdd_pll";
> +
> +	ret = devm_regulator_bulk_get(qproc->dev,
> +			(ARRAY_SIZE(qproc->supply) - 1), qproc->supply);
> +	if (ret < 0) {
> +		dev_err(qproc->dev, "failed to get supplies\n");
> +		return ret;
> +	}
> +
> +
> +	return 0;
> +}

This is very very similar to the existing q6v5_regulator_init, please
figure out a way to make the mss supply optional here instead of
creating a new function.

> +
> +static int q6v55_clk_enable(struct q6v5 *qproc)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(qproc->ahb_clk);
> +	if (ret)
> +		goto out;
> +
> +	ret = clk_prepare_enable(qproc->axi_clk);
> +	if (ret)
> +		goto err_axi_clk;
> +
> +	ret = clk_prepare_enable(qproc->rom_clk);
> +	if (ret)
> +		goto err_rom_clk;
> +
> +	ret = clk_prepare_enable(qproc->gpll0_mss_clk);
> +	if (ret)
> +		goto err_gpll0_mss_clk;
> +
> +	ret = clk_prepare_enable(qproc->snoc_axi_clk);
> +	if (ret)
> +		goto err_snoc_axi_clk;
> +
> +	ret = clk_prepare_enable(qproc->mnoc_axi_clk);
> +	if (ret)
> +		goto err_mnoc_axi_clk;

I believe it would be better to turn the clocks into an array, like the
regulators, so that we can loop over them rather than listing them
explicitly.

> +
> +	return 0;
> +err_mnoc_axi_clk:
> +	clk_disable_unprepare(qproc->snoc_axi_clk);
> +err_snoc_axi_clk:
> +	clk_disable_unprepare(qproc->gpll0_mss_clk);
> +err_gpll0_mss_clk:
> +	clk_disable_unprepare(qproc->rom_clk);
> +err_rom_clk:
> +	clk_disable_unprepare(qproc->axi_clk);
> +err_axi_clk:
> +	clk_disable_unprepare(qproc->ahb_clk);
> +out:
> +	dev_err(qproc->dev, "Clk Vote Failed\n");

I believe the clock framework will complain if above fails, so no need
to add another print here.

> +	return ret;
> +}
> +
> +static void q6v55_clk_disable(struct q6v5 *qproc)
> +{
> +
> +	clk_disable_unprepare(qproc->mnoc_axi_clk);
> +	clk_disable_unprepare(qproc->snoc_axi_clk);
> +	clk_disable_unprepare(qproc->gpll0_mss_clk);
> +	clk_disable_unprepare(qproc->rom_clk);
> +	clk_disable_unprepare(qproc->axi_clk);
> +	if (!qproc->ahb_clk_vote)

Why do you check ahb_clk_vote in the disable case but not in the enable
case?

Also, please move all ahb_clk_vote handling into the same patch.

> +		clk_disable_unprepare(qproc->ahb_clk);
> +}
> +
> +static int q6v55_proxy_vote(struct q6v5 *qproc)
> +{
> +	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
> +	struct regulator *cx = qproc->supply[Q6V5_SUPPLY_CX].consumer;
> +	struct regulator *vdd_pll = qproc->supply[Q6V5_SUPPLY_PLL].consumer;
> +	int ret;
> +
> +	ret = regulator_set_voltage(mx, INT_MAX, INT_MAX);

I'm not convinced that we should vote MAX as minimum voltage here, is it
not 1.05V as in the q6v5?

> +	if (ret) {
> +		dev_err(qproc->dev, "Failed to set voltage for vreg_mx\n");
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(mx);
> +	if (ret) {
> +		dev_err(qproc->dev, "Failed to enable vreg_mx\n");
> +		goto err_mx_enable;
> +	}
> +
> +	ret = regulator_set_voltage(cx, INT_MAX, INT_MAX);
> +	if (ret) {
> +		dev_err(qproc->dev, "Failed to request vdd_cx voltage.\n");
> +		goto err_cx_voltage;
> +	}
> +
> +	ret = regulator_set_load(cx, 100000);
> +	if (ret < 0) {
> +		dev_err(qproc->dev, "Failed to set vdd_cx mode.\n");
> +		goto err_cx_set_load;
> +	}
> +
> +	ret = regulator_enable(cx);
> +	if (ret) {
> +		dev_err(qproc->dev, "Failed to vote for vdd_cx\n");
> +		goto err_cx_enable;
> +	}
> +
> +	ret = regulator_set_voltage(vdd_pll, 1800000, 1800000);

PLL is fed from a fixed voltage regulator of 1.8V, so we do not need to
request a voltage (per Mark Brown's request).

> +	if (ret) {
> +		dev_err(qproc->dev, "Failed to set voltage for  vdd_pll\n");
> +		goto err_vreg_pll_set_vol;
> +	}
> +
> +	ret = regulator_set_load(vdd_pll, 10000);
> +	if (ret < 0) {
> +		dev_err(qproc->dev, "Failed to set vdd_pll mode.\n");
> +		goto err_vreg_pll_load;
> +	}
> +
> +	ret = regulator_enable(vdd_pll);
> +	if (ret) {
> +		dev_err(qproc->dev, "Failed to vote for vdd_pll\n");
> +		goto err_vreg_pll_enable;
> +	}
> +
> +	ret = clk_prepare_enable(qproc->xo);
> +	if (ret)
> +		goto err_xo_vote;
> +
> +	ret = clk_prepare_enable(qproc->pnoc_clk);
> +	if (ret)
> +		goto err_pnoc_vote;
> +
> +	ret = clk_prepare_enable(qproc->qdss_clk);
> +	if (ret)
> +		goto err_qdss_vote;
> +	qproc->unvote_flag = false;
> +	return 0;
> +
> +err_qdss_vote:
> +	clk_disable_unprepare(qproc->pnoc_clk);
> +err_pnoc_vote:
> +	clk_disable_unprepare(qproc->xo);
> +err_xo_vote:
> +	regulator_disable(vdd_pll);
> +err_vreg_pll_enable:
> +	regulator_set_load(vdd_pll, 0);
> +err_vreg_pll_load:
> +	regulator_set_voltage(vdd_pll, 0, INT_MAX);
> +err_vreg_pll_set_vol:
> +	regulator_disable(cx);
> +err_cx_enable:
> +	regulator_set_load(cx, 0);
> +err_cx_set_load:
> +	regulator_set_voltage(cx, 0, INT_MAX);
> +err_cx_voltage:
> +	regulator_disable(mx);
> +err_mx_enable:
> +	regulator_set_voltage(mx, 0, INT_MAX);
> +
> +	return ret;
> +}

As with the init function, this is very similar to the q6v5 case, so I
don't think we need to have separate functions for it.

A side note on this is that clk_prepare_enable(NULL) is valid, so if you
distinguish between the two cases during initialisation and put all
clocks in an array you could just call clk_prepare_enable() on all of
them, which will skip the ones that isn't initialised.

> +
> +static void q6v55_proxy_unvote(struct q6v5 *qproc)
> +{
> +	if (!qproc->unvote_flag) {

I don't think the "unvote_flag" is good design and don't see how you
would end up unvoting multiple times based on my original design.

But again, the answer is probably in some other patch - i.e. please
group your patches so I can see each step in its entirety.

> +		regulator_disable(qproc->supply[Q6V5_SUPPLY_PLL].consumer);
> +		regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 0);
> +		regulator_disable(qproc->supply[Q6V5_SUPPLY_CX].consumer);
> +		regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 0);
> +		regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_CX].consumer,
> +			0, INT_MAX);
> +
> +		clk_disable_unprepare(qproc->qdss_clk);
> +		clk_disable_unprepare(qproc->pnoc_clk);
> +		clk_disable_unprepare(qproc->xo);
> +
> +		regulator_disable(qproc->supply[Q6V5_SUPPLY_MX].consumer);
> +		regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_MX].consumer,
> +			0, INT_MAX);
> +	}
> +	qproc->unvote_flag = true;
> +}
> +
> +static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
> +{
> +	if (qproc->restart_reg) {
> +		writel_relaxed(mss_restart, qproc->restart_reg);
> +		udelay(2);
> +	}
> +}

Drop this

> +
>  static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct q6v5 *qproc = rproc->priv;
> @@ -751,6 +948,65 @@ static int q6v5_init_clocks(struct q6v5 *qproc)
>  	return 0;
>  }
>  
> +static int q6v55_init_clocks(struct q6v5 *qproc)
> +{
> +	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
> +	if (IS_ERR(qproc->ahb_clk)) {
> +		dev_err(qproc->dev, "failed to get iface clock\n");
> +		return PTR_ERR(qproc->ahb_clk);
> +	}
> +
> +	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
> +	if (IS_ERR(qproc->axi_clk)) {
> +		dev_err(qproc->dev, "failed to get bus clock\n");
> +		return PTR_ERR(qproc->axi_clk);
> +	}
> +
> +	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
> +	if (IS_ERR(qproc->rom_clk)) {
> +		dev_err(qproc->dev, "failed to get mem clock\n");
> +		return PTR_ERR(qproc->rom_clk);
> +	}

These three are the same as q6v5...

> +
> +	qproc->snoc_axi_clk = devm_clk_get(qproc->dev, "snoc_axi_clk");
> +	if (IS_ERR(qproc->snoc_axi_clk)) {
> +		dev_err(qproc->dev, "failed to get snoc_axi_clk\n");
> +		return PTR_ERR(qproc->snoc_axi_clk);
> +	}
> +
> +	qproc->mnoc_axi_clk = devm_clk_get(qproc->dev, "mnoc_axi_clk");
> +	if (IS_ERR(qproc->mnoc_axi_clk)) {
> +		dev_err(qproc->dev, "failed to get mnoc_axi_clk clock\n");
> +		return PTR_ERR(qproc->mnoc_axi_clk);
> +	}
> +
> +	qproc->gpll0_mss_clk = devm_clk_get(qproc->dev, "gpll0_mss_clk");
> +	if (IS_ERR(qproc->gpll0_mss_clk)) {
> +		dev_err(qproc->dev, "failed to get gpll0_mss_clk clock\n");
> +		return PTR_ERR(qproc->gpll0_mss_clk);
> +	}
> +
> +	qproc->xo = devm_clk_get(qproc->dev, "xo");
> +	if (IS_ERR(qproc->xo)) {
> +		dev_err(qproc->dev, "failed to get xo\n");
> +		return PTR_ERR(qproc->xo);
> +	}

And q6v5 will need "xo" as well, I missed this one.

> +
> +	qproc->pnoc_clk = devm_clk_get(qproc->dev, "pnoc");
> +	if (IS_ERR(qproc->pnoc_clk)) {
> +		dev_err(qproc->dev, "failed to get pnoc_clk clock\n");
> +		return PTR_ERR(qproc->pnoc_clk);
> +	}
> +
> +	qproc->qdss_clk = devm_clk_get(qproc->dev, "qdss");
> +	if (IS_ERR(qproc->qdss_clk)) {
> +		dev_err(qproc->dev, "failed to get qdss_clk clock\n");
> +		return PTR_ERR(qproc->qdss_clk);
> +	}
> +

I would rather see that we have a single init_clocks function that based
on compatible will get the appropriate clocks.

> +	return 0;
> +}
> +
>  static int q6v5_init_reset(struct q6v5 *qproc)
>  {
>  	qproc->mss_restart = devm_reset_control_get(qproc->dev, NULL);
> @@ -762,6 +1018,21 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>  	return 0;
>  }
>  
> +static int q6v55_init_reset(struct q6v5 *qproc, struct platform_device *pdev)
> +{
> +	struct resource *res;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "restart_reg");
> +	qproc->restart_reg = devm_ioremap(qproc->dev, res->start,
> +							resource_size(res));
> +	if (IS_ERR(qproc->restart_reg)) {
> +		dev_err(qproc->dev, "failed to get restart_reg\n");
> +		return PTR_ERR(qproc->restart_reg);
> +	}
> +
> +	return 0;
> +}

Drop this

> +
>  static int q6v5_request_irq(struct q6v5 *qproc,
>  			     struct platform_device *pdev,
>  			     const char *name,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
> 

Re: [PATCH 2/5] remoteproc: Adding q6v55 specific regulator, clk, reset interface.

[Avaneesh Kumar Dwivedi]

On 10/26/2016 12:35 AM, Bjorn Andersson wrote:
> On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:
>
>> q6v55 use different regulator and clock resource than q6v5, hence
>> using separate resource handling for same.
>> Also reset register
>> programming in q6v55 is non secure so resource controller init-
>> ilization is not required for q6v55.
> The reset comes from GCC, so please use the fact that gcc already is a
> reset-controller.
Already in patch 1/5 have explained reason of not using the GCC to reset 
pasting same here again

Infact i had done it the way suggested before i sent out initial
patchset, but then when i discussed with internal clock team, they said 
they will
not support MSS RESET to be done via GCC because

"MSS_RESET is not a block reset, GCC reset controller is used only when 
we need a BCR to be reset,
and which has a special purpose. MSS RESET doesn't fall under block 
resets, it is not a clock and
cannot be associated with clock."

Downstream also MSS RESET is programmed through ioremap
>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 271 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 271 insertions(+)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 8df95a0..c7dca40 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -215,6 +215,203 @@ static void q6v5_regulator_disable(struct q6v5 *qproc)
>>   	regulator_set_voltage(mss, 0, 1150000);
>>   }
>>   
>> +static int q6v55_regulator_init(struct q6v5 *qproc)
>> +{
>> +	int ret;
>> +
>> +	qproc->supply[Q6V5_SUPPLY_CX].supply = "vdd_cx";
>> +	qproc->supply[Q6V5_SUPPLY_MX].supply = "vdd_mx";
>> +	qproc->supply[Q6V5_SUPPLY_PLL].supply = "vdd_pll";
>> +
>> +	ret = devm_regulator_bulk_get(qproc->dev,
>> +			(ARRAY_SIZE(qproc->supply) - 1), qproc->supply);
>> +	if (ret < 0) {
>> +		dev_err(qproc->dev, "failed to get supplies\n");
>> +		return ret;
>> +	}
>> +
>> +
>> +	return 0;
>> +}
> This is very very similar to the existing q6v5_regulator_init, please
> figure out a way to make the mss supply optional here instead of
> creating a new function.
OK, Have implemented common init and enable function for clock and 
regulator in patchset v2
>
>> +
>> +static int q6v55_clk_enable(struct q6v5 *qproc)
>> +{
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(qproc->ahb_clk);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = clk_prepare_enable(qproc->axi_clk);
>> +	if (ret)
>> +		goto err_axi_clk;
>> +
>> +	ret = clk_prepare_enable(qproc->rom_clk);
>> +	if (ret)
>> +		goto err_rom_clk;
>> +
>> +	ret = clk_prepare_enable(qproc->gpll0_mss_clk);
>> +	if (ret)
>> +		goto err_gpll0_mss_clk;
>> +
>> +	ret = clk_prepare_enable(qproc->snoc_axi_clk);
>> +	if (ret)
>> +		goto err_snoc_axi_clk;
>> +
>> +	ret = clk_prepare_enable(qproc->mnoc_axi_clk);
>> +	if (ret)
>> +		goto err_mnoc_axi_clk;
> I believe it would be better to turn the clocks into an array, like the
> regulators, so that we can loop over them rather than listing them
> explicitly.
OK , Have used clock array to initialize and enable clocks for q6 in 
patchset v2
>
>> +
>> +	return 0;
>> +err_mnoc_axi_clk:
>> +	clk_disable_unprepare(qproc->snoc_axi_clk);
>> +err_snoc_axi_clk:
>> +	clk_disable_unprepare(qproc->gpll0_mss_clk);
>> +err_gpll0_mss_clk:
>> +	clk_disable_unprepare(qproc->rom_clk);
>> +err_rom_clk:
>> +	clk_disable_unprepare(qproc->axi_clk);
>> +err_axi_clk:
>> +	clk_disable_unprepare(qproc->ahb_clk);
>> +out:
>> +	dev_err(qproc->dev, "Clk Vote Failed\n");
> I believe the clock framework will complain if above fails, so no need
> to add another print here.
OK, Have removed print also
>
>> +	return ret;
>> +}
>> +
>> +static void q6v55_clk_disable(struct q6v5 *qproc)
>> +{
>> +
>> +	clk_disable_unprepare(qproc->mnoc_axi_clk);
>> +	clk_disable_unprepare(qproc->snoc_axi_clk);
>> +	clk_disable_unprepare(qproc->gpll0_mss_clk);
>> +	clk_disable_unprepare(qproc->rom_clk);
>> +	clk_disable_unprepare(qproc->axi_clk);
>> +	if (!qproc->ahb_clk_vote)
> Why do you check ahb_clk_vote in the disable case but not in the enable
> case?
>
> Also, please move all ahb_clk_vote handling into the same patch.
OK, Infact check was not required for v56 applicable to 8996, so have 
removed them.
>
>> +		clk_disable_unprepare(qproc->ahb_clk);
>> +}
>> +
>> +static int q6v55_proxy_vote(struct q6v5 *qproc)
>> +{
>> +	struct regulator *mx = qproc->supply[Q6V5_SUPPLY_MX].consumer;
>> +	struct regulator *cx = qproc->supply[Q6V5_SUPPLY_CX].consumer;
>> +	struct regulator *vdd_pll = qproc->supply[Q6V5_SUPPLY_PLL].consumer;
>> +	int ret;
>> +
>> +	ret = regulator_set_voltage(mx, INT_MAX, INT_MAX);
> I'm not convinced that we should vote MAX as minimum voltage here, is it
> not 1.05V as in the q6v5?
OK
>
>> +	if (ret) {
>> +		dev_err(qproc->dev, "Failed to set voltage for vreg_mx\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = regulator_enable(mx);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "Failed to enable vreg_mx\n");
>> +		goto err_mx_enable;
>> +	}
>> +
>> +	ret = regulator_set_voltage(cx, INT_MAX, INT_MAX);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "Failed to request vdd_cx voltage.\n");
>> +		goto err_cx_voltage;
>> +	}
>> +
>> +	ret = regulator_set_load(cx, 100000);
>> +	if (ret < 0) {
>> +		dev_err(qproc->dev, "Failed to set vdd_cx mode.\n");
>> +		goto err_cx_set_load;
>> +	}
>> +
>> +	ret = regulator_enable(cx);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "Failed to vote for vdd_cx\n");
>> +		goto err_cx_enable;
>> +	}
>> +
>> +	ret = regulator_set_voltage(vdd_pll, 1800000, 1800000);
> PLL is fed from a fixed voltage regulator of 1.8V, so we do not need to
> request a voltage (per Mark Brown's request).
OK.
>
>> +	if (ret) {
>> +		dev_err(qproc->dev, "Failed to set voltage for  vdd_pll\n");
>> +		goto err_vreg_pll_set_vol;
>> +	}
>> +
>> +	ret = regulator_set_load(vdd_pll, 10000);
>> +	if (ret < 0) {
>> +		dev_err(qproc->dev, "Failed to set vdd_pll mode.\n");
>> +		goto err_vreg_pll_load;
>> +	}
>> +
>> +	ret = regulator_enable(vdd_pll);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "Failed to vote for vdd_pll\n");
>> +		goto err_vreg_pll_enable;
>> +	}
>> +
>> +	ret = clk_prepare_enable(qproc->xo);
>> +	if (ret)
>> +		goto err_xo_vote;
>> +
>> +	ret = clk_prepare_enable(qproc->pnoc_clk);
>> +	if (ret)
>> +		goto err_pnoc_vote;
>> +
>> +	ret = clk_prepare_enable(qproc->qdss_clk);
>> +	if (ret)
>> +		goto err_qdss_vote;
>> +	qproc->unvote_flag = false;
>> +	return 0;
>> +
>> +err_qdss_vote:
>> +	clk_disable_unprepare(qproc->pnoc_clk);
>> +err_pnoc_vote:
>> +	clk_disable_unprepare(qproc->xo);
>> +err_xo_vote:
>> +	regulator_disable(vdd_pll);
>> +err_vreg_pll_enable:
>> +	regulator_set_load(vdd_pll, 0);
>> +err_vreg_pll_load:
>> +	regulator_set_voltage(vdd_pll, 0, INT_MAX);
>> +err_vreg_pll_set_vol:
>> +	regulator_disable(cx);
>> +err_cx_enable:
>> +	regulator_set_load(cx, 0);
>> +err_cx_set_load:
>> +	regulator_set_voltage(cx, 0, INT_MAX);
>> +err_cx_voltage:
>> +	regulator_disable(mx);
>> +err_mx_enable:
>> +	regulator_set_voltage(mx, 0, INT_MAX);
>> +
>> +	return ret;
>> +}
> As with the init function, this is very similar to the q6v5 case, so I
> don't think we need to have separate functions for it.
>
> A side note on this is that clk_prepare_enable(NULL) is valid, so if you
> distinguish between the two cases during initialisation and put all
> clocks in an array you could just call clk_prepare_enable() on all of
> them, which will skip the ones that isn't initialised.
>
>> +
>> +static void q6v55_proxy_unvote(struct q6v5 *qproc)
>> +{
>> +	if (!qproc->unvote_flag) {
> I don't think the "unvote_flag" is good design and don't see how you
> would end up unvoting multiple times based on my original design.
>
> But again, the answer is probably in some other patch - i.e. please
> group your patches so I can see each step in its entirety.
code is reorganized yet i am using unvote flag but in transparent way.
i am setting unvote flag during enable and making it false during disable.
so that we unvote only if we have voted earlier.
>
>> +		regulator_disable(qproc->supply[Q6V5_SUPPLY_PLL].consumer);
>> +		regulator_set_load(qproc->supply[Q6V5_SUPPLY_PLL].consumer, 0);
>> +		regulator_disable(qproc->supply[Q6V5_SUPPLY_CX].consumer);
>> +		regulator_set_load(qproc->supply[Q6V5_SUPPLY_CX].consumer, 0);
>> +		regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_CX].consumer,
>> +			0, INT_MAX);
>> +
>> +		clk_disable_unprepare(qproc->qdss_clk);
>> +		clk_disable_unprepare(qproc->pnoc_clk);
>> +		clk_disable_unprepare(qproc->xo);
>> +
>> +		regulator_disable(qproc->supply[Q6V5_SUPPLY_MX].consumer);
>> +		regulator_set_voltage(qproc->supply[Q6V5_SUPPLY_MX].consumer,
>> +			0, INT_MAX);
>> +	}
>> +	qproc->unvote_flag = true;
>> +}
>> +
>> +static void pil_mss_restart_reg(struct q6v5 *qproc, u32 mss_restart)
>> +{
>> +	if (qproc->restart_reg) {
>> +		writel_relaxed(mss_restart, qproc->restart_reg);
>> +		udelay(2);
>> +	}
>> +}
> Drop this
can not drop due to reason explained already that i can not use GCC for 
MSS reset.
>
>> +
>>   static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>> @@ -751,6 +948,65 @@ static int q6v5_init_clocks(struct q6v5 *qproc)
>>   	return 0;
>>   }
>>   
>> +static int q6v55_init_clocks(struct q6v5 *qproc)
>> +{
>> +	qproc->ahb_clk = devm_clk_get(qproc->dev, "iface");
>> +	if (IS_ERR(qproc->ahb_clk)) {
>> +		dev_err(qproc->dev, "failed to get iface clock\n");
>> +		return PTR_ERR(qproc->ahb_clk);
>> +	}
>> +
>> +	qproc->axi_clk = devm_clk_get(qproc->dev, "bus");
>> +	if (IS_ERR(qproc->axi_clk)) {
>> +		dev_err(qproc->dev, "failed to get bus clock\n");
>> +		return PTR_ERR(qproc->axi_clk);
>> +	}
>> +
>> +	qproc->rom_clk = devm_clk_get(qproc->dev, "mem");
>> +	if (IS_ERR(qproc->rom_clk)) {
>> +		dev_err(qproc->dev, "failed to get mem clock\n");
>> +		return PTR_ERR(qproc->rom_clk);
>> +	}
> These three are the same as q6v5...
>
>> +
>> +	qproc->snoc_axi_clk = devm_clk_get(qproc->dev, "snoc_axi_clk");
>> +	if (IS_ERR(qproc->snoc_axi_clk)) {
>> +		dev_err(qproc->dev, "failed to get snoc_axi_clk\n");
>> +		return PTR_ERR(qproc->snoc_axi_clk);
>> +	}
>> +
>> +	qproc->mnoc_axi_clk = devm_clk_get(qproc->dev, "mnoc_axi_clk");
>> +	if (IS_ERR(qproc->mnoc_axi_clk)) {
>> +		dev_err(qproc->dev, "failed to get mnoc_axi_clk clock\n");
>> +		return PTR_ERR(qproc->mnoc_axi_clk);
>> +	}
>> +
>> +	qproc->gpll0_mss_clk = devm_clk_get(qproc->dev, "gpll0_mss_clk");
>> +	if (IS_ERR(qproc->gpll0_mss_clk)) {
>> +		dev_err(qproc->dev, "failed to get gpll0_mss_clk clock\n");
>> +		return PTR_ERR(qproc->gpll0_mss_clk);
>> +	}
>> +
>> +	qproc->xo = devm_clk_get(qproc->dev, "xo");
>> +	if (IS_ERR(qproc->xo)) {
>> +		dev_err(qproc->dev, "failed to get xo\n");
>> +		return PTR_ERR(qproc->xo);
>> +	}
> And q6v5 will need "xo" as well, I missed this one.
OK.
>
>> +
>> +	qproc->pnoc_clk = devm_clk_get(qproc->dev, "pnoc");
>> +	if (IS_ERR(qproc->pnoc_clk)) {
>> +		dev_err(qproc->dev, "failed to get pnoc_clk clock\n");
>> +		return PTR_ERR(qproc->pnoc_clk);
>> +	}
>> +
>> +	qproc->qdss_clk = devm_clk_get(qproc->dev, "qdss");
>> +	if (IS_ERR(qproc->qdss_clk)) {
>> +		dev_err(qproc->dev, "failed to get qdss_clk clock\n");
>> +		return PTR_ERR(qproc->qdss_clk);
>> +	}
>> +
> I would rather see that we have a single init_clocks function that based
> on compatible will get the appropriate clocks.
OK.
>
>> +	return 0;
>> +}
>> +
>>   static int q6v5_init_reset(struct q6v5 *qproc)
>>   {
>>   	qproc->mss_restart = devm_reset_control_get(qproc->dev, NULL);
>> @@ -762,6 +1018,21 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>>   	return 0;
>>   }
>>   
>> +static int q6v55_init_reset(struct q6v5 *qproc, struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "restart_reg");
>> +	qproc->restart_reg = devm_ioremap(qproc->dev, res->start,
>> +							resource_size(res));
>> +	if (IS_ERR(qproc->restart_reg)) {
>> +		dev_err(qproc->dev, "failed to get restart_reg\n");
>> +		return PTR_ERR(qproc->restart_reg);
>> +	}
>> +
>> +	return 0;
>> +}
> Drop this
Can not drop as can not use GCC for MSS_RESET due to reason explained above.
>
>> +
>>   static int q6v5_request_irq(struct q6v5 *qproc,
>>   			     struct platform_device *pdev,
>>   			     const char *name,
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>>

[PATCH 3/5] remoteproc: Adding reset sequence and halt seq changes for q6v55

[Avaneesh Kumar Dwivedi]

q6v55 reset sequence is handled separately and removing idle check
before asserting reset as it has been observed it return idle some
time even without being in idle.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 103 +++++++++++++++++++++++++++++++++++--
 1 file changed, 98 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index c7dca40..0fac8d8 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -536,6 +536,104 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 	return ret;
 }
 
+static int q6v6proc_reset(struct q6v5 *qproc)
+{
+	int ret, i, count;
+	u64 val;
+
+	/* Override the ACC value if required */
+	writel_relaxed(QDSP6SS_ACC_OVERRIDE_VAL,
+				qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+	/* Assert resets, stop core */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_RESET_REG);
+	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+	/* BHS require xo cbcr to be enabled */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
+	val |= 0x1;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+	for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) {
+		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
+		if (!(val & BIT(31)))
+			break;
+		udelay(1);
+	}
+
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
+	if ((val & BIT(31)))
+		dev_err(qproc->dev, "Failed to enable xo branch clock.\n");
+
+	/* Enable power block headswitch, and wait for it to stabilize */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= QDSP6v55_BHS_ON;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	udelay(1);
+
+	/* Put LDO in bypass mode */
+	val |= QDSP6v55_LDO_BYP;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	/*
+	 * Turn on memories. L2 banks should be done individually
+	 * to minimize inrush current.
+	 */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val &= ~QDSP6v55_CLAMP_QMC_MEM;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Deassert memory peripheral sleep and L2 memory standby */
+	val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Turn on L1, L2, ETB and JU memories 1 at a time */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+	for (i = 19; i >= 0; i--) {
+		val |= BIT(i);
+		writel_relaxed(val, qproc->reg_base +
+					QDSP6SS_MEM_PWR_CTL);
+		/*
+		 * Wait for 1us for both memory peripheral and
+		 * data array to turn on.
+		 */
+		 mb();
+		udelay(1);
+	}
+
+	/* Remove word line clamp */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val &= ~QDSP6v55_CLAMP_WL;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Remove IO clamp */
+	val &= ~Q6SS_CLAMP_IO;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	/* Bring core out of reset */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_RESET_REG);
+	val &= ~(Q6SS_CORE_ARES | Q6SS_STOP_CORE);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+	/* Turn on core clock */
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+	val |= Q6SS_CLK_ENABLE;
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
+
+
+	/* Wait for PBL status */
+	ret = q6v5_rmb_pbl_wait(qproc, 1000);
+	if (ret == -ETIMEDOUT) {
+		dev_err(qproc->dev, "PBL boot timed out\n");
+	} else if (ret != RMB_PBL_SUCCESS) {
+		dev_err(qproc->dev, "PBL returned unexpected status %d\n", ret);
+		ret = -EINVAL;
+	} else {
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 				   struct regmap *halt_map,
 				   u32 offset)
@@ -544,11 +642,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 	unsigned int val;
 	int ret;
 
-	/* Check if we're already idle */
-	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
-	if (!ret && val)
-		return;
-
 	/* Assert halt request */
 	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 3/5] remoteproc: Adding reset sequence and halt seq changes for q6v55

[Bjorn Andersson]

On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> q6v55 reset sequence is handled separately and removing idle check
> before asserting reset as it has been observed it return idle some
> time even without being in idle.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 103 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 98 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index c7dca40..0fac8d8 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -536,6 +536,104 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  	return ret;
>  }
>  
> +static int q6v6proc_reset(struct q6v5 *qproc)

Which version is this? 5.5, 55 or 6?

It's perfectly fine to introduce q6v55-functions and use those, but if
the version is 6 then the compatible should reflect that at least.

> +{
> +	int ret, i, count;
> +	u64 val;
> +
> +	/* Override the ACC value if required */
> +	writel_relaxed(QDSP6SS_ACC_OVERRIDE_VAL,
> +				qproc->reg_base + QDSP6SS_STRAP_ACC);
> +
> +	/* Assert resets, stop core */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_RESET_REG);
> +	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
> +
> +	/* BHS require xo cbcr to be enabled */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
> +	val |= 0x1;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> +	for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) {
> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
> +		if (!(val & BIT(31)))
> +			break;
> +		udelay(1);
> +	}
> +
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
> +	if ((val & BIT(31)))
> +		dev_err(qproc->dev, "Failed to enable xo branch clock.\n");
> +
> +	/* Enable power block headswitch, and wait for it to stabilize */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= QDSP6v55_BHS_ON;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	udelay(1);
> +
> +	/* Put LDO in bypass mode */
> +	val |= QDSP6v55_LDO_BYP;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	/*
> +	 * Turn on memories. L2 banks should be done individually
> +	 * to minimize inrush current.
> +	 */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val &= ~QDSP6v55_CLAMP_QMC_MEM;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Deassert memory peripheral sleep and L2 memory standby */
> +	val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Turn on L1, L2, ETB and JU memories 1 at a time */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> +	for (i = 19; i >= 0; i--) {
> +		val |= BIT(i);
> +		writel_relaxed(val, qproc->reg_base +
> +					QDSP6SS_MEM_PWR_CTL);
> +		/*
> +		 * Wait for 1us for both memory peripheral and
> +		 * data array to turn on.
> +		 */
> +		 mb();
> +		udelay(1);
> +	}
> +
> +	/* Remove word line clamp */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val &= ~QDSP6v55_CLAMP_WL;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Remove IO clamp */
> +	val &= ~Q6SS_CLAMP_IO;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +	/* Bring core out of reset */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_RESET_REG);
> +	val &= ~(Q6SS_CORE_ARES | Q6SS_STOP_CORE);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
> +
> +	/* Turn on core clock */
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +	val |= Q6SS_CLK_ENABLE;
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
> +
> +
> +	/* Wait for PBL status */
> +	ret = q6v5_rmb_pbl_wait(qproc, 1000);
> +	if (ret == -ETIMEDOUT) {
> +		dev_err(qproc->dev, "PBL boot timed out\n");
> +	} else if (ret != RMB_PBL_SUCCESS) {
> +		dev_err(qproc->dev, "PBL returned unexpected status %d\n", ret);
> +		ret = -EINVAL;
> +	} else {
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}

Most of this function is exactly the same as q6v5proc_reset(), do we
need two copies or can we make the differences conditional based on
compatible?

> +
>  static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  				   struct regmap *halt_map,
>  				   u32 offset)
> @@ -544,11 +642,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  	unsigned int val;
>  	int ret;
>  
> -	/* Check if we're already idle */
> -	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
> -	if (!ret && val)
> -		return;
> -

I presume this check isn't needed on the other versions either?

>  	/* Assert halt request */
>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
>  

Regards,
Bjorn

Re: [PATCH 3/5] remoteproc: Adding reset sequence and halt seq changes for q6v55

[Avaneesh Kumar Dwivedi]

On 10/26/2016 12:45 AM, Bjorn Andersson wrote:
> On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:
>
>> q6v55 reset sequence is handled separately and removing idle check
>> before asserting reset as it has been observed it return idle some
>> time even without being in idle.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 103 +++++++++++++++++++++++++++++++++++--
>>   1 file changed, 98 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index c7dca40..0fac8d8 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -536,6 +536,104 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   	return ret;
>>   }
>>   
>> +static int q6v6proc_reset(struct q6v5 *qproc)
> Which version is this? 5.5, 55 or 6?
>
> It's perfectly fine to introduce q6v55-functions and use those, but if
> the version is 6 then the compatible should reflect that at least.
OK.
>
>> +{
>> +	int ret, i, count;
>> +	u64 val;
>> +
>> +	/* Override the ACC value if required */
>> +	writel_relaxed(QDSP6SS_ACC_OVERRIDE_VAL,
>> +				qproc->reg_base + QDSP6SS_STRAP_ACC);
>> +
>> +	/* Assert resets, stop core */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_RESET_REG);
>> +	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
>> +
>> +	/* BHS require xo cbcr to be enabled */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +	val |= 0x1;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_XO_CBCR);
>> +	for (count = HALT_CHECK_MAX_LOOPS; count > 0; count--) {
>> +		val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +		if (!(val & BIT(31)))
>> +			break;
>> +		udelay(1);
>> +	}
>> +
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +	if ((val & BIT(31)))
>> +		dev_err(qproc->dev, "Failed to enable xo branch clock.\n");
>> +
>> +	/* Enable power block headswitch, and wait for it to stabilize */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= QDSP6v55_BHS_ON;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	udelay(1);
>> +
>> +	/* Put LDO in bypass mode */
>> +	val |= QDSP6v55_LDO_BYP;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	/*
>> +	 * Turn on memories. L2 banks should be done individually
>> +	 * to minimize inrush current.
>> +	 */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val &= ~QDSP6v55_CLAMP_QMC_MEM;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Deassert memory peripheral sleep and L2 memory standby */
>> +	val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Turn on L1, L2, ETB and JU memories 1 at a time */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
>> +	for (i = 19; i >= 0; i--) {
>> +		val |= BIT(i);
>> +		writel_relaxed(val, qproc->reg_base +
>> +					QDSP6SS_MEM_PWR_CTL);
>> +		/*
>> +		 * Wait for 1us for both memory peripheral and
>> +		 * data array to turn on.
>> +		 */
>> +		 mb();
>> +		udelay(1);
>> +	}
>> +
>> +	/* Remove word line clamp */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val &= ~QDSP6v55_CLAMP_WL;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Remove IO clamp */
>> +	val &= ~Q6SS_CLAMP_IO;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +
>> +	/* Bring core out of reset */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_RESET_REG);
>> +	val &= ~(Q6SS_CORE_ARES | Q6SS_STOP_CORE);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_RESET_REG);
>> +
>> +	/* Turn on core clock */
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>> +	val |= Q6SS_CLK_ENABLE;
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_GFMUX_CTL_REG);
>> +
>> +
>> +	/* Wait for PBL status */
>> +	ret = q6v5_rmb_pbl_wait(qproc, 1000);
>> +	if (ret == -ETIMEDOUT) {
>> +		dev_err(qproc->dev, "PBL boot timed out\n");
>> +	} else if (ret != RMB_PBL_SUCCESS) {
>> +		dev_err(qproc->dev, "PBL returned unexpected status %d\n", ret);
>> +		ret = -EINVAL;
>> +	} else {
>> +		ret = 0;
>> +	}
>> +
>> +	return ret;
>> +}
> Most of this function is exactly the same as q6v5proc_reset(), do we
> need two copies or can we make the differences conditional based on
> compatible?
OK. in reorganized code have merged them in one
>
>> +
>>   static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   				   struct regmap *halt_map,
>>   				   u32 offset)
>> @@ -544,11 +642,6 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>>   	unsigned int val;
>>   	int ret;
>>   
>> -	/* Check if we're already idle */
>> -	ret = regmap_read(halt_map, offset + AXI_IDLE_REG, &val);
>> -	if (!ret && val)
>> -		return;
>> -
> I presume this check isn't needed on the other versions either?
YES, i believe so. have removed that
>
>>   	/* Assert halt request */
>>   	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 1);
>>   
> Regards,
> Bjorn

[PATCH 4/5] remoteproc: Add start and shutdown interface for q6v55

[Avaneesh Kumar Dwivedi]

Adding start and shutdown interface to invoke q6v55 remoteproc.
Additionally maintaining boot count and protecting start and
shutdown sequence with lock.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 166 +++++++++++++++++++++++++++++++++++--
 1 file changed, 160 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 0fac8d8..dd19d41 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -789,9 +789,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 	return ret < 0 ? ret : 0;
 }
 
-static int q6v5_start(struct rproc *rproc)
+static int q6v5_start(struct q6v5 *qproc)
 {
-	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
 
 	ret = q6v5_regulator_enable(qproc);
@@ -873,9 +872,75 @@ static int q6v5_start(struct rproc *rproc)
 	return ret;
 }
 
-static int q6v5_stop(struct rproc *rproc)
+static int q6v55_start(struct q6v5 *qproc)
+{
+	int ret;
+
+	ret = q6v55_proxy_vote(qproc);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable supplies\n");
+		return ret;
+	}
+
+	ret = q6v55_clk_enable(qproc);
+	if (ret) {
+		dev_err(qproc->dev, "failed to enable clocks\n");
+		goto err_clks;
+	}
+
+	pil_mss_restart_reg(qproc, 0);
+
+	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
+
+	ret = q6v6proc_reset(qproc);
+	if (ret)
+		goto halt_axi_ports;
+
+	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
+	if (ret == -ETIMEDOUT) {
+		dev_err(qproc->dev, "MBA boot timed out\n");
+		goto halt_axi_ports;
+	} else if (ret != RMB_MBA_XPU_UNLOCKED &&
+		   ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
+		dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
+		ret = -EINVAL;
+		goto halt_axi_ports;
+	}
+
+	dev_info(qproc->dev, "MBA booted, loading mpss\n");
+
+	ret = q6v5_mpss_load(qproc);
+	if (ret)
+		goto halt_axi_ports;
+
+	ret = wait_for_completion_timeout(&qproc->start_done,
+					  msecs_to_jiffies(10000));
+	if (ret == 0) {
+		dev_err(qproc->dev, "start timed out\n");
+		ret = -ETIMEDOUT;
+		goto halt_axi_ports;
+	}
+
+	qproc->running = true;
+
+	/* TODO: All done, release the handover resources */
+
+	return 0;
+
+halt_axi_ports:
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
+	q6v55_clk_disable(qproc);
+err_clks:
+	pil_mss_restart_reg(qproc, 1);
+	q6v55_proxy_unvote(qproc);
+
+	return ret;
+}
+
+static int q6v5_stop(struct q6v5 *qproc)
 {
-	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
 
 	qproc->running = false;
@@ -903,6 +968,93 @@ static int q6v5_stop(struct rproc *rproc)
 	return 0;
 }
 
+static int q6v55_stop(struct q6v5 *qproc)
+{
+	int ret;
+	u64 val;
+
+	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");
+
+	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);
+	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
+
+	/*
+	* Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
+	* memory clamp as a software workaround to avoid high MX
+	* current during LPASS/MSS restart.
+	*/
+
+	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	val |= (Q6SS_CLAMP_IO | QDSP6v55_CLAMP_WL |
+			QDSP6v55_CLAMP_QMC_MEM);
+	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+	pil_mss_restart_reg(qproc, 1);
+	if (qproc->running) {
+		q6v55_clk_disable(qproc);
+		q6v55_proxy_unvote(qproc);
+		qproc->running = false;
+	}
+
+	return 0;
+}
+
+static int mss_boot(struct rproc *rproc)
+{
+	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	int ret;
+
+	mutex_lock(&qproc->q6_lock);
+	if (!qproc->boot_count) {
+		if (qproc->is_q6v55) {
+			ret = q6v55_start(qproc);
+			if (ret)
+				goto err_start;
+		} else {
+			ret = q6v5_start(qproc);
+			if (ret)
+				goto err_start;
+		}
+	}
+	qproc->boot_count++;
+	mutex_unlock(&qproc->q6_lock);
+	return 0;
+
+err_start:
+	mutex_unlock(&qproc->q6_lock);
+	if (qproc->is_q6v55)
+		q6v55_stop(qproc);
+	else
+		q6v5_stop(qproc);
+
+	return ret;
+}
+
+static int mss_stop(struct rproc *rproc)
+{
+	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	int ret;
+
+	mutex_lock(&qproc->q6_lock);
+	if (!--qproc->boot_count) {
+		if (qproc->is_q6v55)
+			ret = q6v55_stop(qproc);
+		else
+			ret = q6v5_stop(qproc);
+	}
+	mutex_unlock(&qproc->q6_lock);
+	return ret;
+}
+
 static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 {
 	struct q6v5 *qproc = rproc->priv;
@@ -916,8 +1068,8 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
 }
 
 static const struct rproc_ops q6v5_ops = {
-	.start = q6v5_start,
-	.stop = q6v5_stop,
+	.start = mss_boot,
+	.stop = mss_stop,
 	.da_to_va = q6v5_da_to_va,
 };
 
@@ -972,6 +1124,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
 	struct q6v5 *qproc = dev;
 
 	complete(&qproc->start_done);
+	if (qproc->is_q6v55)
+		q6v55_proxy_unvote(qproc);
 	return IRQ_HANDLED;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 4/5] remoteproc: Add start and shutdown interface for q6v55

[Bjorn Andersson]

On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> Adding start and shutdown interface to invoke q6v55 remoteproc.
> Additionally maintaining boot count and protecting start and
> shutdown sequence with lock.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 166 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 160 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 0fac8d8..dd19d41 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -789,9 +789,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	return ret < 0 ? ret : 0;
>  }
>  
> -static int q6v5_start(struct rproc *rproc)
> +static int q6v5_start(struct q6v5 *qproc)
>  {
> -	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
>  
>  	ret = q6v5_regulator_enable(qproc);
> @@ -873,9 +872,75 @@ static int q6v5_start(struct rproc *rproc)
>  	return ret;
>  }
>  
> -static int q6v5_stop(struct rproc *rproc)
> +static int q6v55_start(struct q6v5 *qproc)
> +{

I'm sorry, but this function looks to take exactly the same steps as
q6v5_start(). The clock handling differs and you call a different reset
function, so please merge the clock handling and call the appropriate
reset function based on compatible.

> +	int ret;
> +
> +	ret = q6v55_proxy_vote(qproc);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable supplies\n");
> +		return ret;
> +	}
> +
> +	ret = q6v55_clk_enable(qproc);
> +	if (ret) {
> +		dev_err(qproc->dev, "failed to enable clocks\n");
> +		goto err_clks;
> +	}
> +
> +	pil_mss_restart_reg(qproc, 0);
> +
> +	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
> +
> +	ret = q6v6proc_reset(qproc);
> +	if (ret)
> +		goto halt_axi_ports;
> +
> +	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
> +	if (ret == -ETIMEDOUT) {
> +		dev_err(qproc->dev, "MBA boot timed out\n");
> +		goto halt_axi_ports;
> +	} else if (ret != RMB_MBA_XPU_UNLOCKED &&
> +		   ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
> +		dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
> +		ret = -EINVAL;
> +		goto halt_axi_ports;
> +	}
> +
> +	dev_info(qproc->dev, "MBA booted, loading mpss\n");
> +
> +	ret = q6v5_mpss_load(qproc);
> +	if (ret)
> +		goto halt_axi_ports;
> +
> +	ret = wait_for_completion_timeout(&qproc->start_done,
> +					  msecs_to_jiffies(10000));
> +	if (ret == 0) {
> +		dev_err(qproc->dev, "start timed out\n");
> +		ret = -ETIMEDOUT;
> +		goto halt_axi_ports;
> +	}
> +
> +	qproc->running = true;
> +
> +	/* TODO: All done, release the handover resources */
> +
> +	return 0;
> +
> +halt_axi_ports:
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
> +	q6v55_clk_disable(qproc);
> +err_clks:
> +	pil_mss_restart_reg(qproc, 1);
> +	q6v55_proxy_unvote(qproc);
> +
> +	return ret;
> +}
> +
> +static int q6v5_stop(struct q6v5 *qproc)
>  {
> -	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>  	int ret;
>  
>  	qproc->running = false;
> @@ -903,6 +968,93 @@ static int q6v5_stop(struct rproc *rproc)
>  	return 0;
>  }
>  
> +static int q6v55_stop(struct q6v5 *qproc)
> +{
> +	int ret;
> +	u64 val;
> +
> +	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");
> +
> +	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);
> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
> +
> +	/*
> +	* Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
> +	* memory clamp as a software workaround to avoid high MX
> +	* current during LPASS/MSS restart.
> +	*/
> +
> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	val |= (Q6SS_CLAMP_IO | QDSP6v55_CLAMP_WL |
> +			QDSP6v55_CLAMP_QMC_MEM);
> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);

Is this "quirk" only for version 6? (5.5) Or should we clamp these on
the previous versions as well?

> +
> +	pil_mss_restart_reg(qproc, 1);
> +	if (qproc->running) {

Under what circumstances is will q6v55_stop() be called without
q6v55_start() succeeding?

> +		q6v55_clk_disable(qproc);
> +		q6v55_proxy_unvote(qproc);
> +		qproc->running = false;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mss_boot(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> +	int ret;
> +
> +	mutex_lock(&qproc->q6_lock);
> +	if (!qproc->boot_count) {

The remoteproc framework already have a reference count and will only
call this function in the transition from 0 to 1. So please drop this.

> +		if (qproc->is_q6v55) {
> +			ret = q6v55_start(qproc);
> +			if (ret)
> +				goto err_start;
> +		} else {
> +			ret = q6v5_start(qproc);
> +			if (ret)
> +				goto err_start;
> +		}
> +	}
> +	qproc->boot_count++;
> +	mutex_unlock(&qproc->q6_lock);
> +	return 0;
> +
> +err_start:
> +	mutex_unlock(&qproc->q6_lock);
> +	if (qproc->is_q6v55)
> +		q6v55_stop(qproc);
> +	else
> +		q6v5_stop(qproc);

The start functions should already have cleaned up all resources and
stopped the Hexagon in case of an error, so these should not be needed.

> +
> +	return ret;
> +}

Please inline the few differences into the q6v5_start() and q6_stop()
and drop these wrappers.

> +
> +static int mss_stop(struct rproc *rproc)
> +{
> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> +	int ret;
> +
> +	mutex_lock(&qproc->q6_lock);
> +	if (!--qproc->boot_count) {
> +		if (qproc->is_q6v55)
> +			ret = q6v55_stop(qproc);
> +		else
> +			ret = q6v5_stop(qproc);
> +	}
> +	mutex_unlock(&qproc->q6_lock);
> +	return ret;
> +}
> +
>  static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
>  {
>  	struct q6v5 *qproc = rproc->priv;
> @@ -916,8 +1068,8 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
>  }
>  
>  static const struct rproc_ops q6v5_ops = {
> -	.start = q6v5_start,
> -	.stop = q6v5_stop,
> +	.start = mss_boot,
> +	.stop = mss_stop,
>  	.da_to_va = q6v5_da_to_va,
>  };
>  
> @@ -972,6 +1124,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
>  	struct q6v5 *qproc = dev;
>  
>  	complete(&qproc->start_done);
> +	if (qproc->is_q6v55)
> +		q6v55_proxy_unvote(qproc);

q6v5_start() is waiting for start_done to be completed, so please unvote
at the point where I have the comment "TODO: All done, release the
handover resources".

>  	return IRQ_HANDLED;
>  }
>  

Regards,
Bjorn

Re: [PATCH 4/5] remoteproc: Add start and shutdown interface for q6v55

[Avaneesh Kumar Dwivedi]

On 10/26/2016 12:57 AM, Bjorn Andersson wrote:
> On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Adding start and shutdown interface to invoke q6v55 remoteproc.
>> Additionally maintaining boot count and protecting start and
>> shutdown sequence with lock.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 166 +++++++++++++++++++++++++++++++++++--
>>   1 file changed, 160 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index 0fac8d8..dd19d41 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -789,9 +789,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> -static int q6v5_start(struct rproc *rproc)
>> +static int q6v5_start(struct q6v5 *qproc)
>>   {
>> -	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>>   
>>   	ret = q6v5_regulator_enable(qproc);
>> @@ -873,9 +872,75 @@ static int q6v5_start(struct rproc *rproc)
>>   	return ret;
>>   }
>>   
>> -static int q6v5_stop(struct rproc *rproc)
>> +static int q6v55_start(struct q6v5 *qproc)
>> +{
> I'm sorry, but this function looks to take exactly the same steps as
> q6v5_start(). The clock handling differs and you call a different reset
> function, so please merge the clock handling and call the appropriate
> reset function based on compatible.
OK, have done away with different start functions.
>
>> +	int ret;
>> +
>> +	ret = q6v55_proxy_vote(qproc);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable supplies\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = q6v55_clk_enable(qproc);
>> +	if (ret) {
>> +		dev_err(qproc->dev, "failed to enable clocks\n");
>> +		goto err_clks;
>> +	}
>> +
>> +	pil_mss_restart_reg(qproc, 0);
>> +
>> +	writel_relaxed(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG);
>> +
>> +	ret = q6v6proc_reset(qproc);
>> +	if (ret)
>> +		goto halt_axi_ports;
>> +
>> +	ret = q6v5_rmb_mba_wait(qproc, 0, 5000);
>> +	if (ret == -ETIMEDOUT) {
>> +		dev_err(qproc->dev, "MBA boot timed out\n");
>> +		goto halt_axi_ports;
>> +	} else if (ret != RMB_MBA_XPU_UNLOCKED &&
>> +		   ret != RMB_MBA_XPU_UNLOCKED_SCRIBBLED) {
>> +		dev_err(qproc->dev, "MBA returned unexpected status %d\n", ret);
>> +		ret = -EINVAL;
>> +		goto halt_axi_ports;
>> +	}
>> +
>> +	dev_info(qproc->dev, "MBA booted, loading mpss\n");
>> +
>> +	ret = q6v5_mpss_load(qproc);
>> +	if (ret)
>> +		goto halt_axi_ports;
>> +
>> +	ret = wait_for_completion_timeout(&qproc->start_done,
>> +					  msecs_to_jiffies(10000));
>> +	if (ret == 0) {
>> +		dev_err(qproc->dev, "start timed out\n");
>> +		ret = -ETIMEDOUT;
>> +		goto halt_axi_ports;
>> +	}
>> +
>> +	qproc->running = true;
>> +
>> +	/* TODO: All done, release the handover resources */
>> +
>> +	return 0;
>> +
>> +halt_axi_ports:
>> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
>> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>> +	q6v55_clk_disable(qproc);
>> +err_clks:
>> +	pil_mss_restart_reg(qproc, 1);
>> +	q6v55_proxy_unvote(qproc);
>> +
>> +	return ret;
>> +}
>> +
>> +static int q6v5_stop(struct q6v5 *qproc)
>>   {
>> -	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>>   	int ret;
>>   
>>   	qproc->running = false;
>> @@ -903,6 +968,93 @@ static int q6v5_stop(struct rproc *rproc)
>>   	return 0;
>>   }
>>   
>> +static int q6v55_stop(struct q6v5 *qproc)
>> +{
>> +	int ret;
>> +	u64 val;
>> +
>> +	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");
>> +
>> +	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);
>> +	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>> +
>> +	/*
>> +	* Assert QDSP6 I/O clamp, memory wordline clamp, and compiler
>> +	* memory clamp as a software workaround to avoid high MX
>> +	* current during LPASS/MSS restart.
>> +	*/
>> +
>> +	val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> +	val |= (Q6SS_CLAMP_IO | QDSP6v55_CLAMP_WL |
>> +			QDSP6v55_CLAMP_QMC_MEM);
>> +	writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> Is this "quirk" only for version 6? (5.5) Or should we clamp these on
> the previous versions as well?
only for q6v56
>
>> +
>> +	pil_mss_restart_reg(qproc, 1);
>> +	if (qproc->running) {
> Under what circumstances is will q6v55_stop() be called without
> q6v55_start() succeeding?
OK.
>
>> +		q6v55_clk_disable(qproc);
>> +		q6v55_proxy_unvote(qproc);
>> +		qproc->running = false;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mss_boot(struct rproc *rproc)
>> +{
>> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> +	int ret;
>> +
>> +	mutex_lock(&qproc->q6_lock);
>> +	if (!qproc->boot_count) {
> The remoteproc framework already have a reference count and will only
> call this function in the transition from 0 to 1. So please drop this.
OK
>
>> +		if (qproc->is_q6v55) {
>> +			ret = q6v55_start(qproc);
>> +			if (ret)
>> +				goto err_start;
>> +		} else {
>> +			ret = q6v5_start(qproc);
>> +			if (ret)
>> +				goto err_start;
>> +		}
>> +	}
>> +	qproc->boot_count++;
>> +	mutex_unlock(&qproc->q6_lock);
>> +	return 0;
>> +
>> +err_start:
>> +	mutex_unlock(&qproc->q6_lock);
>> +	if (qproc->is_q6v55)
>> +		q6v55_stop(qproc);
>> +	else
>> +		q6v5_stop(qproc);
> The start functions should already have cleaned up all resources and
> stopped the Hexagon in case of an error, so these should not be needed.
OK, this function is removed
>
>> +
>> +	return ret;
>> +}
> Please inline the few differences into the q6v5_start() and q6_stop()
> and drop these wrappers.
OK.
>
>> +
>> +static int mss_stop(struct rproc *rproc)
>> +{
>> +	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
>> +	int ret;
>> +
>> +	mutex_lock(&qproc->q6_lock);
>> +	if (!--qproc->boot_count) {
>> +		if (qproc->is_q6v55)
>> +			ret = q6v55_stop(qproc);
>> +		else
>> +			ret = q6v5_stop(qproc);
>> +	}
>> +	mutex_unlock(&qproc->q6_lock);
>> +	return ret;
>> +}
>> +
>>   static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
>>   {
>>   	struct q6v5 *qproc = rproc->priv;
>> @@ -916,8 +1068,8 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len)
>>   }
>>   
>>   static const struct rproc_ops q6v5_ops = {
>> -	.start = q6v5_start,
>> -	.stop = q6v5_stop,
>> +	.start = mss_boot,
>> +	.stop = mss_stop,
>>   	.da_to_va = q6v5_da_to_va,
>>   };
>>   
>> @@ -972,6 +1124,8 @@ static irqreturn_t q6v5_handover_interrupt(int irq, void *dev)
>>   	struct q6v5 *qproc = dev;
>>   
>>   	complete(&qproc->start_done);
>> +	if (qproc->is_q6v55)
>> +		q6v55_proxy_unvote(qproc);
> q6v5_start() is waiting for start_done to be completed, so please unvote
> at the point where I have the comment "TODO: All done, release the
> handover resources".
OK
>
>>   	return IRQ_HANDLED;
>>   }
>>   
> Regards,
> Bjorn

[PATCH 5/5] remoteproc: Modifying probe for initializing q6v55 specific resources

[Avaneesh Kumar Dwivedi]

Probe is being modified to save q6 version and invoke appropriate
init functions to accommodate q6v55 remoteproc driver.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_pil.c | 43 ++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index dd19d41..c65c904 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -1370,6 +1370,9 @@ static int q6v5_probe(struct platform_device *pdev)
 	init_completion(&qproc->start_done);
 	init_completion(&qproc->stop_done);
 
+	if (of_device_is_compatible(pdev->dev.of_node, "qcom,q6v55-pil"))
+		qproc->is_q6v55 = true;
+
 	ret = q6v5_init_mem(qproc, pdev);
 	if (ret)
 		goto free_rproc;
@@ -1378,17 +1381,39 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
-	ret = q6v5_init_clocks(qproc);
-	if (ret)
-		goto free_rproc;
+	if (qproc->is_q6v55) {
+		ret = q6v55_init_clocks(qproc);
+		if (ret)
+			goto free_rproc;
+	} else {
+		ret = q6v5_init_clocks(qproc);
+		if (ret)
+			goto free_rproc;
+	}
 
-	ret = q6v5_regulator_init(qproc);
-	if (ret)
-		goto free_rproc;
+	if (qproc->is_q6v55) {
+		ret = q6v55_init_reset(qproc, pdev);
+		if (ret)
+			goto free_rproc;
+	} else {
+		ret = q6v5_init_reset(qproc);
+		if (ret)
+			goto free_rproc;
+	}
 
-	ret = q6v5_init_reset(qproc);
-	if (ret)
-		goto free_rproc;
+	if (qproc->is_q6v55) {
+		ret = q6v55_regulator_init(qproc);
+		if (ret)
+			goto free_rproc;
+	} else {
+		ret = q6v5_regulator_init(qproc);
+		if (ret)
+			goto free_rproc;
+	}
+
+	qproc->ahb_clk_vote = of_property_read_bool(pdev->dev.of_node,
+			"qcom,ahb-clk-vote");
+	mutex_init(&qproc->q6_lock);
 
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 5/5] remoteproc: Modifying probe for initializing q6v55 specific resources

[Bjorn Andersson]

On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:

> Probe is being modified to save q6 version and invoke appropriate
> init functions to accommodate q6v55 remoteproc driver.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_pil.c | 43 ++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index dd19d41..c65c904 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -1370,6 +1370,9 @@ static int q6v5_probe(struct platform_device *pdev)
>  	init_completion(&qproc->start_done);
>  	init_completion(&qproc->stop_done);
>  
> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,q6v55-pil"))
> +		qproc->is_q6v55 = true;
> +

With the changes I've suggested in the other patches I would recommend
that you describe the differences as a set of "features" in a struct
that you put as .data in the match table and then use
of_device_get_match_data() to acquire the matching data struct.

>  	ret = q6v5_init_mem(qproc, pdev);
>  	if (ret)
>  		goto free_rproc;
> @@ -1378,17 +1381,39 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_rproc;
>  
> -	ret = q6v5_init_clocks(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	if (qproc->is_q6v55) {
> +		ret = q6v55_init_clocks(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	} else {
> +		ret = q6v5_init_clocks(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	}

Based on the fact that I don't think this is a q6v55, but rather a q6v6,
we will now end up with:

if (is_q6v55) {
} else if (is_q6v6) {
} else if (is_q6v5) {
} else {
	fail
};

And this function will turn into an (even worse) mess.


I would suggest that you instead define each resource as a flag and
provide a struct with these flags as .data with the compatible. Then
pass that to the clock and regulator init and based on the flags they
can acquire the individual resources. That way adding a new version is a
matter of listing which resources that needs to grab.

And in that struct you can also have a function pointer to an
appropriate reset function, completely removing the need for checking
which version we have after initialization.

>  
> -	ret = q6v5_regulator_init(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	if (qproc->is_q6v55) {
> +		ret = q6v55_init_reset(qproc, pdev);
> +		if (ret)
> +			goto free_rproc;
> +	} else {
> +		ret = q6v5_init_reset(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	}
>  
> -	ret = q6v5_init_reset(qproc);
> -	if (ret)
> -		goto free_rproc;
> +	if (qproc->is_q6v55) {
> +		ret = q6v55_regulator_init(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	} else {
> +		ret = q6v5_regulator_init(qproc);
> +		if (ret)
> +			goto free_rproc;
> +	}
> +
> +	qproc->ahb_clk_vote = of_property_read_bool(pdev->dev.of_node,
> +			"qcom,ahb-clk-vote");
> +	mutex_init(&qproc->q6_lock);
>  
>  	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>  	if (ret < 0)

Regards,
Bjorn

Re: [PATCH 5/5] remoteproc: Modifying probe for initializing q6v55 specific resources

[Avaneesh Kumar Dwivedi]

On 10/26/2016 1:05 AM, Bjorn Andersson wrote:
> On Mon 24 Oct 08:55 PDT 2016, Avaneesh Kumar Dwivedi wrote:
>
>> Probe is being modified to save q6 version and invoke appropriate
>> init functions to accommodate q6v55 remoteproc driver.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pil.c | 43 ++++++++++++++++++++++++++++++--------
>>   1 file changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index dd19d41..c65c904 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -1370,6 +1370,9 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	init_completion(&qproc->start_done);
>>   	init_completion(&qproc->stop_done);
>>   
>> +	if (of_device_is_compatible(pdev->dev.of_node, "qcom,q6v55-pil"))
>> +		qproc->is_q6v55 = true;
>> +
> With the changes I've suggested in the other patches I would recommend
> that you describe the differences as a set of "features" in a struct
> that you put as .data in the match table and then use
> of_device_get_match_data() to acquire the matching data struct.
OK, have tried on suggested line in patchset v2 to be send out soon.
>
>>   	ret = q6v5_init_mem(qproc, pdev);
>>   	if (ret)
>>   		goto free_rproc;
>> @@ -1378,17 +1381,39 @@ static int q6v5_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto free_rproc;
>>   
>> -	ret = q6v5_init_clocks(qproc);
>> -	if (ret)
>> -		goto free_rproc;
>> +	if (qproc->is_q6v55) {
>> +		ret = q6v55_init_clocks(qproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	} else {
>> +		ret = q6v5_init_clocks(qproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	}
> Based on the fact that I don't think this is a q6v55, but rather a q6v6,
> we will now end up with:
>
> if (is_q6v55) {
> } else if (is_q6v6) {
> } else if (is_q6v5) {
> } else {
> 	fail
> };
>
> And this function will turn into an (even worse) mess.
>
>
> I would suggest that you instead define each resource as a flag and
> provide a struct with these flags as .data with the compatible. Then
> pass that to the clock and regulator init and based on the flags they
> can acquire the individual resources. That way adding a new version is a
> matter of listing which resources that needs to grab.
>
> And in that struct you can also have a function pointer to an
> appropriate reset function, completely removing the need for checking
> which version we have after initialization.
Agreed
>
>>   
>> -	ret = q6v5_regulator_init(qproc);
>> -	if (ret)
>> -		goto free_rproc;
>> +	if (qproc->is_q6v55) {
>> +		ret = q6v55_init_reset(qproc, pdev);
>> +		if (ret)
>> +			goto free_rproc;
>> +	} else {
>> +		ret = q6v5_init_reset(qproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	}
>>   
>> -	ret = q6v5_init_reset(qproc);
>> -	if (ret)
>> -		goto free_rproc;
>> +	if (qproc->is_q6v55) {
>> +		ret = q6v55_regulator_init(qproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	} else {
>> +		ret = q6v5_regulator_init(qproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	}
>> +
>> +	qproc->ahb_clk_vote = of_property_read_bool(pdev->dev.of_node,
>> +			"qcom,ahb-clk-vote");
>> +	mutex_init(&qproc->q6_lock);
>>   
>>   	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
>>   	if (ret < 0)
> Regards,
> Bjorn