Re: [PATCH v4 3/8] scsi: ufs: ufs-mediatek: Remove useless mediatek,ufs-boost-crypt property

[AngeloGioacchino Del Regno]

Il 16/04/24 15:05, Peter Wang (王信友) ha scritto:
> On Tue, 2024-04-16 at 12:38 +0200, AngeloGioacchino Del Regno wrote:
> > Il 16/04/24 12:31, Peter Wang (王信友) ha scritto:
> > > > Yes this causes -> less than half of a millisecond <- of
> > > > additional
> > > > boot time
> > > > if the dvfsrc-supply is present but boost-microvolt is not.
> > > >
> > > > I really don't see the problem with that :-)
> > >
> > > Adding a little bit of boot time to one smartphone might not be a
> > > problem, but when you consider a billion smartphones each adding a
> > > little bit, the cumulative effect becomes significant. The power
> > > consumption of these accumulated times will continue to increase,
> > > contributing to the Earth's carbon emissions. Moreover, removing
> > > the
> > > master switch for this feature doesn't seem to have any benefits
> > > other
> > > than not having to set it in the DTS. Similarly, the master switch
> > > for
> > > VA09 seems to have more disadvantage.
> >
> > Sorry, but I still don't see how a few *microseconds* more of boot
> > time can
> > be significant, even related to power consumption during boot.
> >
> > If that was a few milliseconds, then I'd agree with you, but that's
> > not the case.
> >
> > Removing the master switch has a benefit: you *lose* a few
> > microseconds of boot
> > time (so, boots in *few microseconds LESS*) on platforms that would
> > have this set
> > in devicetree, as this property is redundant with the other
> > activation checks
> > for those features.
> >
> > So, there you go: if the majority of MediaTek platforms are already
> > using this
> > crypt boost feature, then this commit reduces carbon emissions, as
> > those would
> > boot in a few less microseconds.
>
> But the majority platfomrs dosen't need this feature.
> This feature is only for legacy chip which at least 4 years ago.

Upstream supports platforms that do and don't need this feature, and having
redundant device tree properties performing the same checks is not just
suboptimal but plain wrong.

Adding to this, devicetree describes the hardware - and there is no physical
hardware switch that needs this redundant property, this means that the
property that is getting removed in this commit (and the va09 one in another
commit of this series) is a *software switch*, not HW.

Keep in mind, also, that this feature (and again, the va09 one as well) has
a specific requirement to be supported - and this is what the code does even
without the software switch to add it.

In case there's any need to disallow such feature from a specific SoC, the DT
bindings can be modified such that a specific compatible string would disallow
adding the required regulator and/or boost-microvolt properties.

Besides, I want to remind you that there is no reason to drop support, or have
them unreliably working, or use hacks, for SoCs that are "old" - especially
when this is a driver that works on both old and new ones.

Regards,
Angelo