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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YRuSnXHSZHhBC40r@matsya>
Date:   Tue, 17 Aug 2021 16:12:37 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>, linuxarm@...wei.com,
        mauro.chehab@...wei.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Rob Herring <robh@...nel.org>, linux-kernel@...r.kernel.org,
        linux-phy@...ts.infradead.org
Subject: 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