Re: [PATCH v2 2/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Varadarajan Narayanan <quic_varada@quicinc.com>]

On Thu, Dec 05, 2024 at 05:40:15PM +0100, Konrad Dybcio wrote:
> On 4.12.2024 12:33 PM, Varadarajan Narayanan wrote:
> > From: Nitheesh Sekar <quic_nsekar@quicinc.com>
> >
> > Add Qualcomm PCIe UNIPHY 28LP driver support present
> > in Qualcomm IPQ5332 SoC and the phy init sequence.
> >
> > Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com>
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
>
> [...]
>
> > +struct qcom_uniphy_pcie_regs {
> > +	unsigned int offset;
> > +	unsigned int val;
>
> u32

ok.

> > +};
> > +
> > +struct qcom_uniphy_pcie_data {
> > +	int lanes;
> > +	/* 2nd lane offset */
> > +	int lane_offset;
>
> 'lanes', 'lane_offset' and '2nd lane' together imply one of:
>
> - there can be more lines, all at an equal offset
> - there can only ever be two lines
>
> Please specify which one is the case

There can be more lines all at an equal offset. However, it is
just 2 lines in this SoC's case. Will remove the "2nd lane offset"
comment.

> > +	unsigned int phy_type;
> > +	const struct qcom_uniphy_pcie_regs *init_seq;
> > +	unsigned int init_seq_num;
> > +	unsigned int pipe_clk_rate;
> > +};
> > +
> > +struct qcom_uniphy_pcie {
> > +	struct phy phy;
> > +	struct device *dev;
> > +	const struct qcom_uniphy_pcie_data *data;
> > +	struct clk_bulk_data *clks;
> > +	int num_clks;
> > +	struct reset_control *resets;
> > +	void __iomem *base;
> > +};
> > +
> > +#define	phy_to_dw_phy(x)	container_of((x), struct qca_uni_pcie_phy, phy)
>
> A space after #define, please

ok

> > +
> > +static const struct qcom_uniphy_pcie_regs ipq5332_regs[] = {
> > +	{
> > +		.offset = PHY_CFG_PLLCFG,
> > +		.val = 0x30,
> > +	}, {
> > +		.offset = PHY_CFG_EIOS_DTCT_REG,
> > +		.val = 0x53ef,
> > +	}, {
> > +		.offset = PHY_CFG_GEN3_ALIGN_HOLDOFF_TIME,
> > +		.val = 0xCf,
>
> mixed case hex.. please make it lowercase

ok

> > +	},
> > +};
> > +
> > +static const struct qcom_uniphy_pcie_data ipq5332_x1_data = {
> > +	.lanes		= 1,
> > +	.phy_type	= PHY_TYPE_PCIE_GEN3,
> > +	.init_seq	= ipq5332_regs,
> > +	.init_seq_num	= ARRAY_SIZE(ipq5332_regs),
> > +	.pipe_clk_rate	= 250000000,
> > +};
> > +
> > +static const struct qcom_uniphy_pcie_data ipq5332_x2_data = {
> > +	.lanes		= 2,
> > +	.lane_offset	= 0x800,
> > +	.phy_type	= PHY_TYPE_PCIE_GEN3,
> > +	.init_seq	= ipq5332_regs,
> > +	.init_seq_num	= ARRAY_SIZE(ipq5332_regs),
> > +	.pipe_clk_rate	= 250000000,
> > +};
>
> Are there going to be more UNIPHY-equipped SoCs?

Not sure. Since this driver was initially posted for ipq5018 and
ipq5332 suport was added later, there are two sets of data one
for 5018 and one for 5332.

> > +static void qcom_uniphy_pcie_init(struct qcom_uniphy_pcie *phy)
> > +{
> > +	const struct qcom_uniphy_pcie_data *data = phy->data;
> > +	const struct qcom_uniphy_pcie_regs *init_seq;
> > +	void __iomem *base = phy->base;
> > +	int lane, i;
> > +
> > +	for (lane = 0; lane != data->lanes; lane++) {
>
> while effectively the same, < would be less eyebrow-raising

ok.

> > +		init_seq = data->init_seq;
> > +
> > +		for (i = 0; i < data->init_seq_num; i++, init_seq++)
> > +			writel(init_seq->val, base + init_seq->offset);
>
> writel(init_seq[i].val, ...)

ok.

> > +
> > +		base += data->lane_offset;
> > +	}
> > +}
> > +
> > +static int qcom_uniphy_pcie_power_off(struct phy *x)
> > +{
> > +	struct qcom_uniphy_pcie *phy = phy_get_drvdata(x);
> > +
> > +	clk_bulk_disable_unprepare(phy->num_clks, phy->clks);
> > +
> > +	reset_control_assert(phy->resets);
>
> This can fail, return it instead of zero

ok.

> [...]
>
> > +MODULE_LICENSE("Dual BSD/GPL");
>
> I think this is too vague, there are many BSD variants

Will change it to "GPL v2"

Thanks
Varada