Re: [PATCH phy v2 2/6] phy: marvell: phy-mvebu-a3700-comphy: Add native kernel implementation

From: Vinod Koul <vkoul@kernel.org>
To: "Marek Behún" <kabel@kernel.org>
Cc: linux-phy@lists.infradead.org,
	Kishon Vijay Abraham I <kishon@ti.com>,
	pali@kernel.org, Miquel Raynal <miquel.raynal@bootlin.com>,
	Gregory CLEMENT <gregory.clement@bootlin.com>
Subject: Re: [PATCH phy v2 2/6] phy: marvell: phy-mvebu-a3700-comphy: Add native kernel implementation
Date: Thu, 23 Dec 2021 18:10:52 +0530	[thread overview]
Message-ID: <YcRuVCdXztTTrJs7@matsya> (raw)
In-Reply-To: <20211208024038.8797-3-kabel@kernel.org>

On 08-12-21, 03:40, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>

> +/* COMPHY registers */
> +#define COMPHY_POWER_PLL_CTRL		0x01
> +#define PU_IVREF_BIT			BIT(15)
> +#define PU_PLL_BIT			BIT(14)
> +#define PU_RX_BIT			BIT(13)
> +#define PU_TX_BIT			BIT(12)
> +#define PU_TX_INTP_BIT			BIT(11)
> +#define PU_DFE_BIT			BIT(10)
> +#define RESET_DTL_RX_BIT		BIT(9)
> +#define PLL_LOCK_BIT			BIT(8)
> +#define REF_FREF_SEL_MASK		GENMASK(4, 0)
> +#define REF_FREF_SEL_SERDES_25MHZ	(0x1 << 0)
> +#define REF_FREF_SEL_SERDES_40MHZ	(0x3 << 0)
> +#define REF_FREF_SEL_SERDES_50MHZ	(0x4 << 0)
> +#define REF_FREF_SEL_PCIE_USB3_25MHZ	(0x2 << 0)
> +#define REF_FREF_SEL_PCIE_USB3_40MHZ	(0x3 << 0)
> +#define COMPHY_MODE_MASK		GENMASK(7, 5)
> +#define COMPHY_MODE_SATA		(0x0 << 5)
> +#define COMPHY_MODE_PCIE		(0x3 << 5)
> +#define COMPHY_MODE_SERDES		(0x4 << 5)
> +#define COMPHY_MODE_USB3		(0x5 << 5)

Any reason why these are not using GENMASK. I guess documentation would
define these as BIT x-y right?

> +/* Changes to 40M1G25 mode data required for running 40M3G125 init mode */
> +static struct gbe_phy_init_data_fix gbe_phy_init_fix[] = {
> +	{0x005, 0x07CC}, {0x015, 0x0000}, {0x01B, 0x0000}, {0x01D, 0x0000},
> +	{0x01E, 0x0000}, {0x01F, 0x0000}, {0x020, 0x0000}, {0x021, 0x0030},
> +	{0x026, 0x0888}, {0x04D, 0x0152}, {0x04F, 0xA020}, {0x050, 0x07CC},
> +	{0x053, 0xE9CA}, {0x055, 0xBD97}, {0x071, 0x3015}, {0x076, 0x03AA},
> +	{0x07C, 0x0FDF}, {0x0C2, 0x3030}, {0x0C3, 0x8000}, {0x0E2, 0x5550},
> +	{0x0E3, 0x12A4}, {0x0E4, 0x7D00}, {0x0E6, 0x0C83}, {0x101, 0xFCC0},
> +	{0x104, 0x0C10}

empty space after { and before } makes it look neater and it typically
the convention followed at many places

> +/* PHY selector configures with corresponding modes */
> +static int
> +mvebu_a3700_comphy_set_phy_selector(struct mvebu_a3700_comphy_lane *lane)
> +{
> +	u32 old, new, clr = 0, set = 0;
> +	unsigned long flags;
> +
> +	switch (lane->mode) {
> +	case PHY_MODE_SATA:
> +		/* SATA must be in Lane2 */
> +		if (lane->id == 2)
> +			clr = COMPHY_SELECTOR_USB3_PHY_SEL_BIT;
> +		else
> +			goto error;
> +		break;
> +
> +	case PHY_MODE_ETHERNET:
> +		if (lane->id == 0)
> +			clr = COMPHY_SELECTOR_USB3_GBE1_SEL_BIT;
> +		else if (lane->id == 1)
> +			clr = COMPHY_SELECTOR_PCIE_GBE0_SEL_BIT;
> +		else
> +			goto error;
> +		break;
> +
> +	case PHY_MODE_USB_HOST_SS:
> +		if (lane->id == 2)
> +			set = COMPHY_SELECTOR_USB3_PHY_SEL_BIT;
> +		else if (lane->id == 0)
> +			set = COMPHY_SELECTOR_USB3_GBE1_SEL_BIT;
> +		else
> +			goto error;
> +		break;
> +
> +	case PHY_MODE_PCIE:
> +		/* PCIE must be in Lane1 */
> +		if (lane->id == 1)
> +			set = COMPHY_SELECTOR_PCIE_GBE0_SEL_BIT;
> +		else
> +			goto error;
> +		break;
> +
> +	default:
> +		goto error;
> +	}
> +
> +	raw_spin_lock_irqsave(&lane->priv->lock, flags);
> +
> +	old = readl(lane->priv->comphy_regs + COMPHY_SELECTOR_PHY_REG);
> +	new = (old & ~clr) | set;
> +	writel(new, lane->priv->comphy_regs + COMPHY_SELECTOR_PHY_REG);
> +
> +	raw_spin_unlock_irqrestore(&lane->priv->lock, flags);

any reason to use raw_ variant? why not spin_lock_irqsave()

-- 
~Vinod

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy