From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kishon Vijay Abraham I Subject: Re: [PATCH v3 RESEND 2/2] phy: add Rockchip Innosilicon hdmi phy Date: Thu, 6 Sep 2018 15:08:35 +0530 Message-ID: References: <20180710134949.25203-1-heiko@sntech.de> <4194245.uYZyAYm7Hd@phil> <21954745.lUUHsRAzaY@diego> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <21954745.lUUHsRAzaY@diego> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: =?UTF-8?Q?Heiko_St=c3=bcbner?= Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org, robh+dt@kernel.org, robin.murphy@arm.com, linux-arm-kernel@lists.infradead.org, zhengyang@rock-chips.com List-Id: devicetree@vger.kernel.org CgpPbiBGcmlkYXkgMzEgQXVndXN0IDIwMTggMDI6NTYgUE0sIEhlaWtvIFN0w7xibmVyIHdyb3Rl Ogo+IEFtIERpZW5zdGFnLCAxNC4gQXVndXN0IDIwMTgsIDE0OjUyOjAwIENFU1Qgc2NocmllYiBI ZWlrbyBTdHVlYm5lcjoKPj4gSGkgS2lzaG9uLAo+Pgo+PiB0aGFua3MgZm9yIHlvdXIgcmV2aWV3 Lgo+Pgo+PiBBbSBGcmVpdGFnLCAzLiBBdWd1c3QgMjAxOCwgMTE6MjQ6MDYgQ0VTVCBzY2hyaWVi IEtpc2hvbiBWaWpheSBBYnJhaGFtIEk6Cj4+PiBPbiBUdWVzZGF5IDEwIEp1bHkgMjAxOCAwNzox OSBQTSwgSGVpa28gU3R1ZWJuZXIgd3JvdGU6Cj4+Pj4gK2NvbmZpZyBQSFlfUk9DS0NISVBfSU5O T19IRE1JCj4+Pj4gKwl0cmlzdGF0ZSAiUm9ja2NoaXAgSU5OTyBIRE1JIFBIWSBEcml2ZXIiCj4+ Pj4gKwlkZXBlbmRzIG9uIChBUkNIX1JPQ0tDSElQIHx8IENPTVBJTEVfVEVTVCkgJiYgT0YKPj4+ Cj4+PiBkZXBlbmRzIG9uIENPTU1PTl9DTEsgc2luY2UgdGhlIHBoeSByZWdpc3RlcnMgYSBjbG9j ayBwcm92aWRlcj8KPj4KPj4gb2sKPj4KPj4+PiArc3RhdGljIGNvbnN0IHN0cnVjdCBwaHlfY29u ZmlnIHJrMzIyOF9waHlfY2ZnW10gPSB7Cj4+Pj4gKwl7CTE2NTAwMDAwMCwgewo+Pj4+ICsJCQkw eGFhLCAweDAwLCAweDQ0LCAweDQ0LCAweDAwLCAweDAwLCAweDAwLCAweDAwLCAweDAwLAo+Pj4+ ICsJCQkweDAwLCAweDAwLCAweDAwLCAweDAwLCAweDAwLAo+Pj4KPj4+IElmIHRoZXNlIHJlZ2lz dGVyIGNvbmZpZ3VyYXRpb25zIGFyZSBub3QgYSB0dW5pbmcgdmFsdWUgb3IgZGl2aWRlcnMgKHdo aWNoCj4+PiBjYW4gaGF2ZSBhYnNvbHV0ZSB2YWx1ZXMpLCB0aGVuIHdlIHNob3VsZCBoYXZlIG1h Y3JvcyBmb3IgZWFjaCBvZiB0aGVzZQo+Pj4gY29uZmlndXJhdGlvbnMuCj4+IFRoYXQgbWlnaHQg Z2V0IGRpZmZpY3VsdC4KPj4KPj4gVGhhdCBwaHkgcmVnaXN0ZXIgc2V0IGlzIGNvbXBsZXRlbHkg dW5kb2N1bWVudGVkIGV2ZW4gaW4gdGhlIHZlbmRvciBUUk1zCj4+IG9mIHRoZSAyIHNvY3MuIEkg cGllY2VkIHRvZ2V0aGVyIG1vc3QgZXhpc3RpbmcgbWFjcm9zIGZyb20gY29kZSBjb21tZW50cwo+ PiBhbmQgc3VjaCBmcm9tIHRoZSB2ZW5kb3Iga2VybmVsLiBBbmQgSW5ub3NpbGljb24gaXMgbm90 IGtub3duIHRvIHdpbGxpbmdseQo+PiBzaGFyZSBtdWNoIG9mIHRoZWlyIHJlZ2lzdGVyIGRvY3Vt ZW50YXRpb24gaXQgc2VlbXMuCj4+Cj4+Pj4gK3N0YXRpYyB1bnNpZ25lZCBsb25nIGlubm9faGRt aV9waHlfZ2V0X3RtZHNjbGsoc3RydWN0IGlubm9faGRtaV9waHkKPj4+PiAqaW5ubywKPj4+PiAr CQkJCQkgICAgICAgdW5zaWduZWQgbG9uZyByYXRlKQo+Pj4+ICt7Cj4+Pj4gKwlpbnQgYnVzX3dp ZHRoID0gcGh5X2dldF9idXNfd2lkdGgoaW5uby0+cGh5KTsKPj4+Cj4+PiBobW0sIHRoZSBidXNf d2lkdGggY291bGQgYmUgYSBtZW1iZXIgb2Ygc3RydWN0IGlubm9faGRtaV9waHkuIFBIWSBBUEkn cwo+Pj4gZG9uJ3QgaGF2ZSB0byBiZSB1c2VkIGZvciBnZXR0aW5nIGRhdGEgd2l0aGluIHRoZSBQ SFkgZHJpdmVyIGl0c2VsZi4KPj4+IExvb2tpbmcgYXQgdGhlIHBoeV9nZXRfYnVzX3dpZHRoKCkg aW1wbGVtZW50YXRpb24sIHdlIHNob3VsZCBoYXZlCj4+PiBwcm90ZWN0ZWQKPj4+IGJ1cy13aWR0 aCBzZXQgYW5kIGdldCB3aXRoIG11dGV4LiBXaXRoIHRoYXQgdGhlcmUgbWlnaHQgYmUgYSBkZWFk bG9jawo+Pj4gaGVyZS4KPj4KPj4gb2ssIHNvIGp1c3QgcmVhZCBwaHktPmF0dHJzLmJ1c193aWR0 aCBkaXJlY3RseSBoZXJlPwo+Pgo+PiBJIGRvbid0IHNlZSBob3cgdGhpcyBjYW4gYmUgcGFydCBv ZiBzdHJ1Y3QgaW5ub19oZG1pX3BoeSwgYXMgdGhlIGJ1cy13aWR0aAo+PiBkZXBlbmRzIG9uIHRo ZSBjb2xvci1kZXB0aCB1c2VkIG9uIHRoZSBoZG1pLWNvbnRyb2xsZXIgc2lkZSwgc28KPj4gZGVw ZW5kaW5nIG9uIHRoYXQgaXQgd2lsbCB3YW50IHRvIGFkYXB0IHRoZSBwaHkncyBidXNfd2lkdGgu Cj4+Cj4+IFJpZ2h0IG5vdyBvdXIgZHdfaGRtaSBpbiBtYWlubGluZSBvbmx5IHN1cHBvcnRzIG9u ZSBvdXRwdXQgZm9ybWF0Cj4+IHJlcXVpcmluZyBhIGJ1c193aWR0aCBvZiA4LCBidXQgdGhlIHZl bmRvciBrZXJuZWwgc2hvd3MgWzBdIGhvdyB0aGlzCj4+IHdhbnRzIHRvIGJlIHVzZWQgd2l0aCBt dWx0aXBsZSBvdXRwdXQgZm9ybWF0cy4KCklnbm9yZSBteSBjb21tZW50LiBZb3UgY2FuIHVzZSBw aHlfZ2V0X2J1c193aWR0aC4gSSdsbCBzZWUgaWYgdGhlcmUncyBhIGJldHRlcgp3YXkgdG8gaGFu ZGxlIHRoaXMuCj4+Cj4+IFswXQo+PiBodHRwczovL2dpdGh1Yi5jb20vcm9ja2NoaXAtbGludXgv a2VybmVsL2Jsb2IvcmVsZWFzZS00LjQvZHJpdmVycy9ncHUvZHJtL3IKPj4gb2NrY2hpcC9kd19o ZG1pLXJvY2tjaGlwLmMjTDgxNwo+Pj4+ICtzdGF0aWMgaXJxcmV0dXJuX3QgaW5ub19oZG1pX3Bo eV9yazMzMjhfaXJxKGludCBpcnEsIHZvaWQgKmRldl9pZCkKPj4+PiArewo+Pj4+ICsJc3RydWN0 IGlubm9faGRtaV9waHkgKmlubm8gPSBkZXZfaWQ7Cj4+Pj4gKwo+Pj4+ICsJaW5ub191cGRhdGVf Yml0cyhpbm5vLCAweDAyLCBSSzMzMjhfUERBVEFfRU4sIDApOwo+Pj4+ICsJdXNsZWVwX3Jhbmdl KDksIDEwKTsKPj4+Cj4+PiBUaGlzIHJhbmdlIGxvb2tzIHZlcnkgbmFycm93LiAxMCB0byAyMCBz aG91bGQgYmUgb2theT8KPj4KPj4gb2sKPj4KPj4+PiArc3RhdGljIHZvaWQgaW5ub19oZG1pX3Bo eV9hY3Rpb24odm9pZCAqZGF0YSkKPj4+PiArewo+Pj4+ICsJc3RydWN0IGlubm9faGRtaV9waHkg Kmlubm8gPSBkYXRhOwo+Pj4+ICsKPj4+PiArCWNsa19kaXNhYmxlX3VucHJlcGFyZShpbm5vLT5y ZWZwY2xrKTsKPj4+PiArCWNsa19kaXNhYmxlX3VucHJlcGFyZShpbm5vLT5zeXNjbGspOwo+Pj4+ ICt9Cj4+Pj4gKwo+Pj4+ICtzdGF0aWMgaW50IGlubm9faGRtaV9waHlfcHJvYmUoc3RydWN0IHBs YXRmb3JtX2RldmljZSAqcGRldikKPj4+PiArewo+Pj4+ICsJc3RydWN0IGlubm9faGRtaV9waHkg Kmlubm87Cj4+Pj4gKwljb25zdCBzdHJ1Y3Qgb2ZfZGV2aWNlX2lkICptYXRjaDsKPj4+PiArCXN0 cnVjdCBwaHlfcHJvdmlkZXIgKnBoeV9wcm92aWRlcjsKPj4+PiArCXN0cnVjdCByZXNvdXJjZSAq cmVzOwo+Pj4+ICsJdm9pZCBfX2lvbWVtICpyZWdzOwo+Pj4+ICsJaW50IHJldDsKPj4+PiArCj4+ Cj4+IFsuLi5dCj4+Cj4+Pj4gKwkvKgo+Pj4+ICsJICogUmVmcGNsayBuZWVkcyB0byBiZSBvbiwg b24gYXQgbGVhc3QgdGhlIHJrMzMyOCBmb3Igc3RpbGwKPj4+PiArCSAqIHVua25vd24gcmVhc29u cy4KPj4+PiArCSAqLwo+Pj4+ICsJcmV0ID0gY2xrX3ByZXBhcmVfZW5hYmxlKGlubm8tPnJlZnBj bGspOwo+Pj4+ICsJaWYgKHJldCkgewo+Pj4+ICsJCWRldl9lcnIoaW5uby0+ZGV2LCAiZmFpbGVk IHRvIGVuYWJsZSByZWZwY2xrXG4iKTsKPj4+PiArCQljbGtfZGlzYWJsZV91bnByZXBhcmUoaW5u by0+c3lzY2xrKTsKPj4+PiArCQlyZXR1cm4gcmV0Owo+Pj4+ICsJfQo+Pj4+ICsKPj4+PiArCXJl dCA9IGRldm1fYWRkX2FjdGlvbl9vcl9yZXNldChpbm5vLT5kZXYsIGlubm9faGRtaV9waHlfYWN0 aW9uLAo+Pj4+ICsJCQkJICAgICAgIGlubm8pOwo+Pj4+ICsJaWYgKHJldCkgewo+Pj4+ICsJCWNs a19kaXNhYmxlX3VucHJlcGFyZShpbm5vLT5yZWZwY2xrKTsKPj4+PiArCQljbGtfZGlzYWJsZV91 bnByZXBhcmUoaW5uby0+c3lzY2xrKTsKPj4+PiArCQlyZXR1cm4gcmV0Owo+Pj4+ICsJfQo+Pj4+ ICsKPj4+PiArCWlubm8tPnJlZ21hcCA9IGRldm1fcmVnbWFwX2luaXRfbW1pbyhpbm5vLT5kZXYs IHJlZ3MsCj4+Pj4gKwkJCQkJICAgICAmaW5ub19oZG1pX3BoeV9yZWdtYXBfY29uZmlnKTsKPj4+ PiArCWlmIChJU19FUlIoaW5uby0+cmVnbWFwKSkKPj4+Cj4+PiBoZXJlIHRvbyBjbGtfZGlzYWJs ZV91bnByZXBhcmUgYW5kIGFsbCBlcnJvciBoYW5kbGluZyBiZWxvdz8KPj4+IEl0J3MgYmV0dGVy IGlmIHdlIGp1c3QgaGFuZGxlIGVycm9yIGhhbmRsaW5nIGF0IHRoZSBib3R0b20gb2YgdGhlCj4+ PiBmdW5jdGlvbi4KPj4KPj4gTm9wZSA7LSkgLi4uIEkuZS4gdGhlIGdvYWwgb2YgcmVnaXN0ZXJp bmcgdGhlIGRldm1fYWN0aW9uIGFib3ZlIGlzIHRvIGhhdmUKPj4gdGhlIGNsb2NrcyBnZXQgZGlz YWJsZWQgaW4gdGhlIGNvcnJlY3Qgb3JkZXIgd2hlbiBkZXZtIHVuZG9lcyB0aGUgb3RoZXIKPj4g dGhpbmdzIGluIHNlcXVlbmNlLgo+Pgo+PiBNYW51YWwgZXJyb3IgaGFuZGxpbmcgZm9yIHRoZSBj bG9ja3Mgd291bGQgYWxzbyBicmVhayBkZXZtLCBhcyB0aGVuCj4+IHRoZSBjbG9ja3Mgd291bGQg Z2V0IGRpc2FibGVkIGJlZm9yZSB0aGUgZGV2bSBjbGVhbnVwIGtpY2tzIGluLgo+IAo+IGJ1dCBJ IGp1c3QgcmVhbGl6ZWQsIHRoYXQgSSBkb24ndCBuZWVkIHRoZSBpbml0aWFsIGRpc2FibGVfdW5w cmVwYXJlIGNhbGxzCj4gYW55bW9yZSBhcyB3ZWxsLCBhcyBkZXZtX2FkZF9hY3Rpb25fb3JfcmVz ZXQgd2lsbCBjYWxsIHRoYXQgaXRzZWxmCj4gaW4gdGhlIGVycm9yIGNhc2UuCgpBbGwgcmlnaHQh IEknbGwgbWVyZ2Ugb25jZSB5b3Ugc2VuZCBhIHBhdGNoIGZpeGluZyB0aGlzLgoKVGhhbmtzCktp c2hvbgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGlu dXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRl YWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgt YXJtLWtlcm5lbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: kishon@ti.com (Kishon Vijay Abraham I) Date: Thu, 6 Sep 2018 15:08:35 +0530 Subject: [PATCH v3 RESEND 2/2] phy: add Rockchip Innosilicon hdmi phy In-Reply-To: <21954745.lUUHsRAzaY@diego> References: <20180710134949.25203-1-heiko@sntech.de> <4194245.uYZyAYm7Hd@phil> <21954745.lUUHsRAzaY@diego> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday 31 August 2018 02:56 PM, Heiko St?bner wrote: > Am Dienstag, 14. August 2018, 14:52:00 CEST schrieb Heiko Stuebner: >> Hi Kishon, >> >> thanks for your review. >> >> Am Freitag, 3. August 2018, 11:24:06 CEST schrieb Kishon Vijay Abraham I: >>> On Tuesday 10 July 2018 07:19 PM, Heiko Stuebner wrote: >>>> +config PHY_ROCKCHIP_INNO_HDMI >>>> + tristate "Rockchip INNO HDMI PHY Driver" >>>> + depends on (ARCH_ROCKCHIP || COMPILE_TEST) && OF >>> >>> depends on COMMON_CLK since the phy registers a clock provider? >> >> ok >> >>>> +static const struct phy_config rk3228_phy_cfg[] = { >>>> + { 165000000, { >>>> + 0xaa, 0x00, 0x44, 0x44, 0x00, 0x00, 0x00, 0x00, 0x00, >>>> + 0x00, 0x00, 0x00, 0x00, 0x00, >>> >>> If these register configurations are not a tuning value or dividers (which >>> can have absolute values), then we should have macros for each of these >>> configurations. >> That might get difficult. >> >> That phy register set is completely undocumented even in the vendor TRMs >> of the 2 socs. I pieced together most existing macros from code comments >> and such from the vendor kernel. And Innosilicon is not known to willingly >> share much of their register documentation it seems. >> >>>> +static unsigned long inno_hdmi_phy_get_tmdsclk(struct inno_hdmi_phy >>>> *inno, >>>> + unsigned long rate) >>>> +{ >>>> + int bus_width = phy_get_bus_width(inno->phy); >>> >>> hmm, the bus_width could be a member of struct inno_hdmi_phy. PHY API's >>> don't have to be used for getting data within the PHY driver itself. >>> Looking at the phy_get_bus_width() implementation, we should have >>> protected >>> bus-width set and get with mutex. With that there might be a deadlock >>> here. >> >> ok, so just read phy->attrs.bus_width directly here? >> >> I don't see how this can be part of struct inno_hdmi_phy, as the bus-width >> depends on the color-depth used on the hdmi-controller side, so >> depending on that it will want to adapt the phy's bus_width. >> >> Right now our dw_hdmi in mainline only supports one output format >> requiring a bus_width of 8, but the vendor kernel shows [0] how this >> wants to be used with multiple output formats. Ignore my comment. You can use phy_get_bus_width. I'll see if there's a better way to handle this. >> >> [0] >> https://github.com/rockchip-linux/kernel/blob/release-4.4/drivers/gpu/drm/r >> ockchip/dw_hdmi-rockchip.c#L817 >>>> +static irqreturn_t inno_hdmi_phy_rk3328_irq(int irq, void *dev_id) >>>> +{ >>>> + struct inno_hdmi_phy *inno = dev_id; >>>> + >>>> + inno_update_bits(inno, 0x02, RK3328_PDATA_EN, 0); >>>> + usleep_range(9, 10); >>> >>> This range looks very narrow. 10 to 20 should be okay? >> >> ok >> >>>> +static void inno_hdmi_phy_action(void *data) >>>> +{ >>>> + struct inno_hdmi_phy *inno = data; >>>> + >>>> + clk_disable_unprepare(inno->refpclk); >>>> + clk_disable_unprepare(inno->sysclk); >>>> +} >>>> + >>>> +static int inno_hdmi_phy_probe(struct platform_device *pdev) >>>> +{ >>>> + struct inno_hdmi_phy *inno; >>>> + const struct of_device_id *match; >>>> + struct phy_provider *phy_provider; >>>> + struct resource *res; >>>> + void __iomem *regs; >>>> + int ret; >>>> + >> >> [...] >> >>>> + /* >>>> + * Refpclk needs to be on, on at least the rk3328 for still >>>> + * unknown reasons. >>>> + */ >>>> + ret = clk_prepare_enable(inno->refpclk); >>>> + if (ret) { >>>> + dev_err(inno->dev, "failed to enable refpclk\n"); >>>> + clk_disable_unprepare(inno->sysclk); >>>> + return ret; >>>> + } >>>> + >>>> + ret = devm_add_action_or_reset(inno->dev, inno_hdmi_phy_action, >>>> + inno); >>>> + if (ret) { >>>> + clk_disable_unprepare(inno->refpclk); >>>> + clk_disable_unprepare(inno->sysclk); >>>> + return ret; >>>> + } >>>> + >>>> + inno->regmap = devm_regmap_init_mmio(inno->dev, regs, >>>> + &inno_hdmi_phy_regmap_config); >>>> + if (IS_ERR(inno->regmap)) >>> >>> here too clk_disable_unprepare and all error handling below? >>> It's better if we just handle error handling at the bottom of the >>> function. >> >> Nope ;-) ... I.e. the goal of registering the devm_action above is to have >> the clocks get disabled in the correct order when devm undoes the other >> things in sequence. >> >> Manual error handling for the clocks would also break devm, as then >> the clocks would get disabled before the devm cleanup kicks in. > > but I just realized, that I don't need the initial disable_unprepare calls > anymore as well, as devm_add_action_or_reset will call that itself > in the error case. All right! I'll merge once you send a patch fixing this. Thanks Kishon