From mboxrd@z Thu Jan 1 00:00:00 1970 From: khilman@baylibre.com (Kevin Hilman) Date: Mon, 24 Oct 2016 11:41:08 -0700 Subject: [RFC] ARM: memory: da8xx-ddrctl: new driver In-Reply-To: <20161024174249.GQ15620@leverpostej> (Mark Rutland's message of "Mon, 24 Oct 2016 18:42:49 +0100") References: <1477327596-16060-1-git-send-email-bgolaszewski@baylibre.com> <1477327596-16060-2-git-send-email-bgolaszewski@baylibre.com> <20161024170035.GO15620@leverpostej> <7hd1ipzm3h.fsf@baylibre.com> <20161024174249.GQ15620@leverpostej> Message-ID: <7hfunly4hn.fsf@baylibre.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Mark Rutland writes: > On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote: >> Hi Mark, >> >> Mark Rutland writes: >> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote: >> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev) >> >> +{ >> >> + const struct da8xx_ddrctl_config_knob *knob; >> >> + const struct da8xx_ddrctl_setting *setting; >> >> + u32 regprop[2], base, memsize, reg; >> >> + struct device_node *node, *parent; >> >> + void __iomem *ddrctl; >> >> + const char *board; >> >> + struct device *dev; >> >> + int ret; >> >> + >> >> + dev = &pdev->dev; >> >> + node = dev->of_node; >> >> + >> >> + /* Find the board name. */ >> >> + for (parent = node; >> >> + !of_node_is_root(parent); >> >> + parent = of_get_parent(parent)); >> >> + >> >> + ret = of_property_read_string(parent, "compatible", &board); >> >> + if (ret) { >> >> + dev_err(dev, "unable to read the soc model\n"); >> >> + return ret; >> >> + } >> > >> > I can see that you want to expose sysfs knobs for this, but is it really >> > necessary to match boards like this? It's very fragile, and commits us >> > to maintaining a database of board data (i.e. a board file). >> > >> > I am very much not keen on that. >> >> The original proposal[1] was to create DT properties reflecting the >> various knobs in the DDR Controller, but that was frowned upon since >> that was more HW configuration than hardware description. >> >> That resulted in this approach which keeps the HW configuration values >> in the driver, and selectable based on DT compatible. >> >> IMO, neither aproach is pretty. From a DT maintainer perspective, can >> you comment on your preference? > > From my PoV, it really depends on *why* we need this. What does the > tuning gain us, and is it specific to a given use-case? This is essentially a bus controller which allows you to set relative priorities of the various bus masters (CPU data, CPU instructions, DMA channels, ethernet MAC, SATA, display controller, etc. etc.) The reason behind this work in the first place is a specific use-case. Namely, a display controller on this SoC needs its bus priority to be adjusted in order to work reliably at certain resolutions The vendor BSPs for this chip just setup hard-coded values in the board files (davinci, still hasn't fully migrated to DT) but we're trying to figure out a better way. The first approach was exposing DT knobs for all the priorities. The second approach was the other extermem allowing SoC or board-specific hard-coded values. Another possible approach would be getting rid of the hard-coded values and exporting an API from the driver to set the priorities of the available masters at run-time. I'm not awarye any current need of changing things at run-time, but it seems potentially useful as well. Based on all this discussion, I'm starting to lean towards the runtime API approach, but am hoping for some suggestions. Kevin From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [RFC] ARM: memory: da8xx-ddrctl: new driver Date: Mon, 24 Oct 2016 11:41:08 -0700 Message-ID: <7hfunly4hn.fsf@baylibre.com> References: <1477327596-16060-1-git-send-email-bgolaszewski@baylibre.com> <1477327596-16060-2-git-send-email-bgolaszewski@baylibre.com> <20161024170035.GO15620@leverpostej> <7hd1ipzm3h.fsf@baylibre.com> <20161024174249.GQ15620@leverpostej> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20161024174249.GQ15620@leverpostej> (Mark Rutland's message of "Mon, 24 Oct 2016 18:42:49 +0100") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Mark Rutland Cc: linux-devicetree , Tomi Valkeinen , Michael Turquette , Sekhar Nori , Russell King , linux-drm , LKML , Peter Ujfalusi , Bartosz Golaszewski , Rob Herring , Jyri Sarha , Frank Rowand , arm-soc , Laurent Pinchart List-Id: devicetree@vger.kernel.org TWFyayBSdXRsYW5kIDxtYXJrLnJ1dGxhbmRAYXJtLmNvbT4gd3JpdGVzOgoKPiBPbiBNb24sIE9j dCAyNCwgMjAxNiBhdCAxMDozNTozMEFNIC0wNzAwLCBLZXZpbiBIaWxtYW4gd3JvdGU6Cj4+IEhp IE1hcmssCj4+IAo+PiBNYXJrIFJ1dGxhbmQgPG1hcmsucnV0bGFuZEBhcm0uY29tPiB3cml0ZXM6 Cj4+ID4gT24gTW9uLCBPY3QgMjQsIDIwMTYgYXQgMDY6NDY6MzZQTSArMDIwMCwgQmFydG9zeiBH b2xhc3pld3NraSB3cm90ZToKPj4gPj4gK3N0YXRpYyBpbnQgZGE4eHhfZGRyY3RsX3Byb2JlKHN0 cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpCj4+ID4+ICt7Cj4+ID4+ICsJY29uc3Qgc3RydWN0 IGRhOHh4X2RkcmN0bF9jb25maWdfa25vYiAqa25vYjsKPj4gPj4gKwljb25zdCBzdHJ1Y3QgZGE4 eHhfZGRyY3RsX3NldHRpbmcgKnNldHRpbmc7Cj4+ID4+ICsJdTMyIHJlZ3Byb3BbMl0sIGJhc2Us IG1lbXNpemUsIHJlZzsKPj4gPj4gKwlzdHJ1Y3QgZGV2aWNlX25vZGUgKm5vZGUsICpwYXJlbnQ7 Cj4+ID4+ICsJdm9pZCBfX2lvbWVtICpkZHJjdGw7Cj4+ID4+ICsJY29uc3QgY2hhciAqYm9hcmQ7 Cj4+ID4+ICsJc3RydWN0IGRldmljZSAqZGV2Owo+PiA+PiArCWludCByZXQ7Cj4+ID4+ICsKPj4g Pj4gKwlkZXYgPSAmcGRldi0+ZGV2Owo+PiA+PiArCW5vZGUgPSBkZXYtPm9mX25vZGU7Cj4+ID4+ ICsKPj4gPj4gKwkvKiBGaW5kIHRoZSBib2FyZCBuYW1lLiAqLwo+PiA+PiArCWZvciAocGFyZW50 ID0gbm9kZTsKPj4gPj4gKwkgICAgICFvZl9ub2RlX2lzX3Jvb3QocGFyZW50KTsKPj4gPj4gKwkg ICAgIHBhcmVudCA9IG9mX2dldF9wYXJlbnQocGFyZW50KSk7Cj4+ID4+ICsKPj4gPj4gKwlyZXQg PSBvZl9wcm9wZXJ0eV9yZWFkX3N0cmluZyhwYXJlbnQsICJjb21wYXRpYmxlIiwgJmJvYXJkKTsK Pj4gPj4gKwlpZiAocmV0KSB7Cj4+ID4+ICsJCWRldl9lcnIoZGV2LCAidW5hYmxlIHRvIHJlYWQg dGhlIHNvYyBtb2RlbFxuIik7Cj4+ID4+ICsJCXJldHVybiByZXQ7Cj4+ID4+ICsJfQo+PiA+Cj4+ ID4gSSBjYW4gc2VlIHRoYXQgeW91IHdhbnQgdG8gZXhwb3NlIHN5c2ZzIGtub2JzIGZvciB0aGlz LCBidXQgaXMgaXQgcmVhbGx5Cj4+ID4gbmVjZXNzYXJ5IHRvIG1hdGNoIGJvYXJkcyBsaWtlIHRo aXM/IEl0J3MgdmVyeSBmcmFnaWxlLCBhbmQgY29tbWl0cyB1cwo+PiA+IHRvIG1haW50YWluaW5n IGEgZGF0YWJhc2Ugb2YgYm9hcmQgZGF0YSAoaS5lLiBhIGJvYXJkIGZpbGUpLgo+PiA+Cj4+ID4g SSBhbSB2ZXJ5IG11Y2ggbm90IGtlZW4gb24gdGhhdC4KPj4gCj4+IFRoZSBvcmlnaW5hbCBwcm9w b3NhbFsxXSB3YXMgdG8gY3JlYXRlIERUIHByb3BlcnRpZXMgcmVmbGVjdGluZyB0aGUKPj4gdmFy aW91cyBrbm9icyBpbiB0aGUgRERSIENvbnRyb2xsZXIsIGJ1dCB0aGF0IHdhcyBmcm93bmVkIHVw b24gc2luY2UKPj4gdGhhdCB3YXMgbW9yZSBIVyBjb25maWd1cmF0aW9uIHRoYW4gaGFyZHdhcmUg ZGVzY3JpcHRpb24uCj4+IAo+PiBUaGF0IHJlc3VsdGVkIGluIHRoaXMgYXBwcm9hY2ggd2hpY2gg a2VlcHMgdGhlIEhXIGNvbmZpZ3VyYXRpb24gdmFsdWVzCj4+IGluIHRoZSBkcml2ZXIsIGFuZCBz ZWxlY3RhYmxlIGJhc2VkIG9uIERUIGNvbXBhdGlibGUuCj4+IAo+PiBJTU8sIG5laXRoZXIgYXBy b2FjaCBpcyBwcmV0dHkuICBGcm9tIGEgRFQgbWFpbnRhaW5lciBwZXJzcGVjdGl2ZSwgY2FuCj4+ IHlvdSBjb21tZW50IG9uIHlvdXIgcHJlZmVyZW5jZT8KPgo+IEZyb20gbXkgUG9WLCBpdCByZWFs bHkgZGVwZW5kcyBvbiAqd2h5KiB3ZSBuZWVkIHRoaXMuIFdoYXQgZG9lcyB0aGUKPiB0dW5pbmcg Z2FpbiB1cywgYW5kIGlzIGl0IHNwZWNpZmljIHRvIGEgZ2l2ZW4gdXNlLWNhc2U/CgpUaGlzIGlz IGVzc2VudGlhbGx5IGEgYnVzIGNvbnRyb2xsZXIgd2hpY2ggYWxsb3dzIHlvdSB0byBzZXQgcmVs YXRpdmUKcHJpb3JpdGllcyBvZiB0aGUgdmFyaW91cyBidXMgbWFzdGVycyAoQ1BVIGRhdGEsIENQ VSBpbnN0cnVjdGlvbnMsIERNQQpjaGFubmVscywgZXRoZXJuZXQgTUFDLCBTQVRBLCBkaXNwbGF5 IGNvbnRyb2xsZXIsIGV0Yy4gZXRjLikKClRoZSByZWFzb24gYmVoaW5kIHRoaXMgd29yayBpbiB0 aGUgZmlyc3QgcGxhY2UgaXMgYSBzcGVjaWZpYwp1c2UtY2FzZS4gTmFtZWx5LCBhIGRpc3BsYXkg Y29udHJvbGxlciBvbiB0aGlzIFNvQyBuZWVkcyBpdHMgYnVzCnByaW9yaXR5IHRvIGJlIGFkanVz dGVkIGluIG9yZGVyIHRvIHdvcmsgcmVsaWFibHkgYXQgY2VydGFpbgpyZXNvbHV0aW9ucwoKVGhl IHZlbmRvciBCU1BzIGZvciB0aGlzIGNoaXAganVzdCBzZXR1cCBoYXJkLWNvZGVkIHZhbHVlcyBp biB0aGUgYm9hcmQKZmlsZXMgKGRhdmluY2ksIHN0aWxsIGhhc24ndCBmdWxseSBtaWdyYXRlZCB0 byBEVCkgYnV0IHdlJ3JlIHRyeWluZyB0bwpmaWd1cmUgb3V0IGEgYmV0dGVyIHdheS4KClRoZSBm aXJzdCBhcHByb2FjaCB3YXMgZXhwb3NpbmcgRFQga25vYnMgZm9yIGFsbCB0aGUgcHJpb3JpdGll cy4gIFRoZQpzZWNvbmQgYXBwcm9hY2ggd2FzIHRoZSBvdGhlciBleHRlcm1lbSBhbGxvd2luZyBT b0Mgb3IgYm9hcmQtc3BlY2lmaWMKaGFyZC1jb2RlZCB2YWx1ZXMuCgpBbm90aGVyIHBvc3NpYmxl IGFwcHJvYWNoIHdvdWxkIGJlIGdldHRpbmcgcmlkIG9mIHRoZSBoYXJkLWNvZGVkIHZhbHVlcwph bmQgZXhwb3J0aW5nIGFuIEFQSSBmcm9tIHRoZSBkcml2ZXIgdG8gc2V0IHRoZSBwcmlvcml0aWVz IG9mIHRoZQphdmFpbGFibGUgbWFzdGVycyBhdCBydW4tdGltZS4gIEknbSBub3QgYXdhcnllIGFu eSBjdXJyZW50IG5lZWQgb2YKY2hhbmdpbmcgdGhpbmdzIGF0IHJ1bi10aW1lLCBidXQgaXQgc2Vl bXMgcG90ZW50aWFsbHkgdXNlZnVsIGFzIHdlbGwuCgpCYXNlZCBvbiBhbGwgdGhpcyBkaXNjdXNz aW9uLCBJJ20gc3RhcnRpbmcgdG8gbGVhbiB0b3dhcmRzIHRoZSBydW50aW1lCkFQSSBhcHByb2Fj aCwgYnV0IGFtIGhvcGluZyBmb3Igc29tZSBzdWdnZXN0aW9ucy4KCktldmluCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxp c3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNr dG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965180AbcJXSlP (ORCPT ); Mon, 24 Oct 2016 14:41:15 -0400 Received: from mail-pf0-f178.google.com ([209.85.192.178]:35205 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936474AbcJXSlM (ORCPT ); Mon, 24 Oct 2016 14:41:12 -0400 From: Kevin Hilman To: Mark Rutland Cc: Bartosz Golaszewski , Michael Turquette , Sekhar Nori , Rob Herring , Frank Rowand , Peter Ujfalusi , Russell King , LKML , arm-soc , linux-drm , linux-devicetree , Jyri Sarha , Tomi Valkeinen , David Airlie , Laurent Pinchart Subject: Re: [RFC] ARM: memory: da8xx-ddrctl: new driver Organization: BayLibre References: <1477327596-16060-1-git-send-email-bgolaszewski@baylibre.com> <1477327596-16060-2-git-send-email-bgolaszewski@baylibre.com> <20161024170035.GO15620@leverpostej> <7hd1ipzm3h.fsf@baylibre.com> <20161024174249.GQ15620@leverpostej> Date: Mon, 24 Oct 2016 11:41:08 -0700 In-Reply-To: <20161024174249.GQ15620@leverpostej> (Mark Rutland's message of "Mon, 24 Oct 2016 18:42:49 +0100") Message-ID: <7hfunly4hn.fsf@baylibre.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Mark Rutland writes: > On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote: >> Hi Mark, >> >> Mark Rutland writes: >> > On Mon, Oct 24, 2016 at 06:46:36PM +0200, Bartosz Golaszewski wrote: >> >> +static int da8xx_ddrctl_probe(struct platform_device *pdev) >> >> +{ >> >> + const struct da8xx_ddrctl_config_knob *knob; >> >> + const struct da8xx_ddrctl_setting *setting; >> >> + u32 regprop[2], base, memsize, reg; >> >> + struct device_node *node, *parent; >> >> + void __iomem *ddrctl; >> >> + const char *board; >> >> + struct device *dev; >> >> + int ret; >> >> + >> >> + dev = &pdev->dev; >> >> + node = dev->of_node; >> >> + >> >> + /* Find the board name. */ >> >> + for (parent = node; >> >> + !of_node_is_root(parent); >> >> + parent = of_get_parent(parent)); >> >> + >> >> + ret = of_property_read_string(parent, "compatible", &board); >> >> + if (ret) { >> >> + dev_err(dev, "unable to read the soc model\n"); >> >> + return ret; >> >> + } >> > >> > I can see that you want to expose sysfs knobs for this, but is it really >> > necessary to match boards like this? It's very fragile, and commits us >> > to maintaining a database of board data (i.e. a board file). >> > >> > I am very much not keen on that. >> >> The original proposal[1] was to create DT properties reflecting the >> various knobs in the DDR Controller, but that was frowned upon since >> that was more HW configuration than hardware description. >> >> That resulted in this approach which keeps the HW configuration values >> in the driver, and selectable based on DT compatible. >> >> IMO, neither aproach is pretty. From a DT maintainer perspective, can >> you comment on your preference? > > From my PoV, it really depends on *why* we need this. What does the > tuning gain us, and is it specific to a given use-case? This is essentially a bus controller which allows you to set relative priorities of the various bus masters (CPU data, CPU instructions, DMA channels, ethernet MAC, SATA, display controller, etc. etc.) The reason behind this work in the first place is a specific use-case. Namely, a display controller on this SoC needs its bus priority to be adjusted in order to work reliably at certain resolutions The vendor BSPs for this chip just setup hard-coded values in the board files (davinci, still hasn't fully migrated to DT) but we're trying to figure out a better way. The first approach was exposing DT knobs for all the priorities. The second approach was the other extermem allowing SoC or board-specific hard-coded values. Another possible approach would be getting rid of the hard-coded values and exporting an API from the driver to set the priorities of the available masters at run-time. I'm not awarye any current need of changing things at run-time, but it seems potentially useful as well. Based on all this discussion, I'm starting to lean towards the runtime API approach, but am hoping for some suggestions. Kevin