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

["Yu-chang Lee (李禹璋)" <Yu-chang.Lee@mediatek.com>]

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