[PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Timur Tabi]

Add code that programs the DMA and SSI controllers differently based on the
FIFO depth of the SSI.

The SSI devices on the MPC8610 and the P1022 are identical in every way except
one: the transmit and receive FIFO depth.  On the MPC8610, the depth is eight.
On the P1022, it's fifteen.  The device tree nodes for the SSI include a
"fsl,fifo-depth" property that specifies the FIFO depth.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/fsl/fsl_dma.c |   69 ++++++++++++++++++++++++++++++++++++-----------
 sound/soc/fsl/fsl_ssi.c |   27 ++++++++++++++++--
 2 files changed, 77 insertions(+), 19 deletions(-)

diff --git a/sound/soc/fsl/fsl_dma.c b/sound/soc/fsl/fsl_dma.c
index 57774cb..2f1461a 100644
--- a/sound/soc/fsl/fsl_dma.c
+++ b/sound/soc/fsl/fsl_dma.c
@@ -60,6 +60,7 @@ struct dma_object {
 	struct snd_soc_platform_driver dai;
 	dma_addr_t ssi_stx_phys;
 	dma_addr_t ssi_srx_phys;
+	unsigned int ssi_fifo_depth;
 	struct ccsr_dma_channel __iomem *channel;
 	unsigned int irq;
 	bool assigned;
@@ -99,6 +100,7 @@ struct fsl_dma_private {
 	unsigned int irq;
 	struct snd_pcm_substream *substream;
 	dma_addr_t ssi_sxx_phys;
+	unsigned int ssi_fifo_depth;
 	dma_addr_t ld_buf_phys;
 	unsigned int current_link;
 	dma_addr_t dma_buf_phys;
@@ -431,6 +433,7 @@ static int fsl_dma_open(struct snd_pcm_substream *substream)
 	else
 		dma_private->ssi_sxx_phys = dma->ssi_srx_phys;
 
+	dma_private->ssi_fifo_depth = dma->ssi_fifo_depth;
 	dma_private->dma_channel = dma->channel;
 	dma_private->irq = dma->irq;
 	dma_private->substream = substream;
@@ -544,11 +547,11 @@ static int fsl_dma_hw_params(struct snd_pcm_substream *substream,
 	struct device *dev = rtd->platform->dev;
 
 	/* Number of bits per sample */
-	unsigned int sample_size =
+	unsigned int sample_bits =
 		snd_pcm_format_physical_width(params_format(hw_params));
 
 	/* Number of bytes per frame */
-	unsigned int frame_size = 2 * (sample_size / 8);
+	unsigned int sample_bytes = sample_bits / 8;
 
 	/* Bus address of SSI STX register */
 	dma_addr_t ssi_sxx_phys = dma_private->ssi_sxx_phys;
@@ -588,7 +591,7 @@ static int fsl_dma_hw_params(struct snd_pcm_substream *substream,
 	 * that offset here.  While we're at it, also tell the DMA controller
 	 * how much data to transfer per sample.
 	 */
-	switch (sample_size) {
+	switch (sample_bits) {
 	case 8:
 		mr |= CCSR_DMA_MR_DAHTS_1 | CCSR_DMA_MR_SAHTS_1;
 		ssi_sxx_phys += 3;
@@ -602,22 +605,42 @@ static int fsl_dma_hw_params(struct snd_pcm_substream *substream,
 		break;
 	default:
 		/* We should never get here */
-		dev_err(dev, "unsupported sample size %u\n", sample_size);
+		dev_err(dev, "unsupported sample size %u\n", sample_bits);
 		return -EINVAL;
 	}
 
 	/*
-	 * BWC should always be a multiple of the frame size.  BWC determines
-	 * how many bytes are sent/received before the DMA controller checks the
-	 * SSI to see if it needs to stop.  For playback, the transmit FIFO can
-	 * hold three frames, so we want to send two frames at a time. For
-	 * capture, the receive FIFO is triggered when it contains one frame, so
-	 * we want to receive one frame at a time.
+	 * BWC determines how many bytes are sent/received before the DMA
+	 * controller checks the SSI to see if it needs to stop. BWC should
+	 * always be a multiple of the frame size, so that we always transmit
+	 * whole frames.  Each frame occupies two slots in the FIFO.  The
+	 * parameter for CCSR_DMA_MR_BWC() is rounded down the next power of two
+	 * (MR[BWC] can only represent even powers of two).
+	 *
+	 * To simplify the process, we set BWC to the largest value that is
+	 * less than or equal to the FIFO watermark.  For playback, this ensures
+	 * that we transfer the maximum amount without overrunning the FIFO.
+	 * For capture, this ensures that we transfer the maximum amount without
+	 * underrunning the FIFO.
+	 *
+	 * f = SSI FIFO depth
+	 * w = SSI watermark value (which equals f - 2)
+	 * b = DMA bandwidth count (in bytes)
+	 * s = sample size (in bytes, which equals frame_size * 2)
+	 *
+	 * For playback, we never transmit more than the transmit FIFO
+	 * watermark, otherwise we might write more data than the FIFO can hold.
+	 * The watermark is equal to the FIFO depth minus two.
+	 *
+	 * For capture, two equations must hold:
+	 *	w > f - (b / s)
+	 *	w >= b / s
+	 *
+	 * So, b > 2 * s, but b must also be <= s * w.  To simplify, we set
+	 * b = s * w, which is equal to
+	 *      (dma_private->ssi_fifo_depth - 2) * sample_bytes.
 	 */
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		mr |= CCSR_DMA_MR_BWC(2 * frame_size);
-	else
-		mr |= CCSR_DMA_MR_BWC(frame_size);
+	mr |= CCSR_DMA_MR_BWC((dma_private->ssi_fifo_depth - 2) * sample_bytes);
 
 	out_be32(&dma_channel->mr, mr);
 
@@ -871,6 +894,7 @@ static int __devinit fsl_soc_dma_probe(struct of_device *of_dev,
 	struct device_node *np = of_dev->dev.of_node;
 	struct device_node *ssi_np;
 	struct resource res;
+	const uint32_t *iprop;
 	int ret;
 
 	/* Find the SSI node that points to us. */
@@ -881,15 +905,17 @@ static int __devinit fsl_soc_dma_probe(struct of_device *of_dev,
 	}
 
 	ret = of_address_to_resource(ssi_np, 0, &res);
-	of_node_put(ssi_np);
 	if (ret) {
-		dev_err(&of_dev->dev, "could not determine device resources\n");
+		dev_err(&of_dev->dev, "could not determine resources for %s\n",
+			ssi_np->full_name);
+		of_node_put(ssi_np);
 		return ret;
 	}
 
 	dma = kzalloc(sizeof(*dma) + strlen(np->full_name), GFP_KERNEL);
 	if (!dma) {
 		dev_err(&of_dev->dev, "could not allocate dma object\n");
+		of_node_put(ssi_np);
 		return -ENOMEM;
 	}
 
@@ -902,6 +928,17 @@ static int __devinit fsl_soc_dma_probe(struct of_device *of_dev,
 	dma->ssi_stx_phys = res.start + offsetof(struct ccsr_ssi, stx0);
 	dma->ssi_srx_phys = res.start + offsetof(struct ccsr_ssi, srx0);
 
+	iprop = of_get_property(ssi_np, "fsl,fifo-depth", NULL);
+	if (!iprop) {
+		dev_err(&of_dev->dev, "missing fsl,fifo-depth property in %s\n",
+			ssi_np->full_name);
+		kfree(dma);
+		of_node_put(ssi_np);
+		return -ENODEV;
+	}
+	dma->ssi_fifo_depth = *iprop;
+	of_node_put(ssi_np);
+
 	ret = snd_soc_register_platform(&of_dev->dev, &dma->dai);
 	if (ret) {
 		dev_err(&of_dev->dev, "could not register platform\n");
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 00e3e62..a0e18b8 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -93,6 +93,7 @@ struct fsl_ssi_private {
 	unsigned int playback;
 	unsigned int capture;
 	int asynchronous;
+	unsigned int fifo_depth;
 	struct snd_soc_dai_driver cpu_dai_drv;
 	struct device_attribute dev_attr;
 	struct platform_device *pdev;
@@ -337,11 +338,20 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 
 		/*
 		 * Set the watermark for transmit FIFI 0 and receive FIFO 0. We
-		 * don't use FIFO 1.  Since the SSI only supports stereo, the
-		 * watermark should never be an odd number.
+		 * don't use FIFO 1.  We program the transmit water to signal a
+		 * DMA transfer if there are only two (or fewer) elements left
+		 * in the FIFO.  Two elements equals one frame (left channel,
+		 * right channel).  This value, however, depends on the depth of
+		 * the transmit buffer.
+		 *
+		 * We program the receive FIFO to notify us if at least two
+		 * elements (one frame) have been written to the FIFO.  We could
+		 * make this value larger (and maybe we should), but this way
+		 * data will be written to memory as soon as it's available.
 		 */
 		out_be32(&ssi->sfcsr,
-			 CCSR_SSI_SFCSR_TFWM0(6) | CCSR_SSI_SFCSR_RFWM0(2));
+			CCSR_SSI_SFCSR_TFWM0(ssi_private->fifo_depth - 2) |
+			CCSR_SSI_SFCSR_RFWM0(ssi_private->fifo_depth - 2));
 
 		/*
 		 * We keep the SSI disabled because if we enable it, then the
@@ -622,6 +632,7 @@ static int __devinit fsl_ssi_probe(struct of_device *of_dev,
 	struct device_attribute *dev_attr = NULL;
 	struct device_node *np = of_dev->dev.of_node;
 	const char *p, *sprop;
+	const uint32_t *iprop;
 	struct resource res;
 	char name[64];
 
@@ -671,6 +682,16 @@ static int __devinit fsl_ssi_probe(struct of_device *of_dev,
 	else
 		ssi_private->cpu_dai_drv.symmetric_rates = 1;
 
+	/* Determine the FIFO depth. */
+	iprop = of_get_property(np, "fsl,fifo-depth", NULL);
+	if (!iprop) {
+		dev_err(&of_dev->dev, "missing fsl,fifo-depth property in %s\n",
+			np->full_name);
+		ret = -ENODEV;
+		goto error;
+	}
+	ssi_private->fifo_depth = *iprop;
+
 	/* Initialize the the device_attribute structure */
 	dev_attr = &ssi_private->dev_attr;
 	dev_attr->attr.name = "statistics";
-- 
1.7.0.1

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Wed, Aug 04, 2010 at 03:04:24PM -0500, Timur Tabi wrote:
> Add code that programs the DMA and SSI controllers differently based on the
> FIFO depth of the SSI.
> 
> The SSI devices on the MPC8610 and the P1022 are identical in every way except
> one: the transmit and receive FIFO depth.  On the MPC8610, the depth is eight.
> On the P1022, it's fifteen.  The device tree nodes for the SSI include a
> "fsl,fifo-depth" property that specifies the FIFO depth.

My first thought with this is that it's not something I'd expect to be
in the device tree at all - it looks like it's all properties of the
silicon which I'd expect the drivers to be able to figure out for
themselves without needing to be told by the device tree.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Timur Tabi]

On Thu, Aug 5, 2010 at 6:10 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> My first thought with this is that it's not something I'd expect to be
> in the device tree at all - it looks like it's all properties of the
> silicon which I'd expect the drivers to be able to figure out for
> themselves without needing to be told by the device tree.

There is no way for the software to know what the FIFO depth is, so
this is the sort of thing that should be in the device tree.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Thu, Aug 05, 2010 at 08:14:43AM -0500, Timur Tabi wrote:
> On Thu, Aug 5, 2010 at 6:10 AM, Mark Brown

> > My first thought with this is that it's not something I'd expect to be
> > in the device tree at all - it looks like it's all properties of the
> > silicon which I'd expect the drivers to be able to figure out for
> > themselves without needing to be told by the device tree.

> There is no way for the software to know what the FIFO depth is, so
> this is the sort of thing that should be in the device tree.

Are you saying that every DT for a system doing audio on a PowerPC
system has to manually specify the depth of the FIFO on the silicon?
That doesn't sound particularly sensible, and I had been under the
impression that DT systems had a better way of coping with this.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Tabi Timur-B04825]

On Aug 5, 2010, at 8:52 AM, "Mark Brown" <broonie@opensource.wolfsonmicro.com> wrote:

> Are you saying that every DT for a system doing audio on a PowerPC
> system has to manually specify the depth of the FIFO on the silicon?

Only for SSI devices.

> That doesn't sound particularly sensible, and I had been under the
> impression that DT systems had a better way of coping with this.

The whole point behind having a device tree is to specify device information that cannot be probed.  So every piece of information in the DT is something that the driver needs to be told because there is no way to query the hardware.
> 

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Thu, Aug 05, 2010 at 09:10:08AM -0500, Tabi Timur-B04825 wrote:
> On Aug 5, 2010, at 8:52 AM, "Mark Brown" <broonie@opensource.wolfsonmicro.com> wrote:

> > Are you saying that every DT for a system doing audio on a PowerPC
> > system has to manually specify the depth of the FIFO on the silicon?

> Only for SSI devices.

Right, but that's the audio controller on the overwhelming majority of
PowerPC devices.

> > That doesn't sound particularly sensible, and I had been under the
> > impression that DT systems had a better way of coping with this.

> The whole point behind having a device tree is to specify device
> information that cannot be probed.  So every piece of information in the
> DT is something that the driver needs to be told because there is no way
> to query the hardware.

This seems crazy, it means that we're not able to use new support for
hardware features to the driver which require any kind of flag or data
without also going through and updating the device trees for all
existing boards.  That doesn't seem terribly helpful.

I'm fairly sure that some of the previous discussion with other device
tree people suggested that this was something that there was
infrastructure to cope with.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Tabi Timur-B04825]

Mark Brown wrote:
>> >  The whole point behind having a device tree is to specify device
>> >  information that cannot be probed.  So every piece of information in the
>> >  DT is something that the driver needs to be told because there is no way
>> >  to query the hardware.

> This seems crazy, it means that we're not able to use new support for
> hardware features to the driver which require any kind of flag or data
> without also going through and updating the device trees for all
> existing boards.  That doesn't seem terribly helpful.

Hey, what do you want me to do?  That's just the way the hardware is 
designed.  It is not possible for software to know the FIFO depth of the SSI 
on its own, because there is no register that can be queried.  The device 
tree was designed specifically for this purpose -- the provide a data 
structure (e.g. not kernel code) that describes all of the unprobe-able 
features of the various devices on the SOC.

> I'm fairly sure that some of the previous discussion with other device
> tree people suggested that this was something that there was
> infrastructure to cope with.

I have no idea what you're talking about.

-- 
Timur Tabi
Linux kernel developer

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Thu, Aug 05, 2010 at 08:14:55AM -0700, Tabi Timur-B04825 wrote:
> Mark Brown wrote:

> > This seems crazy, it means that we're not able to use new support for
> > hardware features to the driver which require any kind of flag or data
> > without also going through and updating the device trees for all
> > existing boards.  That doesn't seem terribly helpful.

> Hey, what do you want me to do?  That's just the way the hardware is 
> designed.  It is not possible for software to know the FIFO depth of the SSI 
> on its own, because there is no register that can be queried.  The device 
> tree was designed specifically for this purpose -- the provide a data 
> structure (e.g. not kernel code) that describes all of the unprobe-able 
> features of the various devices on the SOC.

This doesn't seem like a hardware issue, it seems like an issue with the
way we've deployed the device tree.  I'd have strongly expected that the
device tree was able to incoporate all the properties that are standard
to the CPU by reference somehow (with that data being distributed
separately to the per system device tree).

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Tabi Timur-B04825]

Mark Brown wrote:
> This doesn't seem like a hardware issue, it seems like an issue with the
> way we've deployed the device tree.

But the FIFO depth *is* a feature of the hardware.  The SSI on the 8610 has a 
depth of 8, and the SSI on the 1022 is identical in every way except that its 
FIFO depth is 15.  Since the FIFO watermark needs to be programmed based on 
the FIFO depth, the drivers need to know that information.  The only way to 
pass that info to the drivers is via the device tree.

> I'd have strongly expected that the
> device tree was able to incoporate all the properties that are standard
> to the CPU by reference somehow (with that data being distributed
> separately to the per system device tree).

Separately?  Why?  Look at the device trees we have today.  They are full of 
nodes with all sorts of device-specific properties that describe minor 
differences among the devices across various SOCs.

-- 
Timur Tabi
Linux kernel developer

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Thu, Aug 05, 2010 at 09:21:15AM -0700, Tabi Timur-B04825 wrote:

> the FIFO depth, the drivers need to know that information.  The only way to 
> pass that info to the drivers is via the device tree.

Clearly this is not entirely true, people manage to write non-DT kernels
after all.  In a strongly device tree oriented model it does need to be
via the device tree, but even there there's options for how the data
gets there - for example, we can supply it by looking at the CPU type
and fixing up the DTS prior to the driver seeing it, or any one of a
number of other methods.  I'd probably not have queried this so much if
I'd noticed some sort of handling for such a method.

It's not so much this particular property I'm worried about as the
general style for things like this so CCing in Grant.

> Mark Brown wrote:

> > I'd have strongly expected that the
> > device tree was able to incoporate all the properties that are standard
> > to the CPU by reference somehow (with that data being distributed
> > separately to the per system device tree).

> Separately?  Why?  Look at the device trees we have today.  They are full of 
> nodes with all sorts of device-specific properties that describe minor 
> differences among the devices across various SOCs.

Separately because people make and distribute systems with a parts on
them well before software is finalised in mainline.  Requiring that both
the DTS and the kernel match up and get updated for new feature support
means that you've got two different things (potentially maintained by
different groups in the end user environment) to distribute and get
updated on systems separately.  Failure to update one may result in
problems on another, either with features not getting enabled or with
newer kernel versions not wanting to run on systems with older device
trees.

It also seems like busy work for users to have to add things like this
to their device trees.  I think what I'd expect to see in a strongly
device tree model is something like the tree for a board saying it's
using a given SoC and then a standard device tree for the SoC which is
shared between all the different users of that SoC getting merged in
early in boot (probably from the kernel).  That way if we gain support
for a new feature on the SoC or discover something that needs to be
flagged up for workaround then every board using the SoC will pick it
up.  This seems particularly useful for things like crypto engines that
are physically internal to the SoC and so don't normally require
per-board hookup.  In ASoC terms you do need board specific hookup so
that's a bit less of an issue but it looses us some of the benefit of
having standard chip drivers by pushing some of the chip generic
knowledge into a per-machine location.

The fact that the PowerPC systems are currently doing this isn't
something I'd rely on with other architectures, the vendor and customer
bases may be very different.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Grant Likely]

On Thu, Aug 5, 2010 at 12:18 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> It also seems like busy work for users to have to add things like this
> to their device trees.  I think what I'd expect to see in a strongly
> device tree model is something like the tree for a board saying it's
> using a given SoC and then a standard device tree for the SoC which is
> shared between all the different users of that SoC getting merged in
> early in boot (probably from the kernel).  That way if we gain support
> for a new feature on the SoC or discover something that needs to be
> flagged up for workaround then every board using the SoC will pick it
> up.  This seems particularly useful for things like crypto engines that
> are physically internal to the SoC and so don't normally require
> per-board hookup.  In ASoC terms you do need board specific hookup so
> that's a bit less of an issue but it looses us some of the benefit of
> having standard chip drivers by pushing some of the chip generic
> knowledge into a per-machine location.

The lack of shared soc data in device trees is indeed a problem that
has been on my radar for a while now.  Fortunately I do have a
solution[1] which is partially implemented plus a contractual
obligation to deliver it to a client in the near future.  I fully
expect this will become a non-issue between now and about mid
November.

g.

[1] http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg00680.html

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Timur Tabi]

On Thu, Aug 5, 2010 at 2:43 PM, Grant Likely <grant.likely@secretlab.ca> wrote:

> The lack of shared soc data in device trees is indeed a problem that
> has been on my radar for a while now.  Fortunately I do have a
> solution[1] which is partially implemented plus a contractual
> obligation to deliver it to a client in the near future.  I fully
> expect this will become a non-issue between now and about mid
> November.
>
> g.
>
> [1] http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg00680.html

I haven't read through all of it, but it looks like your solution
affects only DTS files.  I think Mark's concern is mostly with DTB
files, because we still need to have a unique DTB for every possible
board variation.

Am I reading that correctly?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Grant Likely]

On Thu, Aug 5, 2010 at 3:13 PM, Timur Tabi <timur@freescale.com> wrote:
> On Thu, Aug 5, 2010 at 2:43 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> The lack of shared soc data in device trees is indeed a problem that
>> has been on my radar for a while now.  Fortunately I do have a
>> solution[1] which is partially implemented plus a contractual
>> obligation to deliver it to a client in the near future.  I fully
>> expect this will become a non-issue between now and about mid
>> November.
>>
>> g.
>>
>> [1] http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg00680.html
>
> I haven't read through all of it, but it looks like your solution
> affects only DTS files.  I think Mark's concern is mostly with DTB
> files, because we still need to have a unique DTB for every possible
> board variation.
>
> Am I reading that correctly?

You are; but the lack of dts factorization must be solved first before
looking at whether or not .dtb overlays make sense.  Otherwise we
don't have a source for the factorized data.  I personally don't think
.dtb overlays are needed, but I'm not closed to the idea either.

g.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On 5 Aug 2010, at 22:19, Grant Likely wrote:

> You are; but the lack of dts factorization must be solved first before
> looking at whether or not .dtb overlays make sense.  Otherwise we
> don't have a source for the factorized data.  I personally don't think
> .dtb overlays are needed, but I'm not closed to the idea either.

I think all the issues from the recent discussion about bootloaders also make it strongly desirable to get the SoC generic stuff out of the DTS that's shipped separately to the kernel. You've got issues on reference boards where the SoC vendor may have shipped a low quality device tree in flash and users are scared to try to reflash the board due to a lack of recovery facilities (so the less we rely on being able to update data flashed into the device the better) and you've got issues on end user projects with large, potentially distributed, teams where coordinating updates between everyone can be tricky especially if those updates need to be done in lockstep (so the less data is shipped outside the kernel the better).

Remember rmk's constant complaint about bootloaders on ARM - these are programs that need to get two registers right on kernel start and don't reliably do so. 

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Grant Likely]

On Thu, Aug 5, 2010 at 3:42 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On 5 Aug 2010, at 22:19, Grant Likely wrote:
>
>> You are; but the lack of dts factorization must be solved first before
>> looking at whether or not .dtb overlays make sense.  Otherwise we
>> don't have a source for the factorized data.  I personally don't think
>> .dtb overlays are needed, but I'm not closed to the idea either.
>
> I think all the issues from the recent discussion about bootloaders also make it strongly desirable to get the SoC generic stuff out of the DTS that's shipped separately to the kernel...

The board specific data has the same issues as the soc specific data
in this regard, so I don't think it helps much to split it up at the
.dtb level.  However....

> ... You've got issues on reference boards where the SoC vendor may have shipped a low quality device tree in flash and users are scared to try to reflash the board due to a lack of recovery facilities (so the less we rely on being able to update data flashed into the device the better) and you've got issues on end user projects with large, potentially distributed, teams where coordinating updates between everyone can be tricky especially if those updates need to be done in lockstep (so the less data is shipped outside the kernel the better).

Absolutely correct.  All of this discussion and lessons learned has
distilled the fact the .dtb file must not be linked into the boot
firmware.  Any design that requires a boot firmware upgrade to
update/replace the .dtb is broken.  There will still be broken cases,
but fortunately for the broken cases the .dtb can always be linked
into the kernel image.  So, for "good" firmware, the .dtb use case
works really well.  In the "broken" firmware case, the .dtb(s) can be
linked into the wrapper image and the correct one can be chosen based
on machine-id or some other discernible board data.  I'm being very
careful to make sure that using a .dtb doesn't paint anybody into a
corner.

> Remember rmk's constant complaint about bootloaders on ARM - these are programs that need to get two registers right on kernel start and don't reliably do so.

Understood.  It is my priority to complete a full-featured sample
implementation so I can prove that it is actually a useful solution.
I'm going to start with U-Boot and make sure upstream u-boot always
passes the correct thing for passing a .dtb

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On 5 Aug 2010, at 23:04, Grant Likely wrote:
> On Thu, Aug 5, 2010 at 3:42 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On 5 Aug 2010, at 22:19, Grant Likely wrote:
>> 
>>> You are; but the lack of dts factorization must be solved first before
>>> looking at whether or not .dtb overlays make sense.  Otherwise we
>>> don't have a source for the factorized data.  I personally don't think
>>> .dtb overlays are needed, but I'm not closed to the idea either.
>> 
>> I think all the issues from the recent discussion about bootloaders also make it strongly desirable to get the SoC generic stuff out of the DTS that's shipped separately to the kernel...
> 
> The board specific data has the same issues as the soc specific data
> in this regard, so I don't think it helps much to split it up at the
> .dtb level.  However....

This is purely on the basis that the error rate and the volume of data are correlated.

> Absolutely correct.  All of this discussion and lessons learned has
> distilled the fact the .dtb file must not be linked into the boot
> firmware.  Any design that requires a boot firmware upgrade to
> update/replace the .dtb is broken.  There will still be broken cases,
> but fortunately for the broken cases the .dtb can always be linked
> into the kernel image.  So, for "good" firmware, the .dtb use case
> works really well.  In the "broken" firmware case, the .dtb(s) can be
> linked into the wrapper image and the correct one can be chosen based
> on machine-id or some other discernible board data.  I'm being very
> careful to make sure that using a .dtb doesn't paint anybody into a
> corner.

On the other hand if you do have to link the device tree into the wrapper image you run into fun and games with any per system data like MAC addresses which might be embedded in the device tree.

Also, it's not just good and bad firmware that's the issue - like I say, there are also coordination issues within large development teams to consider.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Grant Likely]

On Thu, Aug 5, 2010 at 4:34 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On 5 Aug 2010, at 23:04, Grant Likely wrote:
>> On Thu, Aug 5, 2010 at 3:42 PM, Mark Brown
>> <broonie@opensource.wolfsonmicro.com> wrote:
>>> On 5 Aug 2010, at 22:19, Grant Likely wrote:
>>>
>>>> You are; but the lack of dts factorization must be solved first before
>>>> looking at whether or not .dtb overlays make sense.  Otherwise we
>>>> don't have a source for the factorized data.  I personally don't think
>>>> .dtb overlays are needed, but I'm not closed to the idea either.
>>>
>>> I think all the issues from the recent discussion about bootloaders also make it strongly desirable to get the SoC generic stuff out of the DTS that's shipped separately to the kernel...
>>
>> The board specific data has the same issues as the soc specific data
>> in this regard, so I don't think it helps much to split it up at the
>> .dtb level.  However....
>
> This is purely on the basis that the error rate and the volume of data are correlated.
>
>> Absolutely correct.  All of this discussion and lessons learned has
>> distilled the fact the .dtb file must not be linked into the boot
>> firmware.  Any design that requires a boot firmware upgrade to
>> update/replace the .dtb is broken.  There will still be broken cases,
>> but fortunately for the broken cases the .dtb can always be linked
>> into the kernel image.  So, for "good" firmware, the .dtb use case
>> works really well.  In the "broken" firmware case, the .dtb(s) can be
>> linked into the wrapper image and the correct one can be chosen based
>> on machine-id or some other discernible board data.  I'm being very
>> careful to make sure that using a .dtb doesn't paint anybody into a
>> corner.
>
> On the other hand if you do have to link the device tree into the wrapper image you run into fun and games with any per system data like MAC addresses which might be embedded in the device tree.
>
> Also, it's not just good and bad firmware that's the issue - like I say, there are also coordination issues within large development teams to consider.

I'm starting a working group to prepare a set of requirements,
recommendations and sample implementation for an ARM boot
architecture.  Particularly useful for more-or-less general purpose
OSes like Ubuntu on ARM.  We'll be talking about these kinds of
issues.  Would you like me to cc you when I kick it off?



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Thu, Aug 05, 2010 at 04:04:06PM -0600, Grant Likely wrote:
> On Thu, Aug 5, 2010 at 3:42 PM, Mark Brown

> > i think all the issues from the recent discussion about bootloaders
> > also make it strongly desirable to get the soc generic stuff out of
> > the dts that's shipped separately to the kernel...

> The board specific data has the same issues as the soc specific data
> in this regard, so I don't think it helps much to split it up at the
> .dtb level.  However....

Oh, this is purely on the basis that the error count is related to the
amount of data.  The board specific stuff does at least have more chance
someone actually looked at it on this particular board too.

> Absolutely correct.  All of this discussion and lessons learned has
> distilled the fact the .dtb file must not be linked into the boot
> firmware.  Any design that requires a boot firmware upgrade to
> update/replace the .dtb is broken.  There will still be broken cases,
> but fortunately for the broken cases the .dtb can always be linked
> into the kernel image.  So, for "good" firmware, the .dtb use case
> works really well.  In the "broken" firmware case, the .dtb(s) can be
> linked into the wrapper image and the correct one can be chosen based
> on machine-id or some other discernible board data.  I'm being very
> careful to make sure that using a .dtb doesn't paint anybody into a
> corner.

On the other hand if you end up having to replace the device tree with
one in the wrapper then you've got an issue with any per-system data
like MAC addresses that someone put in the device tree unless you can do
some sort of overlay/fixup thing.

> > Remember rmk's constant complaint about bootloaders on ARM - these
> > are programs that need to get two registers right on kernel start
> > and don't reliably do so.

> Understood.  It is my priority to complete a full-featured sample
> implementation so I can prove that it is actually a useful solution.
> I'm going to start with U-Boot and make sure upstream u-boot always
> passes the correct thing for passing a .dtb

u-boot will probably be fine anyway, it's more the less widely used ones
that people cook up which are a concern here.

Re: [PATCH] asoc/multi-component: fsl: add support for variable SSI FIFO depth

[Mark Brown]

On Thu, Aug 05, 2010 at 05:19:44PM -0600, Grant Likely wrote:

> I'm starting a working group to prepare a set of requirements,
> recommendations and sample implementation for an ARM boot
> architecture.  Particularly useful for more-or-less general purpose
> OSes like Ubuntu on ARM.  We'll be talking about these kinds of
> issues.  Would you like me to cc you when I kick it off?

Sure, go ahead.