Messages in this thread | | | Subject | Re: [PATCH] drm/msm/a6xx: fix for kernels without CONFIG_NVMEM | From | Jonathan Marek <> | Date | Wed, 17 Feb 2021 15:35:09 -0500 |
| |
On 2/17/21 3:18 PM, Rob Clark wrote: > On Wed, Feb 17, 2021 at 11:08 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: >> >> On Wed, Feb 17, 2021 at 07:14:16PM +0530, Akhil P Oommen wrote: >>> On 2/17/2021 8:36 AM, Rob Clark wrote: >>>> On Tue, Feb 16, 2021 at 12:10 PM Jonathan Marek <jonathan@marek.ca> wrote: >>>>> >>>>> Ignore nvmem_cell_get() EOPNOTSUPP error in the same way as a ENOENT error, >>>>> to fix the case where the kernel was compiled without CONFIG_NVMEM. >>>>> >>>>> Fixes: fe7952c629da ("drm/msm: Add speed-bin support to a618 gpu") >>>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca> >>>>> --- >>>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>> index ba8e9d3cf0fe..7fe5d97606aa 100644 >>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>> @@ -1356,10 +1356,10 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu, >>>>> >>>>> cell = nvmem_cell_get(dev, "speed_bin"); >>>>> /* >>>>> - * -ENOENT means that the platform doesn't support speedbin which is >>>>> - * fine >>>>> + * -ENOENT means no speed bin in device tree, >>>>> + * -EOPNOTSUPP means kernel was built without CONFIG_NVMEM >>>> >>>> very minor nit, it would be nice to at least preserve the gist of the >>>> "which is fine" (ie. some variation of "this is an optional thing and >>>> things won't catch fire without it" ;-)) >>>> >>>> (which is, I believe, is true, hopefully Akhil could confirm.. if not >>>> we should have a harder dependency on CONFIG_NVMEM..) >>> IIRC, if the gpu opp table in the DT uses the 'opp-supported-hw' property, >>> we will see some error during boot up if we don't call >>> dev_pm_opp_set_supported_hw(). So calling "nvmem_cell_get(dev, "speed_bin")" >>> is a way to test this. >>> >>> If there is no other harm, we can put a hard dependency on CONFIG_NVMEM. >> >> I'm not sure if we want to go this far given the squishiness about module >> dependencies. As far as I know we are the only driver that uses this seriously >> on QCOM SoCs and this is only needed for certain targets. I don't know if we >> want to force every target to build NVMEM and QFPROM on our behalf. But maybe >> I'm just saying that because Kconfig dependencies tend to break my brain (and >> then Arnd has to send a patch to fix it). >> > > Hmm, good point.. looks like CONFIG_NVMEM itself doesn't have any > other dependencies, so I suppose it wouldn't be the end of the world > to select that.. but I guess we don't want to require QFPROM > > I guess at the end of the day, what is the failure mode if you have a > speed-bin device, but your kernel config misses QFPROM (and possibly > NVMEM)? If the result is just not having the highest clk rate(s) > available, that isn't the end of the world. But if it makes things > not-work, that is sub-optimal. Generally, especially on ARM, kconfig > seems to be way harder than it should be to build a kernel that works, > if we could somehow not add to that problem (for both people with a6xx > and older gens) that would be nice ;-) >
There is a "imply" kconfig option which solves exactly this problem. (you would "imply NVMEM" instead of "select NVMEM". then it would be possible to disable NVMEM but it would get enabled by default)
> BR, > -R >
| |