[PATCH v2 0/3] pmdomain: mediatek: solve power domain glitch issue

[PATCH v2 0/3] pmdomain: mediatek: solve power domain glitch issue

[yu-chang.lee]

Hi,

This series aims to solve power-off failures and occasional SMI hang issues that
occur during camera stress tests. The issue arises because, when MTCMOS powers on
or off, signal glitches are sometimes produced. This is fairly normal, but the 
software must address it to avoid mistaking the glitch for a transaction signal.

The solutions in these patches can be summarized as follows:

1. Disable the sub-common port after turning off the Larb CG and before turning 
   off the Larb MTCMOS.
2. Use CLAMP to disable/enable the SMI common port.
3. Implement an AXI reset.
For previous discussion on the direction of the code modifications, please refer
to: https://lore.kernel.org/linux-arm-kernel/c476cc48-17ec-4e14-98d8-35bdffb5d296@collabora.com/

Change in v2
 - fix commit title to "pmdomain: mediatek:"
 - add dt-binding definition
 - remove unused function


yu-chang.lee (3):
  pmdomain: mediatek: add smi_larb_reset function when power on
  dt-bindings: power: Add mediatek larb definition
  pmdomain: mediatek: support smi clamp protection

 .../power/mediatek,power-controller.yaml      |   4 +
 drivers/pmdomain/mediatek/mt8188-pm-domains.h |  69 ++++++-
 drivers/pmdomain/mediatek/mtk-pm-domains.c    | 168 ++++++++++++++----
 drivers/pmdomain/mediatek/mtk-pm-domains.h    |  13 ++
 4 files changed, 218 insertions(+), 36 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] pmdomain: mediatek: add smi_larb_reset function when power on

[yu-chang.lee]

This patch avoid mtcmos power glitch from happening by set and clear
smi larb reset.

Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
---
 drivers/pmdomain/mediatek/mt8188-pm-domains.h | 28 +++++++++
 drivers/pmdomain/mediatek/mtk-pm-domains.c    | 59 +++++++++++++++++++
 drivers/pmdomain/mediatek/mtk-pm-domains.h    | 12 ++++
 3 files changed, 99 insertions(+)

diff --git a/drivers/pmdomain/mediatek/mt8188-pm-domains.h b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
index 06834ab6597c..7bbba4d56a77 100644
--- a/drivers/pmdomain/mediatek/mt8188-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
@@ -573,6 +573,18 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.reset_smi = {
+			SMI_RESET_WR(MT8188_SMI_LARB10_RESET,
+				     MT8188_SMI_LARB10_RESET_ADDR),
+			SMI_RESET_WR(MT8188_SMI_LARB11A_RESET,
+				     MT8188_SMI_LARB11A_RESET_ADDR),
+			SMI_RESET_WR(MT8188_SMI_LARB11C_RESET,
+				     MT8188_SMI_LARB11C_RESET_ADDR),
+			SMI_RESET_WR(MT8188_SMI_LARB11B_RESET,
+				     MT8188_SMI_LARB11B_RESET_ADDR),
+			SMI_RESET_WR(MT8188_SMI_LARB15_RESET,
+				     MT8188_SMI_LARB15_RESET_ADDR),
+		},
 		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
 	},
 	[MT8188_POWER_DOMAIN_IPE] = {
@@ -583,6 +595,10 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.reset_smi = {
+			SMI_RESET_WR(MT8188_SMI_LARB12_RESET,
+				     MT8188_SMI_LARB12_RESET_ADDR),
+		},
 		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
 	},
 	[MT8188_POWER_DOMAIN_CAM_VCORE] = {
@@ -660,6 +676,12 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.reset_smi = {
+			SMI_RESET_WR(MT8188_SMI_LARB16A_RESET,
+				     MT8188_SMI_LARB16A_RESET_ADDR),
+			SMI_RESET_WR(MT8188_SMI_LARB17A_RESET,
+				     MT8188_SMI_LARB17A_RESET_ADDR),
+		},
 		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
 	},
 	[MT8188_POWER_DOMAIN_CAM_SUBB] = {
@@ -670,6 +692,12 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.reset_smi = {
+			SMI_RESET_WR(MT8188_SMI_LARB16B_RESET,
+				     MT8188_SMI_LARB16B_RESET_ADDR),
+			SMI_RESET_WR(MT8188_SMI_LARB17B_RESET,
+				     MT8188_SMI_LARB17B_RESET_ADDR),
+		},
 		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
 	},
 };
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index e274e3315fe7..9ab6fa105c8c 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -48,6 +48,8 @@ struct scpsys_domain {
 	struct regmap *infracfg_nao;
 	struct regmap *infracfg;
 	struct regmap *smi;
+	struct regmap **larb;
+	int num_larb;
 	struct regulator *supply;
 };
 
@@ -230,6 +232,39 @@ static int scpsys_regulator_disable(struct regulator *supply)
 	return supply ? regulator_disable(supply) : 0;
 }
 
+static int _scpsys_smi_larb_reset(const struct smi_reset_data bpd,
+				  struct regmap *regmap)
+{
+	int ret;
+	u32 mask = bpd.smi_reset_mask;
+
+	if (!mask)
+		return 0;
+
+	ret = regmap_set_bits(regmap, bpd.smi_reset_addr, mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(regmap, bpd.smi_reset_addr, mask);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int scpsys_smi_larb_reset(struct scpsys_domain *pd)
+{
+	int ret, i;
+
+	for (i = 0; i < pd->num_larb; i++) {
+		ret = _scpsys_smi_larb_reset(pd->data->reset_smi[i], pd->larb[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int scpsys_power_on(struct generic_pm_domain *genpd)
 {
 	struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
@@ -279,6 +314,10 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 	if (ret < 0)
 		goto err_disable_subsys_clks;
 
+	ret = scpsys_smi_larb_reset(pd);
+	if (ret < 0)
+		goto err_disable_subsys_clks;
+
 	ret = scpsys_bus_protect_disable(pd);
 	if (ret < 0)
 		goto err_disable_sram;
@@ -355,6 +394,7 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	struct scpsys_domain *pd;
 	struct device_node *root_node = scpsys->dev->of_node;
 	struct device_node *smi_node;
+	struct device_node *larb_node;
 	struct property *prop;
 	const char *clk_name;
 	int i, ret, num_clks;
@@ -418,6 +458,25 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 			return ERR_CAST(pd->smi);
 	}
 
+	pd->num_larb = of_count_phandle_with_args(node, "mediatek,larb", NULL);
+	if (pd->num_larb > 0) {
+		pd->larb = devm_kcalloc(scpsys->dev, pd->num_larb, sizeof(*pd->larb), GFP_KERNEL);
+		if (!pd->larb)
+			return ERR_PTR(-ENOMEM);
+
+		for (i = 0; i < pd->num_larb; i++) {
+			larb_node = of_parse_phandle(node, "mediatek,larb", i);
+			if (!larb_node)
+				return ERR_PTR(-EINVAL);
+
+			pd->larb[i] = device_node_to_regmap(larb_node);
+			if (IS_ERR(pd->larb[i]))
+				return ERR_CAST(pd->larb[i]);
+		}
+	} else {
+		pd->num_larb = 0;
+	}
+
 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_HAS_INFRA_NAO)) {
 		pd->infracfg_nao = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg-nao");
 		if (IS_ERR(pd->infracfg_nao))
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index aaba5e6b0536..31c2a1bb500f 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -43,6 +43,7 @@
 #define PWR_STATUS_USB			BIT(25)
 
 #define SPM_MAX_BUS_PROT_DATA		6
+#define SPM_MAX_SMI_RESET_DATA		6
 
 enum scpsys_bus_prot_flags {
 	BUS_PROT_REG_UPDATE = BIT(1),
@@ -79,6 +80,16 @@ enum scpsys_bus_prot_flags {
 				INFRA_TOPAXI_PROTECTEN,		\
 				INFRA_TOPAXI_PROTECTSTA1)
 
+#define SMI_RESET_WR(_mask, _addr) {		\
+		.smi_reset_mask = (_mask),	\
+		.smi_reset_addr = _addr,	\
+	}
+
+struct smi_reset_data {
+	u32 smi_reset_mask;
+	u32 smi_reset_addr;
+};
+
 struct scpsys_bus_prot_data {
 	u32 bus_prot_set_clr_mask;
 	u32 bus_prot_set;
@@ -110,6 +121,7 @@ struct scpsys_domain_data {
 	u32 ext_buck_iso_mask;
 	u16 caps;
 	const struct scpsys_bus_prot_data bp_cfg[SPM_MAX_BUS_PROT_DATA];
+	const struct smi_reset_data reset_smi[SPM_MAX_SMI_RESET_DATA];
 	int pwr_sta_offs;
 	int pwr_sta2nd_offs;
 };
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[yu-chang.lee]

Add Smart Multimedia Interface Local Arbiter to mediatek
power domain.

Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
---
 .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
index 8985e2df8a56..228c0dec5253 100644
--- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
@@ -125,6 +125,10 @@ $defs:
         $ref: /schemas/types.yaml#/definitions/phandle
         description: phandle to the device containing the SMI register range.
 
+     mediatek,larb:
+        $ref: /schemas/types.yaml#/definitions/phandle
+        description: phandle to the device containing the LARB register range.
+
     required:
       - reg
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 27/03/2024 06:57, yu-chang.lee wrote:
> Add Smart Multimedia Interface Local Arbiter to mediatek
> power domain.
> 
> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> ---
>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> index 8985e2df8a56..228c0dec5253 100644
> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
> @@ -125,6 +125,10 @@ $defs:
>          $ref: /schemas/types.yaml#/definitions/phandle
>          description: phandle to the device containing the SMI register range.
>  
> +     mediatek,larb:
> +        $ref: /schemas/types.yaml#/definitions/phandle
> +        description: phandle to the device containing the LARB register range.

Why do you need it?

Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
third one similar.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
> On 27/03/2024 06:57, yu-chang.lee wrote:
>> Add Smart Multimedia Interface Local Arbiter to mediatek
>> power domain.
>>
>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
>> ---
>>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> index 8985e2df8a56..228c0dec5253 100644
>> --- a/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
>> @@ -125,6 +125,10 @@ $defs:
>>          $ref: /schemas/types.yaml#/definitions/phandle
>>          description: phandle to the device containing the SMI register range.
>>  
>> +     mediatek,larb:
>> +        $ref: /schemas/types.yaml#/definitions/phandle
>> +        description: phandle to the device containing the LARB register range.
> 
> Why do you need it?
> 
> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
> third one similar.

... and not even tested!

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Yu-chang Lee (李禹璋)]

On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
> > On 27/03/2024 06:57, yu-chang.lee wrote:
> >> Add Smart Multimedia Interface Local Arbiter to mediatek
> >> power domain.
> >>
> >> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> >> ---
> >>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4
> ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git
> a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> >> index 8985e2df8a56..228c0dec5253 100644
> >> --- a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> >> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> >> @@ -125,6 +125,10 @@ $defs:
> >>          $ref: /schemas/types.yaml#/definitions/phandle
> >>          description: phandle to the device containing the SMI
> register range.
> >>  
> >> +     mediatek,larb:
> >> +        $ref: /schemas/types.yaml#/definitions/phandle
> >> +        description: phandle to the device containing the LARB
> register range.
> > 
> > Why do you need it?
> > 
> > Plus I also see mediatek,larbs and mediatek,larb-id... so now we
> have
> > third one similar.
> 
> ... and not even tested!
> 
> Best regards,
> Krzysztof
> 
Hi,

I will double check the format of yaml for the next version, sorry for
inconvenience. But I did test it on mt8188 chromebook, the reason why
power domain need larb node is that when mtcmos power on, signal glitch
may produce. Power domain driver must reset larb when this happen to 
prevent dummy transaction on bus. That why I need larb node in dts.

Best Regards,
Yu-chang
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 27/03/2024 10:34, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
>>> On 27/03/2024 06:57, yu-chang.lee wrote:
>>>> Add Smart Multimedia Interface Local Arbiter to mediatek
>>>> power domain.
>>>>
>>>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
>>>> ---
>>>>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4
>> ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git
>> a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>> b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>>> index 8985e2df8a56..228c0dec5253 100644
>>>> --- a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>>> @@ -125,6 +125,10 @@ $defs:
>>>>          $ref: /schemas/types.yaml#/definitions/phandle
>>>>          description: phandle to the device containing the SMI
>> register range.
>>>>  
>>>> +     mediatek,larb:
>>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>>> +        description: phandle to the device containing the LARB
>> register range.
>>>
>>> Why do you need it?
>>>
>>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we
>> have
>>> third one similar.
>>
>> ... and not even tested!
>>
>> Best regards,
>> Krzysztof
>>
> Hi,
> 
> I will double check the format of yaml for the next version, sorry for
> inconvenience. But I did test it on mt8188 chromebook, the reason why

How do you test a binding on chromebook?

> power domain need larb node is that when mtcmos power on, signal glitch
> may produce. Power domain driver must reset larb when this happen to 
> prevent dummy transaction on bus. That why I need larb node in dts.

No one talks here about larb node...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Yu-chang Lee (李禹璋)]

On Wed, 2024-03-27 at 10:59 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 27/03/2024 10:34, Yu-chang Lee (李禹璋) wrote:
> > On Wed, 2024-03-27 at 10:23 +0100, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 27/03/2024 09:39, Krzysztof Kozlowski wrote:
> >>> On 27/03/2024 06:57, yu-chang.lee wrote:
> >>>> Add Smart Multimedia Interface Local Arbiter to mediatek
> >>>> power domain.
> >>>>
> >>>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> >>>> ---
> >>>>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 
> 4
> >> ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git
> >> a/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >> b/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >>>> index 8985e2df8a56..228c0dec5253 100644
> >>>> --- a/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
> >> controller.yaml
> >>>> @@ -125,6 +125,10 @@ $defs:
> >>>>          $ref: /schemas/types.yaml#/definitions/phandle
> >>>>          description: phandle to the device containing the SMI
> >> register range.
> >>>>  
> >>>> +     mediatek,larb:
> >>>> +        $ref: /schemas/types.yaml#/definitions/phandle
> >>>> +        description: phandle to the device containing the LARB
> >> register range.
> >>>
> >>> Why do you need it?
> >>>
> >>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we
> >> have
> >>> third one similar.
> >>
> >> ... and not even tested!
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > Hi,
> > 
> > I will double check the format of yaml for the next version, sorry
> for
> > inconvenience. But I did test it on mt8188 chromebook, the reason
> why
> 
> How do you test a binding on chromebook?
> 
> > power domain need larb node is that when mtcmos power on, signal
> glitch
> > may produce. Power domain driver must reset larb when this happen
> to 
> > prevent dummy transaction on bus. That why I need larb node in dts.
> 
> No one talks here about larb node...

Sorry, May you elaborate on what information I need to provide to you
or it is just a syntax problem I need to fix?

Thanks
Best Regards,

Yu-chang
> 
> Best regards,
> Krzysztof
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
>>>>
>>> Hi,
>>>
>>> I will double check the format of yaml for the next version, sorry
>> for
>>> inconvenience. But I did test it on mt8188 chromebook, the reason
>> why
>>
>> How do you test a binding on chromebook?
>>
>>> power domain need larb node is that when mtcmos power on, signal
>> glitch
>>> may produce. Power domain driver must reset larb when this happen
>> to 
>>> prevent dummy transaction on bus. That why I need larb node in dts.
>>
>> No one talks here about larb node...
> 
> Sorry, May you elaborate on what information I need to provide to you
> or it is just a syntax problem I need to fix?

Please explain the purpose of this property (how is it going to be used
by drivers) and what does it represent.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Yu-chang Lee (李禹璋)]

On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> >>>>
> >>> Hi,
> >>>
> >>> I will double check the format of yaml for the next version,
> sorry
> >> for
> >>> inconvenience. But I did test it on mt8188 chromebook, the reason
> >> why
> >>
> >> How do you test a binding on chromebook?
> >>
> >>> power domain need larb node is that when mtcmos power on, signal
> >> glitch
> >>> may produce. Power domain driver must reset larb when this happen
> >> to 
> >>> prevent dummy transaction on bus. That why I need larb node in
> dts.
> >>
> >> No one talks here about larb node...
> > 
> > Sorry, May you elaborate on what information I need to provide to
> you
> > or it is just a syntax problem I need to fix?
> 
> Please explain the purpose of this property (how is it going to be
> used by drivers)and what does it represent.
> 

It represent SMI LARB(Local ARBitration). In power domain driver when
power on power domain, It need to reset LARB to prevent potential power
glitch which may cause dummy transaction on bus. Without taking care of
this issue it often leads to camera hang in stress test.

Best Regards,
Yu-chang
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
>>>>>>
>>>>> Hi,
>>>>>
>>>>> I will double check the format of yaml for the next version,
>> sorry
>>>> for
>>>>> inconvenience. But I did test it on mt8188 chromebook, the reason
>>>> why
>>>>
>>>> How do you test a binding on chromebook?
>>>>
>>>>> power domain need larb node is that when mtcmos power on, signal
>>>> glitch
>>>>> may produce. Power domain driver must reset larb when this happen
>>>> to 
>>>>> prevent dummy transaction on bus. That why I need larb node in
>> dts.
>>>>
>>>> No one talks here about larb node...
>>>
>>> Sorry, May you elaborate on what information I need to provide to
>> you
>>> or it is just a syntax problem I need to fix?
>>
>> Please explain the purpose of this property (how is it going to be
>> used by drivers)and what does it represent.
>>
> 
> It represent SMI LARB(Local ARBitration). In power domain driver when
> power on power domain, It need to reset LARB to prevent potential power
> glitch which may cause dummy transaction on bus. Without taking care of
> this issue it often leads to camera hang in stress test.

That sounds rather like missing resets... or something else connecting
these devices. Maybe the explanation is just imprecise...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Alexandre Mergnat]

Hello Yu-chang Lee,

SMI LARB must have a power domain, according to "mediatek,smi-larb.yaml"
Now you try to create a link from power domain to larb.

Here is my understanding: when you enable/disable power domain, the
larb linked to this power domain may have an issue. Then you want to
retrieve de LARB linked to the power domain though the dts to manage
the LARB. IMHO, using the dts to have this information into the power
driver isn't necessary and may introduce some bugs if the LARB node
and power node in the DTS aren't aligned.

It seems not implemented today but during the LARB probe, it should
"subscribe" to the linked power domain. Then, when the power domain
status is changing, it is able to "notify" (callback) the LARB, then
implement the good stuff to handle this power domain status change
into LARB driver.

Regards,
Alexandre

On Wed, Mar 27, 2024 at 12:04 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
> >>
> >> External email : Please do not click links or open attachments until
> >> you have verified the sender or the content.
> >>  On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> >>>>>>
> >>>>> Hi,
> >>>>>
> >>>>> I will double check the format of yaml for the next version,
> >> sorry
> >>>> for
> >>>>> inconvenience. But I did test it on mt8188 chromebook, the reason
> >>>> why
> >>>>
> >>>> How do you test a binding on chromebook?
> >>>>
> >>>>> power domain need larb node is that when mtcmos power on, signal
> >>>> glitch
> >>>>> may produce. Power domain driver must reset larb when this happen
> >>>> to
> >>>>> prevent dummy transaction on bus. That why I need larb node in
> >> dts.
> >>>>
> >>>> No one talks here about larb node...
> >>>
> >>> Sorry, May you elaborate on what information I need to provide to
> >> you
> >>> or it is just a syntax problem I need to fix?
> >>
> >> Please explain the purpose of this property (how is it going to be
> >> used by drivers)and what does it represent.
> >>
> >
> > It represent SMI LARB(Local ARBitration). In power domain driver when
> > power on power domain, It need to reset LARB to prevent potential power
> > glitch which may cause dummy transaction on bus. Without taking care of
> > this issue it often leads to camera hang in stress test.
>
> That sounds rather like missing resets... or something else connecting
> these devices. Maybe the explanation is just imprecise...
>
> Best regards,
> Krzysztof
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Yu-chang Lee (李禹璋)]

On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hello Yu-chang Lee,
> 
> SMI LARB must have a power domain, according to "mediatek,smi-
> larb.yaml"
> Now you try to create a link from power domain to larb.
> 
> Here is my understanding: when you enable/disable power domain, the
> larb linked to this power domain may have an issue. Then you want to
> retrieve de LARB linked to the power domain though the dts to manage
> the LARB. 

Yes, this is what I am trying to do.

> IMHO, using the dts to have this information into the power
> driver isn't necessary and may introduce some bugs if the LARB node
> and power node in the DTS aren't aligned.
> 
> It seems not implemented today but during the LARB probe, it should
> "subscribe" to the linked power domain. Then, when the power domain
> status is changing, it is able to "notify" (callback) the LARB, then
> implement the good stuff to handle this power domain status change
> into LARB driver.
> 

The problem with this method and why "smi clamp" is in power domain
driver is that our HW designer gave us a programming guide strictly
states the sequence of what we need to do to power on/off power domain.
Using callback, this sequence is no longer guaranteed and the side
effect is unknown... 

So I would like to stick with adding larbs just like smi into power
domain node.

> Regards,
> Alexandre

Best Regards,
Yu-chang
> 
> On Wed, Mar 27, 2024 at 12:04 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> > > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
> > >>
> > >> External email : Please do not click links or open attachments
> until
> > >> you have verified the sender or the content.
> > >>  On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> > >>>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> I will double check the format of yaml for the next version,
> > >> sorry
> > >>>> for
> > >>>>> inconvenience. But I did test it on mt8188 chromebook, the
> reason
> > >>>> why
> > >>>>
> > >>>> How do you test a binding on chromebook?
> > >>>>
> > >>>>> power domain need larb node is that when mtcmos power on,
> signal
> > >>>> glitch
> > >>>>> may produce. Power domain driver must reset larb when this
> happen
> > >>>> to
> > >>>>> prevent dummy transaction on bus. That why I need larb node
> in
> > >> dts.
> > >>>>
> > >>>> No one talks here about larb node...
> > >>>
> > >>> Sorry, May you elaborate on what information I need to provide
> to
> > >> you
> > >>> or it is just a syntax problem I need to fix?
> > >>
> > >> Please explain the purpose of this property (how is it going to
> be
> > >> used by drivers)and what does it represent.
> > >>
> > >
> > > It represent SMI LARB(Local ARBitration). In power domain driver
> when
> > > power on power domain, It need to reset LARB to prevent potential
> power
> > > glitch which may cause dummy transaction on bus. Without taking
> care of
> > > this issue it often leads to camera hang in stress test.
> >
> > That sounds rather like missing resets... or something else
> connecting
> > these devices. Maybe the explanation is just imprecise...
> >
> > Best regards,
> > Krzysztof
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 28/03/2024 07:06, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  Hello Yu-chang Lee,
>>
>> SMI LARB must have a power domain, according to "mediatek,smi-
>> larb.yaml"
>> Now you try to create a link from power domain to larb.
>>
>> Here is my understanding: when you enable/disable power domain, the
>> larb linked to this power domain may have an issue. Then you want to
>> retrieve de LARB linked to the power domain though the dts to manage
>> the LARB. 
> 
> Yes, this is what I am trying to do.
> 
>> IMHO, using the dts to have this information into the power
>> driver isn't necessary and may introduce some bugs if the LARB node
>> and power node in the DTS aren't aligned.
>>
>> It seems not implemented today but during the LARB probe, it should
>> "subscribe" to the linked power domain. Then, when the power domain
>> status is changing, it is able to "notify" (callback) the LARB, then
>> implement the good stuff to handle this power domain status change
>> into LARB driver.
>>
> 
> The problem with this method and why "smi clamp" is in power domain
> driver is that our HW designer gave us a programming guide strictly
> states the sequence of what we need to do to power on/off power domain.
> Using callback, this sequence is no longer guaranteed and the side
> effect is unknown... 
> 
> So I would like to stick with adding larbs just like smi into powe

You want your power domain driver to poke, asynchronously, without
locking into registers of another device. And how does this not cause
issues?

No, work with your hardware engineers or Linux engineers on sane behavior.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Ulf Hansson]

On Thu, 28 Mar 2024 at 07:06, Yu-chang Lee (李禹璋)
<Yu-chang.Lee@mediatek.com> wrote:
>
> On Wed, 2024-03-27 at 12:55 +0100, Alexandre Mergnat wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  Hello Yu-chang Lee,
> >
> > SMI LARB must have a power domain, according to "mediatek,smi-
> > larb.yaml"
> > Now you try to create a link from power domain to larb.
> >
> > Here is my understanding: when you enable/disable power domain, the
> > larb linked to this power domain may have an issue. Then you want to
> > retrieve de LARB linked to the power domain though the dts to manage
> > the LARB.
>
> Yes, this is what I am trying to do.
>
> > IMHO, using the dts to have this information into the power
> > driver isn't necessary and may introduce some bugs if the LARB node
> > and power node in the DTS aren't aligned.
> >
> > It seems not implemented today but during the LARB probe, it should
> > "subscribe" to the linked power domain. Then, when the power domain
> > status is changing, it is able to "notify" (callback) the LARB, then
> > implement the good stuff to handle this power domain status change
> > into LARB driver.
> >
>
> The problem with this method and why "smi clamp" is in power domain
> driver is that our HW designer gave us a programming guide strictly
> states the sequence of what we need to do to power on/off power domain.
> Using callback, this sequence is no longer guaranteed and the side
> effect is unknown...

In most cases, using the runtime PM callbacks in the consumer driver
(LARB driver) is sufficient to deal with resets. For some special
cases drivers are making use of the genpd on/off notifiers
(GENPD_NOTIFY_*), as they really need to know when their devices have
been power collapsed. Have you tried both these options?

[...]

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Yu-chang Lee (李禹璋)]

On Wed, 2024-03-27 at 12:04 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 27/03/2024 11:56, Yu-chang Lee (李禹璋) wrote:
> > On Wed, 2024-03-27 at 11:43 +0100, Krzysztof Kozlowski wrote:
> >>   
> >> External email : Please do not click links or open attachments
> until
> >> you have verified the sender or the content.
> >>  On 27/03/2024 11:39, Yu-chang Lee (李禹璋) wrote:
> >>>>>>
> >>>>> Hi,
> >>>>>
> >>>>> I will double check the format of yaml for the next version,
> >> sorry
> >>>> for
> >>>>> inconvenience. But I did test it on mt8188 chromebook, the
> reason
> >>>> why
> >>>>
> >>>> How do you test a binding on chromebook?
> >>>>
> >>>>> power domain need larb node is that when mtcmos power on,
> signal
> >>>> glitch
> >>>>> may produce. Power domain driver must reset larb when this
> happen
> >>>> to 
> >>>>> prevent dummy transaction on bus. That why I need larb node in
> >> dts.
> >>>>
> >>>> No one talks here about larb node...
> >>>
> >>> Sorry, May you elaborate on what information I need to provide to
> >> you
> >>> or it is just a syntax problem I need to fix?
> >>
> >> Please explain the purpose of this property (how is it going to be
> >> used by drivers)and what does it represent.
> >>
> > 
> > It represent SMI LARB(Local ARBitration). In power domain driver
> when
> > power on power domain, It need to reset LARB to prevent potential
> power
> > glitch which may cause dummy transaction on bus. Without taking
> care of
> > this issue it often leads to camera hang in stress test.
> 
> That sounds rather like missing resets... or something else
> connecting
> these devices. Maybe the explanation is just imprecise...
> 

Maybe the term "reset" is misleading here. What power domain driver
trying to do is "set and then clr" the smi larb to clear potential
glitch signal that is already in there. And this step is strongly
related to power domain onf that why I want to add it in to power
domain node without depending larb driver to do the work to prevent
this setting missing.

> Best regards,
> Krzysztof

Best Regards,
yu-chang
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Yu-chang Lee (李禹璋)]

On Wed, 2024-03-27 at 09:39 +0100, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 27/03/2024 06:57, yu-chang.lee wrote:
> > Add Smart Multimedia Interface Local Arbiter to mediatek
> > power domain.
> > 
> > Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> > ---
> >  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4
> ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git
> a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> > index 8985e2df8a56..228c0dec5253 100644
> > --- a/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> > +++ b/Documentation/devicetree/bindings/power/mediatek,power-
> controller.yaml
> > @@ -125,6 +125,10 @@ $defs:
> >          $ref: /schemas/types.yaml#/definitions/phandle
> >          description: phandle to the device containing the SMI
> register range.
> >  
> > +     mediatek,larb:
> > +        $ref: /schemas/types.yaml#/definitions/phandle
> > +        description: phandle to the device containing the LARB
> register range.
> 
> Why do you need it?
> 
> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
> third one similar.
> 
MM driver used "mediatek,larbs" for it larb node.
Power domain driver used "mediatek,larb".
"mediatek,larb-id" is for larb in dts.

The naming is no related to each other.

Best Regards,
Yu-chang.

> Best regards,
> Krzysztof
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Krzysztof Kozlowski]

On 27/03/2024 11:01, Yu-chang Lee (李禹璋) wrote:
> On Wed, 2024-03-27 at 09:39 +0100, Krzysztof Kozlowski wrote:
>>  	 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>  On 27/03/2024 06:57, yu-chang.lee wrote:
>>> Add Smart Multimedia Interface Local Arbiter to mediatek
>>> power domain.
>>>
>>> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
>>> ---
>>>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4
>> ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git
>> a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>> b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>> index 8985e2df8a56..228c0dec5253 100644
>>> --- a/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>> +++ b/Documentation/devicetree/bindings/power/mediatek,power-
>> controller.yaml
>>> @@ -125,6 +125,10 @@ $defs:
>>>          $ref: /schemas/types.yaml#/definitions/phandle
>>>          description: phandle to the device containing the SMI
>> register range.
>>>  
>>> +     mediatek,larb:
>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>> +        description: phandle to the device containing the LARB
>> register range.
>>
>> Why do you need it?
>>
>> Plus I also see mediatek,larbs and mediatek,larb-id... so now we have
>> third one similar.
>>
> MM driver used "mediatek,larbs" for it larb node.
> Power domain driver used "mediatek,larb".
> "mediatek,larb-id" is for larb in dts.
> 
> The naming is no related to each other.

Then it is just confusing.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[Rob Herring]

On Wed, 27 Mar 2024 13:57:31 +0800, yu-chang.lee wrote:
> Add Smart Multimedia Interface Local Arbiter to mediatek
> power domain.
> 
> Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
> ---
>  .../devicetree/bindings/power/mediatek,power-controller.yaml  | 4 ++++
>  1 file changed, 4 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
./Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/power/mediatek,power-controller.example.dts'
Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/power/mediatek,power-controller.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml:
while parsing a block mapping
  in "<unicode string>", line 64, column 5
did not find expected key
  in "<unicode string>", line 128, column 6
./Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240327055732.28198-3-yu-chang.lee@mediatek.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition

[kernel test robot]

Hi yu-chang.lee,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on krzk-dt/for-next linus/master v6.9-rc1 next-20240328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/yu-chang-lee/pmdomain-mediatek-add-smi_larb_reset-function-when-power-on/20240327-140007
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240327055732.28198-3-yu-chang.lee%40mediatek.com
patch subject: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240331/202403312222.fjYPC06h-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403312222.fjYPC06h-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation)
--
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
>> Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml:
   while parsing a block mapping
     in "<unicode string>", line 64, column 5
   did not find expected key
     in "<unicode string>", line 128, column 6
--
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file

vim +128 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml

     8	
     9	maintainers:
    10	  - MandyJH Liu <mandyjh.liu@mediatek.com>
    11	  - Matthias Brugger <mbrugger@suse.com>
    12	
    13	description: |
    14	  Mediatek processors include support for multiple power domains which can be
    15	  powered up/down by software based on different application scenes to save power.
    16	
    17	  IP cores belonging to a power domain should contain a 'power-domains'
    18	  property that is a phandle for SCPSYS node representing the domain.
    19	
    20	properties:
    21	  $nodename:
    22	    pattern: '^power-controller(@[0-9a-f]+)?$'
    23	
    24	  compatible:
    25	    enum:
    26	      - mediatek,mt6795-power-controller
    27	      - mediatek,mt8167-power-controller
    28	      - mediatek,mt8173-power-controller
    29	      - mediatek,mt8183-power-controller
    30	      - mediatek,mt8186-power-controller
    31	      - mediatek,mt8188-power-controller
    32	      - mediatek,mt8192-power-controller
    33	      - mediatek,mt8195-power-controller
    34	      - mediatek,mt8365-power-controller
    35	
    36	  '#power-domain-cells':
    37	    const: 1
    38	
    39	  '#address-cells':
    40	    const: 1
    41	
    42	  '#size-cells':
    43	    const: 0
    44	
    45	patternProperties:
    46	  "^power-domain@[0-9a-f]+$":
    47	    $ref: "#/$defs/power-domain-node"
    48	    patternProperties:
    49	      "^power-domain@[0-9a-f]+$":
    50	        $ref: "#/$defs/power-domain-node"
    51	        patternProperties:
    52	          "^power-domain@[0-9a-f]+$":
    53	            $ref: "#/$defs/power-domain-node"
    54	            patternProperties:
    55	              "^power-domain@[0-9a-f]+$":
    56	                $ref: "#/$defs/power-domain-node"
    57	                unevaluatedProperties: false
    58	            unevaluatedProperties: false
    59	        unevaluatedProperties: false
    60	    unevaluatedProperties: false
    61	
    62	$defs:
    63	  power-domain-node:
    64	    type: object
    65	    description: |
    66	      Represents the power domains within the power controller node as documented
    67	      in Documentation/devicetree/bindings/power/power-domain.yaml.
    68	
    69	    properties:
    70	
    71	      '#power-domain-cells':
    72	        description:
    73	          Must be 0 for nodes representing a single PM domain and 1 for nodes
    74	          providing multiple PM domains.
    75	
    76	      '#address-cells':
    77	        const: 1
    78	
    79	      '#size-cells':
    80	        const: 0
    81	
    82	      reg:
    83	        description: |
    84	          Power domain index. Valid values are defined in:
    85	              "include/dt-bindings/power/mt6795-power.h" - for MT8167 type power domain.
    86	              "include/dt-bindings/power/mt8167-power.h" - for MT8167 type power domain.
    87	              "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
    88	              "include/dt-bindings/power/mt8183-power.h" - for MT8183 type power domain.
    89	              "include/dt-bindings/power/mediatek,mt8188-power.h" - for MT8188 type power domain.
    90	              "include/dt-bindings/power/mt8192-power.h" - for MT8192 type power domain.
    91	              "include/dt-bindings/power/mt8195-power.h" - for MT8195 type power domain.
    92	              "include/dt-bindings/power/mediatek,mt8365-power.h" - for MT8365 type power domain.
    93	        maxItems: 1
    94	
    95	      clocks:
    96	        description: |
    97	          A number of phandles to clocks that need to be enabled during domain
    98	          power-up sequencing.
    99	
   100	      clock-names:
   101	        description: |
   102	          List of names of clocks, in order to match the power-up sequencing
   103	          for each power domain we need to group the clocks by name. BASIC
   104	          clocks need to be enabled before enabling the corresponding power
   105	          domain, and should not have a '-' in their name (i.e mm, mfg, venc).
   106	          SUSBYS clocks need to be enabled before releasing the bus protection,
   107	          and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
   108	
   109	          In order to follow properly the power-up sequencing, the clocks must
   110	          be specified by order, adding first the BASIC clocks followed by the
   111	          SUSBSYS clocks.
   112	
   113	      domain-supply:
   114	        description: domain regulator supply.
   115	
   116	      mediatek,infracfg:
   117	        $ref: /schemas/types.yaml#/definitions/phandle
   118	        description: phandle to the device containing the INFRACFG register range.
   119	
   120	      mediatek,infracfg-nao:
   121	        $ref: /schemas/types.yaml#/definitions/phandle
   122	        description: phandle to the device containing the INFRACFG-NAO register range.
   123	
   124	      mediatek,smi:
   125	        $ref: /schemas/types.yaml#/definitions/phandle
   126	        description: phandle to the device containing the SMI register range.
   127	
 > 128	     mediatek,larb:
 > 129	        $ref: /schemas/types.yaml#/definitions/phandle
   130	        description: phandle to the device containing the LARB register range.
   131	
   132	    required:
   133	      - reg
   134	
   135	required:
   136	  - compatible
   137	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] pmdomain: mediatek: support smi clamp protection

[yu-chang.lee]

In order to avoid power glitch, this patch use smi clamp
to disable/enable smi common port.

Signed-off-by: yu-chang.lee <yu-chang.lee@mediatek.com>
---
 drivers/pmdomain/mediatek/mt8188-pm-domains.h |  41 ++++++-
 drivers/pmdomain/mediatek/mtk-pm-domains.c    | 109 +++++++++++++-----
 drivers/pmdomain/mediatek/mtk-pm-domains.h    |   1 +
 3 files changed, 115 insertions(+), 36 deletions(-)

diff --git a/drivers/pmdomain/mediatek/mt8188-pm-domains.h b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
index 7bbba4d56a77..39f057dca92c 100644
--- a/drivers/pmdomain/mediatek/mt8188-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mt8188-pm-domains.h
@@ -573,6 +573,18 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.bp_cfg = {
+			BUS_PROT_WR(SMI,
+				    MT8188_SMI_COMMON_SMI_CLAMP_DIP_TO_VDO0,
+				    MT8188_SMI_COMMON_CLAMP_EN_SET,
+				    MT8188_SMI_COMMON_CLAMP_EN_CLR,
+				    MT8188_SMI_COMMON_CLAMP_EN_STA),
+			BUS_PROT_WR(SMI,
+				    MT8188_SMI_COMMON_SMI_CLAMP_DIP_TO_VPP1,
+				    MT8188_SMI_COMMON_CLAMP_EN_SET,
+				    MT8188_SMI_COMMON_CLAMP_EN_CLR,
+				    MT8188_SMI_COMMON_CLAMP_EN_STA),
+		},
 		.reset_smi = {
 			SMI_RESET_WR(MT8188_SMI_LARB10_RESET,
 				     MT8188_SMI_LARB10_RESET_ADDR),
@@ -585,7 +597,7 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 			SMI_RESET_WR(MT8188_SMI_LARB15_RESET,
 				     MT8188_SMI_LARB15_RESET_ADDR),
 		},
-		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+		.caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
 	},
 	[MT8188_POWER_DOMAIN_IPE] = {
 		.name = "ipe",
@@ -595,11 +607,18 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.bp_cfg = {
+			BUS_PROT_WR(SMI,
+				    MT8188_SMI_COMMON_SMI_CLAMP_IPE_TO_VPP1,
+				    MT8188_SMI_COMMON_CLAMP_EN_SET,
+				    MT8188_SMI_COMMON_CLAMP_EN_CLR,
+				    MT8188_SMI_COMMON_CLAMP_EN_STA),
+		},
 		.reset_smi = {
 			SMI_RESET_WR(MT8188_SMI_LARB12_RESET,
 				     MT8188_SMI_LARB12_RESET_ADDR),
 		},
-		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+		.caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
 	},
 	[MT8188_POWER_DOMAIN_CAM_VCORE] = {
 		.name = "cam_vcore",
@@ -676,13 +695,20 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.bp_cfg = {
+			BUS_PROT_WR(SMI,
+				    MT8188_SMI_COMMON_SMI_CLAMP_IPE_TO_VPP1,
+				    MT8188_SMI_COMMON_CLAMP_EN_SET,
+				    MT8188_SMI_COMMON_CLAMP_EN_CLR,
+				    MT8188_SMI_COMMON_CLAMP_EN_STA),
+		},
 		.reset_smi = {
 			SMI_RESET_WR(MT8188_SMI_LARB16A_RESET,
 				     MT8188_SMI_LARB16A_RESET_ADDR),
 			SMI_RESET_WR(MT8188_SMI_LARB17A_RESET,
 				     MT8188_SMI_LARB17A_RESET_ADDR),
 		},
-		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+		.caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
 	},
 	[MT8188_POWER_DOMAIN_CAM_SUBB] = {
 		.name = "cam_subb",
@@ -692,13 +718,20 @@ static const struct scpsys_domain_data scpsys_domain_data_mt8188[] = {
 		.pwr_sta2nd_offs = 0x170,
 		.sram_pdn_bits = BIT(8),
 		.sram_pdn_ack_bits = BIT(12),
+		.bp_cfg = {
+			BUS_PROT_WR(SMI,
+				    MT8188_SMI_COMMON_SMI_CLAMP_CAM_SUBB_TO_VDO0,
+				    MT8188_SMI_COMMON_CLAMP_EN_SET,
+				    MT8188_SMI_COMMON_CLAMP_EN_CLR,
+				    MT8188_SMI_COMMON_CLAMP_EN_STA),
+		},
 		.reset_smi = {
 			SMI_RESET_WR(MT8188_SMI_LARB16B_RESET,
 				     MT8188_SMI_LARB16B_RESET_ADDR),
 			SMI_RESET_WR(MT8188_SMI_LARB17B_RESET,
 				     MT8188_SMI_LARB17B_RESET_ADDR),
 		},
-		.caps = MTK_SCPD_KEEP_DEFAULT_OFF,
+		.caps = MTK_SCPD_KEEP_DEFAULT_OFF | MTK_SCPD_CLAMP_PROTECTION,
 	},
 };
 
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
index 9ab6fa105c8c..2a86ff4bf23e 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
@@ -47,9 +47,10 @@ struct scpsys_domain {
 	struct clk_bulk_data *subsys_clks;
 	struct regmap *infracfg_nao;
 	struct regmap *infracfg;
-	struct regmap *smi;
+	struct regmap **smi;
 	struct regmap **larb;
 	int num_larb;
+	int num_smi;
 	struct regulator *supply;
 };
 
@@ -122,29 +123,19 @@ static int scpsys_sram_disable(struct scpsys_domain *pd)
 					MTK_POLL_TIMEOUT);
 }
 
-static struct regmap *scpsys_bus_protect_get_regmap(struct scpsys_domain *pd,
-						    const struct scpsys_bus_prot_data *bpd)
-{
-	if (bpd->flags & BUS_PROT_COMPONENT_SMI)
-		return pd->smi;
-	else
-		return pd->infracfg;
-}
-
 static struct regmap *scpsys_bus_protect_get_sta_regmap(struct scpsys_domain *pd,
 							const struct scpsys_bus_prot_data *bpd)
 {
 	if (bpd->flags & BUS_PROT_STA_COMPONENT_INFRA_NAO)
 		return pd->infracfg_nao;
 	else
-		return scpsys_bus_protect_get_regmap(pd, bpd);
+		return pd->infracfg;
 }
 
 static int scpsys_bus_protect_clear(struct scpsys_domain *pd,
-				    const struct scpsys_bus_prot_data *bpd)
+				    const struct scpsys_bus_prot_data *bpd,
+					struct regmap *sta_regmap, struct regmap *regmap)
 {
-	struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
-	struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd);
 	u32 sta_mask = bpd->bus_prot_sta_mask;
 	u32 expected_ack;
 	u32 val;
@@ -165,10 +156,9 @@ static int scpsys_bus_protect_clear(struct scpsys_domain *pd,
 }
 
 static int scpsys_bus_protect_set(struct scpsys_domain *pd,
-				  const struct scpsys_bus_prot_data *bpd)
+				  const struct scpsys_bus_prot_data *bpd,
+				  struct regmap *sta_regmap, struct regmap *regmap)
 {
-	struct regmap *sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
-	struct regmap *regmap = scpsys_bus_protect_get_regmap(pd, bpd);
 	u32 sta_mask = bpd->bus_prot_sta_mask;
 	u32 val;
 
@@ -182,19 +172,32 @@ static int scpsys_bus_protect_set(struct scpsys_domain *pd,
 					MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
 }
 
-static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
+static int scpsys_clamp_bus_protection_enable(struct scpsys_domain *pd, bool is_smi)
 {
+	int smi_count = 0;
+
 	for (int i = 0; i < SPM_MAX_BUS_PROT_DATA; i++) {
 		const struct scpsys_bus_prot_data *bpd = &pd->data->bp_cfg[i];
+		struct regmap *sta_regmap, *regmap;
+		bool is_smi = bpd->flags & BUS_PROT_COMPONENT_SMI;
 		int ret;
 
 		if (!bpd->bus_prot_set_clr_mask)
 			break;
 
+		if (is_smi) {
+			sta_regmap = pd->smi[smi_count];
+			regmap = pd->smi[smi_count];
+			smi_count++;
+		} else {
+			sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
+			regmap = pd->infracfg;
+		}
+
 		if (bpd->flags & BUS_PROT_INVERTED)
-			ret = scpsys_bus_protect_clear(pd, bpd);
+			ret = scpsys_bus_protect_clear(pd, bpd, sta_regmap, regmap);
 		else
-			ret = scpsys_bus_protect_set(pd, bpd);
+			ret = scpsys_bus_protect_set(pd, bpd, sta_regmap, regmap);
 		if (ret)
 			return ret;
 	}
@@ -202,19 +205,32 @@ static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
 	return 0;
 }
 
-static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
+static int scpsys_clamp_bus_protection_disable(struct scpsys_domain *pd, bool is_smi)
 {
+	int smi_count = pd->num_smi - 1;
+
 	for (int i = SPM_MAX_BUS_PROT_DATA - 1; i >= 0; i--) {
 		const struct scpsys_bus_prot_data *bpd = &pd->data->bp_cfg[i];
+		struct regmap *sta_regmap, *regmap;
+		bool is_smi = bpd->flags & BUS_PROT_COMPONENT_SMI;
 		int ret;
 
 		if (!bpd->bus_prot_set_clr_mask)
 			continue;
 
+		if (is_smi) {
+			sta_regmap = pd->smi[smi_count];
+			regmap = pd->smi[smi_count];
+			smi_count--;
+		} else {
+			sta_regmap = scpsys_bus_protect_get_sta_regmap(pd, bpd);
+			regmap = pd->infracfg;
+		}
+
 		if (bpd->flags & BUS_PROT_INVERTED)
-			ret = scpsys_bus_protect_set(pd, bpd);
+			ret = scpsys_bus_protect_set(pd, bpd, sta_regmap, regmap);
 		else
-			ret = scpsys_bus_protect_clear(pd, bpd);
+			ret = scpsys_bus_protect_clear(pd, bpd, sta_regmap, regmap);
 		if (ret)
 			return ret;
 	}
@@ -272,6 +288,12 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 	bool tmp;
 	int ret;
 
+	if (MTK_SCPD_CAPS(pd, MTK_SCPD_CLAMP_PROTECTION)) {
+		ret = scpsys_clamp_bus_protection_enable(pd, true);
+		if (ret)
+			return ret;
+	}
+
 	ret = scpsys_regulator_enable(pd->supply);
 	if (ret)
 		return ret;
@@ -318,7 +340,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 	if (ret < 0)
 		goto err_disable_subsys_clks;
 
-	ret = scpsys_bus_protect_disable(pd);
+	if (MTK_SCPD_CAPS(pd, MTK_SCPD_CLAMP_PROTECTION)) {
+		ret = scpsys_clamp_bus_protection_disable(pd, true);
+		if (ret)
+			return ret;
+	}
+
+	ret = scpsys_clamp_bus_protection_disable(pd, false);
 	if (ret < 0)
 		goto err_disable_sram;
 
@@ -332,7 +360,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
 	return 0;
 
 err_enable_bus_protect:
-	scpsys_bus_protect_enable(pd);
+	scpsys_clamp_bus_protection_enable(pd, false);
 err_disable_sram:
 	scpsys_sram_disable(pd);
 err_disable_subsys_clks:
@@ -353,7 +381,13 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
 	bool tmp;
 	int ret;
 
-	ret = scpsys_bus_protect_enable(pd);
+	if (MTK_SCPD_CAPS(pd, MTK_SCPD_CLAMP_PROTECTION)) {
+		ret = scpsys_clamp_bus_protection_enable(pd, true);
+		if (ret)
+			return ret;
+	}
+
+	ret = scpsys_clamp_bus_protection_enable(pd, false);
 	if (ret < 0)
 		return ret;
 
@@ -450,12 +484,23 @@ generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_no
 	if (IS_ERR(pd->infracfg))
 		return ERR_CAST(pd->infracfg);
 
-	smi_node = of_parse_phandle(node, "mediatek,smi", 0);
-	if (smi_node) {
-		pd->smi = device_node_to_regmap(smi_node);
-		of_node_put(smi_node);
-		if (IS_ERR(pd->smi))
-			return ERR_CAST(pd->smi);
+	pd->num_smi = of_count_phandle_with_args(node, "mediatek,smi", NULL);
+	if (pd->num_smi > 0) {
+		pd->smi = devm_kcalloc(scpsys->dev, pd->num_smi, sizeof(*pd->smi), GFP_KERNEL);
+		if (!pd->smi)
+			return ERR_PTR(-ENOMEM);
+
+		for (i = 0; i < pd->num_smi; i++) {
+			smi_node = of_parse_phandle(node, "mediatek,smi", i);
+			if (!smi_node)
+				return ERR_PTR(-EINVAL);
+
+			pd->smi[i] = device_node_to_regmap(smi_node);
+			if (IS_ERR(pd->smi[i]))
+				return ERR_CAST(pd->smi[i]);
+		}
+	} else {
+		pd->num_smi = 0;
 	}
 
 	pd->num_larb = of_count_phandle_with_args(node, "mediatek,larb", NULL);
diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.h b/drivers/pmdomain/mediatek/mtk-pm-domains.h
index 31c2a1bb500f..e0eb7214719e 100644
--- a/drivers/pmdomain/mediatek/mtk-pm-domains.h
+++ b/drivers/pmdomain/mediatek/mtk-pm-domains.h
@@ -13,6 +13,7 @@
 #define MTK_SCPD_EXT_BUCK_ISO		BIT(6)
 #define MTK_SCPD_HAS_INFRA_NAO		BIT(7)
 #define MTK_SCPD_STRICT_BUS_PROTECTION	BIT(8)
+#define MTK_SCPD_CLAMP_PROTECTION	BIT(9)
 #define MTK_SCPD_CAPS(_scpd, _x)	((_scpd)->data->caps & (_x))
 
 #define SPM_VDE_PWR_CON			0x0210
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel