[alsa-devel] [PATCH 0/9] ASoC: sh: fsi: tidyup and cleanup FSI
Dear Mark, Liam
Cc Simon, yoshii-san, Magnus
These are tidyup and cleanup patches for FSI
Kuninori Morimoto (9): ASoC: sh: fsi: tidyup parameter of fsi_stream_push ASoC: sh: fsi: tidyup unclear variable naming ASoC: sh: fsi: remove pm_runtime from fsi_dai_set_fmt. ASoC: sh: fsi: make sure fsi_stream_push/pop access by spin lock ASoC: sh: fsi: Add fsi_set_master_clk ASoC: sh: fsi: irq control moves to fsi_port_start/stop ASoC: sh: fsi: Add fsi_hw_startup/shutdown ASoC: sh: fsi: remove fsi_module_init/kill ASoC: sh: fsi: cleanup suspend/resume
#1 and #2 are tidyup for fsi_fifo_data_ctrl. The variables and sub-functions were unclear nameing. So, it was difficult to understand the detail of calculation. Thank you for pointing it yoshii-san.
#3 - #9 are clean up for RuntimePM. Current FSI driver was wrong for RuntimePM. these patches fixup for RuntimePM and clean up for suspend/resume. last patch remove noisy saved_xxx variable.
Best regards -- Kuninori Morimoto
It is possible to create buff_len and period_len from substream->runtime. This patch is preparation of tidyup unclear variable naming patch.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 14 +++++--------- 1 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index 4a9da6b..7025885 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -344,16 +344,15 @@ static u32 fsi_get_port_shift(struct fsi_priv *fsi, int is_play)
static void fsi_stream_push(struct fsi_priv *fsi, int is_play, - struct snd_pcm_substream *substream, - u32 buffer_len, - u32 period_len) + struct snd_pcm_substream *substream) { struct fsi_stream *io = fsi_get_stream(fsi, is_play); + struct snd_pcm_runtime *runtime = substream->runtime;
io->substream = substream; - io->buff_len = buffer_len; + io->buff_len = frames_to_bytes(runtime, runtime->buffer_size); io->buff_offset = 0; - io->period_len = period_len; + io->period_len = frames_to_bytes(runtime, runtime->period_size); io->period_num = 0; io->oerr_num = -1; /* ignore 1st err */ io->uerr_num = -1; /* ignore 1st err */ @@ -844,15 +843,12 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { struct fsi_priv *fsi = fsi_get_priv(substream); - struct snd_pcm_runtime *runtime = substream->runtime; int is_play = fsi_is_play(substream); int ret = 0;
switch (cmd) { case SNDRV_PCM_TRIGGER_START: - fsi_stream_push(fsi, is_play, substream, - frames_to_bytes(runtime, runtime->buffer_size), - frames_to_bytes(runtime, runtime->period_size)); + fsi_stream_push(fsi, is_play, substream); ret = is_play ? fsi_data_push(fsi) : fsi_data_pop(fsi); fsi_irq_enable(fsi, is_play); fsi_port_start(fsi);
Some variables on this driver were a unclear naming, and were different unit (byte, frame, sample). And some functions had wrong name (ex. it returned "sample width" but name was "fsi_get_frame_width"). This patch tidy-up this issue, and the minimum unit become "sample". Special thanks to Takashi YOSHII.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 182 ++++++++++++++++++++++++++++----------------------- 1 files changed, 100 insertions(+), 82 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index 7025885..14fed47 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -118,10 +118,38 @@ typedef int (*set_rate_func)(struct device *dev, int is_porta, int rate, int ena /* * FSI driver use below type name for variable * - * xxx_len : data length - * xxx_width : data width - * xxx_offset : data offset * xxx_num : number of data + * xxx_pos : position of data + * xxx_capa : capacity of data + */ + +/* + * period/frame/sample image + * + * ex) PCM (2ch) + * + * period pos period pos + * [n] [n + 1] + * |<-------------------- period--------------------->| + * ==|============================================ ... =|== + * | | + * ||<----- frame ----->|<------ frame ----->| ... | + * |+--------------------+--------------------+- ... | + * ||[ sample ][ sample ]|[ sample ][ sample ]| ... | + * |+--------------------+--------------------+- ... | + * ==|============================================ ... =|== + */ + +/* + * FSI FIFO image + * + * | | + * | | + * | [ sample ] | + * | [ sample ] | + * | [ sample ] | + * | [ sample ] | + * --> go to codecs */
/* @@ -131,12 +159,11 @@ typedef int (*set_rate_func)(struct device *dev, int is_porta, int rate, int ena struct fsi_stream { struct snd_pcm_substream *substream;
- int fifo_max_num; - - int buff_offset; - int buff_len; - int period_len; - int period_num; + int fifo_sample_capa; /* sample capacity of FSI FIFO */ + int buff_sample_capa; /* sample capacity of ALSA buffer */ + int buff_sample_pos; /* sample position of ALSA buffer */ + int period_samples; /* sample number / 1 period */ + int period_pos; /* current period position */
int uerr_num; int oerr_num; @@ -342,6 +369,16 @@ static u32 fsi_get_port_shift(struct fsi_priv *fsi, int is_play) return shift; }
+static int fsi_frame2sample(struct fsi_priv *fsi, int frames) +{ + return frames * fsi->chan_num; +} + +static int fsi_sample2frame(struct fsi_priv *fsi, int samples) +{ + return samples / fsi->chan_num; +} + static void fsi_stream_push(struct fsi_priv *fsi, int is_play, struct snd_pcm_substream *substream) @@ -350,10 +387,10 @@ static void fsi_stream_push(struct fsi_priv *fsi, struct snd_pcm_runtime *runtime = substream->runtime;
io->substream = substream; - io->buff_len = frames_to_bytes(runtime, runtime->buffer_size); - io->buff_offset = 0; - io->period_len = frames_to_bytes(runtime, runtime->period_size); - io->period_num = 0; + io->buff_sample_capa = fsi_frame2sample(fsi, runtime->buffer_size); + io->buff_sample_pos = 0; + io->period_samples = fsi_frame2sample(fsi, runtime->period_size); + io->period_pos = 0; io->oerr_num = -1; /* ignore 1st err */ io->uerr_num = -1; /* ignore 1st err */ } @@ -371,47 +408,26 @@ static void fsi_stream_pop(struct fsi_priv *fsi, int is_play) dev_err(dai->dev, "under_run = %d\n", io->uerr_num);
io->substream = NULL; - io->buff_len = 0; - io->buff_offset = 0; - io->period_len = 0; - io->period_num = 0; + io->buff_sample_capa = 0; + io->buff_sample_pos = 0; + io->period_samples = 0; + io->period_pos = 0; io->oerr_num = 0; io->uerr_num = 0; }
-static int fsi_get_fifo_data_num(struct fsi_priv *fsi, int is_play) +static int fsi_get_current_fifo_samples(struct fsi_priv *fsi, int is_play) { u32 status; - int data_num; + int frames;
status = is_play ? fsi_reg_read(fsi, DOFF_ST) : fsi_reg_read(fsi, DIFF_ST);
- data_num = 0x1ff & (status >> 8); - data_num *= fsi->chan_num; - - return data_num; -} - -static int fsi_len2num(int len, int width) -{ - return len / width; -} + frames = 0x1ff & (status >> 8);
-#define fsi_num2offset(a, b) fsi_num2len(a, b) -static int fsi_num2len(int num, int width) -{ - return num * width; -} - -static int fsi_get_frame_width(struct fsi_priv *fsi, int is_play) -{ - struct fsi_stream *io = fsi_get_stream(fsi, is_play); - struct snd_pcm_substream *substream = io->substream; - struct snd_pcm_runtime *runtime = substream->runtime; - - return frames_to_bytes(runtime, 1) / fsi->chan_num; + return fsi_frame2sample(fsi, frames); }
static void fsi_count_fifo_err(struct fsi_priv *fsi) @@ -443,8 +459,10 @@ static u8 *fsi_dma_get_area(struct fsi_priv *fsi, int stream) { int is_play = fsi_stream_is_play(stream); struct fsi_stream *io = fsi_get_stream(fsi, is_play); + struct snd_pcm_runtime *runtime = io->substream->runtime;
- return io->substream->runtime->dma_area + io->buff_offset; + return runtime->dma_area + + samples_to_bytes(runtime, io->buff_sample_pos); }
static void fsi_dma_soft_push16(struct fsi_priv *fsi, int num) @@ -602,13 +620,14 @@ static void fsi_fifo_init(struct fsi_priv *fsi, struct fsi_master *master = fsi_get_master(fsi); struct fsi_stream *io = fsi_get_stream(fsi, is_play); u32 shift, i; + int frame_capa;
/* get on-chip RAM capacity */ shift = fsi_master_read(master, FIFO_SZ); shift >>= fsi_get_port_shift(fsi, is_play); shift &= FIFO_SZ_MASK; - io->fifo_max_num = 256 << shift; - dev_dbg(dai->dev, "fifo = %d words\n", io->fifo_max_num); + frame_capa = 256 << shift; + dev_dbg(dai->dev, "fifo = %d words\n", frame_capa);
/* * The maximum number of sample data varies depending @@ -630,9 +649,11 @@ static void fsi_fifo_init(struct fsi_priv *fsi, * 8 channels: 32 ( 32 x 8 = 256) */ for (i = 1; i < fsi->chan_num; i <<= 1) - io->fifo_max_num >>= 1; + frame_capa >>= 1; dev_dbg(dai->dev, "%d channel %d store\n", - fsi->chan_num, io->fifo_max_num); + fsi->chan_num, frame_capa); + + io->fifo_sample_capa = fsi_frame2sample(fsi, frame_capa);
/* * set interrupt generation factor @@ -653,10 +674,10 @@ static int fsi_fifo_data_ctrl(struct fsi_priv *fsi, int stream) struct snd_pcm_substream *substream = NULL; int is_play = fsi_stream_is_play(stream); struct fsi_stream *io = fsi_get_stream(fsi, is_play); - int data_residue_num; - int data_num; - int data_num_max; - int ch_width; + int sample_residues; + int sample_width; + int samples; + int samples_max; int over_period; void (*fn)(struct fsi_priv *fsi, int size);
@@ -672,36 +693,35 @@ static int fsi_fifo_data_ctrl(struct fsi_priv *fsi, int stream) /* FSI FIFO has limit. * So, this driver can not send periods data at a time */ - if (io->buff_offset >= - fsi_num2offset(io->period_num + 1, io->period_len)) { + if (io->buff_sample_pos >= + io->period_samples * (io->period_pos + 1)) {
over_period = 1; - io->period_num = (io->period_num + 1) % runtime->periods; + io->period_pos = (io->period_pos + 1) % runtime->periods;
- if (0 == io->period_num) - io->buff_offset = 0; + if (0 == io->period_pos) + io->buff_sample_pos = 0; }
- /* get 1 channel data width */ - ch_width = fsi_get_frame_width(fsi, is_play); + /* get 1 sample data width */ + sample_width = samples_to_bytes(runtime, 1);
- /* get residue data number of alsa */ - data_residue_num = fsi_len2num(io->buff_len - io->buff_offset, - ch_width); + /* get number of residue samples */ + sample_residues = io->buff_sample_capa - io->buff_sample_pos;
if (is_play) { /* * for play-back * - * data_num_max : number of FSI fifo free space - * data_num : number of ALSA residue data + * samples_max : number of FSI fifo free samples space + * samples : number of ALSA residue samples */ - data_num_max = io->fifo_max_num * fsi->chan_num; - data_num_max -= fsi_get_fifo_data_num(fsi, is_play); + samples_max = io->fifo_sample_capa; + samples_max -= fsi_get_current_fifo_samples(fsi, is_play);
- data_num = data_residue_num; + samples = sample_residues;
- switch (ch_width) { + switch (sample_width) { case 2: fn = fsi_dma_soft_push16; break; @@ -715,13 +735,13 @@ static int fsi_fifo_data_ctrl(struct fsi_priv *fsi, int stream) /* * for capture * - * data_num_max : number of ALSA free space - * data_num : number of data in FSI fifo + * samples_max : number of ALSA free samples space + * samples : number of samples in FSI fifo */ - data_num_max = data_residue_num; - data_num = fsi_get_fifo_data_num(fsi, is_play); + samples_max = sample_residues; + samples = fsi_get_current_fifo_samples(fsi, is_play);
- switch (ch_width) { + switch (sample_width) { case 2: fn = fsi_dma_soft_pop16; break; @@ -733,12 +753,12 @@ static int fsi_fifo_data_ctrl(struct fsi_priv *fsi, int stream) } }
- data_num = min(data_num, data_num_max); + samples = min(samples, samples_max);
- fn(fsi, data_num); + fn(fsi, samples);
- /* update buff_offset */ - io->buff_offset += fsi_num2offset(data_num, ch_width); + /* update buff_sample_pos */ + io->buff_sample_pos += samples;
if (over_period) snd_pcm_period_elapsed(substream); @@ -1093,16 +1113,14 @@ static int fsi_hw_free(struct snd_pcm_substream *substream)
static snd_pcm_uframes_t fsi_pointer(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; struct fsi_priv *fsi = fsi_get_priv(substream); struct fsi_stream *io = fsi_get_stream(fsi, fsi_is_play(substream)); - long location; + int samples_pos = io->buff_sample_pos - 1;
- location = (io->buff_offset - 1); - if (location < 0) - location = 0; + if (samples_pos < 0) + samples_pos = 0;
- return bytes_to_frames(runtime, location); + return fsi_sample2frame(fsi, samples_pos); }
static struct snd_pcm_ops fsi_pcm_ops = {
pm_runtime_get/put_sync were used to access FSI register in fsi_dai_set_fmt which is called when ALSA probe. But this register value will disappear after pm_runtime_put_sync if platform is supporting RuntimePM. To solve this issue, this patch adds new variable for format, and remove pm_runtime_get/put_sync from fsi_dai_set_fmt.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 52 ++++++++++++++++++++++++++++++++-------------------- 1 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index 14fed47..bdf2d65 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -176,8 +176,12 @@ struct fsi_priv { struct fsi_stream playback; struct fsi_stream capture;
+ u32 do_fmt; + u32 di_fmt; + int chan_num:16; int clk_master:1; + int spdif:1;
long rate;
@@ -298,6 +302,11 @@ static int fsi_is_port_a(struct fsi_priv *fsi) return fsi->master->base == fsi->base; }
+static int fsi_is_spdif(struct fsi_priv *fsi) +{ + return fsi->spdif; +} + static struct snd_soc_dai *fsi_get_dai(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; @@ -812,11 +821,16 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream, { struct fsi_priv *fsi = fsi_get_priv(substream); u32 flags = fsi_get_info_flags(fsi); - u32 data; + u32 data = 0; int is_play = fsi_is_play(substream);
pm_runtime_get_sync(dai->dev);
+ /* clock setting */ + if (fsi_is_clk_master(fsi)) + data = DIMD | DOMD; + + fsi_reg_mask_set(fsi, CKG1, (DIMD | DOMD), data);
/* clock inversion (CKG2) */ data = 0; @@ -831,6 +845,16 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream,
fsi_reg_write(fsi, CKG2, data);
+ /* set format */ + fsi_reg_write(fsi, DO_FMT, fsi->do_fmt); + fsi_reg_write(fsi, DI_FMT, fsi->di_fmt); + + /* spdif ? */ + if (fsi_is_spdif(fsi)) { + fsi_spdif_clk_ctrl(fsi, 1); + fsi_reg_mask_set(fsi, OUT_SEL, DMMD, DMMD); + } + /* irq clear */ fsi_irq_disable(fsi, is_play); fsi_irq_clear_status(fsi); @@ -900,8 +924,8 @@ static int fsi_set_fmt_dai(struct fsi_priv *fsi, unsigned int fmt) return -EINVAL; }
- fsi_reg_write(fsi, DO_FMT, data); - fsi_reg_write(fsi, DI_FMT, data); + fsi->do_fmt = data; + fsi->di_fmt = data;
return 0; } @@ -916,11 +940,10 @@ static int fsi_set_fmt_spdif(struct fsi_priv *fsi)
data = CR_BWS_16 | CR_DTMD_SPDIF_PCM | CR_PCM; fsi->chan_num = 2; - fsi_spdif_clk_ctrl(fsi, 1); - fsi_reg_mask_set(fsi, OUT_SEL, DMMD, DMMD); + fsi->spdif = 1;
- fsi_reg_write(fsi, DO_FMT, data); - fsi_reg_write(fsi, DI_FMT, data); + fsi->do_fmt = data; + fsi->di_fmt = data;
return 0; } @@ -931,32 +954,24 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) struct fsi_master *master = fsi_get_master(fsi); set_rate_func set_rate = fsi_get_info_set_rate(master); u32 flags = fsi_get_info_flags(fsi); - u32 data = 0; int ret;
- pm_runtime_get_sync(dai->dev); - /* set master/slave audio interface */ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBM_CFM: - data = DIMD | DOMD; fsi->clk_master = 1; break; case SND_SOC_DAIFMT_CBS_CFS: break; default: - ret = -EINVAL; - goto set_fmt_exit; + return -EINVAL; }
if (fsi_is_clk_master(fsi) && !set_rate) { dev_err(dai->dev, "platform doesn't have set_rate\n"); - ret = -EINVAL; - goto set_fmt_exit; + return -EINVAL; }
- fsi_reg_mask_set(fsi, CKG1, (DIMD | DOMD), data); - /* set format */ switch (flags & SH_FSI_FMT_MASK) { case SH_FSI_FMT_DAI: @@ -969,9 +984,6 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) ret = -EINVAL; }
-set_fmt_exit: - pm_runtime_put_sync(dai->dev); - return ret; }
fsi_stream_push/pop might be called in same time. This patch protect it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index bdf2d65..33a6bd0 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -394,7 +394,10 @@ static void fsi_stream_push(struct fsi_priv *fsi, { struct fsi_stream *io = fsi_get_stream(fsi, is_play); struct snd_pcm_runtime *runtime = substream->runtime; + struct fsi_master *master = fsi_get_master(fsi); + unsigned long flags;
+ spin_lock_irqsave(&master->lock, flags); io->substream = substream; io->buff_sample_capa = fsi_frame2sample(fsi, runtime->buffer_size); io->buff_sample_pos = 0; @@ -402,13 +405,17 @@ static void fsi_stream_push(struct fsi_priv *fsi, io->period_pos = 0; io->oerr_num = -1; /* ignore 1st err */ io->uerr_num = -1; /* ignore 1st err */ + spin_unlock_irqrestore(&master->lock, flags); }
static void fsi_stream_pop(struct fsi_priv *fsi, int is_play) { struct fsi_stream *io = fsi_get_stream(fsi, is_play); struct snd_soc_dai *dai = fsi_get_dai(io->substream); + struct fsi_master *master = fsi_get_master(fsi); + unsigned long flags;
+ spin_lock_irqsave(&master->lock, flags);
if (io->oerr_num > 0) dev_err(dai->dev, "over_run = %d\n", io->oerr_num); @@ -423,6 +430,7 @@ static void fsi_stream_pop(struct fsi_priv *fsi, int is_play) io->period_pos = 0; io->oerr_num = 0; io->uerr_num = 0; + spin_unlock_irqrestore(&master->lock, flags); }
static int fsi_get_current_fifo_samples(struct fsi_priv *fsi, int is_play)
On Mon, May 23, 2011 at 08:46:13PM +0900, Kuninori Morimoto wrote:
fsi_stream_push/pop might be called in same time. This patch protect it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This one looks like a bug fix which should be done in 2.6.40 also? If that is the case could you please also provide a version of this patch against 2.6.40.
Dear Mark
fsi_stream_push/pop might be called in same time. This patch protect it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This one looks like a bug fix which should be done in 2.6.40 also? If that is the case could you please also provide a version of this patch against 2.6.40.
Thank you for pointing it.
Sorry. the order of patch series (and explain) was a little bit wrong.
My point was "snd_soc_dai_ops :: trigger" (which call fsi_stream_push/pop) and "suspend/resume" (which call fsi_stream_is_working) might be called in same time. This patch was preparation of cleanup suspend/resume patch.
Best regards -- Kuninori Morimoto
Current FSI driver is using set_rate call back function which is for master mode. By this patch, it is used from fsi_set_master_clk. This patch is preparation of cleanup suspend/resume patch.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 164 +++++++++++++++++++++++++++------------------------ 1 files changed, 87 insertions(+), 77 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index 33a6bd0..ae05d8f 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -593,6 +593,82 @@ static void fsi_spdif_clk_ctrl(struct fsi_priv *fsi, int enable) /* * clock function */ +static int fsi_set_master_clk(struct device *dev, struct fsi_priv *fsi, + long rate, int enable) +{ + struct fsi_master *master = fsi_get_master(fsi); + set_rate_func set_rate = fsi_get_info_set_rate(master); + int fsi_ver = master->core->ver; + int ret; + + ret = set_rate(dev, fsi_is_port_a(fsi), rate, enable); + if (ret < 0) /* error */ + return ret; + + if (!enable) + return 0; + + if (ret > 0) { + u32 data = 0; + + switch (ret & SH_FSI_ACKMD_MASK) { + default: + /* FALL THROUGH */ + case SH_FSI_ACKMD_512: + data |= (0x0 << 12); + break; + case SH_FSI_ACKMD_256: + data |= (0x1 << 12); + break; + case SH_FSI_ACKMD_128: + data |= (0x2 << 12); + break; + case SH_FSI_ACKMD_64: + data |= (0x3 << 12); + break; + case SH_FSI_ACKMD_32: + if (fsi_ver < 2) + dev_err(dev, "unsupported ACKMD\n"); + else + data |= (0x4 << 12); + break; + } + + switch (ret & SH_FSI_BPFMD_MASK) { + default: + /* FALL THROUGH */ + case SH_FSI_BPFMD_32: + data |= (0x0 << 8); + break; + case SH_FSI_BPFMD_64: + data |= (0x1 << 8); + break; + case SH_FSI_BPFMD_128: + data |= (0x2 << 8); + break; + case SH_FSI_BPFMD_256: + data |= (0x3 << 8); + break; + case SH_FSI_BPFMD_512: + data |= (0x4 << 8); + break; + case SH_FSI_BPFMD_16: + if (fsi_ver < 2) + dev_err(dev, "unsupported ACKMD\n"); + else + data |= (0x7 << 8); + break; + } + + fsi_reg_mask_set(fsi, CKG1, (ACKMD_MASK | BPFMD_MASK) , data); + udelay(10); + ret = 0; + } + + return ret; + +} + #define fsi_module_init(m, d) __fsi_module_clk_ctrl(m, d, 1) #define fsi_module_kill(m, d) __fsi_module_clk_ctrl(m, d, 0) static void __fsi_module_clk_ctrl(struct fsi_master *master, @@ -878,13 +954,11 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream, { struct fsi_priv *fsi = fsi_get_priv(substream); int is_play = fsi_is_play(substream); - struct fsi_master *master = fsi_get_master(fsi); - set_rate_func set_rate = fsi_get_info_set_rate(master);
fsi_irq_disable(fsi, is_play);
if (fsi_is_clk_master(fsi)) - set_rate(dai->dev, fsi_is_port_a(fsi), fsi->rate, 0); + fsi_set_master_clk(dai->dev, fsi, fsi->rate, 0);
fsi->rate = 0;
@@ -1000,79 +1074,19 @@ static int fsi_dai_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct fsi_priv *fsi = fsi_get_priv(substream); - struct fsi_master *master = fsi_get_master(fsi); - set_rate_func set_rate = fsi_get_info_set_rate(master); - int fsi_ver = master->core->ver; long rate = params_rate(params); int ret;
if (!fsi_is_clk_master(fsi)) return 0;
- ret = set_rate(dai->dev, fsi_is_port_a(fsi), rate, 1); - if (ret < 0) /* error */ + ret = fsi_set_master_clk(dai->dev, fsi, rate, 1); + if (ret < 0) return ret;
fsi->rate = rate; - if (ret > 0) { - u32 data = 0; - - switch (ret & SH_FSI_ACKMD_MASK) { - default: - /* FALL THROUGH */ - case SH_FSI_ACKMD_512: - data |= (0x0 << 12); - break; - case SH_FSI_ACKMD_256: - data |= (0x1 << 12); - break; - case SH_FSI_ACKMD_128: - data |= (0x2 << 12); - break; - case SH_FSI_ACKMD_64: - data |= (0x3 << 12); - break; - case SH_FSI_ACKMD_32: - if (fsi_ver < 2) - dev_err(dai->dev, "unsupported ACKMD\n"); - else - data |= (0x4 << 12); - break; - } - - switch (ret & SH_FSI_BPFMD_MASK) { - default: - /* FALL THROUGH */ - case SH_FSI_BPFMD_32: - data |= (0x0 << 8); - break; - case SH_FSI_BPFMD_64: - data |= (0x1 << 8); - break; - case SH_FSI_BPFMD_128: - data |= (0x2 << 8); - break; - case SH_FSI_BPFMD_256: - data |= (0x3 << 8); - break; - case SH_FSI_BPFMD_512: - data |= (0x4 << 8); - break; - case SH_FSI_BPFMD_16: - if (fsi_ver < 2) - dev_err(dai->dev, "unsupported ACKMD\n"); - else - data |= (0x7 << 8); - break; - } - - fsi_reg_mask_set(fsi, CKG1, (ACKMD_MASK | BPFMD_MASK) , data); - udelay(10); - ret = 0; - }
return ret; - }
static struct snd_soc_dai_ops fsi_dai_ops = { @@ -1339,8 +1353,7 @@ static int fsi_remove(struct platform_device *pdev) }
static void __fsi_suspend(struct fsi_priv *fsi, - struct device *dev, - set_rate_func set_rate) + struct device *dev) { fsi->saved_do_fmt = fsi_reg_read(fsi, DO_FMT); fsi->saved_di_fmt = fsi_reg_read(fsi, DI_FMT); @@ -1349,12 +1362,11 @@ static void __fsi_suspend(struct fsi_priv *fsi, fsi->saved_out_sel = fsi_reg_read(fsi, OUT_SEL);
if (fsi_is_clk_master(fsi)) - set_rate(dev, fsi_is_port_a(fsi), fsi->rate, 0); + fsi_set_master_clk(dev, fsi, fsi->rate, 0); }
static void __fsi_resume(struct fsi_priv *fsi, - struct device *dev, - set_rate_func set_rate) + struct device *dev) { fsi_reg_write(fsi, DO_FMT, fsi->saved_do_fmt); fsi_reg_write(fsi, DI_FMT, fsi->saved_di_fmt); @@ -1363,18 +1375,17 @@ static void __fsi_resume(struct fsi_priv *fsi, fsi_reg_write(fsi, OUT_SEL, fsi->saved_out_sel);
if (fsi_is_clk_master(fsi)) - set_rate(dev, fsi_is_port_a(fsi), fsi->rate, 1); + fsi_set_master_clk(dev, fsi, fsi->rate, 1); }
static int fsi_suspend(struct device *dev) { struct fsi_master *master = dev_get_drvdata(dev); - set_rate_func set_rate = fsi_get_info_set_rate(master);
pm_runtime_get_sync(dev);
- __fsi_suspend(&master->fsia, dev, set_rate); - __fsi_suspend(&master->fsib, dev, set_rate); + __fsi_suspend(&master->fsia, dev); + __fsi_suspend(&master->fsib, dev);
master->saved_a_mclk = fsi_core_read(master, a_mclk); master->saved_b_mclk = fsi_core_read(master, b_mclk); @@ -1393,7 +1404,6 @@ static int fsi_suspend(struct device *dev) static int fsi_resume(struct device *dev) { struct fsi_master *master = dev_get_drvdata(dev); - set_rate_func set_rate = fsi_get_info_set_rate(master);
pm_runtime_get_sync(dev);
@@ -1406,8 +1416,8 @@ static int fsi_resume(struct device *dev) fsi_core_mask_set(master, iemsk, 0xffff, master->saved_iemsk); fsi_core_mask_set(master, imsk, 0xffff, master->saved_imsk);
- __fsi_resume(&master->fsia, dev, set_rate); - __fsi_resume(&master->fsib, dev, set_rate); + __fsi_resume(&master->fsia, dev); + __fsi_resume(&master->fsib, dev);
pm_runtime_put_sync(dev);
Using fsi_irq_enable/disable in fsi_port_start/stop is very natural. This patch is preparation of cleanup suspend/resume patch.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index ae05d8f..a00fe37 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -689,15 +689,20 @@ static void __fsi_module_clk_ctrl(struct fsi_master *master, pm_runtime_put_sync(dev); }
-#define fsi_port_start(f) __fsi_port_clk_ctrl(f, 1) -#define fsi_port_stop(f) __fsi_port_clk_ctrl(f, 0) -static void __fsi_port_clk_ctrl(struct fsi_priv *fsi, int enable) +#define fsi_port_start(f, i) __fsi_port_clk_ctrl(f, i, 1) +#define fsi_port_stop(f, i) __fsi_port_clk_ctrl(f, i, 0) +static void __fsi_port_clk_ctrl(struct fsi_priv *fsi, int is_play, int enable) { struct fsi_master *master = fsi_get_master(fsi); u32 soft = fsi_is_port_a(fsi) ? PASR : PBSR; u32 clk = fsi_is_port_a(fsi) ? CRA : CRB; int is_master = fsi_is_clk_master(fsi);
+ if (enable) + fsi_irq_enable(fsi, is_play); + else + fsi_irq_disable(fsi, is_play); + fsi_master_mask_set(master, SOFT_RST, soft, (enable) ? soft : 0); if (is_master) fsi_master_mask_set(master, CLK_RST, clk, (enable) ? clk : 0); @@ -953,9 +958,6 @@ static void fsi_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct fsi_priv *fsi = fsi_get_priv(substream); - int is_play = fsi_is_play(substream); - - fsi_irq_disable(fsi, is_play);
if (fsi_is_clk_master(fsi)) fsi_set_master_clk(dai->dev, fsi, fsi->rate, 0); @@ -976,12 +978,10 @@ static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_START: fsi_stream_push(fsi, is_play, substream); ret = is_play ? fsi_data_push(fsi) : fsi_data_pop(fsi); - fsi_irq_enable(fsi, is_play); - fsi_port_start(fsi); + fsi_port_start(fsi, is_play); break; case SNDRV_PCM_TRIGGER_STOP: - fsi_port_stop(fsi); - fsi_irq_disable(fsi, is_play); + fsi_port_stop(fsi, is_play); fsi_stream_pop(fsi, is_play); break; }
This patch is preparation of cleanup suspend/resume patch.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 43 +++++++++++++++++++++++++++++-------------- 1 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index a00fe37..c755d21 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -713,7 +713,7 @@ static void __fsi_port_clk_ctrl(struct fsi_priv *fsi, int is_play, int enable) */ static void fsi_fifo_init(struct fsi_priv *fsi, int is_play, - struct snd_soc_dai *dai) + struct device *dev) { struct fsi_master *master = fsi_get_master(fsi); struct fsi_stream *io = fsi_get_stream(fsi, is_play); @@ -725,7 +725,7 @@ static void fsi_fifo_init(struct fsi_priv *fsi, shift >>= fsi_get_port_shift(fsi, is_play); shift &= FIFO_SZ_MASK; frame_capa = 256 << shift; - dev_dbg(dai->dev, "fifo = %d words\n", frame_capa); + dev_dbg(dev, "fifo = %d words\n", frame_capa);
/* * The maximum number of sample data varies depending @@ -748,7 +748,7 @@ static void fsi_fifo_init(struct fsi_priv *fsi, */ for (i = 1; i < fsi->chan_num; i <<= 1) frame_capa >>= 1; - dev_dbg(dai->dev, "%d channel %d store\n", + dev_dbg(dev, "%d channel %d store\n", fsi->chan_num, frame_capa);
io->fifo_sample_capa = fsi_frame2sample(fsi, frame_capa); @@ -905,15 +905,14 @@ static irqreturn_t fsi_interrupt(int irq, void *data) * dai ops */
-static int fsi_dai_startup(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) +static int fsi_hw_startup(struct fsi_priv *fsi, + int is_play, + struct device *dev) { - struct fsi_priv *fsi = fsi_get_priv(substream); u32 flags = fsi_get_info_flags(fsi); u32 data = 0; - int is_play = fsi_is_play(substream);
- pm_runtime_get_sync(dai->dev); + pm_runtime_get_sync(dev);
/* clock setting */ if (fsi_is_clk_master(fsi)) @@ -949,22 +948,38 @@ static int fsi_dai_startup(struct snd_pcm_substream *substream, fsi_irq_clear_status(fsi);
/* fifo init */ - fsi_fifo_init(fsi, is_play, dai); + fsi_fifo_init(fsi, is_play, dev);
return 0; }
+static void fsi_hw_shutdown(struct fsi_priv *fsi, + int is_play, + struct device *dev) +{ + if (fsi_is_clk_master(fsi)) + fsi_set_master_clk(dev, fsi, fsi->rate, 0); + + pm_runtime_put_sync(dev); +} + +static int fsi_dai_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct fsi_priv *fsi = fsi_get_priv(substream); + int is_play = fsi_is_play(substream); + + return fsi_hw_startup(fsi, is_play, dai->dev); +} + static void fsi_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct fsi_priv *fsi = fsi_get_priv(substream); + int is_play = fsi_is_play(substream);
- if (fsi_is_clk_master(fsi)) - fsi_set_master_clk(dai->dev, fsi, fsi->rate, 0); - + fsi_hw_shutdown(fsi, is_play, dai->dev); fsi->rate = 0; - - pm_runtime_put_sync(dai->dev); }
static int fsi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
FSIA/B ports is enabled by default when power-on, and current FSI is supporting RuntimePM. In addition, current fsi_module_init/kill doesn't care simultaneous playback/recorde. This mean FSI port control is not needed. This patch remove fsi_module_init/kill
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 30 ------------------------------ 1 files changed, 0 insertions(+), 30 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index c755d21..a457e5b 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -669,32 +669,11 @@ static int fsi_set_master_clk(struct device *dev, struct fsi_priv *fsi,
}
-#define fsi_module_init(m, d) __fsi_module_clk_ctrl(m, d, 1) -#define fsi_module_kill(m, d) __fsi_module_clk_ctrl(m, d, 0) -static void __fsi_module_clk_ctrl(struct fsi_master *master, - struct device *dev, - int enable) -{ - pm_runtime_get_sync(dev); - - if (enable) { - /* enable only SR */ - fsi_master_mask_set(master, SOFT_RST, FSISR, FSISR); - fsi_master_mask_set(master, SOFT_RST, PASR | PBSR, 0); - } else { - /* clear all registers */ - fsi_master_mask_set(master, SOFT_RST, FSISR, 0); - } - - pm_runtime_put_sync(dev); -} - #define fsi_port_start(f, i) __fsi_port_clk_ctrl(f, i, 1) #define fsi_port_stop(f, i) __fsi_port_clk_ctrl(f, i, 0) static void __fsi_port_clk_ctrl(struct fsi_priv *fsi, int is_play, int enable) { struct fsi_master *master = fsi_get_master(fsi); - u32 soft = fsi_is_port_a(fsi) ? PASR : PBSR; u32 clk = fsi_is_port_a(fsi) ? CRA : CRB; int is_master = fsi_is_clk_master(fsi);
@@ -703,7 +682,6 @@ static void __fsi_port_clk_ctrl(struct fsi_priv *fsi, int is_play, int enable) else fsi_irq_disable(fsi, is_play);
- fsi_master_mask_set(master, SOFT_RST, soft, (enable) ? soft : 0); if (is_master) fsi_master_mask_set(master, CLK_RST, clk, (enable) ? clk : 0); } @@ -1309,8 +1287,6 @@ static int fsi_probe(struct platform_device *pdev) pm_runtime_enable(&pdev->dev); dev_set_drvdata(&pdev->dev, master);
- fsi_module_init(master, &pdev->dev); - ret = request_irq(irq, &fsi_interrupt, IRQF_DISABLED, id_entry->name, master); if (ret) { @@ -1353,8 +1329,6 @@ static int fsi_remove(struct platform_device *pdev)
master = dev_get_drvdata(&pdev->dev);
- fsi_module_kill(master, &pdev->dev); - free_irq(master->irq, master); pm_runtime_disable(&pdev->dev);
@@ -1409,8 +1383,6 @@ static int fsi_suspend(struct device *dev) master->saved_clk_rst = fsi_master_read(master, CLK_RST); master->saved_soft_rst = fsi_master_read(master, SOFT_RST);
- fsi_module_kill(master, dev); - pm_runtime_put_sync(dev);
return 0; @@ -1422,8 +1394,6 @@ static int fsi_resume(struct device *dev)
pm_runtime_get_sync(dev);
- fsi_module_init(master, dev); - fsi_master_mask_set(master, SOFT_RST, 0xffff, master->saved_soft_rst); fsi_master_mask_set(master, CLK_RST, 0xffff, master->saved_clk_rst); fsi_core_mask_set(master, a_mclk, 0xffff, master->saved_a_mclk);
Current FSI driver was using saved_xxx variable for suspend/resume. OTOH, the start and stop of power/clock are controlled by fsi_hw_startup/fsi_hw_shutdown in current FSI driver. The all necessary registers value are set by fsi_hw_startup.
So, if fsi_hw_shutdown is called when "suspend" is generated, and fsi_hw_startup is called at "resume", the saved_xxx are not needed.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/fsi.c | 97 ++++++++++++++++++++++----------------------------- 1 files changed, 42 insertions(+), 55 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index a457e5b..d2f17ce 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -184,13 +184,6 @@ struct fsi_priv { int spdif:1;
long rate; - - /* for suspend/resume */ - u32 saved_do_fmt; - u32 saved_di_fmt; - u32 saved_ckg1; - u32 saved_ckg2; - u32 saved_out_sel; };
struct fsi_core { @@ -211,14 +204,6 @@ struct fsi_master { struct fsi_core *core; struct sh_fsi_platform_info *info; spinlock_t lock; - - /* for suspend/resume */ - u32 saved_a_mclk; - u32 saved_b_mclk; - u32 saved_iemsk; - u32 saved_imsk; - u32 saved_clk_rst; - u32 saved_soft_rst; };
/* @@ -388,6 +373,21 @@ static int fsi_sample2frame(struct fsi_priv *fsi, int samples) return samples / fsi->chan_num; }
+static int fsi_stream_is_working(struct fsi_priv *fsi, + int is_play) +{ + struct fsi_stream *io = fsi_get_stream(fsi, is_play); + struct fsi_master *master = fsi_get_master(fsi); + unsigned long flags; + int ret; + + spin_lock_irqsave(&master->lock, flags); + ret = !!io->substream; + spin_unlock_irqrestore(&master->lock, flags); + + return ret; +} + static void fsi_stream_push(struct fsi_priv *fsi, int is_play, struct snd_pcm_substream *substream) @@ -666,7 +666,6 @@ static int fsi_set_master_clk(struct device *dev, struct fsi_priv *fsi, }
return ret; - }
#define fsi_port_start(f, i) __fsi_port_clk_ctrl(f, i, 1) @@ -675,14 +674,13 @@ static void __fsi_port_clk_ctrl(struct fsi_priv *fsi, int is_play, int enable) { struct fsi_master *master = fsi_get_master(fsi); u32 clk = fsi_is_port_a(fsi) ? CRA : CRB; - int is_master = fsi_is_clk_master(fsi);
if (enable) fsi_irq_enable(fsi, is_play); else fsi_irq_disable(fsi, is_play);
- if (is_master) + if (fsi_is_clk_master(fsi)) fsi_master_mask_set(master, CLK_RST, clk, (enable) ? clk : 0); }
@@ -1342,48 +1340,43 @@ static int fsi_remove(struct platform_device *pdev) }
static void __fsi_suspend(struct fsi_priv *fsi, + int is_play, struct device *dev) { - fsi->saved_do_fmt = fsi_reg_read(fsi, DO_FMT); - fsi->saved_di_fmt = fsi_reg_read(fsi, DI_FMT); - fsi->saved_ckg1 = fsi_reg_read(fsi, CKG1); - fsi->saved_ckg2 = fsi_reg_read(fsi, CKG2); - fsi->saved_out_sel = fsi_reg_read(fsi, OUT_SEL); + if (!fsi_stream_is_working(fsi, is_play)) + return;
- if (fsi_is_clk_master(fsi)) - fsi_set_master_clk(dev, fsi, fsi->rate, 0); + fsi_port_stop(fsi, is_play); + fsi_hw_shutdown(fsi, is_play, dev); }
static void __fsi_resume(struct fsi_priv *fsi, + int is_play, struct device *dev) { - fsi_reg_write(fsi, DO_FMT, fsi->saved_do_fmt); - fsi_reg_write(fsi, DI_FMT, fsi->saved_di_fmt); - fsi_reg_write(fsi, CKG1, fsi->saved_ckg1); - fsi_reg_write(fsi, CKG2, fsi->saved_ckg2); - fsi_reg_write(fsi, OUT_SEL, fsi->saved_out_sel); + if (!fsi_stream_is_working(fsi, is_play)) + return;
- if (fsi_is_clk_master(fsi)) + fsi_hw_startup(fsi, is_play, dev); + + if (fsi_is_clk_master(fsi) && fsi->rate) fsi_set_master_clk(dev, fsi, fsi->rate, 1); + + fsi_port_start(fsi, is_play); + }
static int fsi_suspend(struct device *dev) { struct fsi_master *master = dev_get_drvdata(dev); + struct fsi_priv *fsia = &master->fsia; + struct fsi_priv *fsib = &master->fsib;
- pm_runtime_get_sync(dev); - - __fsi_suspend(&master->fsia, dev); - __fsi_suspend(&master->fsib, dev); + __fsi_suspend(fsia, 1, dev); + __fsi_suspend(fsia, 0, dev);
- master->saved_a_mclk = fsi_core_read(master, a_mclk); - master->saved_b_mclk = fsi_core_read(master, b_mclk); - master->saved_iemsk = fsi_core_read(master, iemsk); - master->saved_imsk = fsi_core_read(master, imsk); - master->saved_clk_rst = fsi_master_read(master, CLK_RST); - master->saved_soft_rst = fsi_master_read(master, SOFT_RST); - - pm_runtime_put_sync(dev); + __fsi_suspend(fsib, 1, dev); + __fsi_suspend(fsib, 0, dev);
return 0; } @@ -1391,20 +1384,14 @@ static int fsi_suspend(struct device *dev) static int fsi_resume(struct device *dev) { struct fsi_master *master = dev_get_drvdata(dev); + struct fsi_priv *fsia = &master->fsia; + struct fsi_priv *fsib = &master->fsib;
- pm_runtime_get_sync(dev); - - fsi_master_mask_set(master, SOFT_RST, 0xffff, master->saved_soft_rst); - fsi_master_mask_set(master, CLK_RST, 0xffff, master->saved_clk_rst); - fsi_core_mask_set(master, a_mclk, 0xffff, master->saved_a_mclk); - fsi_core_mask_set(master, b_mclk, 0xffff, master->saved_b_mclk); - fsi_core_mask_set(master, iemsk, 0xffff, master->saved_iemsk); - fsi_core_mask_set(master, imsk, 0xffff, master->saved_imsk); + __fsi_resume(fsia, 1, dev); + __fsi_resume(fsia, 0, dev);
- __fsi_resume(&master->fsia, dev); - __fsi_resume(&master->fsib, dev); - - pm_runtime_put_sync(dev); + __fsi_resume(fsib, 1, dev); + __fsi_resume(fsib, 0, dev);
return 0; }
On 23/05/11 12:45, Kuninori Morimoto wrote:
Dear Mark, Liam
Cc Simon, yoshii-san, Magnus
These are tidyup and cleanup patches for FSI
Kuninori Morimoto (9): ASoC: sh: fsi: tidyup parameter of fsi_stream_push ASoC: sh: fsi: tidyup unclear variable naming ASoC: sh: fsi: remove pm_runtime from fsi_dai_set_fmt. ASoC: sh: fsi: make sure fsi_stream_push/pop access by spin lock ASoC: sh: fsi: Add fsi_set_master_clk ASoC: sh: fsi: irq control moves to fsi_port_start/stop ASoC: sh: fsi: Add fsi_hw_startup/shutdown ASoC: sh: fsi: remove fsi_module_init/kill ASoC: sh: fsi: cleanup suspend/resume
#1 and #2 are tidyup for fsi_fifo_data_ctrl. The variables and sub-functions were unclear nameing. So, it was difficult to understand the detail of calculation. Thank you for pointing it yoshii-san.
#3 - #9 are clean up for RuntimePM. Current FSI driver was wrong for RuntimePM. these patches fixup for RuntimePM and clean up for suspend/resume. last patch remove noisy saved_xxx variable.
All
Acked-by: Liam Girdwood lrg@ti.com
participants (3)
-
Kuninori Morimoto -
Liam Girdwood -
Mark Brown