On Fri, Jun 10, 2022 at 11:57:05AM +0300, Serge Semin wrote:
> Baikal-T1 SoC is equipped with DWC PCIe v4.60a host controller. It can be
> trained to work up to Gen.3 speed over up to x4 lanes. The host controller
> is attached to the DW PCIe 3.0 PCS via the PIPE-4 interface, which in its
> turn is connected to the DWC 10G PHY. The whole system is supposed to be
> fed up with four clock sources: DBI peripheral clock, AXI application
> clocks and external PHY/core reference clock generating the 100MHz signal.
> In addition to that the platform provide a way to reset each part of the
> controller: sticky/non-sticky bits, host controller core, PIPE interface,
> PCS/PHY and Hot/Power reset signal. The driver also provides a way to
> handle the GPIO-based PERST# signal.
>
> Note due to the Baikal-T1 MMIO peculiarity we have to implement the DBI
> interface accessors which make sure the IO operations are dword-aligned.
>
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> +static int bt1_pcie_start_ltssm(struct dw_pcie *pci)
> +{
> + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> + u32 val;
> + int ret;
> +
> + /*
> + * Enable LTSSM and make sure it was able to establish both PHY and
> + * data links. This procedure shall work fine to reach 2.5 GT/s speed.
> + */
> + regmap_update_bits(btpci->sys_regs, BT1_CCU_PCIE_GENC,
> + BT1_CCU_PCIE_LTSSM_EN, BT1_CCU_PCIE_LTSSM_EN);
> +
> + ret = regmap_read_poll_timeout(btpci->sys_regs, BT1_CCU_PCIE_PMSC, val,
> + (val & BT1_CCU_PCIE_SMLH_LINKUP),
> + 1000, 1000000);
> + if (ret) {
> + dev_err(pci->dev, "LTSSM failed to set PHY link up\n");
> + return ret;
> + }
> +
> + ret = regmap_read_poll_timeout(btpci->sys_regs, BT1_CCU_PCIE_PMSC, val,
> + (val & BT1_CCU_PCIE_RDLH_LINKUP),
> + 1000, 1000000);
> + if (ret) {
> + dev_err(pci->dev, "LTSSM failed to set data link up\n");
> + return ret;
> + }
> +
> + /*
> + * Activate direct speed change after the link is established in an
> + * attempt to reach a higher bus performance (up to Gen.3 - 8.0 GT/s).
> + * This is required at least to get 8.0 GT/s speed.
> + */
> + val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> + val |= PORT_LOGIC_SPEED_CHANGE;
> + dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> +
> + ret = regmap_read_poll_timeout(btpci->sys_regs, BT1_CCU_PCIE_PMSC, val,
> + BT1_CCU_PCIE_LTSSM_LINKUP(val),
> + 1000, 1000000);
> + if (ret)
> + dev_err(pci->dev, "LTSSM failed to get into L0 state\n");
> +
> + return ret;
> +}
> +
> +static void bt1_pcie_stop_ltssm(struct dw_pcie *pci)
> +{
> + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> +
> + regmap_update_bits(btpci->sys_regs, BT1_CCU_PCIE_GENC,
> + BT1_CCU_PCIE_LTSSM_EN, 0);
> +}
> +
> +struct dw_pcie_ops bt1_pcie_dw_ops = {
> + .read_dbi = bt1_pcie_read_dbi,
> + .write_dbi = bt1_pcie_write_dbi,
> + .write_dbi2 = bt1_pcie_write_dbi2,
> + .start_link = bt1_pcie_start_ltssm,
> + .stop_link = bt1_pcie_stop_ltssm,
> +};
Should be static and const. Please rename to "dw_pcie_ops" as most
drivers use. Please rename bt1_pcie_start_ltssm() and
bt1_pcie_stop_ltssm() to bt1_pcie_start_link() and
bt1_pcie_stop_link() for consistency with other drivers to make
maintenance easier.
> +static struct pci_ops bt1_pcie_ops = {
> + .map_bus = dw_pcie_own_conf_map_bus,
> + .read = pci_generic_config_read32,
> + .write = pci_generic_config_write32,
> +};
> +
> +static int bt1_pcie_get_res(struct bt1_pcie *btpci)
Can you name this something similar to what other drivers use? There
are a couple *_pcie_get_resources() functions (normally called from
*_pcie_probe()), but no *_get_res() yet.
> +{
> + struct device *dev = btpci->dw.dev;
> + int i, ret;
> +
> + /* DBI access is supposed to be performed by the dword-aligned IOs */
> + btpci->dw.pp.bridge->ops = &bt1_pcie_ops;
> +
> + /* AXI-interface is configured with 64-bit address bus width */
> + ret = dma_coerce_mask_and_coherent(&btpci->dw.pp.bridge->dev,
> + DMA_BIT_MASK(64));
Just to double-check since this is the first instance of
dma_coerce_mask_and_coherent() in drivers/pci -- I guess Baikal-T1 is
unique in needing this?
> + if (ret) {
> + ret = dma_set_mask_and_coherent(&btpci->dw.pp.bridge->dev,
> + DMA_BIT_MASK(32));
Also the first instance of dma_set_mask_and_coherent() in dwc-based
drivers, so double-checking here, too.
> + if (ret)
> + return ret;
> + }
> +
> + /* These CSRs are in MMIO so we won't check the regmap-methods status */
> + btpci->sys_regs =
> + syscon_regmap_lookup_by_phandle(dev->of_node, "baikal,bt1-syscon");
> + if (IS_ERR(btpci->sys_regs))
> + return dev_err_probe(dev, PTR_ERR(btpci->sys_regs),
> + "Failed to get syscon\n");
> +
> + /* Make sure all the required resources have been specified */
> + for (i = 0; i < BT1_PCIE_NUM_APP_CLKS; i++) {
> + if (!btpci->dw.app_clks[bt1_pcie_app_clks[i]].clk) {
> + dev_err(dev, "App clocks set is incomplete\n");
> + return -ENOENT;
> + }
> + }
> +
> + for (i = 0; i < BT1_PCIE_NUM_CORE_CLKS; i++) {
> + if (!btpci->dw.core_clks[bt1_pcie_core_clks[i]].clk) {
> + dev_err(dev, "Core clocks set is incomplete\n");
> + return -ENOENT;
> + }
> + }
> +
> + for (i = 0; i < BT1_PCIE_NUM_APP_RSTS; i++) {
> + if (!btpci->dw.app_rsts[bt1_pcie_app_rsts[i]].rstc) {
> + dev_err(dev, "App resets set is incomplete\n");
> + return -ENOENT;
> + }
> + }
> +
> + for (i = 0; i < BT1_PCIE_NUM_CORE_RSTS; i++) {
> + if (!btpci->dw.core_rsts[bt1_pcie_core_rsts[i]].rstc) {
> + dev_err(dev, "Core resets set is incomplete\n");
> + return -ENOENT;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void bt1_pcie_full_stop_bus(struct bt1_pcie *btpci, bool init)
Can you name this something similar to what other drivers use?
> +{
> + struct device *dev = btpci->dw.dev;
> + struct dw_pcie *pci = &btpci->dw;
> + int ret;
> +
> + /* Disable LTSSM for sure */
> + regmap_update_bits(btpci->sys_regs, BT1_CCU_PCIE_GENC,
> + BT1_CCU_PCIE_LTSSM_EN, 0);
> +
> + /*
> + * Application reset controls are trigger-based so de-assert the core
> + * resets only.
> + */
> + ret = reset_control_bulk_assert(DW_PCIE_NUM_CORE_RSTS, pci->core_rsts);
> + if (ret)
> + dev_err(dev, "Failed to assert core resets\n");
> +
> + /*
> + * Clocks are disabled by default at least in accordance with the clk
> + * enable counter value on init stage.
> + */
> + if (!init) {
> + clk_bulk_disable_unprepare(DW_PCIE_NUM_CORE_CLKS, pci->core_clks);
> +
> + clk_bulk_disable_unprepare(DW_PCIE_NUM_APP_CLKS, pci->app_clks);
> + }
> +
> + /* The peripheral devices are unavailable anyway so reset them too */
> + gpiod_set_value_cansleep(pci->pe_rst, 1);
> +
> + /* Make sure the reset is settled */
> + usleep_range(1, 10);
Is this duration related to something in the PCIe spec? Or the DWC
spec? I'd really like to use named constants when possible, although
we have a ton of bare magic numbers currently.
Similar for the poll timeouts and the "state settled" sleep below.
> +}
> +
> +/*
> + * Implements the cold reset procedure in accordance with the reference manual
> + * and available PM signals.
> + */
> +static int bt1_pcie_cold_start_bus(struct bt1_pcie *btpci)
> +{
> + struct device *dev = btpci->dw.dev;
> + struct dw_pcie *pci = &btpci->dw;
> + u32 val;
> + int ret;
> +
> + /* First get out of the Power/Hot reset state */
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_PWR_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert PHY reset\n");
> + return ret;
> + }
> +
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_HOT_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert hot reset\n");
> + goto err_assert_pwr_rst;
> + }
> +
> + /* Wait for the PM-core to stop requesting the PHY reset */
> + ret = regmap_read_poll_timeout(btpci->sys_regs, BT1_CCU_PCIE_RSTC, val,
> + !(val & BT1_CCU_PCIE_REQ_PHY_RST), 1, 1000);
> + if (ret) {
> + dev_err(dev, "Timed out waiting for PM to stop PHY resetting\n");
> + goto err_assert_hot_rst;
> + }
> +
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_PHY_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert PHY reset\n");
> + goto err_assert_hot_rst;
> + }
> +
> + /* Clocks can be now enabled, but the ref one is crucial at this stage */
> + ret = clk_bulk_prepare_enable(DW_PCIE_NUM_APP_CLKS, pci->app_clks);
> + if (ret) {
> + dev_err(dev, "Failed to enable app clocks\n");
> + goto err_assert_phy_rst;
> + }
> +
> + ret = clk_bulk_prepare_enable(DW_PCIE_NUM_CORE_CLKS, pci->core_clks);
> + if (ret) {
> + dev_err(dev, "Failed to enable ref clocks\n");
> + goto err_disable_app_clk;
> + }
> +
> + /* Wait for the PM to stop requesting the controller core reset */
> + ret = regmap_read_poll_timeout(btpci->sys_regs, BT1_CCU_PCIE_RSTC, val,
> + !(val & BT1_CCU_PCIE_REQ_CORE_RST), 1, 1000);
> + if (ret) {
> + dev_err(dev, "Timed out waiting for PM to stop core resetting\n");
> + goto err_disable_core_clk;
> + }
> +
> + /* PCS-PIPE interface and controller core can be now activated */
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_PIPE_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert PIPE reset\n");
> + goto err_disable_core_clk;
> + }
> +
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_CORE_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert core reset\n");
> + goto err_assert_pipe_rst;
> + }
> +
> + /* It's recommended to reset the core and application logic together */
> + ret = reset_control_bulk_reset(DW_PCIE_NUM_APP_RSTS, pci->app_rsts);
> + if (ret) {
> + dev_err(dev, "Failed to reset app domain\n");
> + goto err_assert_core_rst;
> + }
> +
> + /* Sticky/Non-sticky CSR flags can be now unreset too */
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_STICKY_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert sticky reset\n");
> + goto err_assert_core_rst;
> + }
> +
> + ret = reset_control_deassert(pci->core_rsts[DW_PCIE_NON_STICKY_RST].rstc);
> + if (ret) {
> + dev_err(dev, "Failed to deassert non-sticky reset\n");
> + goto err_assert_sticky_rst;
> + }
> +
> + /* Activate the PCIe bus peripheral devices */
> + gpiod_set_value_cansleep(pci->pe_rst, 0);
> +
> + /* Make sure the state is settled (LTSSM is still disabled though) */
> + usleep_range(1, 10);
> +
> + return 0;
> +
> +err_assert_sticky_rst:
> + reset_control_assert(pci->core_rsts[DW_PCIE_STICKY_RST].rstc);
> +
> +err_assert_core_rst:
> + reset_control_assert(pci->core_rsts[DW_PCIE_CORE_RST].rstc);
> +
> +err_assert_pipe_rst:
> + reset_control_assert(pci->core_rsts[DW_PCIE_PIPE_RST].rstc);
> +
> +err_disable_core_clk:
> + clk_bulk_disable_unprepare(DW_PCIE_NUM_CORE_CLKS, pci->core_clks);
> +
> +err_disable_app_clk:
> + clk_bulk_disable_unprepare(DW_PCIE_NUM_APP_CLKS, pci->app_clks);
> +
> +err_assert_phy_rst:
> + reset_control_assert(pci->core_rsts[DW_PCIE_PHY_RST].rstc);
> +
> +err_assert_hot_rst:
> + reset_control_assert(pci->core_rsts[DW_PCIE_HOT_RST].rstc);
> +
> +err_assert_pwr_rst:
> + reset_control_assert(pci->core_rsts[DW_PCIE_PWR_RST].rstc);
> +
> + return ret;
> +}
> +
> +static int bt1_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> + int ret;
> +
> + ret = bt1_pcie_get_res(btpci);
> + if (ret)
> + return ret;
> +
> + bt1_pcie_full_stop_bus(btpci, true);
> +
> + return bt1_pcie_cold_start_bus(btpci);
Generally I think the get_res-type stuff happens elsewhere. I'm not
an expert in that, but this doesn't look much like other
*_pcie_host_init() functions, which mainly deal with enabling clocks,
reset assertion/deassertion, PHY init, interrupt enable, etc.
Maybe this is connected with your new common clocks/resets properties.
I'm certainly in favor of making as much of that common as is
practical! I hope we can take advantage of that and make more
consistency across the dwc-based drivers as well.
> +}
> +
> +static void bt1_pcie_host_deinit(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct bt1_pcie *btpci = to_bt1_pcie(pci);
> +
> + bt1_pcie_full_stop_bus(btpci, false);
> +}
> +
> +struct dw_pcie_host_ops bt1_pcie_host_ops = {
> + .host_init = bt1_pcie_host_init,
> + .host_deinit = bt1_pcie_host_deinit,
> +};
> +
> +static struct bt1_pcie *bt1_pcie_create_data(struct platform_device *pdev)
> +{
> + struct bt1_pcie *btpci;
> +
> + btpci = devm_kzalloc(&pdev->dev, sizeof(*btpci), GFP_KERNEL);
> + if (!btpci)
> + return ERR_PTR(-ENOMEM);
> +
> + btpci->pdev = pdev;
> +
> + platform_set_drvdata(pdev, btpci);
> +
> + return btpci;
I don't think it's worth splitting this into a separate function. I
think it would be better to use the same structure as other dwc-based
drivers and keep this in bt1_pcie_probe().
> +}
> +
> +static int bt1_pcie_add_dw_port(struct bt1_pcie *btpci)
All other dwc-based drivers call dw_pcie_host_init() from either
*_pcie_probe() or *_add_pcie_port(). Please use a similar convention.
> +{
> + struct device *dev = &btpci->pdev->dev;
> + int ret;
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
Why do you need this when no other dwc-based drivers do? Is Baikal-T1
different in this respect?
> + if (ret)
> + return ret;
> +
> + btpci->dw.version = DW_PCIE_VER_460A;
> + btpci->dw.dev = dev;
> + btpci->dw.ops = &bt1_pcie_dw_ops;
> +
> + btpci->dw.pp.num_vectors = MAX_MSI_IRQS;
> + btpci->dw.pp.ops = &bt1_pcie_host_ops;
> +
> + dw_pcie_cap_set(&btpci->dw, REQ_RES);
> +
> + ret = dw_pcie_host_init(&btpci->dw.pp);
> + if (ret)
> + dev_err_probe(dev, ret, "Failed to initialize DWC PCIe host\n");
> +
> + return ret;
> +}
> +
> +static void bt1_pcie_del_dw_port(struct bt1_pcie *btpci)
Can you call dw_pcie_host_deinit() from the same place as other
drivers?
$ git grep -p dw_pcie_host_deinit drivers/pci/controller/dwc
> +{
> + dw_pcie_host_deinit(&btpci->dw.pp);
> +}
> +
> +static int bt1_pcie_probe(struct platform_device *pdev)
> +{
> + struct bt1_pcie *btpci;
> +
> + btpci = bt1_pcie_create_data(pdev);
> + if (IS_ERR(btpci))
> + return PTR_ERR(btpci);
> +
> + return bt1_pcie_add_dw_port(btpci);
> +}
> +
> +static int bt1_pcie_remove(struct platform_device *pdev)
> +{
> + struct bt1_pcie *btpci = platform_get_drvdata(pdev);
> +
> + bt1_pcie_del_dw_port(btpci);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id bt1_pcie_of_match[] = {
> + { .compatible = "baikal,bt1-pcie" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, bt1_pcie_of_match);
> +
> +static struct platform_driver bt1_pcie_driver = {
> + .probe = bt1_pcie_probe,
> + .remove = bt1_pcie_remove,
> + .driver = {
> + .name = "bt1-pcie",
> + .of_match_table = bt1_pcie_of_match,
> + },
> +};
> +module_platform_driver(bt1_pcie_driver);
> +
> +MODULE_AUTHOR("Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Baikal-T1 PCIe driver");
> +MODULE_LICENSE("GPL");
> --
> 2.35.1
>
