On Thu, Apr 18, 2024 at 11:57:35AM +0200, Konrad Dybcio wrote:
> On 18.04.2024 1:49 AM, Dmitry Baryshkov wrote:
> > On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
> >> There is no need to reinvent the wheel for simple read-match-set logic.
> >>
> >> Make speedbin discovery and assignment generation independent.
> >>
> >> This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
> >> which has no representation in hardware whatshowever.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dyb...@linaro.org>
> >> ---
>
> [...]
>
> >> + /* No speedbins defined for this GPU SKU => allow all defined OPPs */
> >> + if (!info->speedbins) {
> >> + adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
> >> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
> >
> > BIT(0)
>
> You mean for &supp_hw, or "1"?
>
> 1 is the "count" parameter, supp_hw is a "u32 supported_hw[count]"
I see. It confused me. This way it's getting set to UINT_MAX, which
will match against any non-zero opp-supported-hw. Ack.
>
> >
> >> + }
> >> +
> >> + /*
> >> + * If a real error (not counting older devicetrees having no nvmem
> >> references)
> >> + * occurs when trying to get the fuse value, bail out.
> >> + */
> >> + ret = adreno_read_speedbin(adreno_gpu, dev, &fuse);
> >> + if (ret) {
> >> + return ret;
> >> + } else if (fuse == ADRENO_SPEEDBIN_FUSE_NODATA) {
> >> + /* The info struct has speedbin data, but the DT is too old =>
> >> allow all OPPs */
> >
> > Missing assignment to adeno_gpu->speedbin ? Or is it fine?
>
> Good catch. Only mesa (and I suppose you :D) read this value.
>
> >
> >> + DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your
> >> device tree\n");
> >> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
> >
> > BIT(0)
> > maybe #define it?
>
> (ditto)
>
> Konrad
--
With best wishes
Dmitry
