linux-kernel - Re: [PATCH v2 2/6] phy: qcom: Introduce PCIe UNIPHY 28LP 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: <Z1/JG+RkTBW9JuMO@hu-varada-blr.qualcomm.com>
Date: Mon, 16 Dec 2024 12:00:51 +0530
From: Varadarajan Narayanan <quic_varada@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
CC: <lpieralisi@...nel.org>, <kw@...ux.com>,
        <manivannan.sadhasivam@...aro.org>, <robh@...nel.org>,
        <bhelgaas@...gle.com>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <vkoul@...nel.org>, <kishon@...nel.org>, <andersson@...nel.org>,
        <konradybcio@...nel.org>, <p.zabel@...gutronix.de>,
        <quic_nsekar@...cinc.com>, <dmitry.baryshkov@...aro.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-phy@...ts.infradead.org>
Subject: Re: [PATCH v2 2/6] phy: qcom: Introduce PCIe UNIPHY 28LP driver

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@...cinc.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@...cinc.com>
> > Signed-off-by: Varadarajan Narayanan <quic_varada@...cinc.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