From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 10 Apr 2018 10:24:07 +0200 From: Boris Brezillon To: Marcin Ziemianowicz Cc: Boris Brezillon , Nicolas Ferre , Alexandre Belloni , Greg Kroah-Hartman , Michael Turquette , Stephen Boyd , Alan Stern , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH v2 1/2] clk: at91: Added more information logging. Message-ID: <20180410102407.46e7d416@bbrezillon> In-Reply-To: <20180410001621.GA62230@hak8or> References: <20180410001621.GA62230@hak8or> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII List-ID: Hi Marcin, On Mon, 9 Apr 2018 20:16:21 -0400 Marcin Ziemianowicz wrote: > I noticed that when debugging some USB clocking issue that there weren't > many ways to tell what the state of the USB clocking system was. This > adds a few logging statements to see what the relevant code is trying to > do. > > Signed-off-by: Marcin Ziemianowicz > --- > drivers/clk/at91/clk-pll.c | 6 +++++- > drivers/clk/at91/clk-usb.c | 10 ++++++++-- > drivers/usb/host/ohci-at91.c | 16 ++++++++++------ This should be split in 2 patches (one adding traces to clk drivers and another doing it for the USB host driver), so that those patches can be merged independently by the clk and usb maintainers. > 3 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c > index 7d3223fc7161..534961766ae5 100644 > --- a/drivers/clk/at91/clk-pll.c > +++ b/drivers/clk/at91/clk-pll.c > @@ -133,6 +133,7 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw, > { > struct clk_pll *pll = to_clk_pll(hw); > unsigned int pllr; > + unsigned long recalcedrate; Just name it 'rate' or 'realrate'. > u16 mul; > u8 div; > > @@ -144,7 +145,10 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw, > if (!div || !mul) > return 0; > > - return (parent_rate / div) * (mul + 1); > + recalcedrate = (parent_rate / div) * (mul + 1); > + pr_debug("clk-pll: calculating new rate, (%lu hz / %u) * %u = %lu hz\n", If the prefix is alway "clk-pll: " you could define pr_fmt() to add it to all your pr_xxx() messages. BTW, it's not about calculating the new rate, but retrieving the actual rate. > + parent_rate, div, mul, recalcedrate); Add an empty line here. > + return recalcedrate; > } > > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate, > diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c > index 791770a563fc..2fa877e99bac 100644 > --- a/drivers/clk/at91/clk-usb.c > +++ b/drivers/clk/at91/clk-usb.c > @@ -48,11 +48,15 @@ static unsigned long at91sam9x5_clk_usb_recalc_rate(struct clk_hw *hw, > struct at91sam9x5_clk_usb *usb = to_at91sam9x5_clk_usb(hw); > unsigned int usbr; > u8 usbdiv; > + unsigned int calcdclock; Ditto: s/calcdclock/realrate/, and make it unsigned long. > > regmap_read(usb->regmap, AT91_PMC_USB, &usbr); > usbdiv = (usbr & AT91_PMC_OHCIUSBDIV) >> SAM9X5_USB_DIV_SHIFT; > > - return DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1)); > + calcdclock = DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1)); > + pr_debug("clk-usb: calculating new rate, %lu hz / %u = %u hz\n", > + parent_rate, usbdiv + 1, calcdclock); Empty line here too. > + return calcdclock; > } > > static int at91sam9x5_clk_usb_determine_rate(struct clk_hw *hw, > @@ -98,7 +102,6 @@ static int at91sam9x5_clk_usb_determine_rate(struct clk_hw *hw, > if (!best_diff) > break; > } > - > if (best_rate < 0) > return best_rate; > > @@ -142,6 +145,9 @@ static int at91sam9x5_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate, > if (div > SAM9X5_USB_MAX_DIV + 1 || !div) > return -EINVAL; > > + pr_debug("clk-usb: setting USB clock divider to %lu hz / %lu = %lu hz\n", The formulation is weird. Maybe you should just remove 'divider' here: "setting USB clock to %lu hz / %lu = %lu hz" > + parent_rate, div, rate); > + > regmap_update_bits(usb->regmap, AT91_PMC_USB, AT91_PMC_OHCIUSBDIV, > (div - 1) << SAM9X5_USB_DIV_SHIFT); > > diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c > index 5ad9e9bdc8ee..c57a239918f9 100644 > --- a/drivers/usb/host/ohci-at91.c > +++ b/drivers/usb/host/ohci-at91.c > @@ -70,11 +70,13 @@ static const struct ohci_driver_overrides ohci_at91_drv_overrides __initconst = > > /*-------------------------------------------------------------------------*/ > > -static void at91_start_clock(struct ohci_at91_priv *ohci_at91) > +static void at91_start_clock(struct ohci_at91_priv *ohci_at91, > + struct device *dev) Maybe you could just pass a pdev or dev pointer and let the function extract the ohci_at91 object with: struct usb_hcd *hcd = platform_get_drvdata(pdev); struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd); > { > if (ohci_at91->clocked) > return; > > + dev_dbg(dev, "Enabling hclk, iclk, and setting fclk to 48 Mhz\n"); Add an empty line here. > clk_set_rate(ohci_at91->fclk, 48000000); > clk_prepare_enable(ohci_at91->hclk); > clk_prepare_enable(ohci_at91->iclk); > @@ -82,11 +84,13 @@ static void at91_start_clock(struct ohci_at91_priv *ohci_at91) > ohci_at91->clocked = true; > } > > -static void at91_stop_clock(struct ohci_at91_priv *ohci_at91) > +static void at91_stop_clock(struct ohci_at91_priv *ohci_at91, > + struct device *dev) > { > if (!ohci_at91->clocked) > return; > > + dev_dbg(dev, "Disabling hclk, iclk, and fclk\n"); > clk_disable_unprepare(ohci_at91->fclk); > clk_disable_unprepare(ohci_at91->iclk); > clk_disable_unprepare(ohci_at91->hclk); > @@ -104,7 +108,7 @@ static void at91_start_hc(struct platform_device *pdev) > /* > * Start the USB clocks. > */ > - at91_start_clock(ohci_at91); > + at91_start_clock(ohci_at91, &pdev->dev); > > /* > * The USB host controller must remain in reset. > @@ -128,7 +132,7 @@ static void at91_stop_hc(struct platform_device *pdev) > /* > * Stop the USB clocks. > */ > - at91_stop_clock(ohci_at91); > + at91_stop_clock(ohci_at91, &pdev->dev); > } > > > @@ -623,7 +627,7 @@ ohci_hcd_at91_drv_suspend(struct device *dev) > > /* flush the writes */ > (void) ohci_readl (ohci, &ohci->regs->control); > - at91_stop_clock(ohci_at91); > + at91_stop_clock(ohci_at91, dev); > } > > return ret; > @@ -638,7 +642,7 @@ ohci_hcd_at91_drv_resume(struct device *dev) > if (ohci_at91->wakeup) > disable_irq_wake(hcd->irq); > > - at91_start_clock(ohci_at91); > + at91_start_clock(ohci_at91, dev); > > ohci_resume(hcd, false); > Regards, Boris From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2,1/2] clk: at91: Added more information logging. From: Boris Brezillon Message-Id: <20180410102407.46e7d416@bbrezillon> Date: Tue, 10 Apr 2018 10:24:07 +0200 To: Marcin Ziemianowicz Cc: Boris Brezillon , Nicolas Ferre , Alexandre Belloni , Greg Kroah-Hartman , Michael Turquette , Stephen Boyd , Alan Stern , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org List-ID: SGkgTWFyY2luLAoKT24gTW9uLCA5IEFwciAyMDE4IDIwOjE2OjIxIC0wNDAwCk1hcmNpbiBaaWVt aWFub3dpY3ogPG1hcmNpbkB6aWVtaWFub3dpY3ouY29tPiB3cm90ZToKCj4gSSBub3RpY2VkIHRo YXQgd2hlbiBkZWJ1Z2dpbmcgc29tZSBVU0IgY2xvY2tpbmcgaXNzdWUgdGhhdCB0aGVyZSB3ZXJl bid0Cj4gbWFueSB3YXlzIHRvIHRlbGwgd2hhdCB0aGUgc3RhdGUgb2YgdGhlIFVTQiBjbG9ja2lu ZyBzeXN0ZW0gd2FzLiBUaGlzCj4gYWRkcyBhIGZldyBsb2dnaW5nIHN0YXRlbWVudHMgdG8gc2Vl IHdoYXQgdGhlIHJlbGV2YW50IGNvZGUgaXMgdHJ5aW5nIHRvCj4gZG8uCj4gCj4gU2lnbmVkLW9m Zi1ieTogTWFyY2luIFppZW1pYW5vd2ljeiA8bWFyY2luQHppZW1pYW5vd2ljei5jb20+Cj4gLS0t Cj4gIGRyaXZlcnMvY2xrL2F0OTEvY2xrLXBsbC5jICAgfCAgNiArKysrKy0KPiAgZHJpdmVycy9j bGsvYXQ5MS9jbGstdXNiLmMgICB8IDEwICsrKysrKysrLS0KPiAgZHJpdmVycy91c2IvaG9zdC9v aGNpLWF0OTEuYyB8IDE2ICsrKysrKysrKystLS0tLS0KClRoaXMgc2hvdWxkIGJlIHNwbGl0IGlu IDIgcGF0Y2hlcyAob25lIGFkZGluZyB0cmFjZXMgdG8gY2xrIGRyaXZlcnMgYW5kCmFub3RoZXIg ZG9pbmcgaXQgZm9yIHRoZSBVU0IgaG9zdCBkcml2ZXIpLCBzbyB0aGF0IHRob3NlIHBhdGNoZXMg Y2FuIGJlCm1lcmdlZCBpbmRlcGVuZGVudGx5IGJ5IHRoZSBjbGsgYW5kIHVzYiBtYWludGFpbmVy cy4KCj4gIDMgZmlsZXMgY2hhbmdlZCwgMjMgaW5zZXJ0aW9ucygrKSwgOSBkZWxldGlvbnMoLSkK PiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9jbGsvYXQ5MS9jbGstcGxsLmMgYi9kcml2ZXJzL2Ns ay9hdDkxL2Nsay1wbGwuYwo+IGluZGV4IDdkMzIyM2ZjNzE2MS4uNTM0OTYxNzY2YWU1IDEwMDY0 NAo+IC0tLSBhL2RyaXZlcnMvY2xrL2F0OTEvY2xrLXBsbC5jCj4gKysrIGIvZHJpdmVycy9jbGsv YXQ5MS9jbGstcGxsLmMKPiBAQCAtMTMzLDYgKzEzMyw3IEBAIHN0YXRpYyB1bnNpZ25lZCBsb25n IGNsa19wbGxfcmVjYWxjX3JhdGUoc3RydWN0IGNsa19odyAqaHcsCj4gIHsKPiAgCXN0cnVjdCBj bGtfcGxsICpwbGwgPSB0b19jbGtfcGxsKGh3KTsKPiAgCXVuc2lnbmVkIGludCBwbGxyOwo+ICsJ dW5zaWduZWQgbG9uZyByZWNhbGNlZHJhdGU7CgpKdXN0IG5hbWUgaXQgJ3JhdGUnIG9yICdyZWFs cmF0ZScuCgo+ICAJdTE2IG11bDsKPiAgCXU4IGRpdjsKPiAgCj4gQEAgLTE0NCw3ICsxNDUsMTAg QEAgc3RhdGljIHVuc2lnbmVkIGxvbmcgY2xrX3BsbF9yZWNhbGNfcmF0ZShzdHJ1Y3QgY2xrX2h3 ICpodywKPiAgCWlmICghZGl2IHx8ICFtdWwpCj4gIAkJcmV0dXJuIDA7Cj4gIAo+IC0JcmV0dXJu IChwYXJlbnRfcmF0ZSAvIGRpdikgKiAobXVsICsgMSk7Cj4gKwlyZWNhbGNlZHJhdGUgPSAocGFy ZW50X3JhdGUgLyBkaXYpICogKG11bCArIDEpOwo+ICsJcHJfZGVidWcoImNsay1wbGw6IGNhbGN1 bGF0aW5nIG5ldyByYXRlLCAoJWx1IGh6IC8gJXUpICogJXUgPSAlbHUgaHpcbiIsCgpJZiB0aGUg cHJlZml4IGlzIGFsd2F5ICJjbGstcGxsOiAiIHlvdSBjb3VsZCBkZWZpbmUgcHJfZm10KCkgdG8g YWRkIGl0CnRvIGFsbCB5b3VyIHByX3h4eCgpIG1lc3NhZ2VzLgoKQlRXLCBpdCdzIG5vdCBhYm91 dCBjYWxjdWxhdGluZyB0aGUgbmV3IHJhdGUsIGJ1dCByZXRyaWV2aW5nIHRoZQphY3R1YWwgcmF0 ZS4KCj4gKwkJcGFyZW50X3JhdGUsIGRpdiwgbXVsLCByZWNhbGNlZHJhdGUpOwoKQWRkIGFuIGVt cHR5IGxpbmUgaGVyZS4KCj4gKwlyZXR1cm4gcmVjYWxjZWRyYXRlOwo+ICB9Cj4gIAo+ICBzdGF0 aWMgbG9uZyBjbGtfcGxsX2dldF9iZXN0X2Rpdl9tdWwoc3RydWN0IGNsa19wbGwgKnBsbCwgdW5z aWduZWQgbG9uZyByYXRlLAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2Nsay9hdDkxL2Nsay11c2Iu YyBiL2RyaXZlcnMvY2xrL2F0OTEvY2xrLXVzYi5jCj4gaW5kZXggNzkxNzcwYTU2M2ZjLi4yZmE4 NzdlOTliYWMgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9jbGsvYXQ5MS9jbGstdXNiLmMKPiArKysg Yi9kcml2ZXJzL2Nsay9hdDkxL2Nsay11c2IuYwo+IEBAIC00OCwxMSArNDgsMTUgQEAgc3RhdGlj IHVuc2lnbmVkIGxvbmcgYXQ5MXNhbTl4NV9jbGtfdXNiX3JlY2FsY19yYXRlKHN0cnVjdCBjbGtf aHcgKmh3LAo+ICAJc3RydWN0IGF0OTFzYW05eDVfY2xrX3VzYiAqdXNiID0gdG9fYXQ5MXNhbTl4 NV9jbGtfdXNiKGh3KTsKPiAgCXVuc2lnbmVkIGludCB1c2JyOwo+ICAJdTggdXNiZGl2Owo+ICsJ dW5zaWduZWQgaW50IGNhbGNkY2xvY2s7CgpEaXR0bzogcy9jYWxjZGNsb2NrL3JlYWxyYXRlLywg YW5kIG1ha2UgaXQgdW5zaWduZWQgbG9uZy4KCj4gIAo+ICAJcmVnbWFwX3JlYWQodXNiLT5yZWdt YXAsIEFUOTFfUE1DX1VTQiwgJnVzYnIpOwo+ICAJdXNiZGl2ID0gKHVzYnIgJiBBVDkxX1BNQ19P SENJVVNCRElWKSA+PiBTQU05WDVfVVNCX0RJVl9TSElGVDsKPiAgCj4gLQlyZXR1cm4gRElWX1JP VU5EX0NMT1NFU1QocGFyZW50X3JhdGUsICh1c2JkaXYgKyAxKSk7Cj4gKwljYWxjZGNsb2NrID0g RElWX1JPVU5EX0NMT1NFU1QocGFyZW50X3JhdGUsICh1c2JkaXYgKyAxKSk7Cj4gKwlwcl9kZWJ1 ZygiY2xrLXVzYjogY2FsY3VsYXRpbmcgbmV3IHJhdGUsICVsdSBoeiAvICV1ID0gJXUgaHpcbiIs Cj4gKwkJcGFyZW50X3JhdGUsIHVzYmRpdiArIDEsIGNhbGNkY2xvY2spOwoKRW1wdHkgbGluZSBo ZXJlIHRvby4KCj4gKwlyZXR1cm4gY2FsY2RjbG9jazsKPiAgfQo+ICAKPiAgc3RhdGljIGludCBh dDkxc2FtOXg1X2Nsa191c2JfZGV0ZXJtaW5lX3JhdGUoc3RydWN0IGNsa19odyAqaHcsCj4gQEAg LTk4LDcgKzEwMiw2IEBAIHN0YXRpYyBpbnQgYXQ5MXNhbTl4NV9jbGtfdXNiX2RldGVybWluZV9y YXRlKHN0cnVjdCBjbGtfaHcgKmh3LAo+ICAJCWlmICghYmVzdF9kaWZmKQo+ICAJCQlicmVhazsK PiAgCX0KPiAtCj4gIAlpZiAoYmVzdF9yYXRlIDwgMCkKPiAgCQlyZXR1cm4gYmVzdF9yYXRlOwo+ ICAKPiBAQCAtMTQyLDYgKzE0NSw5IEBAIHN0YXRpYyBpbnQgYXQ5MXNhbTl4NV9jbGtfdXNiX3Nl dF9yYXRlKHN0cnVjdCBjbGtfaHcgKmh3LCB1bnNpZ25lZCBsb25nIHJhdGUsCj4gIAlpZiAoZGl2 ID4gU0FNOVg1X1VTQl9NQVhfRElWICsgMSB8fCAhZGl2KQo+ICAJCXJldHVybiAtRUlOVkFMOwo+ ICAKPiArCXByX2RlYnVnKCJjbGstdXNiOiBzZXR0aW5nIFVTQiBjbG9jayBkaXZpZGVyIHRvICVs dSBoeiAvICVsdSA9ICVsdSBoelxuIiwKClRoZSBmb3JtdWxhdGlvbiBpcyB3ZWlyZC4gTWF5YmUg eW91IHNob3VsZCBqdXN0IHJlbW92ZSAnZGl2aWRlcicgaGVyZToKCgkJInNldHRpbmcgVVNCIGNs b2NrIHRvICVsdSBoeiAvICVsdSA9ICVsdSBoeiIKCj4gKwkJcGFyZW50X3JhdGUsIGRpdiwgcmF0 ZSk7Cj4gKwo+ICAJcmVnbWFwX3VwZGF0ZV9iaXRzKHVzYi0+cmVnbWFwLCBBVDkxX1BNQ19VU0Is IEFUOTFfUE1DX09IQ0lVU0JESVYsCj4gIAkJCSAgIChkaXYgLSAxKSA8PCBTQU05WDVfVVNCX0RJ Vl9TSElGVCk7Cj4gIAo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9ob3N0L29oY2ktYXQ5MS5j IGIvZHJpdmVycy91c2IvaG9zdC9vaGNpLWF0OTEuYwo+IGluZGV4IDVhZDllOWJkYzhlZS4uYzU3 YTIzOTkxOGY5IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvdXNiL2hvc3Qvb2hjaS1hdDkxLmMKPiAr KysgYi9kcml2ZXJzL3VzYi9ob3N0L29oY2ktYXQ5MS5jCj4gQEAgLTcwLDExICs3MCwxMyBAQCBz dGF0aWMgY29uc3Qgc3RydWN0IG9oY2lfZHJpdmVyX292ZXJyaWRlcyBvaGNpX2F0OTFfZHJ2X292 ZXJyaWRlcyBfX2luaXRjb25zdCA9Cj4gIAo+ICAvKi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0qLwo+ICAKPiAt c3RhdGljIHZvaWQgYXQ5MV9zdGFydF9jbG9jayhzdHJ1Y3Qgb2hjaV9hdDkxX3ByaXYgKm9oY2lf YXQ5MSkKPiArc3RhdGljIHZvaWQgYXQ5MV9zdGFydF9jbG9jayhzdHJ1Y3Qgb2hjaV9hdDkxX3By aXYgKm9oY2lfYXQ5MSwKPiArCQkJCXN0cnVjdCBkZXZpY2UgKmRldikKCk1heWJlIHlvdSBjb3Vs ZCBqdXN0IHBhc3MgYSBwZGV2IG9yIGRldiBwb2ludGVyIGFuZCBsZXQgdGhlIGZ1bmN0aW9uCmV4 dHJhY3QgdGhlIG9oY2lfYXQ5MSBvYmplY3Qgd2l0aDoKCglzdHJ1Y3QgdXNiX2hjZCAqaGNkID0g cGxhdGZvcm1fZ2V0X2RydmRhdGEocGRldik7CglzdHJ1Y3Qgb2hjaV9hdDkxX3ByaXYgKm9oY2lf YXQ5MSA9IGhjZF90b19vaGNpX2F0OTFfcHJpdihoY2QpOwoKPiAgewo+ICAJaWYgKG9oY2lfYXQ5 MS0+Y2xvY2tlZCkKPiAgCQlyZXR1cm47Cj4gIAo+ICsJZGV2X2RiZyhkZXYsICJFbmFibGluZyBo Y2xrLCBpY2xrLCBhbmQgc2V0dGluZyBmY2xrIHRvIDQ4IE1oelxuIik7CgpBZGQgYW4gZW1wdHkg bGluZSBoZXJlLgoKPiAgCWNsa19zZXRfcmF0ZShvaGNpX2F0OTEtPmZjbGssIDQ4MDAwMDAwKTsK PiAgCWNsa19wcmVwYXJlX2VuYWJsZShvaGNpX2F0OTEtPmhjbGspOwo+ICAJY2xrX3ByZXBhcmVf ZW5hYmxlKG9oY2lfYXQ5MS0+aWNsayk7Cj4gQEAgLTgyLDExICs4NCwxMyBAQCBzdGF0aWMgdm9p ZCBhdDkxX3N0YXJ0X2Nsb2NrKHN0cnVjdCBvaGNpX2F0OTFfcHJpdiAqb2hjaV9hdDkxKQo+ICAJ b2hjaV9hdDkxLT5jbG9ja2VkID0gdHJ1ZTsKPiAgfQo+ICAKPiAtc3RhdGljIHZvaWQgYXQ5MV9z dG9wX2Nsb2NrKHN0cnVjdCBvaGNpX2F0OTFfcHJpdiAqb2hjaV9hdDkxKQo+ICtzdGF0aWMgdm9p ZCBhdDkxX3N0b3BfY2xvY2soc3RydWN0IG9oY2lfYXQ5MV9wcml2ICpvaGNpX2F0OTEsCj4gKwkJ CQlzdHJ1Y3QgZGV2aWNlICpkZXYpCj4gIHsKPiAgCWlmICghb2hjaV9hdDkxLT5jbG9ja2VkKQo+ ICAJCXJldHVybjsKPiAgCj4gKwlkZXZfZGJnKGRldiwgIkRpc2FibGluZyBoY2xrLCBpY2xrLCBh bmQgZmNsa1xuIik7Cj4gIAljbGtfZGlzYWJsZV91bnByZXBhcmUob2hjaV9hdDkxLT5mY2xrKTsK PiAgCWNsa19kaXNhYmxlX3VucHJlcGFyZShvaGNpX2F0OTEtPmljbGspOwo+ICAJY2xrX2Rpc2Fi bGVfdW5wcmVwYXJlKG9oY2lfYXQ5MS0+aGNsayk7Cj4gQEAgLTEwNCw3ICsxMDgsNyBAQCBzdGF0 aWMgdm9pZCBhdDkxX3N0YXJ0X2hjKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpCj4gIAkv Kgo+ICAJICogU3RhcnQgdGhlIFVTQiBjbG9ja3MuCj4gIAkgKi8KPiAtCWF0OTFfc3RhcnRfY2xv Y2sob2hjaV9hdDkxKTsKPiArCWF0OTFfc3RhcnRfY2xvY2sob2hjaV9hdDkxLCAmcGRldi0+ZGV2 KTsKPiAgCj4gIAkvKgo+ICAJICogVGhlIFVTQiBob3N0IGNvbnRyb2xsZXIgbXVzdCByZW1haW4g aW4gcmVzZXQuCj4gQEAgLTEyOCw3ICsxMzIsNyBAQCBzdGF0aWMgdm9pZCBhdDkxX3N0b3BfaGMo c3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPiAgCS8qCj4gIAkgKiBTdG9wIHRoZSBVU0Ig Y2xvY2tzLgo+ICAJICovCj4gLQlhdDkxX3N0b3BfY2xvY2sob2hjaV9hdDkxKTsKPiArCWF0OTFf c3RvcF9jbG9jayhvaGNpX2F0OTEsICZwZGV2LT5kZXYpOwo+ICB9Cj4gIAo+ICAKPiBAQCAtNjIz LDcgKzYyNyw3IEBAIG9oY2lfaGNkX2F0OTFfZHJ2X3N1c3BlbmQoc3RydWN0IGRldmljZSAqZGV2 KQo+ICAKPiAgCQkvKiBmbHVzaCB0aGUgd3JpdGVzICovCj4gIAkJKHZvaWQpIG9oY2lfcmVhZGwg KG9oY2ksICZvaGNpLT5yZWdzLT5jb250cm9sKTsKPiAtCQlhdDkxX3N0b3BfY2xvY2sob2hjaV9h dDkxKTsKPiArCQlhdDkxX3N0b3BfY2xvY2sob2hjaV9hdDkxLCBkZXYpOwo+ICAJfQo+ICAKPiAg CXJldHVybiByZXQ7Cj4gQEAgLTYzOCw3ICs2NDIsNyBAQCBvaGNpX2hjZF9hdDkxX2Rydl9yZXN1 bWUoc3RydWN0IGRldmljZSAqZGV2KQo+ICAJaWYgKG9oY2lfYXQ5MS0+d2FrZXVwKQo+ICAJCWRp c2FibGVfaXJxX3dha2UoaGNkLT5pcnEpOwo+ICAKPiAtCWF0OTFfc3RhcnRfY2xvY2sob2hjaV9h dDkxKTsKPiArCWF0OTFfc3RhcnRfY2xvY2sob2hjaV9hdDkxLCBkZXYpOwo+ICAKPiAgCW9oY2lf cmVzdW1lKGhjZCwgZmFsc2UpOwo+ICAKClJlZ2FyZHMsCgpCb3JpcwotLS0KVG8gdW5zdWJzY3Jp YmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LXVzYiIg aW4KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcKTW9y ZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5m by5odG1sCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@bootlin.com (Boris Brezillon) Date: Tue, 10 Apr 2018 10:24:07 +0200 Subject: [PATCH v2 1/2] clk: at91: Added more information logging. In-Reply-To: <20180410001621.GA62230@hak8or> References: <20180410001621.GA62230@hak8or> Message-ID: <20180410102407.46e7d416@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Marcin, On Mon, 9 Apr 2018 20:16:21 -0400 Marcin Ziemianowicz wrote: > I noticed that when debugging some USB clocking issue that there weren't > many ways to tell what the state of the USB clocking system was. This > adds a few logging statements to see what the relevant code is trying to > do. > > Signed-off-by: Marcin Ziemianowicz > --- > drivers/clk/at91/clk-pll.c | 6 +++++- > drivers/clk/at91/clk-usb.c | 10 ++++++++-- > drivers/usb/host/ohci-at91.c | 16 ++++++++++------ This should be split in 2 patches (one adding traces to clk drivers and another doing it for the USB host driver), so that those patches can be merged independently by the clk and usb maintainers. > 3 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c > index 7d3223fc7161..534961766ae5 100644 > --- a/drivers/clk/at91/clk-pll.c > +++ b/drivers/clk/at91/clk-pll.c > @@ -133,6 +133,7 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw, > { > struct clk_pll *pll = to_clk_pll(hw); > unsigned int pllr; > + unsigned long recalcedrate; Just name it 'rate' or 'realrate'. > u16 mul; > u8 div; > > @@ -144,7 +145,10 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw *hw, > if (!div || !mul) > return 0; > > - return (parent_rate / div) * (mul + 1); > + recalcedrate = (parent_rate / div) * (mul + 1); > + pr_debug("clk-pll: calculating new rate, (%lu hz / %u) * %u = %lu hz\n", If the prefix is alway "clk-pll: " you could define pr_fmt() to add it to all your pr_xxx() messages. BTW, it's not about calculating the new rate, but retrieving the actual rate. > + parent_rate, div, mul, recalcedrate); Add an empty line here. > + return recalcedrate; > } > > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate, > diff --git a/drivers/clk/at91/clk-usb.c b/drivers/clk/at91/clk-usb.c > index 791770a563fc..2fa877e99bac 100644 > --- a/drivers/clk/at91/clk-usb.c > +++ b/drivers/clk/at91/clk-usb.c > @@ -48,11 +48,15 @@ static unsigned long at91sam9x5_clk_usb_recalc_rate(struct clk_hw *hw, > struct at91sam9x5_clk_usb *usb = to_at91sam9x5_clk_usb(hw); > unsigned int usbr; > u8 usbdiv; > + unsigned int calcdclock; Ditto: s/calcdclock/realrate/, and make it unsigned long. > > regmap_read(usb->regmap, AT91_PMC_USB, &usbr); > usbdiv = (usbr & AT91_PMC_OHCIUSBDIV) >> SAM9X5_USB_DIV_SHIFT; > > - return DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1)); > + calcdclock = DIV_ROUND_CLOSEST(parent_rate, (usbdiv + 1)); > + pr_debug("clk-usb: calculating new rate, %lu hz / %u = %u hz\n", > + parent_rate, usbdiv + 1, calcdclock); Empty line here too. > + return calcdclock; > } > > static int at91sam9x5_clk_usb_determine_rate(struct clk_hw *hw, > @@ -98,7 +102,6 @@ static int at91sam9x5_clk_usb_determine_rate(struct clk_hw *hw, > if (!best_diff) > break; > } > - > if (best_rate < 0) > return best_rate; > > @@ -142,6 +145,9 @@ static int at91sam9x5_clk_usb_set_rate(struct clk_hw *hw, unsigned long rate, > if (div > SAM9X5_USB_MAX_DIV + 1 || !div) > return -EINVAL; > > + pr_debug("clk-usb: setting USB clock divider to %lu hz / %lu = %lu hz\n", The formulation is weird. Maybe you should just remove 'divider' here: "setting USB clock to %lu hz / %lu = %lu hz" > + parent_rate, div, rate); > + > regmap_update_bits(usb->regmap, AT91_PMC_USB, AT91_PMC_OHCIUSBDIV, > (div - 1) << SAM9X5_USB_DIV_SHIFT); > > diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c > index 5ad9e9bdc8ee..c57a239918f9 100644 > --- a/drivers/usb/host/ohci-at91.c > +++ b/drivers/usb/host/ohci-at91.c > @@ -70,11 +70,13 @@ static const struct ohci_driver_overrides ohci_at91_drv_overrides __initconst = > > /*-------------------------------------------------------------------------*/ > > -static void at91_start_clock(struct ohci_at91_priv *ohci_at91) > +static void at91_start_clock(struct ohci_at91_priv *ohci_at91, > + struct device *dev) Maybe you could just pass a pdev or dev pointer and let the function extract the ohci_at91 object with: struct usb_hcd *hcd = platform_get_drvdata(pdev); struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd); > { > if (ohci_at91->clocked) > return; > > + dev_dbg(dev, "Enabling hclk, iclk, and setting fclk to 48 Mhz\n"); Add an empty line here. > clk_set_rate(ohci_at91->fclk, 48000000); > clk_prepare_enable(ohci_at91->hclk); > clk_prepare_enable(ohci_at91->iclk); > @@ -82,11 +84,13 @@ static void at91_start_clock(struct ohci_at91_priv *ohci_at91) > ohci_at91->clocked = true; > } > > -static void at91_stop_clock(struct ohci_at91_priv *ohci_at91) > +static void at91_stop_clock(struct ohci_at91_priv *ohci_at91, > + struct device *dev) > { > if (!ohci_at91->clocked) > return; > > + dev_dbg(dev, "Disabling hclk, iclk, and fclk\n"); > clk_disable_unprepare(ohci_at91->fclk); > clk_disable_unprepare(ohci_at91->iclk); > clk_disable_unprepare(ohci_at91->hclk); > @@ -104,7 +108,7 @@ static void at91_start_hc(struct platform_device *pdev) > /* > * Start the USB clocks. > */ > - at91_start_clock(ohci_at91); > + at91_start_clock(ohci_at91, &pdev->dev); > > /* > * The USB host controller must remain in reset. > @@ -128,7 +132,7 @@ static void at91_stop_hc(struct platform_device *pdev) > /* > * Stop the USB clocks. > */ > - at91_stop_clock(ohci_at91); > + at91_stop_clock(ohci_at91, &pdev->dev); > } > > > @@ -623,7 +627,7 @@ ohci_hcd_at91_drv_suspend(struct device *dev) > > /* flush the writes */ > (void) ohci_readl (ohci, &ohci->regs->control); > - at91_stop_clock(ohci_at91); > + at91_stop_clock(ohci_at91, dev); > } > > return ret; > @@ -638,7 +642,7 @@ ohci_hcd_at91_drv_resume(struct device *dev) > if (ohci_at91->wakeup) > disable_irq_wake(hcd->irq); > > - at91_start_clock(ohci_at91); > + at91_start_clock(ohci_at91, dev); > > ohci_resume(hcd, false); > Regards, Boris