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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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$
>>>   
>>
>>
>>