Received-SPF: pass (google.com: domain of linux-kernel+bounces-4439-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <8e587947-4ae1-49c0-9d54-b95f9d539a7c@linaro.org>
Date: Mon, 18 Dec 2023 23:08:25 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 13/34] media: iris: introduce platform specific
 capabilities for core and instance
Content-Language: en-US
To: Dikshita Agarwal <quic_dikshita@quicinc.com>,
 linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
 stanimir.k.varbanov@gmail.com, quic_vgarodia@quicinc.com, agross@kernel.org,
 andersson@kernel.org, mchehab@kernel.org, bryan.odonoghue@linaro.org
Cc: linux-arm-msm@vger.kernel.org, quic_abhinavk@quicinc.com
References: <1702899149-21321-1-git-send-email-quic_dikshita@quicinc.com>
 <1702899149-21321-14-git-send-email-quic_dikshita@quicinc.com>
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <1702899149-21321-14-git-send-email-quic_dikshita@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 12/18/23 12:32, Dikshita Agarwal wrote:
> Capabilities are set of video specifications and features supported
> by a specific platform(SOC). Each capability is defined with
> min, max, range, default value and corresponding HFI.
> 
> Also, platform data defines different resource details for
> a specific platform(SOC). This change defines resource tables
> for sm8550 platform data and use for initializing these resources.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
[...]
[...]

> -	ret = protect_secure_region(CP_START, CP_SIZE, CP_NONPIXEL_START,
> -				    CP_NONPIXEL_SIZE, IRIS_PAS_ID);
> +	cp_start = core->cap[CP_START].value;
> +	cp_size = core->cap[CP_SIZE].value;
> +	cp_nonpixel_start = core->cap[CP_NONPIXEL_START].value;
> +	cp_nonpixel_size = core->cap[CP_NONPIXEL_SIZE].value;
but you just hardcoded these a couple patches ago.. are they
variable after all?

[...]

> +	{DEC_CODECS, H264 | HEVC | VP9},
> +	{MAX_SESSION_COUNT, 16},
> +	{MAX_MBPF, 278528}, /* ((8192x4352)/256) * 2 */
> +	{MAX_MBPS, 7833600}, /* max_load 7680x4320@60fps */
> +	{NUM_VPP_PIPE, 4},
> +	{HW_RESPONSE_TIMEOUT, HW_RESPONSE_TIMEOUT_VALUE},
> +	{DMA_MASK, GENMASK(31, 29) - 1},
> +	{CP_START, 0},
> +	{CP_SIZE, 0x25800000},
> +	{CP_NONPIXEL_START, 0x01000000},
> +	{CP_NONPIXEL_SIZE, 0x24800000},
Why this and not an array of enum-indexed u32s?

> +};
> +
> +static struct plat_inst_cap instance_cap_data_sm8550[] = {
you know all of the possible members here as well, you can just
create a struct of actual configurations instead of turning them
into generic capabilities that you have to parse either way at
some point

[...]

> +
> +static const struct bus_info sm8550_bus_table[] = {
> +	{ NULL, "iris-cnoc", 1000, 1000     },
> +	{ NULL, "iris-ddr",  1000, 15000000 },
This is a copy of the common data from the previous patches that you're
now dropping (why was it there in the first place then?). Is it specific
to this platform, or can it be reused?
> +};
> +
> +static const struct clock_info sm8550_clk_table[] = {
> +	{ NULL, "gcc_video_axi0", GCC_VIDEO_AXI0_CLK, 0 },
> +	{ NULL, "core_clk",       VIDEO_CC_MVS0C_CLK, 0 },
> +	{ NULL, "vcodec_core",    VIDEO_CC_MVS0_CLK,  1 },
> +};
Are these the input pad names?

> +
> +static const char * const sm8550_clk_reset_table[] = { "video_axi_reset", NULL };
Ditto

> +
> +static const char * const sm8550_pd_table[] = { "iris-ctl", "vcodec", NULL };
Ditto

> +
> +static const char * const sm8550_opp_pd_table[] = { "mxc", "mmcx", NULL };
Ditto

> +
> +static const struct bw_info sm8550_bw_table_dec[] = {
> +	{ 2073600, 1608000, 2742000 },	/* 4096x2160@60 */
> +	{ 1036800,  826000, 1393000 },	/* 4096x2160@30 */
> +	{  489600,  567000,  723000 },	/* 1920x1080@60 */
> +	{  244800,  294000,  372000 },	/* 1920x1080@30 */
> +};
> +
> +static const struct reg_preset_info sm8550_reg_preset_table[] = {
> +	{ 0xB0088, 0x0, 0x11 },
lowercase hex for non-defines, please
> +};
> +
> +static struct ubwc_config_data ubwc_config_sm8550[] = {
> +	UBWC_CONFIG(8, 32, 16, 0, 1, 1, 1),
I think it would be far more telling to drop this #define and fill
in the values inline

> +};
> +
> +struct platform_data sm8550_data = {
> +	.bus_tbl = sm8550_bus_table,
> +	.bus_tbl_size = ARRAY_SIZE(sm8550_bus_table),
> +	.clk_tbl = sm8550_clk_table,
> +	.clk_tbl_size = ARRAY_SIZE(sm8550_clk_table),
> +	.clk_rst_tbl = sm8550_clk_reset_table,
> +	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_reset_table),
> +
> +	.bw_tbl_dec = sm8550_bw_table_dec,
> +	.bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec),
> +
> +	.pd_tbl = sm8550_pd_table,
> +	.pd_tbl_size = ARRAY_SIZE(sm8550_pd_table),
> +	.opp_pd_tbl = sm8550_opp_pd_table,
> +	.opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table),
> +
> +	.reg_prst_tbl = sm8550_reg_preset_table,
> +	.reg_prst_tbl_size = ARRAY_SIZE(sm8550_reg_preset_table),
> +	.fwname = "vpu30_4v.mbn",
> +	.pas_id = 9,
> +
> +	.core_data = core_data_sm8550,
> +	.core_data_size = ARRAY_SIZE(core_data_sm8550),
> +	.inst_cap_data = instance_cap_data_sm8550,
> +	.inst_cap_data_size = ARRAY_SIZE(instance_cap_data_sm8550),
> +	.ubwc_config = ubwc_config_sm8550,
Unless any of these are going to be reused, please inline all of the
values here..

Konrad