linux-kernel - Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7hfunly4hn.fsf@baylibre.com>
Date:   Mon, 24 Oct 2016 11:41:08 -0700
From:   Kevin Hilman <khilman@...libre.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Sekhar Nori <nsekhar@...com>, Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Peter Ujfalusi <peter.ujfalusi@...com>,
        Russell King <linux@...linux.org.uk>,
        LKML <linux-kernel@...r.kernel.org>,
        arm-soc <linux-arm-kernel@...ts.infradead.org>,
        linux-drm <dri-devel@...ts.freedesktop.org>,
        linux-devicetree <devicetree@...r.kernel.org>,
        Jyri Sarha <jsarha@...com>,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        David Airlie <airlied@...ux.ie>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [RFC] ARM: memory: da8xx-ddrctl: new driver

Mark Rutland <mark.rutland@....com> writes:

> On Mon, Oct 24, 2016 at 10:35:30AM -0700, Kevin Hilman wrote:
>> Hi Mark,
>> 
>> Mark Rutland <mark.rutland@....com> 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