Re: [PATCH 07/10] phy: qualcomm: qmp-pcie: Add support for SM8550 g3x2 and g4x2 PCIEs

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 17/01/2023 15:57, Abel Vesa wrote:
> On 23-01-01 22:15:55, Dmitry Baryshkov wrote:
>> On 16/11/2022 14:01, Abel Vesa wrote:
>>> Add the SM8550 both g4 and g3 configurations. In addition, there is a
>>> new "lane shared" table that needs to be configured for g4, along with
>>> the No-CSR list of resets.
>>>
>>> Co-developed-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 354 +++++++++++++++++++++++
>>>    1 file changed, 354 insertions(+)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> index 47cccc4b35b2..87c7c20dfc8d 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>
>> [skipped tables]
>>
>>> @@ -1473,6 +1701,8 @@ struct qmp_pcie_offsets {
>>>    struct qmp_phy_cfg_tbls {
>>>    	const struct qmp_phy_init_tbl *serdes;
>>>    	int serdes_num;
>>> +	const struct qmp_phy_init_tbl *ln_shrd_serdes;
>>> +	int ln_shrd_serdes_num;
>>>    	const struct qmp_phy_init_tbl *tx;
>>>    	int tx_num;
>>>    	const struct qmp_phy_init_tbl *rx;
>>> @@ -1510,6 +1740,9 @@ struct qmp_phy_cfg {
>>>    	/* resets to be requested */
>>>    	const char * const *reset_list;
>>>    	int num_resets;
>>> +	/* no CSR resets to be requested */
>>> +	const char * const *nocsr_reset_list;
>>> +	int num_nocsr_resets;
>>
>> Is there any difference between 'no CSR' resets and the plain ones? Can we
>> handle them in a single array instead?
> 
> Yes, on power on, only the 'No CSR' are necessary to be deasserted.
> 
> So we need to differentiate between 'No CSR' and the rest.

Ack.

> 
>>
>>>    	/* regulators to be requested */
>>>    	const char * const *vreg_list;
>>>    	int num_vregs;

[skipped]

>>> @@ -2502,6 +2847,9 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np
>>>    			return PTR_ERR(qmp->rx2);
>>>    		qmp->pcs_misc = devm_of_iomap(dev, np, 5, NULL);
>>> +
>>> +		if (cfg->has_ln_shrd_serdes_tbl)
>>> +			qmp->ln_shrd_serdes = devm_of_iomap(dev, np, 6, NULL);
>>
>> I think we also need to check the returned value. Also, I think we can drop
>> the conditional check here. we don't have to validate the DT, so if the reg
>> is present in DT, then it's present. If not, it's not required.
> 
> Yeah, but I was trying to make sure that if there is a new type of reg
> for some upcomming SoC, we don't take that as ln_shrd. That's why the check
> was added.
> 
> I'll add the returned value check though.

This is the parse_dt_legacy() function. I think we can leave it as is 
and support sm8550 only using the new style of parsing.

> 
>>
>>>    	} else {
>>>    		qmp->pcs_misc = devm_of_iomap(dev, np, 3, NULL);
>>>    	}
>>> @@ -2729,6 +3077,12 @@ static const struct of_device_id qmp_pcie_of_match_table[] = {
>>>    	}, {
>>>    		.compatible = "qcom,sm8450-qmp-gen4x2-pcie-phy",
>>>    		.data = &sm8450_qmp_gen4x2_pciephy_cfg,
>>> +	}, {
>>> +		.compatible = "qcom,sm8550-qmp-gen3x2-pcie-phy",
>>> +		.data = &sm8550_qmp_gen3x2_pciephy_cfg,
>>> +	}, {
>>> +		.compatible = "qcom,sm8550-qmp-gen4x2-pcie-phy",
>>> +		.data = &sm8550_qmp_gen4x2_pciephy_cfg,
>>>    	},
>>>    	{ },
>>>    };
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry