Re: [PATCH V4 2/8] soc: mediatek: pwrap: add caps flag for pwrap

Re: [PATCH V4 2/8] soc: mediatek: pwrap: add caps flag for pwrap

[Matthias Brugger]

On 05/02/2018 11:21 AM, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> We use caps to describe pwrap's capability, used
> to replace has_bridge flag for single meaning.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index e9e054a15b7d..3a25ff6e6907 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -76,6 +76,12 @@
>  #define PWRAP_SLV_CAP_SECURITY	BIT(2)
>  #define HAS_CAP(_c, _x)	(((_c) & (_x)) == (_x))
>  
> +/* Group of bits used for shown pwrap capability */
> +#define PWRAP_CAP_BRIDGE	BIT(0)
> +#define PWRAP_CAP_RESET		BIT(1)
> +#define PWRAP_CAP_DCM		BIT(2)
> +#define PWRAP_CAP_INT1_EN	BIT(3)
> +

You are missing the logic to handle the caps.
With out this logic, this patch is useless.

Regards,
Matthias

>  /* defines for slave device wrapper registers */
>  enum dew_regs {
>  	PWRAP_DEW_BASE,
> @@ -684,6 +690,8 @@ struct pmic_wrapper_type {
>  	u32 spi_w;
>  	u32 wdt_src;
>  	unsigned int has_bridge:1;
> +	/* Flags indicating the capability for the target pwrap */
> +	u8 caps;
>  	int (*init_reg_clock)(struct pmic_wrapper *wrp);
>  	int (*init_soc_specific)(struct pmic_wrapper *wrp);
>  };
> @@ -1394,6 +1402,7 @@ static const struct pmic_wrapper_type pwrap_mt2701 = {
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE_NEW,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
>  	.has_bridge = 0,
> +	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_mt2701_init_reg_clock,
>  	.init_soc_specific = pwrap_mt2701_init_soc_specific,
>  };
> @@ -1406,6 +1415,7 @@ static const struct pmic_wrapper_type pwrap_mt7622 = {
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
>  	.has_bridge = 0,
> +	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_common_init_reg_clock,
>  	.init_soc_specific = pwrap_mt7622_init_soc_specific,
>  };
> @@ -1418,6 +1428,7 @@ static const struct pmic_wrapper_type pwrap_mt8135 = {
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
>  	.has_bridge = 1,
> +	.caps = PWRAP_CAP_BRIDGE | PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_common_init_reg_clock,
>  	.init_soc_specific = pwrap_mt8135_init_soc_specific,
>  };
> -- 
> 2.12.5
> 
> ************* Email Confidentiality Notice ********************
> The information contained in this e-mail message (including any 
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be 
> conveyed only to the designated recipient(s). Any use, dissemination, 
> distribution, printing, retaining or copying of this e-mail (including its 
> attachments) by unintended recipient(s) is strictly prohibited and may 
> be unlawful. If you are not an intended recipient of this e-mail, or believe 
> that you have received this e-mail in error, please notify the sender 
> immediately (by replying to this e-mail), delete any and all copies of 
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!
> 

Re: [PATCH V4 3/8] soc: mediatek: pwrap: remove has_bridge flag

[Matthias Brugger]

On 05/02/2018 11:21 AM, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> We use new flag caps to replace has_bridge.
> Legacy chips support bridge use PWRAP_CAP_BRIDGE
> to explain such capability.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---

Squash this one into 2/8.

Regards,
Matthias

>  drivers/soc/mediatek/mtk-pmic-wrap.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 3a25ff6e6907..911a8508f39f 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -689,7 +689,6 @@ struct pmic_wrapper_type {
>  	u32 int_en_all;
>  	u32 spi_w;
>  	u32 wdt_src;
> -	unsigned int has_bridge:1;
>  	/* Flags indicating the capability for the target pwrap */
>  	u8 caps;
>  	int (*init_reg_clock)(struct pmic_wrapper *wrp);
> @@ -1306,7 +1305,7 @@ static int pwrap_init(struct pmic_wrapper *wrp)
>  	pwrap_writel(wrp, 1, PWRAP_INIT_DONE0);
>  	pwrap_writel(wrp, 1, PWRAP_INIT_DONE1);
>  
> -	if (wrp->master->has_bridge) {
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
>  		writel(1, wrp->bridge_base + PWRAP_MT8135_BRIDGE_INIT_DONE3);
>  		writel(1, wrp->bridge_base + PWRAP_MT8135_BRIDGE_INIT_DONE4);
>  	}
> @@ -1401,7 +1400,6 @@ static const struct pmic_wrapper_type pwrap_mt2701 = {
>  	.int_en_all = ~(u32)(BIT(31) | BIT(2)),
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE_NEW,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> -	.has_bridge = 0,
>  	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_mt2701_init_reg_clock,
>  	.init_soc_specific = pwrap_mt2701_init_soc_specific,
> @@ -1414,7 +1412,6 @@ static const struct pmic_wrapper_type pwrap_mt7622 = {
>  	.int_en_all = ~(u32)BIT(31),
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> -	.has_bridge = 0,
>  	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_common_init_reg_clock,
>  	.init_soc_specific = pwrap_mt7622_init_soc_specific,
> @@ -1427,7 +1424,6 @@ static const struct pmic_wrapper_type pwrap_mt8135 = {
>  	.int_en_all = ~(u32)(BIT(31) | BIT(1)),
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> -	.has_bridge = 1,
>  	.caps = PWRAP_CAP_BRIDGE | PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_common_init_reg_clock,
>  	.init_soc_specific = pwrap_mt8135_init_soc_specific,
> @@ -1440,7 +1436,7 @@ static const struct pmic_wrapper_type pwrap_mt8173 = {
>  	.int_en_all = ~(u32)(BIT(31) | BIT(1)),
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_NO_STAUPD,
> -	.has_bridge = 0,
> +	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
>  	.init_reg_clock = pwrap_common_init_reg_clock,
>  	.init_soc_specific = pwrap_mt8173_init_soc_specific,
>  };
> @@ -1509,7 +1505,7 @@ static int pwrap_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	if (wrp->master->has_bridge) {
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  				"pwrap-bridge");
>  		wrp->bridge_base = devm_ioremap_resource(wrp->dev, res);
> -- 
> 2.12.5
> 
> ************* Email Confidentiality Notice ********************
> The information contained in this e-mail message (including any 
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be 
> conveyed only to the designated recipient(s). Any use, dissemination, 
> distribution, printing, retaining or copying of this e-mail (including its 
> attachments) by unintended recipient(s) is strictly prohibited and may 
> be unlawful. If you are not an intended recipient of this e-mail, or believe 
> that you have received this e-mail in error, please notify the sender 
> immediately (by replying to this e-mail), delete any and all copies of 
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!
> 

Re: [PATCH V4 4/8] soc: mediatek: pwrap: add int1_en_all flag

[Matthias Brugger]

On 05/02/2018 11:21 AM, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> MT6797 support int1_en flag for starvation
> and cmd_miss exception interrupt. We add
> a new flag int1_en_all to check if we need
> to enable interrupt source or not.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
Same as 2/8. We need the logic that uses this flag in the same patch not in a
later one.

> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 911a8508f39f..a6366f147b79 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -687,6 +687,7 @@ struct pmic_wrapper_type {
>  	enum pwrap_type type;
>  	u32 arb_en_all;
>  	u32 int_en_all;
> +	u32 int1_en_all;
>  	u32 spi_w;
>  	u32 wdt_src;
>  	/* Flags indicating the capability for the target pwrap */
> @@ -1398,6 +1399,7 @@ static const struct pmic_wrapper_type pwrap_mt2701 = {
>  	.type = PWRAP_MT2701,
>  	.arb_en_all = 0x3f,
>  	.int_en_all = ~(u32)(BIT(31) | BIT(2)),
> +	.int1_en_all = 0,
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE_NEW,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
>  	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
> @@ -1410,6 +1412,7 @@ static const struct pmic_wrapper_type pwrap_mt7622 = {
>  	.type = PWRAP_MT7622,
>  	.arb_en_all = 0xff,
>  	.int_en_all = ~(u32)BIT(31),
> +	.int1_en_all = 0,
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
>  	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
> @@ -1422,6 +1425,7 @@ static const struct pmic_wrapper_type pwrap_mt8135 = {
>  	.type = PWRAP_MT8135,
>  	.arb_en_all = 0x1ff,
>  	.int_en_all = ~(u32)(BIT(31) | BIT(1)),
> +	.int1_en_all = 0,
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
>  	.caps = PWRAP_CAP_BRIDGE | PWRAP_CAP_RESET | PWRAP_CAP_DCM,
> @@ -1434,6 +1438,7 @@ static const struct pmic_wrapper_type pwrap_mt8173 = {
>  	.type = PWRAP_MT8173,
>  	.arb_en_all = 0x3f,
>  	.int_en_all = ~(u32)(BIT(31) | BIT(1)),
> +	.int1_en_all = 0,
>  	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
>  	.wdt_src = PWRAP_WDT_SRC_MASK_NO_STAUPD,
>  	.caps = PWRAP_CAP_RESET | PWRAP_CAP_DCM,
> -- 
> 2.12.5
> 
> ************* Email Confidentiality Notice ********************
> The information contained in this e-mail message (including any 
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be 
> conveyed only to the designated recipient(s). Any use, dissemination, 
> distribution, printing, retaining or copying of this e-mail (including its 
> attachments) by unintended recipient(s) is strictly prohibited and may 
> be unlawful. If you are not an intended recipient of this e-mail, or believe 
> that you have received this e-mail in error, please notify the sender 
> immediately (by replying to this e-mail), delete any and all copies of 
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!
> 

Re: [PATCH V4 1/8] dt-bindings: pwrap: mediatek: add MT6351 PMIC support for MT6797

[Sean Wang]

On Wed, 2018-05-02 at 17:21 +0800, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> We add MT6351 PMIC support for MT6797 SoCs.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  Documentation/devicetree/bindings/soc/mediatek/pwrap.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> index bf80e3f96f8c..f9987c30f0d5 100644
> --- a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +++ b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> @@ -19,6 +19,7 @@ IP Pairing
>  Required properties in pwrap device node.
>  - compatible:
>  	"mediatek,mt2701-pwrap" for MT2701/7623 SoCs
> +	"mediatek,mt6797-pwrap" for MT6797 SoCs
>  	"mediatek,mt7622-pwrap" for MT7622 SoCs
>  	"mediatek,mt8135-pwrap" for MT8135 SoCs
>  	"mediatek,mt8173-pwrap" for MT8173 SoCs

The content doesn't describe anything about MT6351, which seems to be
inconsistent with its title

Re: [PATCH V4 8/8] arm64: dts: mt6797: add pwrap support for mt6797

[Sean Wang]

On Wed, 2018-05-02 at 17:21 +0800, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> mt6797 is a highly integrated SoCs, and it uses
> mt6351 as Power Management IC.
> We need to add pwrap device to communicate with
> mt6351 by SPI.
> The base address of pwrap is 0x1000d000, and IRQ
> number is 178. It also using fixed 26Mhz clock
> as SPI CLK.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  arch/arm64/boot/dts/mediatek/mt6797.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt6797.dtsi b/arch/arm64/boot/dts/mediatek/mt6797.dtsi
> index 4beaa71107d7..485546efc9bf 100644
> --- a/arch/arm64/boot/dts/mediatek/mt6797.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt6797.dtsi
> @@ -161,6 +161,20 @@
>  		      <0 0x10220690 0 0x10>;
>  	};
>  
> +	pwrap: pwrap@1000d000 {
> +		compatible = "mediatek,mt6797-pwrap";
> +		reg = <0 0x1000d000 0 0x1000>;
> +		reg-names = "pwrap";
> +		interrupts = <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clk26m>, <&clk26m>;
> +		clock-names = "spi", "wrap";
> +
> +		pmic: mt6351 {
> +			compatible = "mediatek,mt6351";
> +			interrupt-controller;

the child node can't be added until MT6351 support is added to
dt-binding

> +		};
> +	};
> +
>  	uart0: serial@11002000 {
>  		compatible = "mediatek,mt6797-uart",
>  			     "mediatek,mt6577-uart";

Re: [PATCH V4 7/8] soc: mediatek: pwrap: add mt6351 for mt6797 SoCs

[Sean Wang]

Hi, Argus

On Wed, 2018-05-02 at 17:21 +0800, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> mt6351 is a new power management IC and it is
> used for mt6797 SoCs. We need to add mt6351_regs for
> pmic register mapping and pmic_mt6351 for
> register accessing by regmap.
> 

suggest line wrapping closely at 75 columns

> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 285bfa76249f..26076900eee0 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -152,6 +152,21 @@ static const u32 mt6397_regs[] = {
>  	[PWRAP_DEW_CIPHER_SWRST] =	0xbc24,
>  };
>  
> +static const u32 mt6351_regs[] = {
> +	[PWRAP_DEW_DIO_EN] =		0x02F2,
> +	[PWRAP_DEW_READ_TEST] =		0x02F4,
> +	[PWRAP_DEW_WRITE_TEST] =	0x02F6,
> +	[PWRAP_DEW_CRC_EN] =		0x02FA,
> +	[PWRAP_DEW_CRC_VAL] =		0x02FC,
> +	[PWRAP_DEW_CIPHER_KEY_SEL] =	0x0300,
> +	[PWRAP_DEW_CIPHER_IV_SEL] =	0x0302,
> +	[PWRAP_DEW_CIPHER_EN] =		0x0304,
> +	[PWRAP_DEW_CIPHER_RDY] =	0x0306,
> +	[PWRAP_DEW_CIPHER_MODE] =	0x0308,
> +	[PWRAP_DEW_CIPHER_SWRST] =	0x030A,
> +	[PWRAP_DEW_RDDMY_NO] =		0x030C,
> +};
> +

trim the unused registers if any

>  enum pwrap_regs {
>  	PWRAP_MUX_SEL,
>  	PWRAP_WRAP_EN,
> @@ -684,6 +699,7 @@ static int mt8135_regs[] = {
>  
>  enum pmic_type {
>  	PMIC_MT6323,
> +	PMIC_MT6351,
>  	PMIC_MT6380,
>  	PMIC_MT6397,
>  };
> @@ -1150,6 +1166,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>  			    0x1);
>  		break;
>  	case PMIC_MT6323:
> +	case PMIC_MT6351:
>  		pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_CIPHER_EN],
>  			    0x1);
>  		break;
> @@ -1435,6 +1452,15 @@ static const struct pwrap_slv_type pmic_mt6397 = {
>  	.pwrap_write = pwrap_write16,
>  };
>  
> +static const struct pwrap_slv_type pmic_mt6351 = {
> +	.dew_regs = mt6351_regs,
> +	.type = PMIC_MT6351,
> +	.regmap = &pwrap_regmap_config16,
> +	.caps = 0,

the caps should be

PWRAP_SLV_CAP_SPI | PWRAP_SLV_CAP_DUALIO |
                 PWRAP_SLV_CAP_SECURITY,
 
otherwise, the registers you defined here cannot be accessed by its
function.


> +	.pwrap_read = pwrap_read16,
> +	.pwrap_write = pwrap_write16,
> +};
> +
>  static const struct of_device_id of_slave_match_tbl[] = {
>  	{
>  		.compatible = "mediatek,mt6323",
> @@ -1449,6 +1475,9 @@ static const struct of_device_id of_slave_match_tbl[] = {
>  		.compatible = "mediatek,mt6397",
>  		.data = &pmic_mt6397,
>  	}, {
> +		.compatible = "mediatek,mt6351",
> +		.data = &pmic_mt6351,
> +	}, {

need to be sorted din alphabetical order

>  		/* sentinel */
>  	}
>  };

Re: [PATCH V4 5/8] soc: mediatek: pwrap: add pwrap for mt6797 SoCs

[Matthias Brugger]

On 05/02/2018 11:21 AM, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> mt6797 is a highly integrated SoCs, it uses mt6351 for power
> management. We need to add pwrap support to access mt6351.
> Pwrap of mt6797 support new feature include starvation and channel
> request exception interrupt, dynamic starvation priority
> adjustment mechanism.
> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 110 ++++++++++++++++++++++++++++++++---
>  1 file changed, 102 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index a6366f147b79..0d4a2dae6912 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c

[...]

> @@ -1076,11 +1126,14 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>  		break;
>  	case PWRAP_MT2701:
>  	case PWRAP_MT8173:
> +	case PWRAP_MT6797:
>  		pwrap_writel(wrp, 1, PWRAP_CIPHER_EN);
>  		break;
>  	case PWRAP_MT7622:
>  		pwrap_writel(wrp, 0, PWRAP_CIPHER_EN);
>  		break;
> +	default:
> +		break;

Why do we need that now?

>  	}
>  
>  	/* Config cipher mode @PMIC */
> @@ -1325,6 +1378,15 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>  
>  	pwrap_writel(wrp, 0xffffffff, PWRAP_INT_CLR);
>  
> +	/* If we support INT1 interrupt, we also need to clear it */
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
> +		rdata = pwrap_readl(wrp, PWRAP_INT1_FLG);
> +
> +		dev_err(wrp->dev, "unexpected interrupt int1=0x%x\n", rdata);
> +
> +		pwrap_writel(wrp, rdata, PWRAP_INT1_CLR);
> +	}
> +

This should go in 4/8. You will need to add PWRAP_INT1_FLG to the enum
pwrap_regs as well.

>  	return IRQ_HANDLED;
>  }
>  
> @@ -1446,6 +1508,19 @@ static const struct pmic_wrapper_type pwrap_mt8173 = {
>  	.init_soc_specific = pwrap_mt8173_init_soc_specific,
>  };
>  
> +static const struct pmic_wrapper_type pwrap_mt6797 = {
> +	.regs = mt6797_regs,
> +	.type = PWRAP_MT6797,
> +	.arb_en_all = 0x01fff,
> +	.int_en_all = 0xffffffc6,
> +	.int1_en_all = 0x0001ffff,
> +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> +	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> +	.caps = PWRAP_CAP_DCM | PWRAP_CAP_INT1_EN,
> +	.init_reg_clock = pwrap_common_init_reg_clock,
> +	.init_soc_specific = NULL,
> +};
> +
>  static const struct of_device_id of_pwrap_match_tbl[] = {
>  	{
>  		.compatible = "mediatek,mt2701-pwrap",
> @@ -1460,6 +1535,9 @@ static const struct of_device_id of_pwrap_match_tbl[] = {
>  		.compatible = "mediatek,mt8173-pwrap",
>  		.data = &pwrap_mt8173,
>  	}, {
> +		.compatible = "mediatek,mt6797-pwrap",
> +		.data = &pwrap_mt6797,
> +	}, {
>  		/* sentinel */
>  	}
>  };
> @@ -1503,11 +1581,13 @@ static int pwrap_probe(struct platform_device *pdev)
>  	if (IS_ERR(wrp->base))
>  		return PTR_ERR(wrp->base);
>  
> -	wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
> -	if (IS_ERR(wrp->rstc)) {
> -		ret = PTR_ERR(wrp->rstc);
> -		dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
> -		return ret;
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_RESET)) {
> +		wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
> +		if (IS_ERR(wrp->rstc)) {
> +			ret = PTR_ERR(wrp->rstc);
> +			dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
> +			return ret;
> +		}

This goes into 2/8.

>  	}
>  
>  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
> @@ -1549,9 +1629,17 @@ static int pwrap_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_out1;
>  
> -	/* Enable internal dynamic clock */
> -	pwrap_writel(wrp, 1, PWRAP_DCM_EN);
> -	pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
> +	/*
> +	 * add dcm capability check
> +	 */
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_DCM)) {

Into 2/8 please.

> +		if (wrp->master->type == PWRAP_MT6797)
> +			pwrap_writel(wrp, 3, PWRAP_DCM_EN);
> +		else
> +			pwrap_writel(wrp, 1, PWRAP_DCM_EN);

The if statement is ok here, but it should change the if of the caps check
introduced in 2/8. Does this make sense?

> +
> +		pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
> +	}

Into 2/8 please.

>  
>  	/*
>  	 * The PMIC could already be initialized by the bootloader.
> @@ -1580,6 +1668,12 @@ static int pwrap_probe(struct platform_device *pdev)
>  	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
>  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
>  	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
> +	/*
> +	 * We add INT1 interrupt to handle starvation and request exception
> +	 * If we support it, we should enable them here.
> +	 */
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
> +		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);

Into 4/8 please.

Best regards,
Matthias

Re: [PATCH V4 5/8] soc: mediatek: pwrap: add pwrap for mt6797 SoCs

[Sean Wang]

Hi, Argus

On Wed, 2018-05-02 at 17:21 +0800, argus.lin@mediatek.com wrote:
> From: Argus Lin <argus.lin@mediatek.com>
> 
> mt6797 is a highly integrated SoCs, it uses mt6351 for power
> management. We need to add pwrap support to access mt6351.
> Pwrap of mt6797 support new feature include starvation and channel
> request exception interrupt, dynamic starvation priority
> adjustment mechanism.

suggest line wrapping closely at 75 columns

> 
> Signed-off-by: Argus Lin <argus.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 110 ++++++++++++++++++++++++++++++++---
>  1 file changed, 102 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index a6366f147b79..0d4a2dae6912 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -284,6 +284,12 @@ enum pwrap_regs {
>  	PWRAP_DVFS_WDATA7,
>  	PWRAP_SPMINF_STA,
>  	PWRAP_CIPHER_EN,
> +
> +	/* MT6797 series regs */
> +	PWRAP_INT1_EN,
> +	PWRAP_INT1_FLG_RAW,
> +	PWRAP_INT1_FLG,
> +	PWRAP_INT1_CLR,
>  };
>  
>  static int mt2701_regs[] = {
> @@ -372,6 +378,43 @@ static int mt2701_regs[] = {
>  	[PWRAP_ADC_RDATA_ADDR2] =	0x154,
>  };
>  
> +static int mt6797_regs[] = {
> +	[PWRAP_MUX_SEL] =		0x0,
> +	[PWRAP_WRAP_EN] =		0x4,
> +	[PWRAP_DIO_EN] =		0x8,
> +	[PWRAP_SIDLY] =			0xC,
> +	[PWRAP_RDDMY] =			0x10,
> +	[PWRAP_CSHEXT_WRITE] =		0x18,
> +	[PWRAP_CSHEXT_READ] =		0x1C,
> +	[PWRAP_CSLEXT_START] =		0x20,
> +	[PWRAP_CSLEXT_END] =		0x24,
> +	[PWRAP_STAUPD_PRD] =		0x28,
> +	[PWRAP_HARB_HPRIO] =		0x50,
> +	[PWRAP_HIPRIO_ARB_EN] =		0x54,
> +	[PWRAP_MAN_EN] =		0x60,
> +	[PWRAP_MAN_CMD] =		0x64,
> +	[PWRAP_WACS0_EN] =		0x70,
> +	[PWRAP_WACS1_EN] =		0x84,
> +	[PWRAP_WACS2_EN] =		0x98,
> +	[PWRAP_INIT_DONE2] =		0x9C,
> +	[PWRAP_WACS2_CMD] =		0xA0,
> +	[PWRAP_WACS2_RDATA] =		0xA4,
> +	[PWRAP_WACS2_VLDCLR] =		0xA8,
> +	[PWRAP_INT_EN] =		0xC0,
> +	[PWRAP_INT_FLG_RAW] =		0xC4,
> +	[PWRAP_INT_FLG] =		0xC8,
> +	[PWRAP_INT_CLR] =		0xCC,
> +	[PWRAP_INT1_EN] =		0xD0,
> +	[PWRAP_INT1_FLG_RAW] =		0xD4,
> +	[PWRAP_INT1_FLG] =		0xD8,
> +	[PWRAP_INT1_CLR] =		0xDC,
> +	[PWRAP_TIMER_EN] =		0xF4,
> +	[PWRAP_WDT_UNIT] =		0xFC,
> +	[PWRAP_WDT_SRC_EN] =		0x100,
> +	[PWRAP_DCM_EN] =		0x1CC,
> +	[PWRAP_DCM_DBC_PRD] =		0x1D4,
> +};
> +

trim unused registers if any

>  static int mt7622_regs[] = {
>  	[PWRAP_MUX_SEL] =		0x0,
>  	[PWRAP_WRAP_EN] =		0x4,
> @@ -647,6 +690,7 @@ enum pmic_type {
>  
>  enum pwrap_type {
>  	PWRAP_MT2701,
> +	PWRAP_MT6797,
>  	PWRAP_MT7622,
>  	PWRAP_MT8135,
>  	PWRAP_MT8173,
> @@ -1006,6 +1050,12 @@ static void pwrap_init_chip_select_ext(struct pmic_wrapper *wrp, u8 hext_write,
>  static int pwrap_common_init_reg_clock(struct pmic_wrapper *wrp)
>  {
>  	switch (wrp->master->type) {
> +	case PWRAP_MT6797:
> +		pwrap_writel(wrp, 0x8, PWRAP_RDDMY);
> +		pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_RDDMY_NO],
> +			    0x8);
> +		pwrap_init_chip_select_ext(wrp, 0x88, 0x55, 3, 0);
> +		break;

the setup for timing is much similar to mt2701 + mt6323

so we can merge the both logic into one, and then hope to eliminate specific pwrap_mt2701_init_reg_clock totally

>  	case PWRAP_MT8173:
>  		pwrap_init_chip_select_ext(wrp, 0, 4, 2, 2);
>  		break;
> @@ -1076,11 +1126,14 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>  		break;
>  	case PWRAP_MT2701:
>  	case PWRAP_MT8173:
> +	case PWRAP_MT6797:

need to be listed in alphabetical order

>  		pwrap_writel(wrp, 1, PWRAP_CIPHER_EN);
>  		break;
>  	case PWRAP_MT7622:
>  		pwrap_writel(wrp, 0, PWRAP_CIPHER_EN);
>  		break;
> +	default:
> +		break;
>  	}
>  
>  	/* Config cipher mode @PMIC */
> @@ -1325,6 +1378,15 @@ static irqreturn_t pwrap_interrupt(int irqno, void *dev_id)
>  
>  	pwrap_writel(wrp, 0xffffffff, PWRAP_INT_CLR);
>  
> +	/* If we support INT1 interrupt, we also need to clear it */
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN)) {
> +		rdata = pwrap_readl(wrp, PWRAP_INT1_FLG);
> +
> +		dev_err(wrp->dev, "unexpected interrupt int1=0x%x\n", rdata);
> +
> +		pwrap_writel(wrp, rdata, PWRAP_INT1_CLR);
> +	}
> +

it seems no required to add PWRAP_CAP_INT1_EN:

the CAP is not to take any action for any INTs raised, instead, it can be disabled at initial.

>  	return IRQ_HANDLED;
>  }
>  
> @@ -1446,6 +1508,19 @@ static const struct pmic_wrapper_type pwrap_mt8173 = {
>  	.init_soc_specific = pwrap_mt8173_init_soc_specific,
>  };
>  
> +static const struct pmic_wrapper_type pwrap_mt6797 = {
> +	.regs = mt6797_regs,
> +	.type = PWRAP_MT6797,
> +	.arb_en_all = 0x01fff,
> +	.int_en_all = 0xffffffc6,
> +	.int1_en_all = 0x0001ffff,
> +	.spi_w = PWRAP_MAN_CMD_SPI_WRITE,
> +	.wdt_src = PWRAP_WDT_SRC_MASK_ALL,
> +	.caps = PWRAP_CAP_DCM | PWRAP_CAP_INT1_EN,
> +	.init_reg_clock = pwrap_common_init_reg_clock,
> +	.init_soc_specific = NULL,
> +};
> +
>  static const struct of_device_id of_pwrap_match_tbl[] = {
>  	{
>  		.compatible = "mediatek,mt2701-pwrap",
> @@ -1460,6 +1535,9 @@ static const struct of_device_id of_pwrap_match_tbl[] = {
>  		.compatible = "mediatek,mt8173-pwrap",
>  		.data = &pwrap_mt8173,
>  	}, {
> +		.compatible = "mediatek,mt6797-pwrap",
> +		.data = &pwrap_mt6797,

each entry needs to be sorted in alphabetical order

> +	}, {
>  		/* sentinel */
>  	}
>  };
> @@ -1503,11 +1581,13 @@ static int pwrap_probe(struct platform_device *pdev)
>  	if (IS_ERR(wrp->base))
>  		return PTR_ERR(wrp->base);
>  
> -	wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
> -	if (IS_ERR(wrp->rstc)) {
> -		ret = PTR_ERR(wrp->rstc);
> -		dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
> -		return ret;
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_RESET)) {
> +		wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");

there should be a reset bit present for pwrap on infrasys.

the specific condition can be dropped when the reset cell is exported from infrasys and then the device has a reference to it.

> +		if (IS_ERR(wrp->rstc)) {
> +			ret = PTR_ERR(wrp->rstc);
> +			dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
> +			return ret;
> +		}
>  	}
>  
>  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
> @@ -1549,9 +1629,17 @@ static int pwrap_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_out1;
>  
> -	/* Enable internal dynamic clock */
> -	pwrap_writel(wrp, 1, PWRAP_DCM_EN);
> -	pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
> +	/*
> +	 * add dcm capability check
> +	 */
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_DCM)) {

the specific condition can be dropped since so far all devices the driver can support are owning PWRAP_CAP_DCM

> +		if (wrp->master->type == PWRAP_MT6797)
> +			pwrap_writel(wrp, 3, PWRAP_DCM_EN);

the setup for MT6797 can be moved into .init_soc_specific callback ?

> +		else
> +			pwrap_writel(wrp, 1, PWRAP_DCM_EN);
> +
> +		pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
> +	}
>  
>  	/*
>  	 * The PMIC could already be initialized by the bootloader.
> @@ -1580,6 +1668,12 @@ static int pwrap_probe(struct platform_device *pdev)
>  	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
>  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
>  	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
> +	/*
> +	 * We add INT1 interrupt to handle starvation and request exception
> +	 * If we support it, we should enable them here.
> +	 */
> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
> +		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>  

if there is no explicitly enabling on INT1, then ISR handling for INT1 is also unnecessary

>  	irq = platform_get_irq(pdev, 0);
>  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,

Re: [PATCH V4 5/8] soc: mediatek: pwrap: add pwrap for mt6797 SoCs

[Sean Wang]

On Thu, 2018-05-03 at 14:20 +0800, Argus Lin wrote:
> On Thu, 2018-05-03 at 12:01 +0800, Sean Wang wrote:
> >  };

[...]

> > > @@ -1503,11 +1581,13 @@ static int pwrap_probe(struct platform_device *pdev)
> > >  	if (IS_ERR(wrp->base))
> > >  		return PTR_ERR(wrp->base);
> > >  
> > > -	wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
> > > -	if (IS_ERR(wrp->rstc)) {
> > > -		ret = PTR_ERR(wrp->rstc);
> > > -		dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
> > > -		return ret;
> > > +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_RESET)) {
> > > +		wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
> > 
> > there should be a reset bit present for pwrap on infrasys.
> > 
> > the specific condition can be dropped when the reset cell is exported from infrasys and then the device has a reference to it.
> hmm, I think it need to keep here.
> because after pwrap initialized, it can't be reset alone.
> It needs to reset PMIC simultaneously, too.

Reset a pair, either a master or its slave, all had been a part of
pwrap_init.

The reset controller provided here is just to reset pwrap device.
And for its slave reset, it should be done by pwrap_reset_spislave. 

So for MT6397, it should be able to fall into the same procedure.

> > 
> > > +		if (IS_ERR(wrp->rstc)) {
> > > +			ret = PTR_ERR(wrp->rstc);
> > > +			dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
> > > +			return ret;
> > > +		}
> > >  	}
> > >  
> > >  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
> > > @@ -1549,9 +1629,17 @@ static int pwrap_probe(struct platform_device *pdev)
> > >  	if (ret)
> > >  		goto err_out1;
> > >  
> > > -	/* Enable internal dynamic clock */
> > > -	pwrap_writel(wrp, 1, PWRAP_DCM_EN);
> > > -	pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
> > > +	/*
> > > +	 * add dcm capability check
> > > +	 */
> > > +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_DCM)) {
> > 
> > the specific condition can be dropped since so far all devices the driver can support are owning PWRAP_CAP_DCM
> We did not support DCM for future chips.
> MT6797 is the last one.
> This why I want to add judgement here.

The series is only for MT6797 pwrap, so it's fine with only adding these
things the SoC actually relies on. 

PWRAP_CAP_DCM should not be added until a new SoC without dcm is really
introduced.

> > 
> > > +		if (wrp->master->type == PWRAP_MT6797)
> > > +			pwrap_writel(wrp, 3, PWRAP_DCM_EN);
> > 
> > the setup for MT6797 can be moved into .init_soc_specific callback ?
> 
> I think put it here is more generally.
> > 
> > > +		else
> > > +			pwrap_writel(wrp, 1, PWRAP_DCM_EN);
> > > +
> > > +		pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
> > > +	}
> > >  
> > >  	/*
> > >  	 * The PMIC could already be initialized by the bootloader.
> > > @@ -1580,6 +1668,12 @@ static int pwrap_probe(struct platform_device *pdev)
> > >  	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
> > >  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
> > >  	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
> > > +	/*
> > > +	 * We add INT1 interrupt to handle starvation and request exception
> > > +	 * If we support it, we should enable them here.
> > > +	 */
> > > +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
> > > +		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
> > >  
> > 
> > if there is no explicitly enabling on INT1, then ISR handling for INT1 is also unnecessary
> 
> It's ok for me.
> > 
> > >  	irq = platform_get_irq(pdev, 0);
> > >  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
> > 
> > 
> > 
> 
> 

Re: [PATCH V4 5/8] soc: mediatek: pwrap: add pwrap for mt6797 SoCs

[Matthias Brugger]

On 05/04/2018 05:04 AM, Sean Wang wrote:
> On Thu, 2018-05-03 at 14:20 +0800, Argus Lin wrote:
>> On Thu, 2018-05-03 at 12:01 +0800, Sean Wang wrote:
>>>  };
> 
> [...]
> 
>>>> @@ -1503,11 +1581,13 @@ static int pwrap_probe(struct platform_device *pdev)
>>>>  	if (IS_ERR(wrp->base))
>>>>  		return PTR_ERR(wrp->base);
>>>>  
>>>> -	wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
>>>> -	if (IS_ERR(wrp->rstc)) {
>>>> -		ret = PTR_ERR(wrp->rstc);
>>>> -		dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
>>>> -		return ret;
>>>> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_RESET)) {
>>>> +		wrp->rstc = devm_reset_control_get(wrp->dev, "pwrap");
>>>
>>> there should be a reset bit present for pwrap on infrasys.
>>>
>>> the specific condition can be dropped when the reset cell is exported from infrasys and then the device has a reference to it.
>> hmm, I think it need to keep here.
>> because after pwrap initialized, it can't be reset alone.
>> It needs to reset PMIC simultaneously, too.
> 
> Reset a pair, either a master or its slave, all had been a part of
> pwrap_init.
> 
> The reset controller provided here is just to reset pwrap device.
> And for its slave reset, it should be done by pwrap_reset_spislave. 
> 
> So for MT6397, it should be able to fall into the same procedure.
> 
>>>
>>>> +		if (IS_ERR(wrp->rstc)) {
>>>> +			ret = PTR_ERR(wrp->rstc);
>>>> +			dev_dbg(wrp->dev, "cannot get pwrap reset: %d\n", ret);
>>>> +			return ret;
>>>> +		}
>>>>  	}
>>>>  
>>>>  	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_BRIDGE)) {
>>>> @@ -1549,9 +1629,17 @@ static int pwrap_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		goto err_out1;
>>>>  
>>>> -	/* Enable internal dynamic clock */
>>>> -	pwrap_writel(wrp, 1, PWRAP_DCM_EN);
>>>> -	pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
>>>> +	/*
>>>> +	 * add dcm capability check
>>>> +	 */
>>>> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_DCM)) {
>>>
>>> the specific condition can be dropped since so far all devices the driver can support are owning PWRAP_CAP_DCM
>> We did not support DCM for future chips.
>> MT6797 is the last one.
>> This why I want to add judgement here.
> 
> The series is only for MT6797 pwrap, so it's fine with only adding these
> things the SoC actually relies on. 
> 
> PWRAP_CAP_DCM should not be added until a new SoC without dcm is really
> introduced.
> 

I agree (and I think I said this already in a previous review).

Regards,
Matthias

>>>
>>>> +		if (wrp->master->type == PWRAP_MT6797)
>>>> +			pwrap_writel(wrp, 3, PWRAP_DCM_EN);
>>>
>>> the setup for MT6797 can be moved into .init_soc_specific callback ?
>>
>> I think put it here is more generally.
>>>
>>>> +		else
>>>> +			pwrap_writel(wrp, 1, PWRAP_DCM_EN);
>>>> +
>>>> +		pwrap_writel(wrp, 0, PWRAP_DCM_DBC_PRD);
>>>> +	}
>>>>  
>>>>  	/*
>>>>  	 * The PMIC could already be initialized by the bootloader.
>>>> @@ -1580,6 +1668,12 @@ static int pwrap_probe(struct platform_device *pdev)
>>>>  	pwrap_writel(wrp, wrp->master->wdt_src, PWRAP_WDT_SRC_EN);
>>>>  	pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN);
>>>>  	pwrap_writel(wrp, wrp->master->int_en_all, PWRAP_INT_EN);
>>>> +	/*
>>>> +	 * We add INT1 interrupt to handle starvation and request exception
>>>> +	 * If we support it, we should enable them here.
>>>> +	 */
>>>> +	if (HAS_CAP(wrp->master->caps, PWRAP_CAP_INT1_EN))
>>>> +		pwrap_writel(wrp, wrp->master->int1_en_all, PWRAP_INT1_EN);
>>>>  
>>>
>>> if there is no explicitly enabling on INT1, then ISR handling for INT1 is also unnecessary
>>
>> It's ok for me.
>>>
>>>>  	irq = platform_get_irq(pdev, 0);
>>>>  	ret = devm_request_irq(wrp->dev, irq, pwrap_interrupt,
>>>
>>>
>>>
>>
>>
> 
>