From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-io1-f52.google.com (mail-io1-f52.google.com [209.85.166.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B6FF61898F8 for ; Tue, 18 Nov 2025 14:11:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.166.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763475119; cv=none; b=p037mwCDKOsGgvEOMmvXUQ1KBZ7NovNLGD4j+8NmY+5FgDmsYBLrhuH1tgKhJwXVgGuV6By7XrdixEasS+hxRzMs/Y/nogl4OqixIOht7XPwnwmoAmd+fhBkBWL5Dfed5WuoB0C2oO+BOMxwtLfnjiRq3R7y975Ud5Vf0rgnYho= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763475119; c=relaxed/simple; bh=Jx8fxT75dmMAjJJkYHCTKQsivov+RobrcMhVbSbrbSo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=rqm71XkZqpabqN2rCogTvYl38OUj3CFjUdoJetco/kiGc3C58SskOVH3oI/O0Lk1PV1MYbvvdVfg+ykF20oQLw6uaK1IN2334XgCAsWTbcJeTyvDfe+b0kkugmPLwg7+d9rLEyHjySe17bkQStQ9pVHG3+zflwnoHAytptoiNU0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=riscstar.com; spf=pass smtp.mailfrom=riscstar.com; dkim=pass (2048-bit key) header.d=riscstar-com.20230601.gappssmtp.com header.i=@riscstar-com.20230601.gappssmtp.com header.b=qBx3hKOr; arc=none smtp.client-ip=209.85.166.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=riscstar.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=riscstar.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=riscstar-com.20230601.gappssmtp.com header.i=@riscstar-com.20230601.gappssmtp.com header.b="qBx3hKOr" Received: by mail-io1-f52.google.com with SMTP id ca18e2360f4ac-9491ba846b2so29666039f.2 for ; Tue, 18 Nov 2025 06:11:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=riscstar-com.20230601.gappssmtp.com; s=20230601; t=1763475116; x=1764079916; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=XQOHp/GwI3fcgmlB0YvLeMTllnQu/TZutZS/btTjmOc=; b=qBx3hKOrH0stj2v9tfd4MHL22++tnJp0yCrcpLUkPujgPMGoe6j8PuwJ2fXcRdhyMf 8J/BB4QuiD1IpAiMs5G1W4bJ0ZqBncM7Ra8WQgkd1XWWrnLy+AjKsrbZG1G8QjWKUMQa dy9qz6Smxp9tpVDhYDX/GaCJg5IhFmGsPeK7yRdc6RVSM78Yhi7HmSTenJsico8b6WN6 Dvm7aeNuPXeNQY5ezJoTz55tpQJanzJmZ1ROS/Oajgoc7UkYzUo8g5Q1OQmMXsKeeyPm vlwE918iREaRx5w7Zj2IC1e5K006lpJCI+PDWernCtsQgQJEXP19BJ5Ov6Xv90eoOUBq t62A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763475116; x=1764079916; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=XQOHp/GwI3fcgmlB0YvLeMTllnQu/TZutZS/btTjmOc=; b=tJkxh10lFihvPffot7C62QXf9gsBINvOHU5/si3rVLUQeB7ZNoWp2YDxBprq+LX/rQ +OMta5YQqFmmzxi7QJ9et9z1E5YccY40mRGiotMqPAzUlupK4FhmucoSDiMKxjpqNgF/ 0NK9BY7s8kiihQ5p4LhCVMH429U92VPkL109M4JebttL1INmDWMuivsaYpp0qkKLODLq xwc2H6rUtr+M5EeDh251cVJmSjSGaTnhgLd37h2VjGprund0RI4HDKJOuBQgDbH16Arg xAZGMhrOMYfp+2/uvo8ZBzGFiPmk4R8FAo235MD7xzuO8/Vsz1TyOjV7ukju5JKfc5qO o+yw== X-Forwarded-Encrypted: i=1; AJvYcCWdybfh0DJZJf5Sq/6n4nKdbi7xj2FNvVkdfwdvCGJGj/snKhxQZHEiuCqs5zLlBywzU+Vp+DopSdNmtSM=@vger.kernel.org X-Gm-Message-State: AOJu0Yy6uunj0L2Rgo65nY5a9cW7PcIXu5YfbkR8TwVTqgnJc+dAA5ra 5VBVZwFzbivBPrqbRfgah927HwBrAR4LejTnSJx4nqAlBk46aMCl4YYiShQZg74Nfgc= X-Gm-Gg: ASbGncvYbkyjKnLHTuVbRzYcnc81lg+sQNUM1dOZfwmem4EGCy5Qeq8rSFEjQYInb9h msTS2Oy1bBOGaVhZkz0CXP9rpC2TzeDi6pLkvAussP8fns+J5MEUcd4amiAim7MYKF06xT3Zo/l eAnxxHg0D1MPD6tmw5GidBt8B6LzInKZiVOh6AztM9FJWp/hmjzz57ylnw1m/ulvUS4jUDLM03O FrKUovJ8UE4qT3QvxgwCu2MC2TtVUnnTfCmfWrwfGMQySMs8Ans12NCMCW0rsDD9nEkGIbY6dN3 AE6ko0cjkUwPxUv9yo6OCVBRh+VMItRhHWkXx8CNwSF9mL/epnHoH7BfZiZB9dZhm83NZctwo2w 2YY6qTy17vCCdOXm/b/4Pr1O3HDP8mlS8y8JHas7k2Ao6W0OVkjrki956xRKcWRgpHz24TzUUzh EJ6PEVCKgj5ueOerTTImlzpDCHYjiWq2gLZdqVzLkLlA0+akGRyg== X-Google-Smtp-Source: AGHT+IF3S+JfrYl1iTQg9Cf+cciD5dTfnTSo4c8+gQm0I9ddxsueA+d0+U+Ih0ju8axq16SoJtC3pA== X-Received: by 2002:a05:6638:2047:b0:573:15bb:830c with SMTP id 8926c6da1cb9f-5b7c9dd4a58mr12679050173.13.1763475115710; Tue, 18 Nov 2025 06:11:55 -0800 (PST) Received: from [172.22.22.28] (c-75-72-117-212.hsd1.mn.comcast.net. [75.72.117.212]) by smtp.gmail.com with ESMTPSA id 8926c6da1cb9f-5b7bd27fa4asm6008728173.26.2025.11.18.06.11.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Nov 2025 06:11:55 -0800 (PST) Message-ID: <21f0aa62-464f-4d8d-b8d7-5a4455179534@riscstar.com> Date: Tue, 18 Nov 2025 08:11:53 -0600 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 4/7] phy: spacemit: Introduce PCIe/combo PHY To: Neil Armstrong , vkoul@kernel.org, kishon@kernel.org Cc: p.zabel@pengutronix.de, dlan@gentoo.org, aurelien@aurel32.net, guodong@riscstar.com, linux-phy@lists.infradead.org, spacemit@lists.linux.dev, linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, Junzhong Pan References: <20251113214540.2623070-1-elder@riscstar.com> <20251113214540.2623070-5-elder@riscstar.com> Content-Language: en-US From: Alex Elder In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >> Signed-off-by: Alex Elder >> --- . . . >> 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +/* >> + * 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 Thank you very much for reviewing this Neil. -Alex > Thanks, > Neil >