Messages in this thread | | | Subject | Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver | From | Srinivas Kandagatla <> | Date | Sat, 7 Oct 2017 11:24:40 +0100 |
| |
Thanks for the review comments
On 07/10/17 08:45, Jonathan Neuschäfer wrote: > Hi, > > On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandagatla@linaro.org wrote: >> From: Sagar Dharia <sdharia@codeaurora.org> >> >> This controller driver programs manager, interface, and framer >> devices for Qualcomm's slimbus HW block. >> Manager component currently implements logical address setting, >> and messaging interface. >> Interface device reports bus synchronization information, and framer >> device clocks the bus from the time it's woken up, until clock-pause >> is executed by the manager device. >> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt | 43 ++ >> drivers/slimbus/Kconfig | 9 + >> drivers/slimbus/Makefile | 3 + >> drivers/slimbus/slim-qcom-ctrl.c | 594 +++++++++++++++++++++ >> drivers/slimbus/slim-qcom.h | 63 +++ >> 5 files changed, 712 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt >> create mode 100644 drivers/slimbus/slim-qcom-ctrl.c >> create mode 100644 drivers/slimbus/slim-qcom.h >> >> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt >> new file mode 100644 >> index 0000000..081110d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt >> @@ -0,0 +1,43 @@ >> +Qualcomm SLIMBUS controller >> +This controller is used if applications processor driver controls slimbus >> +master component. >> + >> +Required properties: >> + >> + - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt >> + - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt >> + >> + - reg : Offset and length of the register region(s) for the device >> + - reg-names : Register region name(s) referenced in reg above >> + Required register resource entries are: >> + "ctrl": Physical adderess of controller register blocks > > s/adderess/address/ Will fix this in next version.
> >> +} > [...] >> +static void msm_slim_prg_slew(struct platform_device *pdev, >> + struct msm_slim_ctrl *dev) >> +{ >> + void __iomem *slew_reg; >> + >> + /* SLEW RATE register for this slimbus */ >> + dev->slew_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> + "slew"); >> + if (!dev->slew_mem) { >> + dev_warn(&pdev->dev, "no slimbus slew resource\n"); >> + return; >> + } >> + >> + slew_reg = devm_ioremap(&pdev->dev, dev->slew_mem->start, >> + resource_size(dev->slew_mem)); > > How often will the driver program a slew rate? > This should be programmed only once after power on.
> If it's often, you'll have a "soft" memory leak over the life time of a > SLIM controller instance, because the mappings for slew_reg will > accumulate in the driver instance's devm area until they are all freed > in the end (If I'm reading the code correctly). I think you'll either > have to unmap slew_reg when this function returns (and not use devm), or > cache slew_reg so that subsequent calls to msm_slim_prg_slew won't > create more mappings. Yep .. I revisit this part once again before sending next version to see if we can do any better! > >> + if (!slew_reg) { >> + dev_err(dev->dev, "slew register mapping failed"); >> + release_mem_region(dev->slew_mem->start, >> + resource_size(dev->slew_mem)); >> + dev->slew_mem = NULL; >> + return; >> + } >> + writel_relaxed(1, slew_reg); >> + /* Make sure slimbus-slew rate enabling goes through */ >> + wmb(); >> +} >> + >> +static int msm_slim_probe(struct platform_device *pdev) >> +{ >> + struct msm_slim_ctrl *dev; >> + struct slim_controller *ctrl; >> + struct resource *slim_mem; >> + struct resource *irq; >> + struct clk *hclk, *rclk; >> + int ret; >> + >> + hclk = devm_clk_get(&pdev->dev, "iface_clk"); >> + if (IS_ERR(hclk)) >> + return PTR_ERR(hclk); >> + >> + rclk = devm_clk_get(&pdev->dev, "core_clk"); >> + if (IS_ERR(rclk)) { >> + /* unlikely that this is probe-defer */ >> + dev_err(&pdev->dev, "rclk get failed:%ld\n", PTR_ERR(rclk)); >> + return PTR_ERR(rclk); >> + } >> + >> + ret = clk_set_rate(rclk, SLIM_ROOT_FREQ); >> + if (ret) { >> + dev_err(&pdev->dev, "ref-clock set-rate failed:%d\n", ret); >> + return ret; >> + } >> + >> + slim_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl"); >> + if (!slim_mem) { >> + dev_err(&pdev->dev, "no slimbus physical memory resource\n"); >> + return -ENODEV; >> + } >> + >> + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> + if (!irq) { >> + dev_err(&pdev->dev, "no slimbus IRQ resource\n"); >> + return -ENODEV; >> + } >> + >> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); >> + if (!dev) >> + return -ENOMEM; >> + >> + dev->hclk = hclk; >> + dev->rclk = rclk; >> + ctrl = &dev->ctrl; >> + dev->dev = &pdev->dev; >> + platform_set_drvdata(pdev, dev); >> + slim_set_ctrldata(&dev->ctrl, dev); >> + dev->base = devm_ioremap(dev->dev, slim_mem->start, >> + resource_size(slim_mem)); >> + if (!dev->base) { >> + dev_err(&pdev->dev, "IOremap failed\n"); >> + return -ENOMEM; >> + } >> + >> + dev->ctrl.set_laddr = msm_set_laddr; >> + dev->ctrl.xfer_msg = msm_xfer_msg; >> + dev->ctrl.tx.n = MSM_TX_MSGS; >> + dev->ctrl.rx.n = MSM_RX_MSGS; >> + dev->ctrl.tx.sl_sz = SLIM_MSGQ_BUF_LEN; >> + dev->ctrl.rx.sl_sz = SLIM_MSGQ_BUF_LEN; >> + >> + dev->irq = irq->start; >> + >> + INIT_WORK(&dev->wd, msm_slim_rxwq); >> + dev->rxwq = create_singlethread_workqueue("msm_slim_rx"); >> + if (!dev->rxwq) { >> + dev_err(dev->dev, "Failed to start Rx WQ\n"); >> + return -ENOMEM; >> + } >> + >> + dev->framer.rootfreq = SLIM_ROOT_FREQ >> 3; >> + dev->framer.superfreq = >> + dev->framer.rootfreq / SLIM_CL_PER_SUPERFRAME_DIV8; >> + dev->ctrl.a_framer = &dev->framer; >> + dev->ctrl.clkgear = SLIM_MAX_CLK_GEAR; >> + dev->ctrl.dev.parent = &pdev->dev; >> + dev->ctrl.dev.of_node = pdev->dev.of_node; >> + >> + msm_slim_prg_slew(pdev, dev); >> + >> + ret = devm_request_irq(&pdev->dev, dev->irq, msm_slim_interrupt, >> + IRQF_TRIGGER_HIGH, "msm_slim_irq", dev); >> + if (ret) { >> + dev_err(&pdev->dev, "request IRQ failed\n"); >> + goto err_request_irq_failed; >> + } >> + >> + ret = clk_prepare_enable(hclk); >> + if (ret) >> + goto err_hclk_enable_failed; >> + >> + ret = clk_prepare_enable(rclk); >> + if (ret) >> + goto err_rclk_enable_failed; >> + >> + >> + ctrl->tx.base = dma_alloc_coherent(&pdev->dev, >> + (ctrl->tx.sl_sz * ctrl->tx.n), >> + &ctrl->tx.phy, GFP_KERNEL); > > Use dmam_alloc_coherent? > Yep, makes sense.. Will use it as suggested.
>> + if (!ctrl->tx.base) { >> + ret = -ENOMEM; >> + goto tx_alloc_failed; >> + } >> + >> + ctrl->rx.base = dma_alloc_coherent(&pdev->dev, >> + (ctrl->rx.sl_sz * ctrl->rx.n), >> + &ctrl->rx.phy, GFP_KERNEL); >
| |