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

Konrad Dybcio
2024-12-05T16:40:22Z

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

> +};
> +
> +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

> + 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

> +
> +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

> + },
> +};
> +
> +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?

> +
> +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

> + 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, ...)

> +
> + 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

[...]

> +MODULE_LICENSE("Dual BSD/GPL");

I think this is too vague, there are many BSD variants

Konrad