Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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);
>