Re: Discuss Mediatek Power Domain Driver Support Mulitle Smi Clamp Protection

Re: Discuss Mediatek Power Domain Driver Support Mulitle Smi Clamp Protection

[Yu-chang Lee (李禹璋)]

On Wed, 2023-12-20 at 14:11 +0100, AngeloGioacchino Del Regno wrote:
> Il 20/12/23 10:36, Yu-chang Lee (李禹璋) ha scritto:
> > Dear Markus, Matthias, Angelo:
> > 
> > I am writing to seek your expertise and advice regarding the
> > revision
> > of the Mediatek power domain driver for the MT8188 platform.
> > Specifically, the display power domain requires multiple SMI clamp
> > protections (refer to [1] in mt8188.dts "MT8188_POWER_DOMAIN_DIP"
> > "mediatek,smi"). However, the current version on the main branch,
> > which
> > has already merged SMI and infra bus protection (mainly [2]),
> > complicates the mapping of individual SMIs to their respective bus
> > protections.
> > 
> > While not a complete solution, I believe the patch in [3] may
> > better
> > illustrate my intentions.
> > 
> > Markus has provided some thoughts and recommendations that I want
> > to
> > bring forward, hoping to gain further direction based on your
> > expertise.
> > 
> > 1.My initial thought is to revert the changes made in [2], which
> > separated the SMI infra bus protection operations. This approach
> > would
> > simplify the mapping of multiple SMIs to bus protections by their
> > order
> > in the DTS. However, reverting to an older version may not be
> > desired.
> > 
> 
> Relying on the order of bus protections in DTS can be flaky and prone
> to generate
> future mistakes; remember that whatever we send upstream is something
> that not
> only has to be working, clean, solid and reliable, but we should also
> take care
> of avoiding future mistakes that could happen when somebody from the
> community, or
> other MediaTek engineers, implement new platforms.

> Reverting commits is not prohibited as long as we're not breaking
> support for any
> platform (so, as long as we're not removing features), but still, I
> don't think
> that this is the best solution, honestly.
> 
> Besides, I don't get why the order of SMI phandles is important, can
> you please
> explain why?

I apologize for the confusion, The bp_cfg order in mt8188-pm-domain.h
is important because the order of bus protection is depned on it.
However smi clamp protection should be done before infra one. So rely
on this is also a little shaky I think. Also it will take some overhead
to loop through bp_cfg, mixed with infra and smi, to find the correct
name smi to do clamp protection.

That why the first approach seems straight forward to me because it
separate smi and infra. And we can do different protection with them.

> 
> > 2.The second approach, suggested by Markus, is to introduce
> > "mediatek,smi-names" and convert "mediatek,smi" to an array of
> > phandles. We could add a new field to the struct
> > scpsys_bus_prot_data,
> > such as const char *smi_name, which could be NULL (for device trees
> > with only a single property). This way, we can use the smi-names to
> > specify the desired SMI. While this method retains the current code
> > structure, the implementation may not be as straightforward.
> > 
> 
> To be completely honest, neither of the two approaches are state of
> the art,
> but the mediatek,smi-names one is better than the first, granted that
> we
> really need to guarantee that the SMIs are ordered.
> 
> But again, my question is: why should those be ordered?
> Can't we clamp IMG1 before clamping IMG0?
> 
We can clamp IMG1 before clamping IMG0, I am more concerned with the
order between infra and smi rather than the order between smi. And I
think current structure mixed smi and infra may also introduce
misunderstanding in the future. Maybe separate the bp_cfg and match the
smi name with smi's bus protection step could be a direction to fix the
problem?

Thanks for your timely feedback!

Best Regards,
yu-chang.lee


> Cheers,
> Angelo
> 
> > Attempting to resolve this issue, I find the first option easier
> > and
> > more direct. However, I would greatly appreciate hearing the
> > thoughts
> > of experts on this matter before proceeding with upstreaming.
> > 
> > I look forward to your valued response.
> > 
> > Cheers!
> > Best Regards,
> > Yu-chang.lee
> > 
> > [1]:
> > 
https://urldefense.com/v3/__https://chromium.googlesource.com/chromiumos/third_party/kernel/*/87222665c8f974e4ff9dca408cedf6b540a9e770__;Kw!!CTRNKA9wMg0ARbw!kDNdDHejJnDRc6HgYp1-LOvU7Ij4m23UJizlQOGgaJET_rH1jNegg7tYmhOf8XkHVZF_dqG94-kHyhzX6nZE_6aGNBv2CSyx6g$
> >  
> > [2]:
> > 
https://lore.kernel.org/all/20230918093751.1188668-6-msp@baylibre.com/
> > [3]:
> > 
https://urldefense.com/v3/__https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/*/5098295__;Kw!!CTRNKA9wMg0ARbw!kDNdDHejJnDRc6HgYp1-LOvU7Ij4m23UJizlQOGgaJET_rH1jNegg7tYmhOf8XkHVZF_dqG94-kHyhzX6nZE_6aGNBvmL2Hiaw$
> >  
> 
> 
> 

Re: Discuss Mediatek Power Domain Driver Support Mulitle Smi Clamp Protection

[AngeloGioacchino Del Regno]

Il 21/12/23 09:29, Yu-chang Lee (李禹璋) ha scritto:
> On Wed, 2023-12-20 at 14:11 +0100, AngeloGioacchino Del Regno wrote:
>> Il 20/12/23 10:36, Yu-chang Lee (李禹璋) ha scritto:
>>> Dear Markus, Matthias, Angelo:
>>>
>>> I am writing to seek your expertise and advice regarding the
>>> revision
>>> of the Mediatek power domain driver for the MT8188 platform.
>>> Specifically, the display power domain requires multiple SMI clamp
>>> protections (refer to [1] in mt8188.dts "MT8188_POWER_DOMAIN_DIP"
>>> "mediatek,smi"). However, the current version on the main branch,
>>> which
>>> has already merged SMI and infra bus protection (mainly [2]),
>>> complicates the mapping of individual SMIs to their respective bus
>>> protections.
>>>
>>> While not a complete solution, I believe the patch in [3] may
>>> better
>>> illustrate my intentions.
>>>
>>> Markus has provided some thoughts and recommendations that I want
>>> to
>>> bring forward, hoping to gain further direction based on your
>>> expertise.
>>>
>>> 1.My initial thought is to revert the changes made in [2], which
>>> separated the SMI infra bus protection operations. This approach
>>> would
>>> simplify the mapping of multiple SMIs to bus protections by their
>>> order
>>> in the DTS. However, reverting to an older version may not be
>>> desired.
>>>
>>
>> Relying on the order of bus protections in DTS can be flaky and prone
>> to generate
>> future mistakes; remember that whatever we send upstream is something
>> that not
>> only has to be working, clean, solid and reliable, but we should also
>> take care
>> of avoiding future mistakes that could happen when somebody from the
>> community, or
>> other MediaTek engineers, implement new platforms.
> 
>> Reverting commits is not prohibited as long as we're not breaking
>> support for any
>> platform (so, as long as we're not removing features), but still, I
>> don't think
>> that this is the best solution, honestly.
>>
>> Besides, I don't get why the order of SMI phandles is important, can
>> you please
>> explain why?
> 
> I apologize for the confusion, The bp_cfg order in mt8188-pm-domain.h
> is important because the order of bus protection is depned on it.
> However smi clamp protection should be done before infra one. So rely
> on this is also a little shaky I think. Also it will take some overhead
> to loop through bp_cfg, mixed with infra and smi, to find the correct
> name smi to do clamp protection.
> 
> That why the first approach seems straight forward to me because it
> separate smi and infra. And we can do different protection with them.
> 
>>
>>> 2.The second approach, suggested by Markus, is to introduce
>>> "mediatek,smi-names" and convert "mediatek,smi" to an array of
>>> phandles. We could add a new field to the struct
>>> scpsys_bus_prot_data,
>>> such as const char *smi_name, which could be NULL (for device trees
>>> with only a single property). This way, we can use the smi-names to
>>> specify the desired SMI. While this method retains the current code
>>> structure, the implementation may not be as straightforward.
>>>
>>
>> To be completely honest, neither of the two approaches are state of
>> the art,
>> but the mediatek,smi-names one is better than the first, granted that
>> we
>> really need to guarantee that the SMIs are ordered.
>>
>> But again, my question is: why should those be ordered?
>> Can't we clamp IMG1 before clamping IMG0?
>>
> We can clamp IMG1 before clamping IMG0, I am more concerned with the
> order between infra and smi rather than the order between smi. And I
> think current structure mixed smi and infra may also introduce
> misunderstanding in the future. Maybe separate the bp_cfg and match the
> smi name with smi's bus protection step could be a direction to fix the
> problem?
> 

That could work, yes. You could alternatively build an array, or a 32 bit value for
which each bit corresponds to the position of a BUS_PROT_WR entry in .bp_cfg, and
then use that to locate SMI vs INFRA bus protection entries.

Taking as an example a "messy" bp_cfg:

.bp_cfg = {
	BUS_PROT_WR(INFRA, other_params),
	BUS_PROT_WR(SMI, other_params),
	BUS_PROT_WR(INFRA, ...),
	BUS_PROT_WR(INFRA, ...),
	BUS_PROT_WR(SMI, ...),
}

probe_function() {
	...

	for (....) {
		if (.bp_cfg[i] == SMI)
			pd->smi_bp_map |= BIT(i);
	}

	....
}

powerup_function() {
	...
	u16 smi_bp_map = pd->smi_bp_map;

	for_each_set_bit(i, &smi_pd_map, 16)
		bus_protect_smi_write(somewhere->bp_cfg[i]);

	...

	bus_protect_infra_write()

	...
}


Or, simply, we can *enforce* ordering in bp_cfg, so that all of the SMI entries
are before INFRA, like:

.bp_cfg = {
	/* Ordering is important here: SMI bus prot always before INFRA */
	BUS_PROT_WR(SMI, other_params),
	BUS_PROT_WR(SMI, ...),
	BUS_PROT_WR(INFRA, other_params),
	BUS_PROT_WR(INFRA, ...),
	BUS_PROT_WR(INFRA, ...),
}

...so that we never build any positional information / bitmaps.

Since the SMI vs INFRA order is really important, we could add a "failsafe" check
in the probe function, like:

probe() {
	bool smi_prot_found;

	for (....) {
		if ((...->bp_cfg[i].flags & BUS_PROT_COMPONENT_INFRA) &&
		    smi_prot_found) {
			dev_err(... , "SMI bus prot cannot be mixed with INFRA");
			return -EINVAL;
		} else if (....->bp_cfg[i] & BUS_PROT_COMPONENT_SMI)
			smi_prot_found = true;
	}
}

Obviously the error message etc should be reworded, all the above is just
examples to solve the issue without having any meaningful overhead in the
performance paths [scpsys_power_on() and scpsys_power_off()].

Does that help you?

Cheers,
Angelo

> Thanks for your timely feedback!
> 
> Best Regards,
> yu-chang.lee
> 
> 
>> Cheers,
>> Angelo
>>
>>> Attempting to resolve this issue, I find the first option easier
>>> and
>>> more direct. However, I would greatly appreciate hearing the
>>> thoughts
>>> of experts on this matter before proceeding with upstreaming.
>>>
>>> I look forward to your valued response.
>>>
>>> Cheers!
>>> Best Regards,
>>> Yu-chang.lee
>>>
>>> [1]:
>>>
> https://urldefense.com/v3/__https://chromium.googlesource.com/chromiumos/third_party/kernel/*/87222665c8f974e4ff9dca408cedf6b540a9e770__;Kw!!CTRNKA9wMg0ARbw!kDNdDHejJnDRc6HgYp1-LOvU7Ij4m23UJizlQOGgaJET_rH1jNegg7tYmhOf8XkHVZF_dqG94-kHyhzX6nZE_6aGNBv2CSyx6g$
>>>   
>>> [2]:
>>>
> https://lore.kernel.org/all/20230918093751.1188668-6-msp@baylibre.com/
>>> [3]:
>>>
> https://urldefense.com/v3/__https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/*/5098295__;Kw!!CTRNKA9wMg0ARbw!kDNdDHejJnDRc6HgYp1-LOvU7Ij4m23UJizlQOGgaJET_rH1jNegg7tYmhOf8XkHVZF_dqG94-kHyhzX6nZE_6aGNBvmL2Hiaw$
>>>   
>>
>>
>>

Re: Discuss Mediatek Power Domain Driver Support Mulitle Smi Clamp Protection

[Yu-chang Lee (李禹璋)]

On Thu, 2023-12-21 at 13:31 +0100, AngeloGioacchino Del Regno wrote:
> > 
> > We can clamp IMG1 before clamping IMG0, I am more concerned with
> > the
> > order between infra and smi rather than the order between smi. And
> > I
> > think current structure mixed smi and infra may also introduce
> > misunderstanding in the future. Maybe separate the bp_cfg and match
> > the
> > smi name with smi's bus protection step could be a direction to fix
> > the
> > problem?
> > 
> 
> That could work, yes. You could alternatively build an array, or a 32
> bit value for
> which each bit corresponds to the position of a BUS_PROT_WR entry in
> .bp_cfg, and
> then use that to locate SMI vs INFRA bus protection entries.
> 
> Taking as an example a "messy" bp_cfg:
> 
> .bp_cfg = {
> 	BUS_PROT_WR(INFRA, other_params),
> 	BUS_PROT_WR(SMI, other_params),
> 	BUS_PROT_WR(INFRA, ...),
> 	BUS_PROT_WR(INFRA, ...),
> 	BUS_PROT_WR(SMI, ...),
> }
> 
> probe_function() {
> 	...
> 
> 	for (....) {
> 		if (.bp_cfg[i] == SMI)
> 			pd->smi_bp_map |= BIT(i);
> 	}
> 
> 	....
> }
> 
> powerup_function() {
> 	...
> 	u16 smi_bp_map = pd->smi_bp_map;
> 
> 	for_each_set_bit(i, &smi_pd_map, 16)
> 		bus_protect_smi_write(somewhere->bp_cfg[i]);
> 
> 	...
> 
> 	bus_protect_infra_write()
> 
> 	...
> }
> 
> 
> Or, simply, we can *enforce* ordering in bp_cfg, so that all of the
> SMI entries
> are before INFRA, like:
> 
> .bp_cfg = {
> 	/* Ordering is important here: SMI bus prot always before INFRA
> */
> 	BUS_PROT_WR(SMI, other_params),
> 	BUS_PROT_WR(SMI, ...),
> 	BUS_PROT_WR(INFRA, other_params),
> 	BUS_PROT_WR(INFRA, ...),
> 	BUS_PROT_WR(INFRA, ...),
> }
> 
> ...so that we never build any positional information / bitmaps.
> 
> Since the SMI vs INFRA order is really important, we could add a
> "failsafe" check
> in the probe function, like:
> 
> probe() {
> 	bool smi_prot_found;
> 
> 	for (....) {
> 		if ((...->bp_cfg[i].flags & BUS_PROT_COMPONENT_INFRA)
> &&
> 		    smi_prot_found) {
> 			dev_err(... , "SMI bus prot cannot be mixed
> with INFRA");
> 			return -EINVAL;
> 		} else if (....->bp_cfg[i] & BUS_PROT_COMPONENT_SMI)
> 			smi_prot_found = true;
> 	}
> }
> 
> Obviously the error message etc should be reworded, all the above is
> just
> examples to solve the issue without having any meaningful overhead in
> the
> performance paths [scpsys_power_on() and scpsys_power_off()].
> 
> Does that help you?
> 
> Cheers,
> Angelo

Hi Angelo:

Thanks for your generous feedback and advice, I will think about it.
See you upstream then.

Best Regards,
Yu-Chang.Lee