Message-ID: <21f0aa62-464f-4d8d-b8d7-5a4455179534@riscstar.com>
Date: Tue, 18 Nov 2025 08:11:53 -0600
From: Alex Elder <elder@...cstar.com>
To: Neil Armstrong <neil.armstrong@...aro.org>, vkoul@...nel.org,
 kishon@...nel.org
Cc: p.zabel@...gutronix.de, dlan@...too.org, aurelien@...el32.net,
 guodong@...cstar.com, linux-phy@...ts.infradead.org,
 spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org, Junzhong Pan <panjunzhong@...ux.spacemit.com>
Subject: Re: [PATCH v6 4/7] phy: spacemit: Introduce PCIe/combo PHY

On 11/18/25 2:56 AM, Neil Armstrong wrote:
> On 11/13/25 22:45, Alex Elder wrote:
>> Introduce a driver that supports three PHYs found on the SpacemiT
>> K1 SoC.  The first PHY is a combo PHY that can be configured for
>> use for either USB 3 or PCIe.  The other two PHYs support PCIe
>> only.
>>
>> All three PHYs must be programmed with an 8 bit receiver termination
>> value, which must be determined dynamically.  Only the combo PHY is
>> able to determine this value.  The combo PHY performs a special
>> calibration step at probe time to discover this, and that value is
>> used to program each PHY that operates in PCIe mode.  The combo
>> PHY must therefore be probed before either of the PCIe-only PHYs
>> will be used.
>>
>> Each PHY has an internal PLL driven from an external oscillator.
>> This PLL started when the PHY is first initialized, and stays
>> on thereafter.
>>
>> During normal operation, the USB or PCIe driver using the PHY must
>> ensure (other) clocks and resets are set up properly.
>>
>> However PCIe mode clocks are enabled and resets are de-asserted
>> temporarily by this driver to perform the calibration step on the
>> combo PHY.
>>
>> Tested-by: Junzhong Pan <panjunzhong@...ux.spacemit.com>
>> Signed-off-by: Alex Elder <elder@...cstar.com>
>> ---

. . .

>> diff --git a/drivers/phy/phy-spacemit-k1-pcie.c b/drivers/phy/phy- 
>> spacemit-k1-pcie.c
>> new file mode 100644
>> index 0000000000000..75477bea7f700
>> --- /dev/null
>> +++ b/drivers/phy/phy-spacemit-k1-pcie.c
>> @@ -0,0 +1,670 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SpacemiT K1 PCIe and PCIe/USB 3 combo PHY driver
>> + *
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights 
>> reserved.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#include <dt-bindings/phy/phy.h>
>> +
>> +/*
>> + * Three PCIe ports are supported in the SpacemiT K1 SoC, and this 
>> driver
>> + * supports their PHYs.
>> + *
>> + * The PHY for PCIe port A is different from the PHYs for ports B and C:
>> + * - It has one lane, while ports B and C have two
>> + * - It is a combo PHY can be used for PCIe or USB 3
>> + * - It can automatically calibrate PCIe TX and RX termination settings
>> + *
>> + * The PHY functionality for PCIe ports B and C is identical:
>> + * - They have two PCIe lanes (but can be restricted to 1 via device 
>> tree)
>> + * - They are used for PCIe only
>> + * - They are configured using TX and RX values computed for port A
>> + *
>> + * A given board is designed to use the combo PHY for either PCIe or 
>> USB 3.
>> + * Whether the combo PHY is configured for PCIe or USB 3 is specified in
>> + * device tree using a phandle plus an argument.  The argument indicates
>> + * the type (either PHY_TYPE_PCIE or PHY_TYPE_USB3).
>> + *
>> + * Each PHY has a reset that it gets and deasserts during 
>> initialization.
>> + * Each depends also on other clocks and resets provided by the 
>> controller
>> + * hardware (PCIe or USB) it is associated with.  The controller drivers
>> + * are required to enable any clocks and de-assert any resets that 
>> affect
>> + * PHY operation.  In addition each PHY implements an internal PLL, 
>> driven
>> + * by an external (24 MHz) oscillator.
>> + *
>> + * PCIe PHYs must be programmed with RX and TX calibration values.  The
>> + * combo PHY is the only one that can determine these values.  They are
>> + * determined by temporarily enabling the combo PHY in PCIe mode at 
>> probe
>> + * time (if necessary).  This calibration only needs to be done once, 
>> and
>> + * when it has completed the TX and RX values are saved.
>> + *
>> + * To allow the combo PHY to be enabled for calibration, the resets and
>> + * clocks it uses in PCIe mode must be supplied.
>> + */
>> +
>> +struct k1_pcie_phy {
>> +    struct device *dev;        /* PHY provider device */
>> +    struct phy *phy;
>> +    void __iomem *regs;
>> +    u32 pcie_lanes;            /* Max (1 or 2) unless limited by DT */
>> +    struct clk *pll;
>> +    struct clk_hw pll_hw;        /* Private PLL clock */
>> +
>> +    /* The remaining fields are only used for the combo PHY */
>> +    u32 type;            /* PHY_TYPE_PCIE or PHY_TYPE_USB3 */
>> +    struct regmap *pmu;        /* MMIO regmap (no errors) */
>> +};
>> +
>> +#define CALIBRATION_TIMEOUT        500000    /* For combo PHY (usec) */
>> +#define PLL_TIMEOUT            500000    /* For PHY PLL lock (usec) */
>> +#define POLL_DELAY            500    /* Time between polls (usec) */
>> +
>> +/* Selecting the combo PHY operating mode requires APMU regmap access */
>> +#define SYSCON_APMU            "spacemit,apmu"
>> +
>> +/* PMU space, for selecting between PCIe and USB 3 mode (combo PHY 
>> only) */
>> +
>> +#define PMUA_USB_PHY_CTRL0            0x0110
>> +#define COMBO_PHY_SEL            BIT(3)    /* 0: PCIe; 1: USB 3 */
>> +
>> +#define PCIE_CLK_RES_CTRL            0x03cc
>> +#define PCIE_APP_HOLD_PHY_RST        BIT(30)
>> +
>> +/* PHY register space */
>> +
>> +/* Offset between lane 0 and lane 1 registers when there are two */
>> +#define PHY_LANE_OFFSET                0x0400
>> +
>> +/* PHY PLL configuration */
>> +#define PCIE_PU_ADDR_CLK_CFG            0x0008
>> +#define PLL_READY            BIT(0)        /* read-only */
>> +#define CFG_INTERNAL_TIMER_ADJ        GENMASK(10, 7)
>> +#define TIMER_ADJ_USB        0x2
>> +#define TIMER_ADJ_PCIE        0x6
>> +#define CFG_SW_PHY_INIT_DONE        BIT(11)    /* We set after PLL 
>> config */
>> +
>> +#define PCIE_RC_DONE_STATUS            0x0018
>> +#define CFG_FORCE_RCV_RETRY        BIT(10)        /* Used for PCIe */
>> +
>> +/* PCIe PHY lane calibration; assumes 24MHz input clock */
>> +#define PCIE_RC_CAL_REG2            0x0020
>> +#define RC_CAL_TOGGLE            BIT(22)
>> +#define CLKSEL                GENMASK(31, 29)
>> +#define CLKSEL_24M        0x3
>> +
>> +/* Additional PHY PLL configuration (USB 3 and PCIe) */
>> +#define PCIE_PU_PLL_1                0x0048
>> +#define REF_100_WSSC            BIT(12)    /* 1: input is 100MHz, SSC */
>> +#define FREF_SEL            GENMASK(15, 13)
>> +#define FREF_24M        0x1
>> +#define SSC_DEP_SEL            GENMASK(19, 16)
>> +#define SSC_DEP_NONE        0x0
>> +#define SSC_DEP_5000PPM        0xa
>> +
>> +/* PCIe PHY configuration */
>> +#define PCIE_PU_PLL_2                0x004c
>> +#define GEN_REF100            BIT(4)    /* 1: generate 100MHz clk */
>> +
>> +#define PCIE_RX_REG1                0x0050
>> +#define EN_RTERM            BIT(3)
>> +#define AFE_RTERM_REG            GENMASK(11, 8)
>> +
>> +#define PCIE_RX_REG2                0x0054
>> +#define RX_RTERM_SEL            BIT(5)    /* 0: use AFE_RTERM_REG 
>> value */
>> +
>> +#define PCIE_LTSSM_DIS_ENTRY            0x005c
>> +#define CFG_REFCLK_MODE            GENMASK(9, 8)
>> +#define RFCLK_MODE_DRIVER    0x1
>> +#define OVRD_REFCLK_MODE        BIT(10)    /* 1: use CFG_RFCLK_MODE */
>> +
>> +#define PCIE_TX_REG1                0x0064
>> +#define TX_RTERM_REG            GENMASK(15, 12)
>> +#define TX_RTERM_SEL            BIT(25)    /* 1: use TX_RTERM_REG */
>> +
>> +/* Zeroed for the combo PHY operating in USB mode */
>> +#define USB3_TEST_CTRL                0x0068
>> +
>> +/* PHY calibration values, determined by the combo PHY at probe time */
>> +#define PCIE_RCAL_RESULT            0x0084    /* Port A PHY only */
>> +#define RTERM_VALUE_RX            GENMASK(3, 0)
>> +#define RTERM_VALUE_TX            GENMASK(7, 4)
>> +#define R_TUNE_DONE            BIT(10)
>> +
>> +static u32 k1_phy_rterm = ~0;     /* Invalid initial value */
> 
> This global variable would have deserved a comment explaining
> why the value is global, instead the reason is only present in the
> commit message....

The (somewhat strange) overall scheme is described in a comment
block at the top of the file.  But I think you're right, a short
comment could at least refer back to that, and say "this is
where the calibration value is held" or something.

>> +/* Save the RX and TX receiver termination values */
>> +static void k1_phy_rterm_set(u32 val)
>> +{
>> +    k1_phy_rterm = val & (RTERM_VALUE_RX | RTERM_VALUE_TX);
>> +}
>> +
>> +static bool k1_phy_rterm_valid(void)
>> +{
>> +    /* Valid if no bits outside those we care about are set */
>> +    return !(k1_phy_rterm & ~(RTERM_VALUE_RX | RTERM_VALUE_TX));
>> +}

. . .

>> +module_platform_driver(k1_pcie_phy_driver);
>> +
>> +MODULE_DESCRIPTION("SpacemiT K1 PCIe and USB 3 PHY driver");
>> +MODULE_LICENSE("GPL");
> 
> The code is quite hard to read, with pleny of useless comments
> when the actual comments we needs are either in the wrong place
> or just missing.

I think the way this calibration works is unusual and a bit
confusing, and I might have gone too far trying to explain it.

It would be more helpful for you to provide examples of what
comments you think are useless, and where there are things that
would benefit from better explanation (as you did for the lack
of a comment for the k1_phy_rterm value).  I want very much to
make things better, but your statement offers no guidance on
what you're looking for.

> Anyway, no big comment on the code or the API usage, all looks good:
> 
> Reviewed-by: Neil Armstrong <neil.armstrong@...aro.org>

Thank you very much for reviewing this Neil.

					-Alex

> Thanks,
> Neil
> 

Powered by blists - more mailing lists