From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: From: Gregory CLEMENT To: Thomas Petazzoni Subject: Re: [PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock References: <20180228144704.12947-1-gregory.clement@bootlin.com> <20180228144704.12947-3-gregory.clement@bootlin.com> <20180228155350.0221a90f@windsurf.lan> Date: Wed, 28 Feb 2018 16:37:24 +0100 In-Reply-To: <20180228155350.0221a90f@windsurf.lan> (Thomas Petazzoni's message of "Wed, 28 Feb 2018 15:53:50 +0100") Message-ID: <874lm1f7m3.fsf@bootlin.com> MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrew Lunn , Lorenzo Pieralisi , Jason Cooper , Antoine Tenart , linux-pci@vger.kernel.org, Hanna Hawa , Omri Itach , Nadav Haklai , Shadi Ammouri , Igal Liberman , =?utf-8?Q?Miqu=C3=A8l?= Raynal , Bjorn Helgaas , Marcin Wojtas , linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth Content-Type: text/plain; charset="utf-8" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: SGkgVGhvbWFzLAogCiBPbiBtZXIuLCBmw6l2ci4gMjggMjAxOCwgVGhvbWFzIFBldGF6em9uaSA8 dGhvbWFzLnBldGF6em9uaUBib290bGluLmNvbT4gd3JvdGU6Cgo+IEhlbGxvLAo+Cj4gT24gV2Vk LCAyOCBGZWIgMjAxOCAxNTo0NzowNCArMDEwMCwgR3JlZ29yeSBDTEVNRU5UIHdyb3RlOgo+PiBP biBBcm1hZGEgN0svOEsgd2UgbmVlZCB0byBleHBsaWNpdGx5IGVuYWJsZSB0aGUgcmVnaXN0ZXIg Y2xvY2suIFRoaXMKPj4gY2xvY2sgaXMgb3B0aW9uYWwgYmVjYXVzZSBub3QgYWxsIHRoZSBTb0Nz IHVzaW5nIHRoaXMgSVAgbmVlZCBpdCBidXQgYXQKPj4gbGVhc3QgZm9yIEFybWFkYSA3Sy84SyBp dCBpcyBhY3R1YWxseSBtYW5kYXRvcnkuCj4+IAo+PiBUaGUgYmluZGluZyBkb2N1bWVudGF0aW9u IGlzIHVwZGF0ZWQgYWNjb3JkaW5nbHkuCj4+IAo+PiBTaWduZWQtb2ZmLWJ5OiBHcmVnb3J5IENM RU1FTlQgPGdyZWdvcnkuY2xlbWVudEBib290bGluLmNvbT4KPj4gLS0tCj4+ICBEb2N1bWVudGF0 aW9uL2RldmljZXRyZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQgfCAgNiArKysrKy0K Pj4gIGRyaXZlcnMvcGNpL2R3Yy9wY2llLWFybWFkYThrLmMgICAgICAgICAgICAgICAgICAgICAg ICB8IDExICsrKysrKysrKysrCj4+ICAyIGZpbGVzIGNoYW5nZWQsIDE2IGluc2VydGlvbnMoKyks IDEgZGVsZXRpb24oLSkKPj4gCj4+IGRpZmYgLS1naXQgYS9Eb2N1bWVudGF0aW9uL2RldmljZXRy ZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRy ZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQKPj4gaW5kZXggYzFlNGMzZDEwYTc0Li45 OTQ4YjFlOWE4ZTUgMTAwNjQ0Cj4+IC0tLSBhL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5k aW5ncy9wY2kvcGNpLWFybWFkYThrLnR4dAo+PiArKysgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRy ZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQKPj4gQEAgLTEyLDcgKzEyLDExIEBAIFJl cXVpcmVkIHByb3BlcnRpZXM6Cj4+ICAgICAtICJjdHJsIiBmb3IgdGhlIGNvbnRyb2wgcmVnaXN0 ZXIgcmVnaW9uCj4+ICAgICAtICJjb25maWciIGZvciB0aGUgY29uZmlnIHNwYWNlIHJlZ2lvbgo+ PiAgLSBpbnRlcnJ1cHRzOiBJbnRlcnJ1cHQgc3BlY2lmaWVyIGZvciB0aGUgUENJZSBjb250cm9s ZXIKPj4gLS0gY2xvY2tzOiByZWZlcmVuY2UgdG8gdGhlIFBDSWUgY29udHJvbGxlciBjbG9jawo+ PiArLSBjbG9ja3M6IHJlZmVyZW5jZSB0byB0aGUgUENJZSBjb250cm9sbGVyIGNsb2Nrcwo+PiAr LSBjbG9jay1uYW1lczogbWFuZGF0b3J5IGlmIHRoZXJlIGlzIGEgc2Vjb25kIGNsb2NrLCBpbiB0 aGlzIGNhc2UgdGhlCj4+ICsgICBuYW1lIG11c3QgYmUgImNvcmUiIGZvciB0aGUgZmlyc3QgY2xv Y2sgYW5kICJyZWciIGZvciB0aGUgc2Vjb25kCj4+ICsgICBvbmUKPj4gKwo+Cj4gVW5uZWVkZWQg bmV3IGxpbmUgYWRkZWQgaGVyZS4Kd2lsbCByZW1vdmVkIGl0Cgo+Cj4+ICAKPj4gIEV4YW1wbGU6 Cj4+ICAKPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2R3Yy9wY2llLWFybWFkYThrLmMgYi9k cml2ZXJzL3BjaS9kd2MvcGNpZS1hcm1hZGE4ay5jCj4+IGluZGV4IGY5YjFhZWMyNWM1Yy4uYWE0 ZTVjYzRhYjdiIDEwMDY0NAo+PiAtLS0gYS9kcml2ZXJzL3BjaS9kd2MvcGNpZS1hcm1hZGE4ay5j Cj4+ICsrKyBiL2RyaXZlcnMvcGNpL2R3Yy9wY2llLWFybWFkYThrLmMKPj4gQEAgLTI4LDYgKzI4 LDcgQEAKPj4gIHN0cnVjdCBhcm1hZGE4a19wY2llIHsKPj4gIAlzdHJ1Y3QgZHdfcGNpZSAqcGNp Owo+PiAgCXN0cnVjdCBjbGsgKmNsazsKPj4gKwlzdHJ1Y3QgY2xrICpjbGtfcmVnOwo+PiAgfTsK Pj4gIAo+PiAgI2RlZmluZSBQQ0lFX1ZFTkRPUl9SRUdTX09GRlNFVAkJMHg4MDAwCj4+IEBAIC0y MjksNiArMjMwLDE1IEBAIHN0YXRpYyBpbnQgYXJtYWRhOGtfcGNpZV9wcm9iZShzdHJ1Y3QgcGxh dGZvcm1fZGV2aWNlICpwZGV2KQo+PiAgCWlmIChyZXQpCj4+ICAJCXJldHVybiByZXQ7Cj4+ICAK Pj4gKwlpZiAoSVNfRVJSKHBjaWUtPmNsa19yZWcpICYmIFBUUl9FUlIocGNpZS0+Y2xrX3JlZykg PT0gLUVQUk9CRV9ERUZFUikgewo+PiArCQljbGtfZGlzYWJsZV91bnByZXBhcmUocGNpZS0+Y2xr KTsKPj4gKwkJcmV0dXJuIC1FUFJPQkVfREVGRVI7Cj4+ICsJfQo+PiArCWlmICghSVNfRVJSKHBj aWUtPmNsa19yZWcpKSB7Cj4+ICsJCXJldCA9IGNsa19wcmVwYXJlX2VuYWJsZShwY2llLT5jbGtf cmVnKTsKPj4gKwkJaWYgKHJldCkKPj4gKwkJCWdvdG8gZmFpbDsKPj4gKwl9Cj4+ICAJLyogR2V0 IHRoZSBkdy1wY2llIHVuaXQgY29uZmlndXJhdGlvbi9jb250cm9sIHJlZ2lzdGVycyBiYXNlLiAq Lwo+Cj4gTWlzc2luZyBuZXcgbGluZSBiZXR3ZWVuIHRoZSBlbmQgb2YgdGhlIGJsb2NrIGFuZCB0 aGUgbmV4dCBjb21tZW50Lgo+Ck9LCgo+IFJlZ2FyZGluZyB0aGUgZXJyb3IgaGFuZGxpbmcsIGRv ZXNuJ3QgaXQgbWFrZSBtb3JlIHNlbnNlIHRvIGFsc28gdXNlIGEKPiBnb3RvIGxhYmVsIHRvIGRp c2FibGUgcGNpZS0+Y2xrIHdoZW4gZ2V0dGluZyB0aGUgc2Vjb25kIGNsb2NrIGdldHMgYQo+IC1F UFJPQkVfREVGRVIgPwo+Cj4+ICAJYmFzZSA9IHBsYXRmb3JtX2dldF9yZXNvdXJjZV9ieW5hbWUo cGRldiwgSU9SRVNPVVJDRV9NRU0sICJjdHJsIik7Cj4+ICAJcGNpLT5kYmlfYmFzZSA9IGRldm1f cGNpX3JlbWFwX2NmZ19yZXNvdXJjZShkZXYsIGJhc2UpOwo+PiBAQCAtMjQ3LDYgKzI1Nyw3IEBA IHN0YXRpYyBpbnQgYXJtYWRhOGtfcGNpZV9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpw ZGV2KQo+PiAgCXJldHVybiAwOwo+PiAgCj4+ICBmYWlsOgo+PiArCWNsa19kaXNhYmxlX3VucHJl cGFyZShwY2llLT5jbGtfcmVnKTsKPgo+IFNvIHlvdSBhcmUgZGlzYWJsaW5nL3VucHJlcGFyaW5n IHRoZSBjbG9jaywgd2hpY2ggZmFpbGVkIHRvCj4gcHJlcGFyZS9lbmFibGUgPwoKSSB3YXMgdGhp bmtpbmcgdG8gYSBzaW5nbGUgdXNlciwgaW4gdGhpcyBjYXNlIGlmIHRoZSBwcmVwYXJlL2VuYWJs ZQpmYWlsZWQgdGhlbiB0aGUgZGlzYWJsaW5nL3VucHJlcGFyaW5nIGRvIG5vdGhpbmcgYmVjYXVz ZSB0aGUgY291bnRlciBpcwphbHJlYWR5IHRvIDAuIEJ1dCBpbmRlZWQgaW4gY2FzZSBvZiBtdWx0 aXBsZSB1c2VycyBvZiB0aGUgY2xvY2ssIHRoZW4gdGhlCmNvdW50ZXIgY291bGQgYmUgd3Jvbmds eSBkZWNyZWFzZS4gSSB3aWxsIG1vZGlmeSBpdCBieSBhZGRpbmcgYSBvdGhlcgpsYWJlbC4KClRo YW5rcywKCkdyZWdvcnkKCj4KPj4gIAljbGtfZGlzYWJsZV91bnByZXBhcmUocGNpZS0+Y2xrKTsK Pj4gIAo+PiAgCXJldHVybiByZXQ7Cj4KPiBUaG9tYXMKPiAtLSAKPiBUaG9tYXMgUGV0YXp6b25p LCBDVE8sIEJvb3RsaW4gKGZvcm1lcmx5IEZyZWUgRWxlY3Ryb25zKQo+IEVtYmVkZGVkIExpbnV4 IGFuZCBLZXJuZWwgZW5naW5lZXJpbmcKPiBodHRwOi8vYm9vdGxpbi5jb20KCi0tIApHcmVnb3J5 IENsZW1lbnQsIEJvb3RsaW4gKGZvcm1lcmx5IEZyZWUgRWxlY3Ryb25zKQpFbWJlZGRlZCBMaW51 eCBhbmQgS2VybmVsIGVuZ2luZWVyaW5nCmh0dHA6Ly9ib290bGluLmNvbQoKX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBtYWls aW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0 cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: gregory.clement@bootlin.com (Gregory CLEMENT) Date: Wed, 28 Feb 2018 16:37:24 +0100 Subject: [PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock In-Reply-To: <20180228155350.0221a90f@windsurf.lan> (Thomas Petazzoni's message of "Wed, 28 Feb 2018 15:53:50 +0100") References: <20180228144704.12947-1-gregory.clement@bootlin.com> <20180228144704.12947-3-gregory.clement@bootlin.com> <20180228155350.0221a90f@windsurf.lan> Message-ID: <874lm1f7m3.fsf@bootlin.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Thomas, On mer., f?vr. 28 2018, Thomas Petazzoni 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 >> --- >> 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