linux-kernel - Re: [PATCH v2 3/9] clk: qcom: Add SDM660 Multimedia Clock Controller (MMCC) driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ba8ac3c-d33b-3ab2-5855-f99d431b397a@somainline.org>
Date:   Tue, 26 Jan 2021 15:11:53 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
To:     Stanimir Varbanov <stanimir.varbanov@...aro.org>, agross@...nel.org
Cc:     bjorn.andersson@...aro.org, mturquette@...libre.com,
        robh+dt@...nel.org, linux-arm-msm@...r.kernel.org,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org
Subject: Re: [PATCH v2 3/9] clk: qcom: Add SDM660 Multimedia Clock Controller
 (MMCC) driver

Il 26/01/21 14:39, Stanimir Varbanov ha scritto:
> 
> 
> On 1/13/21 8:38 PM, AngeloGioacchino Del Regno wrote:
>> From: Martin Botka <martin.botka@...ainline.org>
>>
>> Add a driver for the multimedia clock controller found on SDM660
>> based devices. This should allow most multimedia device drivers
>> to probe and control their clocks.
>>
>> Signed-off-by: Martin Botka <martin.botka@...ainline.org>
>> Co-developed-by: Konrad Dybcio <konrad.dybcio@...ainline.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...ainline.org>
>> [angelogioacchino.delregno@...ainline.org: Cleaned up SDM630 clock fixups]
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
>> ---
>>   drivers/clk/qcom/Kconfig                     |    9 +
>>   drivers/clk/qcom/Makefile                    |    1 +
>>   drivers/clk/qcom/mmcc-sdm660.c               | 2864 ++++++++++++++++++
>>   include/dt-bindings/clock/qcom,mmcc-sdm660.h |  162 +
>>   4 files changed, 3036 insertions(+)
>>   create mode 100644 drivers/clk/qcom/mmcc-sdm660.c
>>   create mode 100644 include/dt-bindings/clock/qcom,mmcc-sdm660.h
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index d32bb12cd8d0..eb9746e84556 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -366,6 +366,15 @@ config SDM_GCC_660
>>   	  Say Y if you want to use peripheral devices such as UART, SPI,
>>   	  i2C, USB, UFS, SDDC, PCIe, etc.
>>   
>> +config SDM_MMCC_660
>> +	tristate "SDM660 Multimedia Clock Controller"
>> +	select SDM_GCC_660
>> +	select QCOM_GDSC
>> +	help
>> +	  Support for the multimedia clock controller on SDM660 devices.
>> +	  Say Y if you want to support multimedia devices such as display,
>> +	  graphics, video encode/decode, camera, etc.
>> +
>>   config QCS_TURING_404
>>   	tristate "QCS404 Turing Clock Controller"
>>   	help
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 9e5e0e3cb7b4..bfa8350f088d 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -62,6 +62,7 @@ obj-$(CONFIG_SC_VIDEOCC_7180) += videocc-sc7180.o
>>   obj-$(CONFIG_SDM_CAMCC_845) += camcc-sdm845.o
>>   obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
>>   obj-$(CONFIG_SDM_GCC_660) += gcc-sdm660.o
>> +obj-$(CONFIG_SDM_MMCC_660) += mmcc-sdm660.o
>>   obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
>>   obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
>>   obj-$(CONFIG_SDM_LPASSCC_845) += lpasscc-sdm845.o
>> diff --git a/drivers/clk/qcom/mmcc-sdm660.c b/drivers/clk/qcom/mmcc-sdm660.c
>> new file mode 100644
>> index 000000000000..d268e1c89f57
>> --- /dev/null
>> +++ b/drivers/clk/qcom/mmcc-sdm660.c
>> @@ -0,0 +1,2864 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2020, Martin Botka <martin.botka@...ainline.org>
>> + * Copyright (c) 2020, Konrad Dybcio <konrad.dybcio@...ainline.org>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/clk.h>
>> +
>> +
>> +#include <dt-bindings/clock/qcom,mmcc-sdm660.h>
>> +
>> +#include "common.h"
>> +#include "clk-regmap.h"
>> +#include "clk-regmap-divider.h"
>> +#include "clk-alpha-pll.h"
>> +#include "clk-rcg.h"
>> +#include "clk-branch.h"
>> +#include "reset.h"
>> +#include "gdsc.h"
>> +
> 
> <cut>
> 
>> +
>> +static struct gdsc venus_gdsc = {
>> +	.gdscr = 0x1024,
>> +	.pd = {
>> +		.name = "venus",
>> +	},
>> +	.pwrsts = PWRSTS_OFF_ON,
>> +};
>> +
>> +static struct gdsc venus_core0_gdsc = {
>> +	.gdscr = 0x1040,
>> +	.pd = {
>> +		.name = "venus_core0",
>> +	},
>> +	.parent = &venus_gdsc.pd,
>> +	.pwrsts = PWRSTS_OFF_ON,
> 
> I think this gdsc should be under hw control?
> 
> +	.flags = HW_CTRL,
> 

Feels strange, eh? Was the same for me, but then noupe, there is no
hw control for this GDSC downstream, nor a hw_ctrl address for it, so
on this SoC it shouldn't be under hw control.

Besides that, testing also agrees with this (enc/dec works)...

P.S.: Thanks for the review!
-- Angelo

>> +};
>> +