Re: [PATCH v11 01/11] phy: HiSilicon: Add driver for Kirin 970 PCIe PHY

On 12-08-21, 10:02, Mauro Carvalho Chehab wrote:

+static void hi3670_pcie_set_eyeparam(struct hi3670_pcie_phy *phy)
+{
+	u32 val;
+
+	val = kirin_apb_natural_phy_readl(phy,
+					  RAWLANEN_DIG_PCS_XF_TX_OVRD_IN_1);

Maybe use one line for this (hint: we can go beyong 80 now)

+static void hi3670_pcie_natural_cfg(struct hi3670_pcie_phy *phy)
+{
+	u32 val;
+
+	/* change 2p mem_ctrl */
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL20_ADDR,
+		     SOC_PCIECTRL_CTRL20_2P_MEM_CTRL);
+
+	regmap_read(phy->apb, SOC_PCIECTRL_CTRL7_ADDR, &val);
+	val |= PCIE_PULL_UP_SYS_AUX_PWR_DET;
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL7_ADDR, val);
+
+	/* output, pull down */
+	regmap_read(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, &val);
+	val &= ~PCIE_OUTPUT_PULL_BITS;
+	val |= PCIE_OUTPUT_PULL_DOWN;
+	regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val);
+
+	/* Handle phy_reset and lane0_reset to HW */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL1_ADDR);
+	val |= PCIEPHY_RESET_BIT;
+	val &= ~PCIEPHY_PIPE_LINE0_RESET_BIT;
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL1_ADDR);
+
+	/* fix chip bug: TxDetectRx fail */
+	val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_CTRL38_ADDR);
+	val |= PCIE_TXDETECT_RX_FAIL;
+	hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_CTRL38_ADDR);

maybe add a hi3670_apb_phy_updatel() so that above would become:

        hi3670_apb_phy_updatel(phy, val, mask);

+static int hi3670_pcie_pll_ctrl(struct hi3670_pcie_phy *phy, bool enable)
+{
+	struct device *dev = phy->dev;
+	u32 val;
+	int time = PLL_CTRL_WAIT_TIME;
+
+	if (enable) {
+		/* pd = 0 */
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_CTRL16);
+		val &= ~PCIE_PHY_MMC1PLL_DISABLE;
+		hi3670_apb_phy_writel(phy, val, SOC_PCIEPHY_MMC1PLL_CTRL16);
+
+		val = hi3670_apb_phy_readl(phy, SOC_PCIEPHY_MMC1PLL_STAT0);
+
+		/* choose FNPLL */
+		while (!(val & FNPLL_HAS_LOCKED)) {
+			if (!time) {
+				dev_err(dev, "wait for pll_lock timeout\n");
+				return -EINVAL;
+			}
+			time--;
+			udelay(1);
+			val = hi3670_apb_phy_readl(phy,
+						   SOC_PCIEPHY_MMC1PLL_STAT0);

single line here too

+static void hi3670_pcie_phyref_gt(struct hi3670_pcie_phy *phy, bool open)
+{
+	unsigned int val;
+
+	regmap_read(phy->crgctrl, CRGPERIPH_PCIECTRL0, &val);
+
+	if (open)
+		val &= ~IO_OE_HARD_GT_MODE; // enable hard gt mode
+	else
+		val |= IO_OE_HARD_GT_MODE; // disable hard gt mode

pls change the comment style here and above, we dont use c99 style!

+static int hi3670_pcie_phy_power_off(struct phy *generic_phy)
+{
+	struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy);
+
+	hi3670_pcie_phy_oe_enable(phy, false);
+
+	hi3670_pcie_allclk_ctrl(phy, false);
+
+	/* Drop power supply for Host */
+	regmap_write(phy->sysctrl, SCTRL_PCIE_CMOS_OFFSET, 0);
+
+	/* FIXME: calling it causes an Asynchronous SError interrupt */
+//	kirin_pcie_clk_ctrl(phy, false);

when will you fix the fixme and pls remove the deadcode
-- 
~Vinod

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