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 at linaro.org wrote:
>> From: Sagar Dharia <sdharia at 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 at codeaurora.org>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla at 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);
>