[PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Thomas,
 
 On mer., févr. 28 2018, Thomas Petazzoni <thomas.petazzoni at bootlin.com> wrote:

> Hello,
>
> On Wed, 28 Feb 2018 15:47:04 +0100, Gregory CLEMENT wrote:
>> On Armada 7K/8K we need to explicitly enable the register clock. This
>> clock is optional because not all the SoCs using this IP need it but at
>> least for Armada 7K/8K it is actually mandatory.
>> 
>> The binding documentation is updated accordingly.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement at bootlin.com>
>> ---
>>  Documentation/devicetree/bindings/pci/pci-armada8k.txt |  6 +++++-
>>  drivers/pci/dwc/pcie-armada8k.c                        | 11 +++++++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
>> index c1e4c3d10a74..9948b1e9a8e5 100644
>> --- a/Documentation/devicetree/bindings/pci/pci-armada8k.txt
>> +++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
>> @@ -12,7 +12,11 @@ Required properties:
>>     - "ctrl" for the control register region
>>     - "config" for the config space region
>>  - interrupts: Interrupt specifier for the PCIe controler
>> -- clocks: reference to the PCIe controller clock
>> +- clocks: reference to the PCIe controller clocks
>> +- clock-names: mandatory if there is a second clock, in this case the
>> +   name must be "core" for the first clock and "reg" for the second
>> +   one
>> +
>
> Unneeded new line added here.
will removed it

>
>>  
>>  Example:
>>  
>> diff --git a/drivers/pci/dwc/pcie-armada8k.c b/drivers/pci/dwc/pcie-armada8k.c
>> index f9b1aec25c5c..aa4e5cc4ab7b 100644
>> --- a/drivers/pci/dwc/pcie-armada8k.c
>> +++ b/drivers/pci/dwc/pcie-armada8k.c
>> @@ -28,6 +28,7 @@
>>  struct armada8k_pcie {
>>  	struct dw_pcie *pci;
>>  	struct clk *clk;
>> +	struct clk *clk_reg;
>>  };
>>  
>>  #define PCIE_VENDOR_REGS_OFFSET		0x8000
>> @@ -229,6 +230,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (IS_ERR(pcie->clk_reg) && PTR_ERR(pcie->clk_reg) == -EPROBE_DEFER) {
>> +		clk_disable_unprepare(pcie->clk);
>> +		return -EPROBE_DEFER;
>> +	}
>> +	if (!IS_ERR(pcie->clk_reg)) {
>> +		ret = clk_prepare_enable(pcie->clk_reg);
>> +		if (ret)
>> +			goto fail;
>> +	}
>>  	/* Get the dw-pcie unit configuration/control registers base. */
>
> Missing new line between the end of the block and the next comment.
>
OK

> Regarding the error handling, doesn't it make more sense to also use a
> goto label to disable pcie->clk when getting the second clock gets a
> -EPROBE_DEFER ?
>
>>  	base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
>>  	pci->dbi_base = devm_pci_remap_cfg_resource(dev, base);
>> @@ -247,6 +257,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>>  	return 0;
>>  
>>  fail:
>> +	clk_disable_unprepare(pcie->clk_reg);
>
> So you are disabling/unpreparing the clock, which failed to
> prepare/enable ?

I was thinking to a single user, in this case if the prepare/enable
failed then the disabling/unpreparing do nothing because the counter is
already to 0. But indeed in case of multiple users of the clock, then the
counter could be wrongly decrease. I will modify it by adding a other
label.

Thanks,

Gregory

>
>>  	clk_disable_unprepare(pcie->clk);
>>  
>>  	return ret;
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> http://bootlin.com

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com