Hi Angelo,
Thanks for all these suggestions, I'll fix them in the next version.
Thanks.
On Fri, 2022-03-18 at 11:57 +0100, AngeloGioacchino Del Regno wrote:
> Il 18/03/22 10:54, Jianjun Wang ha scritto:
> > Add PCIe GEN3 PHY driver support on MediaTek chipsets.
> >
> > Signed-off-by: Jianjun Wang <jianjun.wang@xxxxxxxxxxxx>
> > ---
> > drivers/phy/mediatek/Kconfig | 11 ++
> > drivers/phy/mediatek/Makefile | 1 +
> > drivers/phy/mediatek/phy-mtk-pcie.c | 246
> > ++++++++++++++++++++++++++++
> > 3 files changed, 258 insertions(+)
> > create mode 100644 drivers/phy/mediatek/phy-mtk-pcie.c
> >
> > diff --git a/drivers/phy/mediatek/Kconfig
> > b/drivers/phy/mediatek/Kconfig
> > index 55f8e6c048ab..387ed1b3f2cc 100644
> > --- a/drivers/phy/mediatek/Kconfig
> > +++ b/drivers/phy/mediatek/Kconfig
> > @@ -55,3 +55,14 @@ config PHY_MTK_MIPI_DSI
> > select GENERIC_PHY
> > help
> > Support MIPI DSI for Mediatek SoCs.
> > +
> > +config PHY_MTK_PCIE
> > + tristate "MediaTek PCIe-PHY Driver"
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + depends on OF
> > + select GENERIC_PHY
> > + help
> > + Say 'Y' here to add support for MediaTek PCIe PHY driver.
> > + This driver create the basic PHY instance and provides
> > initialize
> > + callback for PCIe GEN3 port, it supports software efuse
> > + initialization.
> > diff --git a/drivers/phy/mediatek/Makefile
> > b/drivers/phy/mediatek/Makefile
> > index ace660fbed3a..788c13147f63 100644
> > --- a/drivers/phy/mediatek/Makefile
> > +++ b/drivers/phy/mediatek/Makefile
> > @@ -6,6 +6,7 @@
> > obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o
> > obj-$(CONFIG_PHY_MTK_UFS) += phy-mtk-ufs.o
> > obj-$(CONFIG_PHY_MTK_XSPHY) += phy-mtk-xsphy.o
> > +obj-$(CONFIG_PHY_MTK_PCIE) += phy-mtk-pcie.o
> >
> > phy-mtk-hdmi-drv-y := phy-mtk-hdmi.o
> > phy-mtk-hdmi-drv-y += phy-mtk-hdmi-
> > mt2701.o
> > diff --git a/drivers/phy/mediatek/phy-mtk-pcie.c
> > b/drivers/phy/mediatek/phy-mtk-pcie.c
> > new file mode 100644
> > index 000000000000..0f5d7c7e2b7e
> > --- /dev/null
> > +++ b/drivers/phy/mediatek/phy-mtk-pcie.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Jianjun Wang <jianjun.wang@xxxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include "phy-mtk-io.h"
> > +
> > +#define PEXTP_ANA_GLB_00_REG 0x9000
> > +#define PEXTP_ANA_LN0_TRX_REG 0xa000
> > +#define PEXTP_ANA_TX_OFFSET 0x04
> > +#define PEXTP_ANA_RX_OFFSET 0x3c
> > +#define PEXTP_ANA_LANE_OFFSET 0x100
> > +
> > +/* PEXTP_GLB_00_REG[28:24] Internal Resistor Selection of TX Bias
> > Current */
> > +#define EFUSE_GLB_INTR_SEL GENMASK(28, 24)
> > +#define EFUSE_GLB_INTR_VAL(x) ((0x1f & (x)) << 24)
> > +
> > +/* PEXTP_ANA_LN_RX_REG[3:0] RX impedance selection */
> > +#define EFUSE_LN_RX_SEL GENMASK(3, 0)
> > +#define EFUSE_LN_RX_VAL(x) (0xf & (x))
> > +
> > +/* PEXTP_ANA_LN_TX_REG[5:2] TX PMOS impedance selection */
> > +#define EFUSE_LN_TX_PMOS_SEL GENMASK(5, 2)
> > +#define EFUSE_LN_TX_PMOS_VAL(x) ((0xf & (x)) << 2)
> > +
> > +/* PEXTP_ANA_LN_TX_REG[11:8] TX NMOS impedance selection */
> > +#define EFUSE_LN_TX_NMOS_SEL GENMASK(11, 8)
> > +#define EFUSE_LN_TX_NMOS_VAL(x) ((0xf & (x)) << 8)
> > +
> > +/* Efuse data for each lane */
>
> What about some kerneldoc?
>
> /**
> * struct mtk_pcie_lane_efuse - eFuse data for each lane
> * @tx_pmos:
> ......etc :))
>
> > +struct mtk_pcie_lane_efuse {
> > + u32 tx_pmos;
> > + u32 tx_nmos;
> > + u32 rx_data;
> > + bool lane_efuse_supported;
> > +};
> > +
>
> Same here
>
> /**
> * struct mtk_pcie_phy - PCIe phy driver main structure
> * @dev: ......
>
> > +struct mtk_pcie_phy {
> > + struct device *dev;
> > + struct phy *phy;
> > + void __iomem *sif_base;
> > +
> > + /*
> > + * Support software efuse initialization,
> > + * currently we only support 2 lane in maximum.
> > + */
>
> Obviously, if you add kerneldoc, this comment would get moved to that
> kerneldoc.
>
> > + bool sw_efuse_supported;
> > + u32 efuse_glb_intr;
>
>
> > + struct mtk_pcie_lane_efuse efuse[2];
>
> If you dynamically allocate this one, you will be able to support any
> number
> of lanes, futureproofing this driver and giving it more flexibility.
>
> > +};
> > +
>
> ..snip..
>
> > +
> > +static int mtk_pcie_efuse_read_for_lane(struct mtk_pcie_phy
> > *pcie_phy,
> > + unsigned int lane)
> > +{
> > + struct device *dev = pcie_phy->dev;
> > + struct mtk_pcie_lane_efuse *data;
> > + char efuse_id[15];
> > + int ret;
> > +
> > + if (lane >= ARRAY_SIZE(pcie_phy->efuse))
> > + return dev_err_probe(pcie_phy->dev, -EINVAL,
> > + "Requested lane number %d exceeds
> > maximum %ld\n",
> > + lane, ARRAY_SIZE(pcie_phy->efuse)
> > - 1);
>
> I don't like seeing dev_err_probe() outside of a probe function, but
> I acknowledge
> that the Linux documentation doesn't seem to give any direction about
> that, so
> this is a personal preference, at this point.
>
> > +
> > + data = &pcie_phy->efuse[lane];
> > +
> > + snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_pmos", lane);
> > + ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data-
> > >tx_pmos);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read %s\n",
> > efuse_id);
> > +
> > + snprintf(efuse_id, sizeof(efuse_id), "tx_ln%d_nmos", lane);
> > + ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data-
> > >tx_nmos);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read %s\n",
> > efuse_id);
> > +
> > + snprintf(efuse_id, sizeof(efuse_id), "rx_ln%d", lane);
> > + ret = nvmem_cell_read_variable_le_u32(dev, efuse_id, &data-
> > >rx_data);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read %s\n",
> > efuse_id);
> > +
> > + if (!(data->tx_pmos || data->tx_nmos || data->rx_data))
> > + return dev_err_probe(dev, -EINVAL,
> > + "No efuse data found for lane%d,
> > but dts enable it\n",
> > + lane);
> > +
> > + data->lane_efuse_supported = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_pcie_read_efuse(struct mtk_pcie_phy *pcie_phy)
> > +{
> > + struct device *dev = pcie_phy->dev;
> > + bool nvmem_enabled;
> > + int ret;
> > +
> > + nvmem_enabled = device_property_read_bool(dev, "nvmem-cells");
> > + if (!nvmem_enabled)
> > + return -ENODEV;
> > +
> > + ret = nvmem_cell_read_variable_le_u32(dev, "glb_intr",
> > + &pcie_phy-
> > >efuse_glb_intr);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to read
> > glb_intr\n");
> > +
> > + pcie_phy->sw_efuse_supported = true;
> > +
> > + ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 0);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mtk_pcie_efuse_read_for_lane(pcie_phy, 1);
> > + if (ret)
> > + return ret;
>
> To give some more future-proofing to this driver, I would instead
> either add a
> u32 devicetree property "num-lanes" or, if the same SoC may not have
> a different
> number of lanes across controller instances, I would add a number of
> lanes
> parameter as data for each of_match.
>
> You'd be at that point using a for loop here like:
>
> for (i = 0; i < pcie_phy->num_lanes, i++) {
> ret = mtk_pcie_efuse_read_for_lane(pcie_phy, i);
> if (ret)
> return ret;
> }
>
> Of course, the same logic would apply to mtk_pcie_phy_init(), where
> you are
> instead calling mtk_pcie_efuse_set_lane().
>
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_pcie_phy_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct phy_provider *provider;
> > + struct mtk_pcie_phy *pcie_phy;
> > +
> > + pcie_phy = devm_kzalloc(dev, sizeof(*pcie_phy), GFP_KERNEL);
> > + if (!pcie_phy)
> > + return -ENOMEM;
> > +
> > + pcie_phy->dev = dev;
> > +
> > + pcie_phy->sif_base =
> > devm_platform_ioremap_resource_byname(pdev, "sif");
> > + if (IS_ERR(pcie_phy->sif_base))
> > + return dev_err_probe(dev, PTR_ERR(pcie_phy->sif_base),
> > + "Failed to map phy-sif base\n");
> > +
> > + pcie_phy->phy = devm_phy_create(dev, dev->of_node,
> > &mtk_pcie_phy_ops);
> > + if (IS_ERR(pcie_phy->phy))
> > + return dev_err_probe(dev, PTR_ERR(pcie_phy->phy),
> > + "Failed to create PCIe phy\n");
> > +
> > + /*
> > + * Failed to read the efuse data is not a fatal problem,
> > + * ignore the failure and keep going.
> > + */
> > + mtk_pcie_read_efuse(pcie_phy);
>
> If you get an -EPROBE_DEFER here, you surely want to defer probing
> this driver,
> so, yes you're free to ignore the other failures, but you should fix
> that corner
> case.
>
> Everything else looks good, so, please make sure to add me to the
> Cc's for the
> next version of this series for me to give you a faster review.
>
> Regards,
> Angelo
>
