| Message ID | 20180621134009.27116-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | slimbus: Add Stream Support | expand |
On 21-06-18, 14:40, Srinivas Kandagatla wrote: > This patch adds support to SLIMbus stream apis for slimbus device. > SLIMbus streaming involves adding support to Data Channel Management and > channel Reconfiguration Messages to slim core plus few stream apis. > >From slim device side the apis are very simple mostly inline with other ^^ Bad char > > +/** > + * enum slim_port_direction: SLIMbus port direction blank line here makes it more readable > +/** > + * struct slim_presence_rate - Presense Rate table for all Natural Frequencies > + * The Presense rate of a constant bitrate stram is mean flow rate of the ^^^^^ Do you mean stream? > +static struct slim_presence_rate { > + int rate; > + int pr_code; > +} prate_table[] = { > + {12000, 0x01}, > + {24000, 0x02}, > + {48000, 0x03}, > + {96000, 0x04}, > + {192000, 0x05}, > + {384000, 0x06}, > + {768000, 0x07}, > + {110250, 0x09}, > + {220500, 0x0a}, > + {441000, 0x0b}, > + {882000, 0x0c}, > + {176400, 0x0d}, > + {352800, 0x0e}, > + {705600, 0x0f}, > + {4000, 0x10}, > + {8000, 0x11}, > + {16000, 0x12}, > + {32000, 0x13}, > + {64000, 0x14}, > + {128000, 0x15}, > + {256000, 0x16}, > + {512000, 0x17}, this table values are indices, so how about using only rate and removing pr_code and use array index for that, saves half the space.. > +struct slim_stream_runtime *slim_stream_allocate(struct slim_device *dev, > + const char *name) > +{ > + struct slim_stream_runtime *rt; > + unsigned long flags; > + > + rt = kzalloc(sizeof(*rt), GFP_KERNEL); > + if (!rt) > + return ERR_PTR(-ENOMEM); > + > + rt->name = kasprintf(GFP_KERNEL, "slim-%s", name); > + if (!rt->name) { > + kfree(rt); > + return ERR_PTR(-ENOMEM); > + } > + > + rt->dev = dev; > + rt->state = SLIM_STREAM_STATE_ALLOCATED; > + spin_lock_irqsave(&dev->stream_list_lock, flags); > + list_add_tail(&rt->node, &dev->stream_list); > + spin_unlock_irqrestore(&dev->stream_list_lock, flags); Any reason for _irqsave variant? Do you expect stream APIs to be called from ISR? > +/* > + * slim_stream_prepare() - Prepare a SLIMbus Stream > + * > + * @rt: instance of slim stream runtime to configure > + * @cfg: new configuration for the stream > + * > + * This API will configure SLIMbus stream with config parameters from cfg. > + * return zero on success and error code on failure. From ASoC DPCM framework, > + * this state is linked to hw_params()/prepare() operation. so would this be called from either.. btw prepare can be invoked multiple times, so that should be taken into consideration by caller. > + */ > +int slim_stream_prepare(struct slim_stream_runtime *rt, > + struct slim_stream_config *cfg) > +{ > + struct slim_controller *ctrl = rt->dev->ctrl; > + struct slim_port *port; > + int num_ports, i, port_id; > + > + num_ports = hweight32(cfg->port_mask); > + rt->ports = kcalloc(num_ports, sizeof(*port), GFP_ATOMIC); since this is supposed to be invoked in hw_params()/prepare, why would we need GFP_ATOMIC here? > +static int slim_activate_channel(struct slim_stream_runtime *stream, > + struct slim_port *port) > +{ > + struct slim_device *sdev = stream->dev; > + struct slim_val_inf msg = {0, 0, NULL, NULL}; > + u8 mc = SLIM_MSG_MC_NEXT_ACTIVATE_CHANNEL; > + DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg); > + u8 wbuf[1]; > + > + txn.msg->num_bytes = 1; > + txn.msg->wbuf = wbuf; > + wbuf[0] = port->ch.id; > + port->ch.state = SLIM_CH_STATE_ACTIVE; > + > + return slim_do_transfer(sdev->ctrl, &txn); > +} how about adding a macro for sending message, which fills slim_val_inf and you invoke that with required parameters to be filled. > +/* > + * slim_stream_enable() - Enable a prepared SLIMbus Stream Do you want to check if it is already prepared ..? > +/** > + * slim_stream_direction: SLIMbus stream direction > + * > + * @SLIM_STREAM_DIR_PLAYBACK: Playback > + * @SLIM_STREAM_DIR_CAPTURE: Capture > + */ > +enum slim_stream_direction { > + SLIM_STREAM_DIR_PLAYBACK = 0, > + SLIM_STREAM_DIR_CAPTURE, this is same as SNDRV_PCM_STREAM_PLAYBACK, so should we use that here? -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Vinod for the Review, On 22/06/18 13:50, Vinod wrote: > On 21-06-18, 14:40, Srinivas Kandagatla wrote: >> This patch adds support to SLIMbus stream apis for slimbus device. >> SLIMbus streaming involves adding support to Data Channel Management and >> channel Reconfiguration Messages to slim core plus few stream apis. >> >From slim device side the apis are very simple mostly inline with other > ^^ > Bad char > Yep, will fix it. > >> +/** >> + * enum slim_port_direction: SLIMbus port direction > > blank line here makes it more readable > Sure it makes sense. >> +/** >> + * struct slim_presence_rate - Presense Rate table for all Natural Frequencies >> + * The Presense rate of a constant bitrate stram is mean flow rate of the > ^^^^^ > Do you mean stream? Yep will fix it. > >> +static struct slim_presence_rate { >> + int rate; >> + int pr_code; >> +} prate_table[] = { >> + {12000, 0x01}, >> + {24000, 0x02}, >> + {48000, 0x03}, >> + {96000, 0x04}, >> + {192000, 0x05}, >> + {384000, 0x06}, >> + {768000, 0x07}, >> + {110250, 0x09}, >> + {220500, 0x0a}, >> + {441000, 0x0b}, >> + {882000, 0x0c}, >> + {176400, 0x0d}, >> + {352800, 0x0e}, >> + {705600, 0x0f}, >> + {4000, 0x10}, >> + {8000, 0x11}, >> + {16000, 0x12}, >> + {32000, 0x13}, >> + {64000, 0x14}, >> + {128000, 0x15}, >> + {256000, 0x16}, >> + {512000, 0x17}, > > this table values are indices, so how about using only rate and removing > pr_code and use array index for that, saves half the space.. > look like I over done it, I will fix this in next version. >> +struct slim_stream_runtime *slim_stream_allocate(struct slim_device *dev, >> + const char *name) >> +{ >> + struct slim_stream_runtime *rt; >> + unsigned long flags; >> + >> + rt = kzalloc(sizeof(*rt), GFP_KERNEL); >> + if (!rt) >> + return ERR_PTR(-ENOMEM); >> + >> + rt->name = kasprintf(GFP_KERNEL, "slim-%s", name); >> + if (!rt->name) { >> + kfree(rt); >> + return ERR_PTR(-ENOMEM); >> + } >> + >> + rt->dev = dev; >> + rt->state = SLIM_STREAM_STATE_ALLOCATED; >> + spin_lock_irqsave(&dev->stream_list_lock, flags); >> + list_add_tail(&rt->node, &dev->stream_list); >> + spin_unlock_irqrestore(&dev->stream_list_lock, flags); > > Any reason for _irqsave variant? Do you expect stream APIs to be called > from ISR?We can move to non irqsave variant here, as i do not see a case where this list would be interrupted from irq context. > >> +/* >> + * slim_stream_prepare() - Prepare a SLIMbus Stream >> + * >> + * @rt: instance of slim stream runtime to configure >> + * @cfg: new configuration for the stream >> + * >> + * This API will configure SLIMbus stream with config parameters from cfg. >> + * return zero on success and error code on failure. From ASoC DPCM framework, >> + * this state is linked to hw_params()/prepare() operation. > > so would this be called from either.. btw prepare can be invoked > multiple times, so that should be taken into consideration by caller. This should be just hw_params() where we have more information on the audio parameters, I will make this more clear in the doc about this. > >> + */ >> +int slim_stream_prepare(struct slim_stream_runtime *rt, >> + struct slim_stream_config *cfg) >> +{ >> + struct slim_controller *ctrl = rt->dev->ctrl; >> + struct slim_port *port; >> + int num_ports, i, port_id; >> + >> + num_ports = hweight32(cfg->port_mask); >> + rt->ports = kcalloc(num_ports, sizeof(*port), GFP_ATOMIC); > > since this is supposed to be invoked in hw_params()/prepare, why would > we need GFP_ATOMIC here? No, we do not need this to be ATOMIC, will remove this! > >> +static int slim_activate_channel(struct slim_stream_runtime *stream, >> + struct slim_port *port) >> +{ >> + struct slim_device *sdev = stream->dev; >> + struct slim_val_inf msg = {0, 0, NULL, NULL}; >> + u8 mc = SLIM_MSG_MC_NEXT_ACTIVATE_CHANNEL; >> + DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg); >> + u8 wbuf[1]; >> + >> + txn.msg->num_bytes = 1; >> + txn.msg->wbuf = wbuf; >> + wbuf[0] = port->ch.id; >> + port->ch.state = SLIM_CH_STATE_ACTIVE; >> + >> + return slim_do_transfer(sdev->ctrl, &txn); >> +} > > how about adding a macro for sending message, which fills slim_val_inf > and you invoke that with required parameters to be filled. > Sounds sensible thing, I will give that a try and see! >> +/* >> + * slim_stream_enable() - Enable a prepared SLIMbus Stream > > Do you want to check if it is already prepared ..? Yep, I think most of the code needs similar state machine check, I will add this in next version. > >> +/** >> + * slim_stream_direction: SLIMbus stream direction >> + * >> + * @SLIM_STREAM_DIR_PLAYBACK: Playback >> + * @SLIM_STREAM_DIR_CAPTURE: Capture >> + */ >> +enum slim_stream_direction { >> + SLIM_STREAM_DIR_PLAYBACK = 0, >> + SLIM_STREAM_DIR_CAPTURE, > > this is same as SNDRV_PCM_STREAM_PLAYBACK, so should we use that here? Sure will do, makes it clear! > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Stephen for review, On 25/06/18 17:12, Stephen Boyd wrote: > Quoting Srinivas Kandagatla (2018-06-21 06:40:08) >> new file mode 100644 >> index 000000000000..f8af9474d286 >> --- /dev/null >> +++ b/drivers/slimbus/stream.c >> @@ -0,0 +1,493 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2018, Linaro Limited >> + >> +#include <linux/kernel.h> >> +#include <linux/errno.h> >> +#include <linux/slab.h> >> +#include <linux/list.h> >> +#include <linux/slimbus.h> >> +#include "slimbus.h" >> + >> +/** >> + * struct segdist_code - Segment Distributions code from >> + * Table 20 of SLIMbus Specs Version 2.0 >> + * >> + * @ratem: Channel Rate Multipler(Segments per Superframe) >> + * @seg_interval: Number of slots between the first Slot of Segment >> + * and the first slot of the next consecutive Segment. >> + * @segdist_code: Segment Distribution Code SD[11:0] >> + * @seg_offset_mask: Segment offset mask in SD[11:0] >> + * @segdist_codes: List of all possible Segmet Distribution codes. >> + */ >> +static struct segdist_code { > > const? > Yep, Will fix this and presence rate in next version.! >> + int ratem; >> + int seg_interval; >> + int segdist_code; >> + u32 seg_offset_mask; >> + >> +} segdist_codes[] = { >> + {1, 1536, 0x200, 0xdff}, >> + {2, 768, 0x100, 0xcff}, >> + {4, 384, 0x080, 0xc7f}, >> + {8, 192, 0x040, 0xc3f}, >> + {16, 96, 0x020, 0xc1f}, >> + {32, 48, 0x010, 0xc0f}, >> + {64, 24, 0x008, 0xc07}, >> + {128, 12, 0x004, 0xc03}, >> + {256, 6, 0x002, 0xc01}, >> + {512, 3, 0x001, 0xc00}, >> + {3, 512, 0xe00, 0x1ff}, >> + {6, 256, 0xd00, 0x0ff}, >> + {12, 128, 0xc80, 0x07f}, >> + {24, 64, 0xc40, 0x03f}, >> + {48, 32, 0xc20, 0x01f}, >> + {96, 16, 0xc10, 0x00f}, >> + {192, 8, 0xc08, 0x007}, >> + {364, 4, 0xc04, 0x003}, >> + {768, 2, 0xc02, 0x001}, >> +}; >> + >> +/** >> + * struct slim_presence_rate - Presense Rate table for all Natural Frequencies >> + * The Presense rate of a constant bitrate stram is mean flow rate of the >> + * stream expressed in occupied Segments of that Data Channel per second. >> + * Table 66 from SLIMbus 2.0 Specs >> + * >> + * @rate: data rate >> + * @pr_code: presence rate code PR[6:0] >> + * @prate_table: All possible presence rate code for Natural Frequencies >> + */ >> +static struct slim_presence_rate { > > const? > >> + int rate; >> + int pr_code; >> +} prate_table[] = { -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25-06-18, 11:11, Srinivas Kandagatla wrote: > > > +struct slim_stream_runtime *slim_stream_allocate(struct slim_device *dev, > > > + const char *name) > > > +{ > > > + struct slim_stream_runtime *rt; > > > + unsigned long flags; > > > + > > > + rt = kzalloc(sizeof(*rt), GFP_KERNEL); > > > + if (!rt) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + rt->name = kasprintf(GFP_KERNEL, "slim-%s", name); > > > + if (!rt->name) { > > > + kfree(rt); > > > + return ERR_PTR(-ENOMEM); > > > + } > > > + > > > + rt->dev = dev; > > > + rt->state = SLIM_STREAM_STATE_ALLOCATED; > > > + spin_lock_irqsave(&dev->stream_list_lock, flags); > > > + list_add_tail(&rt->node, &dev->stream_list); > > > + spin_unlock_irqrestore(&dev->stream_list_lock, flags); > > > > Any reason for _irqsave variant? Do you expect stream APIs to be called > > from ISR?We can move to non irqsave variant here, as i do not see a case > > where > this list would be interrupted from irq context. That's interesting can you please describe how? Also, won't it be modified from bottom half... > > > +/* > > > + * slim_stream_enable() - Enable a prepared SLIMbus Stream > > > > Do you want to check if it is already prepared ..? > Yep, I think most of the code needs similar state machine check, I will add > this in next version. so if you are tying to snd stream states then I don't think you need a state machine here. ALSA already does that for you so you can skip it :D -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/06/18 17:21, Vinod wrote: >>>> +/* >>>> + * slim_stream_enable() - Enable a prepared SLIMbus Stream >>> Do you want to check if it is already prepared ..? >> Yep, I think most of the code needs similar state machine check, I will add >> this in next version. > so if you are tying to snd stream states then I don't think you need a > state machine here. ALSA already does that for you so you can skip it :D Stream state machine looks redundant, I will try to skip it and see how things look at the end! before sending next version. thanks, srini -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html