On Mon, Oct 07, 2024 at 01:12:12PM +0900, Damien Le Moal wrote:
> Introduce the function rockchip_pcie_ep_get_resources() to parse the DT
> node of a rockchip PCIe endpoint controller and allocate the outbound
> memory region and memory needed for IRQ handling. This function tidies
> up rockchip_pcie_ep_probe(). No functional change.
>
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
> drivers/pci/controller/pcie-rockchip-ep.c | 109 ++++++++++++----------
> 1 file changed, 62 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
> index a9b319d4e507..523e9cdfd241 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -524,15 +524,70 @@ static const struct of_device_id rockchip_pcie_ep_of_match[] = {
> {},
> };
>
> +static int rockchip_pcie_ep_get_resources(struct rockchip_pcie_ep *ep)
Almost all controller drivers use get_resources() function to acquire controller
resources like MMIO, clk, PHY etc... So if you were to refactor, I'd suggest to
first rename rockchip_pcie_parse_ep_dt() to rockchip_pcie_get_resources() to
maintain uniformity.
And if you want to move ob memory allocation to a single function to keep
probe() shorter, you should use a different function like
rockchip_pcie_ob_alloc() or something similar.
- Mani
> +{
> + struct rockchip_pcie *rockchip = &ep->rockchip;
> + struct device *dev = rockchip->dev;
> + struct pci_epc_mem_window *windows = NULL;
> + int err, i;
> +
> + err = rockchip_pcie_parse_ep_dt(rockchip, ep);
> + if (err)
> + return err;
> +
> + ep->ob_addr = devm_kcalloc(dev, ep->max_regions, sizeof(*ep->ob_addr),
> + GFP_KERNEL);
> +
> + if (!ep->ob_addr)
> + return -ENOMEM;
> +
> + windows = devm_kcalloc(dev, ep->max_regions,
> + sizeof(struct pci_epc_mem_window), GFP_KERNEL);
> + if (!windows)
> + return -ENOMEM;
> +
> + for (i = 0; i < ep->max_regions; i++) {
> + windows[i].phys_base = rockchip->mem_res->start + (SZ_1M * i);
> + windows[i].size = SZ_1M;
> + windows[i].page_size = SZ_1M;
> + }
> + err = pci_epc_multi_mem_init(ep->epc, windows, ep->max_regions);
> + devm_kfree(dev, windows);
> +
> + if (err < 0) {
> + dev_err(dev, "failed to initialize the memory space\n");
> + return err;
> + }
> +
> + ep->irq_cpu_addr = pci_epc_mem_alloc_addr(ep->epc, &ep->irq_phys_addr,
> + SZ_1M);
> + if (!ep->irq_cpu_addr) {
> + dev_err(dev, "failed to reserve memory space for MSI\n");
> + goto err_epc_mem_exit;
> + }
> +
> + ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;
> +
> + return 0;
> +
> +err_epc_mem_exit:
> + pci_epc_mem_exit(ep->epc);
> +
> + return err;
> +}
> +
> +static void rockchip_pcie_ep_release_resources(struct rockchip_pcie_ep *ep)
> +{
> + pci_epc_mem_exit(ep->epc);
> +}
> +
> static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct rockchip_pcie_ep *ep;
> struct rockchip_pcie *rockchip;
> struct pci_epc *epc;
> - size_t max_regions;
> - struct pci_epc_mem_window *windows = NULL;
> - int err, i;
> + int err;
> u32 cfg_msi, cfg_msix_cp;
>
> ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> @@ -552,13 +607,13 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> ep->epc = epc;
> epc_set_drvdata(epc, ep);
>
> - err = rockchip_pcie_parse_ep_dt(rockchip, ep);
> + err = rockchip_pcie_ep_get_resources(ep);
> if (err)
> return err;
>
> err = rockchip_pcie_enable_clocks(rockchip);
> if (err)
> - return err;
> + goto err_release_resources;
>
> err = rockchip_pcie_init_port(rockchip);
> if (err)
> @@ -568,47 +623,9 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
> PCIE_CLIENT_CONFIG);
>
> - max_regions = ep->max_regions;
> - ep->ob_addr = devm_kcalloc(dev, max_regions, sizeof(*ep->ob_addr),
> - GFP_KERNEL);
> -
> - if (!ep->ob_addr) {
> - err = -ENOMEM;
> - goto err_uninit_port;
> - }
> -
> /* Only enable function 0 by default */
> rockchip_pcie_write(rockchip, BIT(0), PCIE_CORE_PHY_FUNC_CFG);
>
> - windows = devm_kcalloc(dev, ep->max_regions,
> - sizeof(struct pci_epc_mem_window), GFP_KERNEL);
> - if (!windows) {
> - err = -ENOMEM;
> - goto err_uninit_port;
> - }
> - for (i = 0; i < ep->max_regions; i++) {
> - windows[i].phys_base = rockchip->mem_res->start + (SZ_1M * i);
> - windows[i].size = SZ_1M;
> - windows[i].page_size = SZ_1M;
> - }
> - err = pci_epc_multi_mem_init(epc, windows, ep->max_regions);
> - devm_kfree(dev, windows);
> -
> - if (err < 0) {
> - dev_err(dev, "failed to initialize the memory space\n");
> - goto err_uninit_port;
> - }
> -
> - ep->irq_cpu_addr = pci_epc_mem_alloc_addr(epc, &ep->irq_phys_addr,
> - SZ_1M);
> - if (!ep->irq_cpu_addr) {
> - dev_err(dev, "failed to reserve memory space for MSI\n");
> - err = -ENOMEM;
> - goto err_epc_mem_exit;
> - }
> -
> - ep->irq_pci_addr = ROCKCHIP_PCIE_EP_DUMMY_IRQ_ADDR;
> -
> /*
> * MSI-X is not supported but the controller still advertises the MSI-X
> * capability by default, which can lead to the Root Complex side
> @@ -638,10 +655,8 @@ static int rockchip_pcie_ep_probe(struct platform_device *pdev)
> pci_epc_init_notify(epc);
>
> return 0;
> -err_epc_mem_exit:
> - pci_epc_mem_exit(epc);
> -err_uninit_port:
> - rockchip_pcie_deinit_phys(rockchip);
> +err_release_resources:
> + rockchip_pcie_ep_release_resources(ep);
> err_disable_clocks:
> rockchip_pcie_disable_clocks(rockchip);
> return err;
> --
> 2.46.2
>
--
மணிவண்ணன் சதாசிவம்
