* [RESEND PATCH v2 03/15] ASoC: qcom: qdsp6: Add common qdsp6 helper functions
From: Bjorn Andersson @ 2018-01-02 0:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-4-srinivas.kandagatla@linaro.org>
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
> +static inline int q6dsp_map_channels(u8 *ch_map, int ch)
> +{
> + memset(ch_map, 0, PCM_FORMAT_MAX_NUM_CHANNEL);
This implies that ch_map is always an array of
PCM_FORMAT_MAX_NUM_CHANNEL elements. As such it would be better to
express this in the prototype; i.e u8 ch_map[PCM_FORMAT_MAX_NUM_CHANNEL]
> +
> + if (ch == 1) {
This is a switch statement.
> + ch_map[0] = PCM_CHANNEL_FC;
> + } else if (ch == 2) {
[..]
> +struct adsp_err_code {
> + int lnx_err_code;
Indentation, and these could be given more succinct names.
> + char *adsp_err_str;
> +};
> +
> +static struct adsp_err_code adsp_err_code_info[ADSP_ERR_MAX+1] = {
> + { 0, ADSP_EOK_STR},
> + { -ENOTRECOVERABLE, ADSP_EFAILED_STR},
> + { -EINVAL, ADSP_EBADPARAM_STR},
> + { -ENOSYS, ADSP_EUNSUPPORTED_STR},
> + { -ENOPROTOOPT, ADSP_EVERSION_STR},
> + { -ENOTRECOVERABLE, ADSP_EUNEXPECTED_STR},
> + { -ENOTRECOVERABLE, ADSP_EPANIC_STR},
> + { -ENOSPC, ADSP_ENORESOURCE_STR},
> + { -EBADR, ADSP_EHANDLE_STR},
> + { -EALREADY, ADSP_EALREADY_STR},
> + { -EPERM, ADSP_ENOTREADY_STR},
> + { -EINPROGRESS, ADSP_EPENDING_STR},
> + { -EBUSY, ADSP_EBUSY_STR},
> + { -ECANCELED, ADSP_EABORTED_STR},
> + { -EAGAIN, ADSP_EPREEMPTED_STR},
> + { -EAGAIN, ADSP_ECONTINUE_STR},
> + { -EAGAIN, ADSP_EIMMEDIATE_STR},
> + { -EAGAIN, ADSP_ENOTIMPL_STR},
> + { -ENODATA, ADSP_ENEEDMORE_STR},
> + { -EADV, ADSP_ERR_MAX_STR},
This, element 0x13, is not listed among the defined errors. Is this a
placeholder?
How about making this even more descriptive by using the format
[ADSP_EBADPARAM] = { -EINVAL, ADSP_EBADPARAM_STR },
That way the mapping table is self-describing.
And you can use ARRAY_SIZE() instead of specifying the fixed size of
ADSP_ERR_MAX + 1...
> + { -ENOMEM, ADSP_ENOMEMORY_STR},
> + { -ENODEV, ADSP_ENOTEXIST_STR},
> + { -EADV, ADSP_ERR_MAX_STR},
"Advertise error"?
> +};
> +
> +static inline int adsp_err_get_lnx_err_code(u32 adsp_error)
Can this be made internal to some c-file? So that any third party deals
only with linux error codes?
How about renaming this q6dsp_errno()?
> +{
> + if (adsp_error > ADSP_ERR_MAX)
> + return adsp_err_code_info[ADSP_ERR_MAX].lnx_err_code;
> + else
> + return adsp_err_code_info[adsp_error].lnx_err_code;
I think this would look better if you assign a local variable and have a
single return. And just hard code the "invalid error code" errno, rather
than looking up ADSP_ERR_MAX in the list.
> +}
> +
> +static inline char *adsp_err_get_err_str(u32 adsp_error)
q6dsp_strerror(), to match strerror(3)?
> +{
> + if (adsp_error > ADSP_ERR_MAX)
> + return adsp_err_code_info[ADSP_ERR_MAX].adsp_err_str;
> + else
> + return adsp_err_code_info[adsp_error].adsp_err_str;
And I do think that, as with strerror, this should return a human
readable error, not the stringified define.
> +}
I'm puzzled to why these helper functions lives in a header file, at
least some aspects of this would better be hidden...
Regards,
Bjorn
^ permalink raw reply
* [RESEND PATCH v2 04/15] ASoC: qcom: qdsp6: Add support to Q6AFE
From: Bjorn Andersson @ 2018-01-02 0:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-5-srinivas.kandagatla@linaro.org>
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
[..]
> +
> +config SND_SOC_QDSP6_AFE
> + tristate
> + default n
Do you see a particular benefit of having one kernel module per
function? Why not just compile them all into the same q6dsp.ko?
> +
> +config SND_SOC_QDSP6
> + tristate "SoC ALSA audio driver for QDSP6"
> + select SND_SOC_QDSP6_AFE
> + help
> + To add support for MSM QDSP6 Soc Audio.
> + This will enable sound soc platform specific
> + audio drivers. This includes q6asm, q6adm,
> + q6afe interfaces to DSP using apr.
[..]
> +struct q6afev2 {
> + void *apr;
apr has a type, even if it's definition is hidden you should use the
proper type here.
> + struct device *dev;
> + int state;
> + int status;
> + struct platform_device *daidev;
> +
> + struct mutex afe_cmd_lock;
You shouldn't need to include afe_ in this name.
> + struct list_head port_list;
> + spinlock_t port_list_lock;
> + struct list_head node;
> +};
> +
[..]
> +/* Port map of index vs real hw port ids */
> +static struct afe_port_map port_maps[AFE_PORT_MAX] = {
> + [AFE_PORT_HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX,
Looks like you have an extra tab here, consider breaking the 80 char
"rule" to not have to wrap these.
> + AFE_PORT_HDMI_RX, 1},
> +};
> +
> +static struct q6afe_port *afe_find_port(struct q6afev2 *afe, int token)
> +{
> + struct q6afe_port *p = NULL;
> +
> + spin_lock(&afe->port_list_lock);
> + list_for_each_entry(p, &afe->port_list, node)
> + if (p->token == token)
> + break;
> +
> + spin_unlock(&afe->port_list_lock);
> + return p;
> +}
Make port_list an idr and you can just use idr_find() instead of rolling
your own search function.
> +
> +static int afe_callback(struct apr_device *adev, struct apr_client_data *data)
> +{
> + struct q6afev2 *afe = dev_get_drvdata(&adev->dev);//priv;
This is perfectly fine, no need to extend the interface with a priv (so
drop the comment).
> + struct q6afe_port *port;
> +
> + if (!data) {
> + dev_err(afe->dev, "%s: Invalid param data\n", __func__);
> + return -EINVAL;
> + }
Just define on in the apr layer that data will never be NULL, that will
save you 4 lines of code in every apr client.
> +
> + if (data->payload_size) {
> + uint32_t *payload = data->payload;
So the payload is 2 ints, where the first is a command and the second is
the status of it. This you can express in a simple struct to make the
code even easier on the eye.
> +
> + if (data->opcode == APR_BASIC_RSP_RESULT) {
> + if (payload[1] != 0) {
> + afe->status = payload[1];
> + dev_err(afe->dev,
> + "cmd = 0x%x returned error = 0x%x\n",
> + payload[0], payload[1]);
> + }
> + switch (payload[0]) {
> + case AFE_PORT_CMD_SET_PARAM_V2:
> + case AFE_PORT_CMD_DEVICE_STOP:
> + case AFE_PORT_CMD_DEVICE_START:
> + afe->state = AFE_CMD_RESP_AVAIL;
> + port = afe_find_port(afe, data->token);
> + if (port)
> + wake_up(&port->wait);
> +
> + break;
> + default:
> + dev_err(afe->dev, "Unknown cmd 0x%x\n",
> + payload[0]);
If you flip the check for payload_size to return early if there isn't a
payload then you can reduce the indentation level one step and probably
doesn't have to wrap this line.
> + break;
> + }
> + }
> + }
> + return 0;
> +}
> +/**
> + * q6afe_get_port_id() - Get port id from a given port index
> + *
> + * @index: port index
> + *
> + * Return: Will be an negative on error or valid port_id on success
> + */
> +int q6afe_get_port_id(int index)
> +{
> + if (index < 0 || index > AFE_PORT_MAX)
> + return -EINVAL;
> +
> + return port_maps[index].port_id;
> +}
> +EXPORT_SYMBOL_GPL(q6afe_get_port_id);
> +
> +static int afe_apr_send_pkt(struct q6afev2 *afe, void *data,
> + wait_queue_head_t *wait)
Rather than conditionally passing the wait, split this function in
afe_send_sync(*afe, *data, wait) and afe_send_async(*afe, *data).
> +{
> + int ret;
> +
> + if (wait)
> + afe->state = AFE_CMD_RESP_NONE;
> +
> + afe->status = 0;
> + ret = apr_send_pkt(afe->apr, data);
> + if (ret > 0) {
Check ret < 0 and return here, this saves you one indentation level in
the following chunk.
If you then check !wait and return early you can reduce another level.
> + if (wait) {
> + ret = wait_event_timeout(*wait,
> + (afe->state ==
> + AFE_CMD_RESP_AVAIL),
> + msecs_to_jiffies(TIMEOUT_MS));
> + if (!ret) {
> + ret = -ETIMEDOUT;
> + } else if (afe->status > 0) {
> + dev_err(afe->dev, "DSP returned error[%s]\n",
> + adsp_err_get_err_str(afe->status));
> + ret = adsp_err_get_lnx_err_code(afe->status);
> + } else {
> + ret = 0;
> + }
> + } else {
> + ret = 0;
> + }
> + } else {
> + dev_err(afe->dev, "packet not transmitted\n");
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static int afe_send_cmd_port_start(struct q6afe_port *port)
> +{
> + u16 port_id = port->id;
> + struct afe_port_cmd_device_start start;
> + struct q6afev2 *afe = port->afe.v2;
> + int ret, index;
> +
> + index = port->token;
> + start.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
> + APR_HDR_LEN(APR_HDR_SIZE),
> + APR_PKT_VER);
> + start.hdr.pkt_size = sizeof(start);
> + start.hdr.src_port = 0;
> + start.hdr.dest_port = 0;
> + start.hdr.token = index;
Just put port->token here, saves you a local variable.
> + start.hdr.opcode = AFE_PORT_CMD_DEVICE_START;
> + start.port_id = port_id;
> +
> + ret = afe_apr_send_pkt(afe, &start, &port->wait);
> + if (ret)
> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",
> + port_id, ret);
> +
> + return ret;
> +}
> +
> +static int afe_port_start(struct q6afe_port *port,
> + union afe_port_config *afe_config)
> +{
> + struct afe_audioif_config_command config;
> + struct q6afev2 *afe = port->afe.v2;
> + int ret = 0;
> + int port_id = port->id;
> + int cfg_type;
> + int index = 0;
> +
> + if (!afe_config) {
Looking at the one caller of this function, afe_config can't be NULL, so
no need for this error handling.
> + dev_err(afe->dev, "Error, no configuration data\n");
> + ret = -EINVAL;
> + return ret;
> + }
> +
> + index = port->token;
> +
> + mutex_lock(&afe->afe_cmd_lock);
> + /* Also send the topology id here: */
> + config.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
> + APR_HDR_LEN(APR_HDR_SIZE),
> + APR_PKT_VER);
> + config.hdr.pkt_size = sizeof(config);
> + config.hdr.src_port = 0;
> + config.hdr.dest_port = 0;
> + config.hdr.token = index;
> +
> + cfg_type = port->cfg_type;
> + config.hdr.opcode = AFE_PORT_CMD_SET_PARAM_V2;
> + config.param.port_id = port_id;
> + config.param.payload_size = sizeof(config) - sizeof(struct apr_hdr) -
> + sizeof(config.param);
> + config.param.payload_address_lsw = 0x00;
> + config.param.payload_address_msw = 0x00;
> + config.param.mem_map_handle = 0x00;
> + config.pdata.module_id = AFE_MODULE_AUDIO_DEV_INTERFACE;
> + config.pdata.param_id = cfg_type;
> + config.pdata.param_size = sizeof(config.port);
This looks like a good candidate for a afe_port_set_param() function.
> +
> + config.port = *afe_config;
> +
> + ret = afe_apr_send_pkt(afe, &config, &port->wait);
> + if (ret) {
> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",
> + port_id, ret);
> + goto fail_cmd;
> + }
> +
> + ret = afe_send_cmd_port_start(port);
> +
> +fail_cmd:
> + mutex_unlock(&afe->afe_cmd_lock);
> + return ret;
> +}
[..]
> +/**
> + * q6afe_port_get_from_id() - Get port instance from a port id
> + *
> + * @dev: Pointer to afe child device.
> + * @id: port id
> + *
> + * Return: Will be an error pointer on error or a valid afe port
> + * on success.
> + */
> +struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id)
Will there be any other getter? Otherwise you can just call this
q6afe_port_get().
> +{
> + int port_id;
> + struct q6afev2 *afe = dev_get_drvdata(dev->parent);
> + struct q6afe_port *port;
> + int token;
> + int cfg_type;
> +
> + if (!afe) {
> + dev_err(dev, "Unable to find instance of afe service\n");
> + return ERR_PTR(-ENOENT);
> + }
As this comes from the passed dev this check will catch bugs withing
this driver, but if the client accidentally passes the wrong dev it's
likely that you don't catch it here anyways. Consider dropping the
check.
> +
> + token = id;
> + if (token < 0 || token > AFE_PORT_MAX) {
> + dev_err(dev, "AFE port token[%d] invalid!\n", token);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + port_id = port_maps[id].port_id;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return ERR_PTR(-ENOMEM);
> +
> + init_waitqueue_head(&port->wait);
> +
> + port->token = token;
> + port->id = port_id;
> +
> + port->afe.v2 = afe;
> + switch (port_id) {
How about moving this switch statement above the allocation?
> + case AFE_PORT_ID_MULTICHAN_HDMI_RX:
> + cfg_type = AFE_PARAM_ID_HDMI_CONFIG;
> + break;
> + default:
> + dev_err(dev, "Invalid port id 0x%x\n", port_id);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + port->cfg_type = cfg_type;
> +
> + spin_lock(&afe->port_list_lock);
> + list_add_tail(&port->node, &afe->port_list);
> + spin_unlock(&afe->port_list_lock);
> +
> + return port;
> +
> +}
> +EXPORT_SYMBOL_GPL(q6afe_port_get_from_id);
Regards,
Bjorn
^ permalink raw reply
* [RFC] does ioremap() cause memory leak?
From: Hanjun Guo @ 2018-01-02 1:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5A3DEA6A.9080709@huawei.com>
On 2017/12/23 13:32, Xishi Qiu wrote:
> On 2017/12/21 16:55, Xishi Qiu wrote:
>
>> When we use iounmap() to free the mapping, it calls unmap_vmap_area() to clear page table,
>> but do not free the memory of page table, right?
>>
>> So when use ioremap() to mapping another area(incluce the area before), it may use
>> large mapping(e.g. ioremap_pmd_enabled()), so the original page table memory(e.g. pte memory)
>> will be lost, it cause memory leak, right?
>
>
>
> So I have two questions for this scene.
>
> 1. When the same virtual address allocated from ioremap, first is 4K size, second is 2M size, if Kernel would leak memory.
>
> 2. Kernel modifies the old invalid 4K pagetable to 2M, but doesn`t follow the ARM break-before-make flow, CPU maybe get the old invalid 4K pagetable information, then Kernel would panic.
I sent a RFC patch for this one [1].
[1]: https://patchwork.kernel.org/patch/10134581/
Thanks
Hanjun
^ permalink raw reply
* [PATCH 1/3] phy: phy-mtk-tphy: use auto instead of force to bypass utmi signals
From: Chunfeng Yun @ 2018-01-02 1:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3eb63ed0-5e56-d664-4953-b1998d7bd19f@ti.com>
On Thu, 2017-12-28 at 16:44 +0530, Kishon Vijay Abraham I wrote:
>
> On Thursday 07 December 2017 05:23 PM, Chunfeng Yun wrote:
> > When system is running, if usb2 phy is forced to bypass utmi signals,
> > all PLL will be turned off, and it can't detect device connection
> > anymore, so replace force mode with auto mode which can bypass utmi
> > signals automatically if no device attached for normal flow.
> > But keep the force mode to fix RX sensitivity degradation issue.
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>
> merged this series.
Thanks a lot
>
> Thanks
> Kishon
> > ---
> > drivers/phy/mediatek/phy-mtk-tphy.c | 19 +++++++------------
> > 1 file changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c b/drivers/phy/mediatek/phy-mtk-tphy.c
> > index fb8aba4..5d9d7f3 100644
> > --- a/drivers/phy/mediatek/phy-mtk-tphy.c
> > +++ b/drivers/phy/mediatek/phy-mtk-tphy.c
> > @@ -440,9 +440,9 @@ static void u2_phy_instance_init(struct mtk_tphy *tphy,
> > u32 index = instance->index;
> > u32 tmp;
> >
> > - /* switch to USB function. (system register, force ip into usb mode) */
> > + /* switch to USB function, and enable usb pll */
> > tmp = readl(com + U3P_U2PHYDTM0);
> > - tmp &= ~P2C_FORCE_UART_EN;
> > + tmp &= ~(P2C_FORCE_UART_EN | P2C_FORCE_SUSPENDM);
> > tmp |= P2C_RG_XCVRSEL_VAL(1) | P2C_RG_DATAIN_VAL(0);
> > writel(tmp, com + U3P_U2PHYDTM0);
> >
> > @@ -502,10 +502,8 @@ static void u2_phy_instance_power_on(struct mtk_tphy *tphy,
> > u32 index = instance->index;
> > u32 tmp;
> >
> > - /* (force_suspendm=0) (let suspendm=1, enable usb 480MHz pll) */
> > tmp = readl(com + U3P_U2PHYDTM0);
> > - tmp &= ~(P2C_FORCE_SUSPENDM | P2C_RG_XCVRSEL);
> > - tmp &= ~(P2C_RG_DATAIN | P2C_DTM0_PART_MASK);
> > + tmp &= ~(P2C_RG_XCVRSEL | P2C_RG_DATAIN | P2C_DTM0_PART_MASK);
> > writel(tmp, com + U3P_U2PHYDTM0);
> >
> > /* OTG Enable */
> > @@ -540,7 +538,6 @@ static void u2_phy_instance_power_off(struct mtk_tphy *tphy,
> >
> > tmp = readl(com + U3P_U2PHYDTM0);
> > tmp &= ~(P2C_RG_XCVRSEL | P2C_RG_DATAIN);
> > - tmp |= P2C_FORCE_SUSPENDM;
> > writel(tmp, com + U3P_U2PHYDTM0);
> >
> > /* OTG Disable */
> > @@ -548,18 +545,16 @@ static void u2_phy_instance_power_off(struct mtk_tphy *tphy,
> > tmp &= ~PA6_RG_U2_OTG_VBUSCMP_EN;
> > writel(tmp, com + U3P_USBPHYACR6);
> >
> > - /* let suspendm=0, set utmi into analog power down */
> > - tmp = readl(com + U3P_U2PHYDTM0);
> > - tmp &= ~P2C_RG_SUSPENDM;
> > - writel(tmp, com + U3P_U2PHYDTM0);
> > - udelay(1);
> > -
> > tmp = readl(com + U3P_U2PHYDTM1);
> > tmp &= ~(P2C_RG_VBUSVALID | P2C_RG_AVALID);
> > tmp |= P2C_RG_SESSEND;
> > writel(tmp, com + U3P_U2PHYDTM1);
> >
> > if (tphy->pdata->avoid_rx_sen_degradation && index) {
> > + tmp = readl(com + U3P_U2PHYDTM0);
> > + tmp &= ~(P2C_RG_SUSPENDM | P2C_FORCE_SUSPENDM);
> > + writel(tmp, com + U3P_U2PHYDTM0);
> > +
> > tmp = readl(com + U3D_U2PHYDCR0);
> > tmp &= ~P2C_RG_SIF_U2PLL_FORCE_ON;
> > writel(tmp, com + U3D_U2PHYDCR0);
> >
^ permalink raw reply
* [RESEND PATCH v2 05/15] ASoC: qcom: qdsp6: Add support to Q6ADM
From: Bjorn Andersson @ 2018-01-02 1:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-6-srinivas.kandagatla@linaro.org>
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds support to q6 ADM (Audio Device Manager) module in
> q6dsp. ADM performs routing between audio streams and AFE ports.
> It does Rate matching for streams going to devices driven by
lower case "Rate"
> different clocks, it handles volume ramping, Mixing with channel
and "Mixing"
> and bit-width. ADM creates and destroys dynamic COPP services
> for device-related audio processing as needed.
What's a "copp"?
>
> This patch adds basic support to ADM.
Wouldn't s/to/for/ be better?
[..]
> +struct copp {
> + int afe_port;
> + int copp_idx;
> + int id;
> + int cnt;
Please rename this "refcnt" to match other kernel code.
> + int topology;
> + int mode;
> + int stat;
> + int rate;
> + int bit_width;
> + int channels;
> + int app_type;
> + int acdb_id;
> + wait_queue_head_t wait;
> + struct list_head node;
> + struct q6adm *adm;
> +};
> +
> +struct q6adm {
> + struct apr_device *apr;
> + struct device *dev;
> + unsigned long copp_bitmap[AFE_MAX_PORTS];
> + struct list_head copps_list;
> + spinlock_t copps_list_lock;
> + int matrix_map_stat;
> + struct platform_device *routing_dev;
> +
> + wait_queue_head_t matrix_map_wait;
> +};
> +
> +static struct copp *adm_find_copp(struct q6adm *adm, int port_idx, int copp_idx)
> +{
> + struct copp *c;
> +
> + spin_lock(&adm->copps_list_lock);
> + list_for_each_entry(c, &adm->copps_list, node) {
> + if ((port_idx == c->afe_port) && (copp_idx == c->copp_idx)) {
> + spin_unlock(&adm->copps_list_lock);
> + return c;
> + }
> + }
> +
> + spin_unlock(&adm->copps_list_lock);
> + return NULL;
> +
> +}
> +
> +static struct copp *adm_find_matching_copp(struct q6adm *adm,
> + int port_idx, int topology,
> + int mode, int rate,
> + int bit_width, int app_type)
> +{
> + struct copp *c;
> +
> + spin_lock(&adm->copps_list_lock);
> +
> + list_for_each_entry(c, &adm->copps_list, node) {
> + if ((port_idx == c->afe_port) && (topology == c->topology) &&
> + (mode == c->mode) && (rate == c->rate) &&
> + (bit_width == c->bit_width) && (app_type == c->app_type)) {
> + spin_unlock(&adm->copps_list_lock);
> + return c;
> + }
> + }
> + spin_unlock(&adm->copps_list_lock);
> +
> + return NULL;
> +
> +}
> +
> +static int adm_callback(struct apr_device *adev, struct apr_client_data *data)
> +{
> + uint32_t *payload;
> + int port_idx, copp_idx;
> + struct copp *copp;
> + struct q6adm *adm = dev_get_drvdata(&adev->dev);
> +
> + payload = data->payload;
> +
> + if (data->payload_size) {
Bail if you don't have a payload and save yourself one indentation
level.
> + copp_idx = (data->token) & 0XFF;
> + port_idx = ((data->token) >> 16) & 0xFF;
> + if (port_idx < 0 || port_idx >= AFE_MAX_PORTS) {
> + dev_err(&adev->dev, "Invalid port idx %d token %d\n",
> + port_idx, data->token);
> + return 0;
> + }
> + if (copp_idx < 0 || copp_idx >= MAX_COPPS_PER_PORT) {
> + dev_err(&adev->dev, "Invalid copp idx %d token %d\n",
> + copp_idx, data->token);
> + return 0;
> + }
> +
> + if (data->opcode == APR_BASIC_RSP_RESULT) {
This is a case in the following switch statement.
> + if (payload[1] != 0) {
> + dev_err(&adev->dev, "cmd = 0x%x returned error = 0x%x\n",
> + payload[0], payload[1]);
This would again benefit from a small struct...
> + }
> + switch (payload[0]) {
> + case ADM_CMD_DEVICE_OPEN_V5:
> + case ADM_CMD_DEVICE_CLOSE_V5:
> + copp = adm_find_copp(adm, port_idx, copp_idx);
> + if (IS_ERR_OR_NULL(copp))
> + return 0;
> +
> + copp->stat = payload[1];
> + wake_up(&copp->wait);
> + break;
> + case ADM_CMD_MATRIX_MAP_ROUTINGS_V5:
> + adm->matrix_map_stat = payload[1];
> + wake_up(&adm->matrix_map_wait);
> + break;
> +
> + default:
> + dev_err(&adev->dev, "Unknown Cmd: 0x%x\n",
> + payload[0]);
> + break;
> + }
> + return 0;
> + }
> +
> + switch (data->opcode) {
> + case ADM_CMDRSP_DEVICE_OPEN_V5:{
Perhaps it would be cleaner to break these out in separate functions?
> + struct adm_cmd_rsp_device_open_v5 {
> + u32 status;
> + u16 copp_id;
> + u16 reserved;
> + } __packed * open = data->payload;
> +
> + open = data->payload;
> + copp = adm_find_copp(adm, port_idx, copp_idx);
> + if (IS_ERR_OR_NULL(copp))
> + return 0;
> +
> + if (open->copp_id == INVALID_COPP_ID) {
> + dev_err(&adev->dev, "Invalid coppid rxed %d\n",
> + open->copp_id);
> + copp->stat = ADSP_EBADPARAM;
> + wake_up(&copp->wait);
> + break;
> + }
> + copp->stat = payload[0];
> + copp->id = open->copp_id;
> + pr_debug("%s: coppid rxed=%d\n", __func__,
You have a dev, use dev_dbg()
> + open->copp_id);
> + wake_up(&copp->wait);
> +
> + }
> + break;
> + default:
> + dev_err(&adev->dev, "Unknown cmd:0x%x\n",
> + data->opcode);
> + break;
> + }
> + }
> + return 0;
> +}
> +
> +static struct copp *adm_alloc_copp(struct q6adm *adm, int port_idx)
> +{
> + struct copp *c;
> + int idx;
> +
> + idx = find_first_zero_bit(&adm->copp_bitmap[port_idx],
> + MAX_COPPS_PER_PORT);
> +
> + if (idx > MAX_COPPS_PER_PORT)
> + return ERR_PTR(-EBUSY);
> +
> + set_bit(idx, &adm->copp_bitmap[port_idx]);
> +
> + c = devm_kzalloc(adm->dev, sizeof(*c), GFP_KERNEL);
> + if (!c)
Set the bit after doing the allocation and you don't need to clear it
here.
> + return ERR_PTR(-ENOMEM);
> + c->copp_idx = idx;
> + c->afe_port = port_idx;
> + c->adm = adm;
> +
> + init_waitqueue_head(&c->wait);
> +
> + spin_lock(&adm->copps_list_lock);
> + list_add_tail(&c->node, &adm->copps_list);
> + spin_unlock(&adm->copps_list_lock);
> +
> + return c;
> +}
> +
> +static void adm_free_copp(struct q6adm *adm, struct copp *c, int port_idx)
> +{
> + clear_bit(c->copp_idx, &adm->copp_bitmap[port_idx]);
> + spin_lock(&adm->copps_list_lock);
> + list_del(&c->node);
> + spin_unlock(&adm->copps_list_lock);
This function clear the copp_bitmap, so after recycling this once
c->copp_idx will be "dangling".
This seems to put the copp in a state where it is invalid and as the
copp is "reset" i don't think adm_find_matching_copp() will actually
find this again, ever...
So shouldn't you free the copp here too - rather than relying on devm
doing that at some later point in time?
> +}
> +/**
> + * q6adm_open() - open adm to get hold of free copp
"open adm and grab a free copp"?
> + *
> + * @dev: Pointer to adm child device.
> + * @port_id: port id
> + * @path: playback or capture path.
> + * @rate: rate at which copp is required.
> + * @channel_mode: channel mode
> + * @topology: adm topology id
> + * @perf_mode: performace mode.
> + * @bit_width: audio sample bit width
> + * @app_type: Application type.
> + * @acdb_id: ACDB id
> + *
> + * Return: Will be an negative on error or a valid copp index on success.
> + */
> +int q6adm_open(struct device *dev, int port_id, int path, int rate,
> + int channel_mode, int topology, int perf_mode,
> + uint16_t bit_width, int app_type, int acdb_id)
> +{
> + struct adm_cmd_device_open_v5 {
> + struct apr_hdr hdr;
> + u16 flags;
> + u16 mode_of_operation;
> + u16 endpoint_id_1;
> + u16 endpoint_id_2;
> + u32 topology_id;
> + u16 dev_num_channel;
> + u16 bit_width;
> + u32 sample_rate;
> + u8 dev_channel_mapping[8];
> + } __packed open;
> + int ret = 0;
> + int port_idx, flags;
> + int tmp_port = q6afe_get_port_id(port_id);
> + struct copp *copp;
> + struct q6adm *adm = dev_get_drvdata(dev->parent);
> +
> + port_idx = port_id;
I'm not seeing the reason for having two different variables with the
same value.
> + if (port_idx < 0) {
> + dev_err(dev, "Invalid port_id 0x%x\n", port_id);
> + return -EINVAL;
> + }
> +
> + flags = ADM_LEGACY_DEVICE_SESSION;
Just put ADM_LEGACY_DEVICE_SESSION in the assignment below.
> + copp = adm_find_matching_copp(adm, port_idx, topology, perf_mode,
> + rate, bit_width, app_type);
> +
> + if (!copp) {
> + copp = adm_alloc_copp(adm, port_idx);
> + if (IS_ERR_OR_NULL(copp))
> + return PTR_ERR(copp);
> +
> + copp->cnt = 0;
> + copp->topology = topology;
> + copp->mode = perf_mode;
> + copp->rate = rate;
> + copp->channels = channel_mode;
> + copp->bit_width = bit_width;
> + copp->app_type = app_type;
> + }
I would suggest that you make adm_find_matching_copp() allocate the new
copp if none is found.
> +
> + /* Create a COPP if port id are not enabled */
> + if (copp->cnt == 0) {
Doesn't this scheme require some locking? What about concurrent close()?
> +
> + open.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
> + APR_HDR_LEN(APR_HDR_SIZE),
> + APR_PKT_VER);
> + open.hdr.pkt_size = sizeof(open);
> + open.hdr.src_svc = APR_SVC_ADM;
> + open.hdr.src_domain = APR_DOMAIN_APPS;
> + open.hdr.src_port = tmp_port;
> + open.hdr.dest_svc = APR_SVC_ADM;
> + open.hdr.dest_domain = APR_DOMAIN_ADSP;
> + open.hdr.dest_port = tmp_port;
> + open.hdr.token = port_idx << 16 | copp->copp_idx;
> + open.hdr.opcode = ADM_CMD_DEVICE_OPEN_V5;
> + open.flags = flags;
> + open.mode_of_operation = path;
> + open.endpoint_id_1 = tmp_port;
> + open.topology_id = topology;
> + open.dev_num_channel = channel_mode & 0x00FF;
> + open.bit_width = bit_width;
> + open.sample_rate = rate;
This looks like a q6adm_device_open() helper function.
> +
> + ret = q6dsp_map_channels(&open.dev_channel_mapping[0],
> + channel_mode);
> +
> + if (ret)
> + return ret;
> +
> + copp->stat = -1;
> + ret = apr_send_pkt(adm->apr, (uint32_t *)&open);
> + if (ret < 0) {
> + dev_err(dev, "port_id: 0x%x for[0x%x] failed %d\n",
> + tmp_port, port_id, ret);
> + return -EINVAL;
> + }
> + /* Wait for the callback with copp id */
> + ret =
> + wait_event_timeout(copp->wait, copp->stat >= 0,
> + msecs_to_jiffies(TIMEOUT_MS));
> + if (!ret) {
> + dev_err(dev, "ADM timedout port_id: 0x%x for [0x%x]\n",
> + tmp_port, port_id);
> + return -EINVAL;
> + } else if (copp->stat > 0) {
> + dev_err(dev, "DSP returned error[%s]\n",
> + adsp_err_get_err_str(copp->stat));
> + return adsp_err_get_lnx_err_code(copp->stat);
> + }
> + }
> + copp->cnt++;
> + return copp->copp_idx;
> +}
> +EXPORT_SYMBOL_GPL(q6adm_open);
> +/**
> + * q6adm_matrix_map() - Map asm streams and afe ports using payload
> + *
> + * @dev: Pointer to adm child device.
> + * @path: playback or capture path.
> + * @payload_map: map between session id and afe ports.
> + * @perf_mode: Performace mode.
> + *
> + * Return: Will be an negative on error or a zero on success.
> + */
> +int q6adm_matrix_map(struct device *dev, int path,
> + struct route_payload payload_map, int perf_mode)
> +{
> + struct adm_cmd_matrix_map_routings_v5 {
> + struct apr_hdr hdr;
> + u32 matrix_id;
> + u32 num_sessions;
> + } __packed * route;
> +
> + struct adm_session_map_node_v5 {
> + u16 session_id;
> + u16 num_copps;
> + } __packed * node;
> + struct q6adm *adm = dev_get_drvdata(dev->parent);
> + uint16_t *copps_list;
> + int cmd_size = 0;
> + int ret = 0, i = 0;
> + void *payload = NULL;
> + void *matrix_map = NULL;
> + int port_idx, copp_idx;
> + struct copp *copp;
> +
> + /* Assumes port_ids have already been validated during adm_open */
> + cmd_size = (sizeof(*route) +
> + sizeof(*node) +
> + (sizeof(uint32_t) * payload_map.num_copps));
> + matrix_map = kzalloc(cmd_size, GFP_KERNEL);
> + if (!matrix_map)
> + return -ENOMEM;
> +
> + route = (struct adm_cmd_matrix_map_routings_v5 *)matrix_map;
> + route->hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
> + APR_HDR_LEN(APR_HDR_SIZE),
> + APR_PKT_VER);
> + route->hdr.pkt_size = cmd_size;
> + route->hdr.src_svc = 0;
> + route->hdr.src_domain = APR_DOMAIN_APPS;
> + route->hdr.src_port = 0; /* Ignored */
Omit the ignored members instead.
> + route->hdr.dest_svc = APR_SVC_ADM;
> + route->hdr.dest_domain = APR_DOMAIN_ADSP;
> + route->hdr.dest_port = 0; /* Ignored */
> + route->hdr.token = 0;
> + route->hdr.opcode = ADM_CMD_MATRIX_MAP_ROUTINGS_V5;
> + route->num_sessions = 1;
> +
> + switch (path) {
> + case ADM_PATH_PLAYBACK:
> + route->matrix_id = ADM_MATRIX_ID_AUDIO_RX;
> + break;
> + default:
> + dev_err(dev, "Wrong path set[%d]\n", path);
> +
> + break;
> + }
> +
> + payload = ((u8 *) matrix_map + sizeof(*route));
matrix_map is a void *, so no need to cast it to u8 * to calculate the
payload address.
> + node = (struct adm_session_map_node_v5 *)payload;
payload is void *, so no need to typecast here. And for that matter, I'm
not sure about the benefits of having this intermediate "payload"
variable, just assign node directly.
> +
> + node->session_id = payload_map.session_id;
> + node->num_copps = payload_map.num_copps;
> + payload = (u8 *) node + sizeof(*node);
> + copps_list = (uint16_t *) payload;
As with node above, drop the temporary variable and drop the type casts.
> +
> + for (i = 0; i < payload_map.num_copps; i++) {
> + port_idx = payload_map.port_id[i];
> + if (port_idx < 0) {
> + dev_err(dev, "Invalid port_id 0x%x\n",
> + payload_map.port_id[i]);
Leaking matrix_map.
> + return -EINVAL;
> + }
> + copp_idx = payload_map.copp_idx[i];
> +
> + copp = adm_find_copp(adm, port_idx, copp_idx);
> + if (IS_ERR_OR_NULL(copp))
Dito.
> + return -EINVAL;
> +
> + copps_list[i] = copp->id;
> + }
> +
> + adm->matrix_map_stat = -1;
> +
> + ret = apr_send_pkt(adm->apr, (uint32_t *) matrix_map);
> + if (ret < 0) {
> + dev_err(dev, "routing for syream %d failed ret %d\n",
> + payload_map.session_id, ret);
> + ret = -EINVAL;
> + goto fail_cmd;
> + }
> + ret = wait_event_timeout(adm->matrix_map_wait,
> + adm->matrix_map_stat >= 0,
> + msecs_to_jiffies(TIMEOUT_MS));
> + if (!ret) {
> + dev_err(dev, "routing for syream %d failed\n",
> + payload_map.session_id);
> + ret = -EINVAL;
> + goto fail_cmd;
> + } else if (adm->matrix_map_stat > 0) {
> + dev_err(dev, "DSP returned error[%s]\n",
> + adsp_err_get_err_str(adm->matrix_map_stat));
> + ret = adsp_err_get_lnx_err_code(adm->matrix_map_stat);
> + goto fail_cmd;
> + }
> +
> +fail_cmd:
> + kfree(matrix_map);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(q6adm_matrix_map);
> +
> +static void adm_reset_copp(struct copp *c)
As far as I can see this will decommission the copp, so I don't think
there is a point in updating any of this and then keep it around?
> +{
> + c->id = RESET_COPP_ID;
> + c->cnt = 0;
> + c->topology = 0;
> + c->mode = 0;
> + c->stat = -1;
> + c->rate = 0;
> + c->channels = 0;
> + c->bit_width = 0;
> + c->app_type = 0;
> +}
> +/**
> + * q6adm_close() - Close adm copp
> + *
> + * @dev: Pointer to adm child device.
> + * @port_id: afe port id.
> + * @perf_mode: perf_mode mode
> + * @copp_idx: copp index to close
> + *
> + * Return: Will be an negative on error or a zero on success.
> + */
> +int q6adm_close(struct device *dev, int port_id, int perf_mode, int copp_idx)
> +{
> + struct apr_hdr close;
> + struct copp *copp;
> +
> + int ret = 0, port_idx;
> + int copp_id = RESET_COPP_ID;
> + struct q6adm *adm = dev_get_drvdata(dev->parent);
> +
> + port_idx = port_id;
> + if (port_idx < 0) {
> + dev_err(dev, "Invalid port_id 0x%x\n", port_id);
> + return -EINVAL;
> + }
> +
> + if ((copp_idx < 0) || (copp_idx >= MAX_COPPS_PER_PORT)) {
> + dev_err(dev, "Invalid copp idx: %d\n", copp_idx);
> + return -EINVAL;
> + }
> +
> + copp = adm_find_copp(adm, port_id, copp_idx);
> + if (IS_ERR_OR_NULL(copp))
> + return -EINVAL;
> +
> + copp->cnt--;
> + if (!copp->cnt) {
Again, this needs some locking.
> + copp_id = copp->id;
> +
> + close.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
> + APR_HDR_LEN(APR_HDR_SIZE),
> + APR_PKT_VER);
> + close.pkt_size = sizeof(close);
> + close.src_svc = APR_SVC_ADM;
> + close.src_domain = APR_DOMAIN_APPS;
> + close.src_port = port_id;
> + close.dest_svc = APR_SVC_ADM;
> + close.dest_domain = APR_DOMAIN_ADSP;
> + close.dest_port = copp_id;
> + close.token = port_idx << 16 | copp_idx;
> + close.opcode = ADM_CMD_DEVICE_CLOSE_V5;
> +
Split this out into a separate helper function.
> + ret = apr_send_pkt(adm->apr, (uint32_t *) &close);
> + if (ret < 0) {
> + dev_err(dev, "ADM close failed %d\n", ret);
> + return -EINVAL;
> + }
> +
> + ret = wait_event_timeout(copp->wait, copp->stat >= 0,
> + msecs_to_jiffies(TIMEOUT_MS));
> + if (!ret) {
> + dev_err(dev, "ADM cmd Route timedout for port 0x%x\n",
> + port_id);
> + return -EINVAL;
> + } else if (copp->stat > 0) {
> + dev_err(dev, "DSP returned error[%s]\n",
> + adsp_err_get_err_str(copp->stat));
> + return adsp_err_get_lnx_err_code(copp->stat);
> + }
> +
> + adm_reset_copp(copp);
> + adm_free_copp(adm, copp, port_id);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(q6adm_close);
[..]
> +static struct apr_driver qcom_q6adm_driver = {
> + .probe = q6adm_probe,
> + .remove = q6adm_exit,
> + .callback = adm_callback,
> + .id_table = adm_id,
> + .driver = {
> + .name = "qcom-q6adm",
> + },
Indentation.
Regards,
Bjorn
^ permalink raw reply
* [PATCH] ARM: sunxi_defconfig: Enable SUNXI_CCU
From: Chen-Yu Tsai @ 2018-01-02 1:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101192208.11738-1-clabbe.montjoie@gmail.com>
On Tue, Jan 2, 2018 at 3:22 AM, Corentin Labbe
<clabbe.montjoie@gmail.com> wrote:
> While looking for missing symbol by diffing sunxi_defconfig and my
> .config, I just found that no CLK symbol are present in sunxi_defconfig.
This does not take into account default values in Kconfig. Use
`make savedefconfg` then compare the resulting defconfig file.
>
> This patch add the missing CONFIG_SUNXI_CCU in sunxi_defconfig.
> All other symbol will be automatically selected via their default value.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> arch/arm/configs/sunxi_defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig
> index 5caaf971fb50..5ed920e75842 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -16,6 +16,7 @@ CONFIG_CPU_FREQ=y
> CONFIG_CPUFREQ_DT=y
> CONFIG_VFP=y
> CONFIG_NEON=y
> +CONFIG_SUNXI_CCU=y
Not needed. See the last line of:
config SUNXI_CCU
bool "Clock support for Allwinner SoCs"
depends on ARCH_SUNXI || COMPILE_TEST
select RESET_CONTROLLER
default ARCH_SUNXI
Surely kernelci hasn't been broken since the introduction of
sunxi-ng. :)
ChenYu
> CONFIG_NET=y
> CONFIG_PACKET=y
> CONFIG_UNIX=y
> --
> 2.13.6
>
^ permalink raw reply
* [PATCH v5 7/9] arm64: Topology, rename cluster_id
From: Xiongfeng Wang @ 2018-01-02 2:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171218124229.GG507@e105550-lin.cambridge.arm.com>
Hi,
On 2017/12/18 20:42, Morten Rasmussen wrote:
> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>> Hi,
>>
>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>> [+Morten, Dietmar]
>>>
>>> $SUBJECT should be:
>>>
>>> arm64: topology: rename cluster_id
>>
[cut]
>>
>> I was hoping someone else would comment here, but my take at this point is
>> that it doesn't really matter in a functional sense at the moment.
>> Like the chiplet discussion it can be the subject of a future patch along
>> with the patches which tweak the scheduler to understand the split.
>>
>> BTW, given that i'm OoO next week, and the following that are the holidays,
>> I don't intend to repost this for a couple weeks. I don't think there are
>> any issues with this set.
>>
>>>
>>> There is also arch/arm to take into account, again, this patch is
>>> just renaming (as it should have named since the beginning) a
>>> topology level but we should consider everything from a legacy
>>> perspective.
>
> arch/arm has gone for thread/core/socket for the three topology levels
> it supports.
>
> I'm not sure what short term value keeping cluster_id has? Isn't it just
> about where we make the package = cluster assignment? Currently it is in
> the definition of topology_physical_package_id. If we keep cluster_id
> and add package_id, it gets moved into the MPIDR/DT parsing code.
>
> Keeping cluster_id and introducing a topology_cluster_id function could
> help cleaning up some of the users of topology_physical_package_id that
> currently assumes package_id == cluster_id though.
I think we still need the information describing which cores are in one cluster.
Many arm64 chips have the architecture core/cluster/socket. Cores in one cluster may
share a same L2 cache. That information can be used to build the sched_domain. If we put
cores in one cluster in one sched_domain, the performance will be better.(please see
kernel/sched/topology.c:1197, cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
sched_domain)
So I think we still need variable to record which cores are in one sched_domain for future use.
Thanks,
Xiongfeng
>
> Morten
>
> .
>
^ permalink raw reply
* [RESEND PATCH v2 06/15] ASoC: qcom: qdsp6: Add support to Q6ASM
From: Bjorn Andersson @ 2018-01-02 4:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-7-srinivas.kandagatla@linaro.org>
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds basic support to Q6 ASM (Audio Stream Manager) module on
> Q6DSP. ASM supports up to 8 concurrent streams. each stream can be setup
> as playback/capture.
"...streams, each one setup as either playback or capture".
or "each" need to be capitalized.
> ASM provides top control functions like
> Pause/flush/resume for playback and record. ASM can Create/destroy encoder,
lower case p and c
> decoder and also provides POPP dynamic services.
Please describe what POPP is.
[..]
> +struct audio_client {
> + int session;
> + app_cb cb;
> + int cmd_state;
> + void *priv;
> + uint32_t io_mode;
> + uint64_t time_stamp;
Unused.
> + struct apr_device *adev;
> + struct mutex cmd_lock;
> + wait_queue_head_t cmd_wait;
> + int perf_mode;
> + int stream_id;
> + struct device *dev;
> +};
> +
> +struct q6asm {
> + struct apr_device *adev;
> + int mem_state;
> + struct device *dev;
> + wait_queue_head_t mem_wait;
> + struct mutex session_lock;
> + struct platform_device *pcmdev;
> + struct audio_client *session[MAX_SESSIONS + 1];
> +};
> +
> +static int q6asm_session_alloc(struct audio_client *ac, struct q6asm *a)
Move the allocation of ac into this function, and return the newly
allocated ac - that way the name of this function makes more sense.
> +{
> + int n = -EINVAL;
You're returning MAX_SESSIONS if no free sessions are found, but are
checking for <= 0 in the caller.
> +
> + mutex_lock(&a->session_lock);
> + for (n = 1; n <= MAX_SESSIONS; n++) {
Is there an external reason for session 0 not being considered?
> + if (!a->session[n]) {
> + a->session[n] = ac;
> + break;
> + }
> + }
If you make session an idr this function would become idr_alloc(1,
MAX_SESSIONS + 1).
> + mutex_unlock(&a->session_lock);
> +
> + return n;
> +}
> +
> +static bool q6asm_is_valid_audio_client(struct audio_client *ac)
> +{
> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
> + int n;
> +
> + for (n = 1; n <= MAX_SESSIONS; n++) {
> + if (a->session[n] == ac)
> + return 1;
"true"
> + }
> +
> + return 0;
"false"
> +}
> +
> +static void q6asm_session_free(struct audio_client *ac)
> +{
> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
> +
> + if (!a)
> + return;
> +
> + mutex_lock(&a->session_lock);
> + a->session[ac->session] = 0;
> + ac->session = 0;
> + ac->perf_mode = LEGACY_PCM_MODE;
No need to update ac->*, as you kfree ac as soon as you return from
here.
> + mutex_unlock(&a->session_lock);
> +}
> +
> +/**
> + * q6asm_audio_client_free() - Freee allocated audio client
> + *
> + * @ac: audio client to free
> + */
> +void q6asm_audio_client_free(struct audio_client *ac)
> +{
> + q6asm_session_free(ac);
Inline q6asm_session_free() here.
> + kfree(ac);
> +}
> +EXPORT_SYMBOL_GPL(q6asm_audio_client_free);
> +
> +static struct audio_client *q6asm_get_audio_client(struct q6asm *a,
> + int session_id)
> +{
> + if ((session_id <= 0) || (session_id > MAX_SESSIONS)) {
> + dev_err(a->dev, "invalid session: %d\n", session_id);
> + goto err;
Just return NULL here instead.
> + }
> +
> + if (!a->session[session_id]) {
> + dev_err(a->dev, "session not active: %d\n", session_id);
> + goto err;
Dito
> + }
But this is another place where an idr would be preferable, as both
these cases would be covered with a call to idr_find()
> + return a->session[session_id];
> +err:
> + return NULL;
> +}
> +
> +static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
> +{
> + struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
> + struct audio_client *ac = NULL;
> + uint32_t sid = 0;
This is 4 bits, so just use int.
> + uint32_t *payload;
payload is unused.
> +
> + if (!data) {
> + dev_err(&adev->dev, "%s: Invalid CB\n", __func__);
> + return 0;
> + }
Again, define the apr to never invoke the callback with data = NULL
> +
> + payload = data->payload;
> + sid = (data->token >> 8) & 0x0F;
> + ac = q6asm_get_audio_client(q6asm, sid);
> + if (!ac) {
> + dev_err(&adev->dev, "Audio Client not active\n");
> + return 0;
> + }
> +
> + if (ac->cb)
> + ac->cb(data->opcode, data->token, data->payload, ac->priv);
> + return 0;
> +}
> +
> +/**
> + * q6asm_get_session_id() - get session id for audio client
> + *
> + * @ac: audio client pointer
> + *
> + * Return: Will be an session id of the audio client.
> + */
> +int q6asm_get_session_id(struct audio_client *c)
> +{
> + return c->session;
> +}
> +EXPORT_SYMBOL_GPL(q6asm_get_session_id);
> +
> +/**
> + * q6asm_audio_client_alloc() - Allocate a new audio client
> + *
> + * @dev: Pointer to asm child device.
> + * @cb: event callback.
> + * @priv: private data associated with this client.
> + *
> + * Return: Will be an error pointer on error or a valid audio client
> + * on success.
> + */
> +struct audio_client *q6asm_audio_client_alloc(struct device *dev,
> + app_cb cb, void *priv)
> +{
> + struct q6asm *a = dev_get_drvdata(dev->parent);
> + struct audio_client *ac;
> + int n;
> +
> + ac = kzalloc(sizeof(struct audio_client), GFP_KERNEL);
sizeof(*ac)
> + if (!ac)
> + return NULL;
> +
> + n = q6asm_session_alloc(ac, a);
As stated above, moving the kzalloc into q6asm_session_alloc() would
clean the code up here, as you only need to deal with one possible
error case here.
> + if (n <= 0) {
> + dev_err(dev, "ASM Session alloc fail n=%d\n", n);
> + kfree(ac);
> + return NULL;
Per the kerneldoc I expect an ERR_PTR(n) here.
> + }
> +
> + ac->session = n;
> + ac->cb = cb;
> + ac->dev = dev;
> + ac->priv = priv;
> + ac->io_mode = SYNC_IO_MODE;
> + ac->perf_mode = LEGACY_PCM_MODE;
> + /* DSP expects stream id from 1 */
> + ac->stream_id = 1;
> + ac->adev = a->adev;
> +
> + init_waitqueue_head(&ac->cmd_wait);
> + mutex_init(&ac->cmd_lock);
> + ac->cmd_state = 0;
> +
> + return ac;
> +}
> +EXPORT_SYMBOL_GPL(q6asm_audio_client_alloc);
> +
> +
Extra newline.
> +static int q6asm_probe(struct apr_device *adev)
> +{
> + struct q6asm *q6asm;
> +
> + q6asm = devm_kzalloc(&adev->dev, sizeof(*q6asm), GFP_KERNEL);
> + if (!q6asm)
> + return -ENOMEM;
> +
> + q6asm->dev = &adev->dev;
> + q6asm->adev = adev;
> + q6asm->mem_state = 0;
> + init_waitqueue_head(&q6asm->mem_wait);
> + mutex_init(&q6asm->session_lock);
> + dev_set_drvdata(&adev->dev, q6asm);
> +
> + q6asm->pcmdev = platform_device_register_data(&adev->dev,
> + "q6asm_dai", -1, NULL, 0);
> +
> + return 0;
> +}
> +
> +static int q6asm_remove(struct apr_device *adev)
> +{
> + struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
> +
> + platform_device_unregister(q6asm->pcmdev);
> +
> + return 0;
> +}
> +
> +static const struct apr_device_id q6asm_id[] = {
> + {"Q6ASM", APR_DOMAIN_ADSP, APR_SVC_ASM, APR_CLIENT_AUDIO},
> + {}
> +};
> +
> +static struct apr_driver qcom_q6asm_driver = {
> + .probe = q6asm_probe,
> + .remove = q6asm_remove,
> + .callback = q6asm_srvc_callback,
> + .id_table = q6asm_id,
> + .driver = {
> + .name = "qcom-q6asm",
> + },
Indentation
> +};
> +
> +module_apr_driver(qcom_q6asm_driver);
> +MODULE_DESCRIPTION("Q6 Audio Stream Manager driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h
> new file mode 100644
> index 000000000000..7a8a9039fd89
> --- /dev/null
> +++ b/sound/soc/qcom/qdsp6/q6asm.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __Q6_ASM_H__
> +#define __Q6_ASM_H__
> +
> +#define MAX_SESSIONS 16
> +
> +typedef void (*app_cb) (uint32_t opcode, uint32_t token,
> + uint32_t *payload, void *priv);
This name of a type is too generic.
And make payload void *, unless the payload really really is an
unstructured uint32_t array.
Regards,
Bjorn
^ permalink raw reply
* [PATCH v6 4/5] clk: aspeed: Register gated clocks
From: Benjamin Herrenschmidt @ 2018-01-02 5:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1514584997.2743.107.camel@kernel.crashing.org>
On Sat, 2017-12-30 at 09:03 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2017-12-26 at 17:32 -0800, Stephen Boyd wrote:
> > > I noticed we do have a few i2c based clock drivers... how are they ever
> > > supposed to work ? i2c bus controllers are allowed to sleep and the i2c
> > > core takes mutexes...
> >
> > We have clk_prepare()/clk_unprepare() for sleeping suckage. You
> > can use that, and i2c based clk drivers do that today.
>
> "suckage" ? Hehe ... the suckage should rather be stuff that cannot
> sleep. Arbitrary latencies and jitter caused by too much code wanting
> to be "atomic" when unnecessary are a bad thing.
>
> In the case of clocks like the aspeed where we have to wait for a
> rather long stabilization delay, way too long to legitimately do a non-
> sleepable delay with a lock held, do we need to do everything in
> prepare() then ?
BTW. Pls don't hold Joel's patches for this. Without that clk framework
a lot of the aspeed stuff already upstream doesn't actually work
without additional out-of-tree hacks or uboot black magic.
We can sort out the sleeping issues (and possibly move to using prepare
for the clocks that have that delay requirement) via subsequent
improvements.
Cheers,
Ben.
^ permalink raw reply
* [PATCH v7 1/5] clk: Add clock driver for ASPEED BMC SoCs
From: Benjamin Herrenschmidt @ 2018-01-02 5:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-2-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> This adds the stub of a driver for the ASPEED SoCs. The clocks are
> defined and the static registration is set up.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v7:
> - Rebase on dt-bindings patch
> - Add ASPEED_NUM_CLKS as it no longer lives in the header
> v6:
> - Add SPDX copyright notices
> v5:
> - Add Andrew's reviewed-by
> - Make aspeed_gates not initconst to avoid section mismatch warning
> v3:
> - use named initlisers for aspeed_gates table
> - fix clocks typo
> - Move ASPEED_NUM_CLKS to the bottom of the list
> - Put gates at the start of the list, so we can use them to initalise
> the aspeed_gates table
> - Add ASPEED_CLK_SELECTION_2
> - Set parent of network MAC gates
> ---
> drivers/clk/Kconfig | 12 ++++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-aspeed.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 drivers/clk/clk-aspeed.c
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1c4e1aa6767e..9abe063ef8d2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -142,6 +142,18 @@ config COMMON_CLK_GEMINI
> This driver supports the SoC clocks on the Cortina Systems Gemini
> platform, also known as SL3516 or CS3516.
>
> +config COMMON_CLK_ASPEED
> + bool "Clock driver for Aspeed BMC SoCs"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + default ARCH_ASPEED
> + select MFD_SYSCON
> + select RESET_CONTROLLER
> + ---help---
> + This driver supports the SoC clocks on the Aspeed BMC platforms.
> +
> + The G4 and G5 series, including the ast2400 and ast2500, are supported
> + by this driver.
> +
> config COMMON_CLK_S2MPS11
> tristate "Clock driver for S2MPS1X/S5M8767 MFD"
> depends on MFD_SEC_CORE || COMPILE_TEST
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index f7f761b02bed..d260da4809f8 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o
> obj-$(CONFIG_COMMON_CLK_CS2000_CP) += clk-cs2000-cp.o
> obj-$(CONFIG_ARCH_EFM32) += clk-efm32gg.o
> obj-$(CONFIG_COMMON_CLK_GEMINI) += clk-gemini.o
> +obj-$(CONFIG_COMMON_CLK_ASPEED) += clk-aspeed.o
> obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o
> obj-$(CONFIG_CLK_HSDK) += clk-hsdk-pll.o
> obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> new file mode 100644
> index 000000000000..7a86ee08ea4f
> --- /dev/null
> +++ b/drivers/clk/clk-aspeed.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#define pr_fmt(fmt) "clk-aspeed: " fmt
> +
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include <dt-bindings/clock/aspeed-clock.h>
> +
> +#define ASPEED_NUM_CLKS 35
> +
> +#define ASPEED_STRAP 0x70
> +
> +/* Keeps track of all clocks */
> +static struct clk_hw_onecell_data *aspeed_clk_data;
> +
> +static void __iomem *scu_base;
> +
> +/**
> + * struct aspeed_gate_data - Aspeed gated clocks
> + * @clock_idx: bit used to gate this clock in the clock register
> + * @reset_idx: bit used to reset this IP in the reset register. -1 if no
> + * reset is required when enabling the clock
> + * @name: the clock name
> + * @parent_name: the name of the parent clock
> + * @flags: standard clock framework flags
> + */
> +struct aspeed_gate_data {
> + u8 clock_idx;
> + s8 reset_idx;
> + const char *name;
> + const char *parent_name;
> + unsigned long flags;
> +};
> +
> +/**
> + * struct aspeed_clk_gate - Aspeed specific clk_gate structure
> + * @hw: handle between common and hardware-specific interfaces
> + * @reg: register controlling gate
> + * @clock_idx: bit used to gate this clock in the clock register
> + * @reset_idx: bit used to reset this IP in the reset register. -1 if no
> + * reset is required when enabling the clock
> + * @flags: hardware-specific flags
> + * @lock: register lock
> + *
> + * Some of the clocks in the Aspeed SoC must be put in reset before enabling.
> + * This modified version of clk_gate allows an optional reset bit to be
> + * specified.
> + */
> +struct aspeed_clk_gate {
> + struct clk_hw hw;
> + struct regmap *map;
> + u8 clock_idx;
> + s8 reset_idx;
> + u8 flags;
> + spinlock_t *lock;
> +};
> +
> +#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
> +
> +/* TODO: ask Aspeed about the actual parent data */
> +static const struct aspeed_gate_data aspeed_gates[] = {
> + /* clk rst name parent flags */
> + [ASPEED_CLK_GATE_ECLK] = { 0, -1, "eclk-gate", "eclk", 0 }, /* Video Engine */
> + [ASPEED_CLK_GATE_GCLK] = { 1, 7, "gclk-gate", NULL, 0 }, /* 2D engine */
> + [ASPEED_CLK_GATE_MCLK] = { 2, -1, "mclk-gate", "mpll", CLK_IS_CRITICAL }, /* SDRAM */
> + [ASPEED_CLK_GATE_VCLK] = { 3, 6, "vclk-gate", NULL, 0 }, /* Video Capture */
> + [ASPEED_CLK_GATE_BCLK] = { 4, 10, "bclk-gate", "bclk", 0 }, /* PCIe/PCI */
> + [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", NULL, 0 }, /* DAC */
> + [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", "clkin", CLK_IS_CRITICAL },
> + [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", NULL, 0 }, /* USB2.0 Host port 2 */
> + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", NULL, 0 }, /* LPC */
> + [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", NULL, 0 }, /* USB1.1 (requires port 2 enabled) */
> + [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", NULL, 0 }, /* GFX CRT */
> + [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", NULL, 0 }, /* HAC */
> + [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
> + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", "uart", 0 }, /* UART1 */
> + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", "uart", 0 }, /* UART2 */
> + [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, "uart5clk-gate", "uart", 0 }, /* UART5 */
> + [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL, 0 }, /* eSPI */
> + [ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate", "mac", 0 }, /* MAC1 */
> + [ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate", "mac", 0 }, /* MAC2 */
> + [ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate", NULL, 0 }, /* RSA */
> + [ASPEED_CLK_GATE_UART3CLK] = { 25, -1, "uart3clk-gate", "uart", 0 }, /* UART3 */
> + [ASPEED_CLK_GATE_UART4CLK] = { 26, -1, "uart4clk-gate", "uart", 0 }, /* UART4 */
> + [ASPEED_CLK_GATE_SDCLKCLK] = { 27, 16, "sdclk-gate", NULL, 0 }, /* SDIO/SD */
> + [ASPEED_CLK_GATE_LHCCLK] = { 28, -1, "lhclk-gate", "lhclk", 0 }, /* LPC master/LPC+ */
> +};
> +
> +static void __init aspeed_cc_init(struct device_node *np)
> +{
> + struct regmap *map;
> + u32 val;
> + int ret;
> + int i;
> +
> + scu_base = of_iomap(np, 0);
> + if (IS_ERR(scu_base))
> + return;
> +
> + aspeed_clk_data = kzalloc(sizeof(*aspeed_clk_data) +
> + sizeof(*aspeed_clk_data->hws) * ASPEED_NUM_CLKS,
> + GFP_KERNEL);
> + if (!aspeed_clk_data)
> + return;
> +
> + /*
> + * This way all clocks fetched before the platform device probes,
> + * except those we assign here for early use, will be deferred.
> + */
> + for (i = 0; i < ASPEED_NUM_CLKS; i++)
> + aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> + map = syscon_node_to_regmap(np);
> + if (IS_ERR(map)) {
> + pr_err("no syscon regmap\n");
> + return;
> + }
> + /*
> + * We check that the regmap works on this very first access,
> + * but as this is an MMIO-backed regmap, subsequent regmap
> + * access is not going to fail and we skip error checks from
> + * this point.
> + */
> + ret = regmap_read(map, ASPEED_STRAP, &val);
> + if (ret) {
> + pr_err("failed to read strapping register\n");
> + return;
> + }
> +
> + aspeed_clk_data->num = ASPEED_NUM_CLKS;
> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> + if (ret)
> + pr_err("failed to add DT provider: %d\n", ret);
> +};
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g5, "aspeed,ast2500-scu", aspeed_cc_init);
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g4, "aspeed,ast2400-scu", aspeed_cc_init);
^ permalink raw reply
* [RESEND PATCH v2 07/15] ASoC: qcom: q6asm: Add support to memory map and unmap
From: Bjorn Andersson @ 2018-01-02 5:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-8-srinivas.kandagatla@linaro.org>
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds support to memory map and unmap regions commands in
> q6asm module.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> sound/soc/qcom/qdsp6/q6asm.c | 343 ++++++++++++++++++++++++++++++++++++++++++-
> sound/soc/qcom/qdsp6/q6asm.h | 5 +
> 2 files changed, 347 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
> index 9cc583afef4d..4be92441f524 100644
> --- a/sound/soc/qcom/qdsp6/q6asm.c
> +++ b/sound/soc/qcom/qdsp6/q6asm.c
> @@ -14,9 +14,46 @@
> #include "q6asm.h"
> #include "common.h"
>
> +#define ASM_CMD_SHARED_MEM_MAP_REGIONS 0x00010D92
> +#define ASM_CMDRSP_SHARED_MEM_MAP_REGIONS 0x00010D93
> +#define ASM_CMD_SHARED_MEM_UNMAP_REGIONS 0x00010D94
> +
> #define TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */
> #define SYNC_IO_MODE 0x0001
> #define ASYNC_IO_MODE 0x0002
> +#define ASM_SHIFT_GAPLESS_MODE_FLAG 31
> +#define ADSP_MEMORY_MAP_SHMEM8_4K_POOL 3
> +
> +struct avs_cmd_shared_mem_map_regions {
> + struct apr_hdr hdr;
> + u16 mem_pool_id;
> + u16 num_regions;
> + u32 property_flag;
> +} __packed;
> +
> +struct avs_shared_map_region_payload {
> + u32 shm_addr_lsw;
> + u32 shm_addr_msw;
> + u32 mem_size_bytes;
> +} __packed;
> +
> +struct avs_cmd_shared_mem_unmap_regions {
> + struct apr_hdr hdr;
> + u32 mem_map_handle;
> +} __packed;
> +
> +struct audio_buffer {
> + dma_addr_t phys;
> + uint32_t used;
> + uint32_t size; /* size of buffer */
> +};
> +
> +struct audio_port_data {
> + struct audio_buffer *buf;
> + uint32_t max_buf_cnt;
This seems to denote the number of audio_buffers in the buf array, so
I'm not sure about the meaning of "max_".
> + uint32_t dsp_buf;
> + uint32_t mem_map_handle;
> +};
>
> struct audio_client {
> int session;
> @@ -27,6 +64,8 @@ struct audio_client {
> uint64_t time_stamp;
> struct apr_device *adev;
> struct mutex cmd_lock;
> + /* idx:1 out port, 0: in port */
> + struct audio_port_data port[2];
> wait_queue_head_t cmd_wait;
> int perf_mode;
> int stream_id;
> @@ -86,6 +125,260 @@ static void q6asm_session_free(struct audio_client *ac)
> mutex_unlock(&a->session_lock);
> }
>
> +static inline void q6asm_add_mmaphdr(struct audio_client *ac,
> + struct apr_hdr *hdr, u32 pkt_size,
> + bool cmd_flg, u32 token)
cmd_flg is true in both callers, so this function ends up simply
assigning hdr_field, pkt_size and token. Inlining these assignments
would make for cleaner call sites as well.
> +{
> + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD;
> + hdr->src_port = 0;
> + hdr->dest_port = 0;
> + hdr->pkt_size = pkt_size;
> + if (cmd_flg)
> + hdr->token = token;
> +}
> +
> +static inline void q6asm_add_hdr(struct audio_client *ac, struct apr_hdr *hdr,
This is unused.
> + uint32_t pkt_size, bool cmd_flg,
> + uint32_t stream_id)
> +{
> + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD;
> + hdr->src_svc = ac->adev->svc_id;
> + hdr->src_domain = APR_DOMAIN_APPS;
> + hdr->dest_svc = APR_SVC_ASM;
> + hdr->dest_domain = APR_DOMAIN_ADSP;
> + hdr->src_port = ((ac->session << 8) & 0xFF00) | (stream_id);
> + hdr->dest_port = ((ac->session << 8) & 0xFF00) | (stream_id);
> + hdr->pkt_size = pkt_size;
> + if (cmd_flg)
> + hdr->token = ac->session;
> +}
> +
> +static int __q6asm_memory_unmap(struct audio_client *ac,
> + phys_addr_t buf_add, int dir)
> +{
> + struct avs_cmd_shared_mem_unmap_regions mem_unmap;
If you name this "cmd" you will declutter below code a bit.
> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
> + int rc;
> +
> + if (!a)
> + return -ENODEV;
Does this NULL check add any real value?
> +
> + q6asm_add_mmaphdr(ac, &mem_unmap.hdr, sizeof(mem_unmap), true,
> + ((ac->session << 8) | dir));
> + a->mem_state = -1;
> +
> + mem_unmap.hdr.opcode = ASM_CMD_SHARED_MEM_UNMAP_REGIONS;
> + mem_unmap.mem_map_handle = ac->port[dir].mem_map_handle;
> +
> + if (mem_unmap.mem_map_handle == 0) {
Start the function by checking for !ac->port[dir].mem_map_handle
> + dev_err(ac->dev, "invalid mem handle\n");
> + return -EINVAL;
> + }
> +
> + rc = apr_send_pkt(a->adev, (uint32_t *) &mem_unmap);
> + if (rc < 0)
> + return rc;
> +
> + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0),
> + 5 * HZ);
> + if (!rc) {
> + dev_err(ac->dev, "CMD timeout for memory_unmap 0x%x\n",
> + mem_unmap.mem_map_handle);
> + return -ETIMEDOUT;
> + } else if (a->mem_state > 0) {
> + return adsp_err_get_lnx_err_code(a->mem_state);
> + }
> + ac->port[dir].mem_map_handle = 0;
Does all errors indicate that the region is left mapped?
> +
> + return 0;
> +}
> +
> +/**
> + * q6asm_unmap_memory_regions() - unmap memory regions in the dsp.
> + *
> + * @dir: direction of audio stream
> + * @ac: audio client instanace
> + *
> + * Return: Will be an negative value on failure or zero on success
> + */
> +int q6asm_unmap_memory_regions(unsigned int dir, struct audio_client *ac)
> +{
> + struct audio_port_data *port;
> + int cnt = 0;
> + int rc = 0;
> +
> + mutex_lock(&ac->cmd_lock);
> + port = &ac->port[dir];
> + if (!port->buf) {
> + mutex_unlock(&ac->cmd_lock);
> + return 0;
Put a label right before the mutex_unlock below and return rc instead of
0, then you can replace these two lines with "goto unlock"
> + }
> + cnt = port->max_buf_cnt - 1;
What if we mapped 1 period? Why shouldn't we enter the unmap path?
> + if (cnt >= 0) {
> + rc = __q6asm_memory_unmap(ac, port->buf[dir].phys, dir);
> + if (rc < 0) {
> + dev_err(ac->dev, "%s: Memory_unmap_regions failed %d\n",
> + __func__, rc);
Most of the code paths through __q6asm_memory_unmap() will print an
error, make this consistent and print the warning once.
> + mutex_unlock(&ac->cmd_lock);
> + return rc;
Same here.
> + }
> + }
> +
> + port->max_buf_cnt = 0;
> + kfree(port->buf);
> + port->buf = NULL;
> + mutex_unlock(&ac->cmd_lock);
I think however that if you rearrange this function somewhat you can
start off by doing:
mutex_lock(&ac->cmd_lock);
port = &ac->port[dir];
bufs = port->buf;
cnt = port->max_buf_cnt;
port->max_buf_cnt = 0;
port->buf = NULL;
mutex_unlock(&ac->cmd_lock);
And then you can do the rest without locks.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(q6asm_unmap_memory_regions);
> +
> +static int __q6asm_memory_map_regions(struct audio_client *ac, int dir,
> + uint32_t period_sz, uint32_t periods,
period_sz is typical size_t material.
> + bool is_contiguous)
> +{
> + struct avs_cmd_shared_mem_map_regions *mmap_regions = NULL;
Calling this "cmd" would declutter the function.
> + struct avs_shared_map_region_payload *mregions = NULL;
> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
> + struct audio_port_data *port = NULL;
> + struct audio_buffer *ab = NULL;
> + void *mmap_region_cmd = NULL;
No need to initialize this.
Also, this really is a avs_cmd_shared_mem_map_regions with some extra
data at the end, not a void *.
> + void *payload = NULL;
> + int rc = 0;
> + int i = 0;
> + int cmd_size = 0;
Most of these can be left uninitialized.
> + uint32_t num_regions;
> + uint32_t buf_sz;
> +
> + if (!a)
> + return -ENODEV;
> + num_regions = is_contiguous ? 1 : periods;
> + buf_sz = is_contiguous ? (period_sz * periods) : period_sz;
> + buf_sz = PAGE_ALIGN(buf_sz);
> +
> + cmd_size = sizeof(*mmap_regions) + (sizeof(*mregions) * num_regions);
> +
> + mmap_region_cmd = kzalloc(cmd_size, GFP_KERNEL);
> + if (!mmap_region_cmd)
> + return -ENOMEM;
> +
> + mmap_regions = (struct avs_cmd_shared_mem_map_regions *)mmap_region_cmd;
> + q6asm_add_mmaphdr(ac, &mmap_regions->hdr, cmd_size, true,
> + ((ac->session << 8) | dir));
> + a->mem_state = -1;
> +
> + mmap_regions->hdr.opcode = ASM_CMD_SHARED_MEM_MAP_REGIONS;
> + mmap_regions->mem_pool_id = ADSP_MEMORY_MAP_SHMEM8_4K_POOL;
> + mmap_regions->num_regions = num_regions;
> + mmap_regions->property_flag = 0x00;
> +
> + payload = ((u8 *) mmap_region_cmd +
> + sizeof(struct avs_cmd_shared_mem_map_regions));
mmap_region_cmd is void *, so no need to type cast.
> +
> + mregions = (struct avs_shared_map_region_payload *)payload;
Payload is void *, so no need to type cast. But there's also no benefit
of having "payload", as this line can be written as:
mregions = mmap_region_cmd + sizeof(*mmap_regions);
But adding a flexible array member to the avs_cmd_shared_mem_map_regions
struct would make things even clearer, in particular you would be able
to read the struct definition and see that there's a payload following
the command.
> +
> + ac->port[dir].mem_map_handle = 0;
Isn't this already 0?
> + port = &ac->port[dir];
> +
> + for (i = 0; i < num_regions; i++) {
> + ab = &port->buf[i];
> + mregions->shm_addr_lsw = lower_32_bits(ab->phys);
> + mregions->shm_addr_msw = upper_32_bits(ab->phys);
> + mregions->mem_size_bytes = buf_sz;
Here you're dereferencing port->buf without holding cmd_lock.
> + ++mregions;
> + }
> +
> + rc = apr_send_pkt(a->adev, (uint32_t *) mmap_region_cmd);
> + if (rc < 0)
> + goto fail_cmd;
> +
> + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0),
> + 5 * HZ);
> + if (!rc) {
> + dev_err(ac->dev, "timeout. waited for memory_map\n");
> + rc = -ETIMEDOUT;
> + goto fail_cmd;
> + }
> +
> + if (a->mem_state > 0) {
> + rc = adsp_err_get_lnx_err_code(a->mem_state);
> + goto fail_cmd;
> + }
> + rc = 0;
> +fail_cmd:
> + kfree(mmap_region_cmd);
> + return rc;
> +}
> +
> +/**
> + * q6asm_map_memory_regions() - map memory regions in the dsp.
> + *
> + * @dir: direction of audio stream
This sounds boolean, perhaps worth mentioning here if true means rx or
tx.
And it's idiomatic to have such a parameter later in the list, it would
probably be more natural to read the call sight if the order was:
q6asm_map_memory_regions(ac, phys, periods, size, true);
> + * @ac: audio client instanace
> + * @phys: physcial address that needs mapping.
> + * @period_sz: audio period size
> + * @periods: number of periods
> + *
> + * Return: Will be an negative value on failure or zero on success
> + */
> +int q6asm_map_memory_regions(unsigned int dir, struct audio_client *ac,
> + dma_addr_t phys,
> + unsigned int period_sz, unsigned int periods)
period_sz could with benefit be of type size_t.
> +{
> + struct audio_buffer *buf;
> + int cnt;
> + int rc;
> +
> + if (ac->port[dir].buf) {
> + dev_err(ac->dev, "Buffer already allocated\n");
> + return 0;
> + }
> +
> + mutex_lock(&ac->cmd_lock);
I believe this lock should cover above check.
> +
> + buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL);
Loose a few of those parenthesis and use *buf in the sizeof.
> + if (!buf) {
> + mutex_unlock(&ac->cmd_lock);
> + return -ENOMEM;
> + }
> +
> +
> + ac->port[dir].buf = buf;
> +
> + buf[0].phys = phys;
> + buf[0].used = dir ^ 1;
Why would this be called "used", and it's probably cleaner to just use
!!dir.
> + buf[0].size = period_sz;
> + cnt = 1;
> + while (cnt < periods) {
cnt goes from 1 to periods and is incremented 1 each step, this would be
more succinct as a for loop.
> + if (period_sz > 0) {
> + buf[cnt].phys = buf[0].phys + (cnt * period_sz);
> + buf[cnt].used = dir ^ 1;
> + buf[cnt].size = period_sz;
> + }
> + cnt++;
> + }
> +
> + ac->port[dir].max_buf_cnt = periods;
> + mutex_unlock(&ac->cmd_lock);
The only possible purpose of taking cmd_lock here is to protect
ac->port[dir].buf, but
> +
> + rc = __q6asm_memory_map_regions(ac, dir, period_sz, periods, 1);
The last parameter should be "true".
> + if (rc < 0) {
> + dev_err(ac->dev,
> + "CMD Memory_map_regions failed %d for size %d\n", rc,
> + period_sz);
> +
> +
> + ac->port[dir].max_buf_cnt = 0;
> + kfree(buf);
> + ac->port[dir].buf = NULL;
These operations are done without holding cmd_lock.
> +
> + return rc;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(q6asm_map_memory_regions);
> +
> /**
> * q6asm_audio_client_free() - Freee allocated audio client
> *
> @@ -117,8 +410,10 @@ static struct audio_client *q6asm_get_audio_client(struct q6asm *a,
>
> static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
> {
> - struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
> + struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev);
> struct audio_client *ac = NULL;
> + struct audio_port_data *port;
> + uint32_t dir = 0;
> uint32_t sid = 0;
> uint32_t *payload;
>
> @@ -135,6 +430,52 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *
> return 0;
> }
>
> + a = dev_get_drvdata(ac->dev->parent);
> + if (data->opcode == APR_BASIC_RSP_RESULT) {
This is a case in below switch statement.
> + switch (payload[0]) {
> + case ASM_CMD_SHARED_MEM_MAP_REGIONS:
> + case ASM_CMD_SHARED_MEM_UNMAP_REGIONS:
> + if (payload[1] != 0) {
> + dev_err(ac->dev,
> + "cmd = 0x%x returned error = 0x%x sid:%d\n",
> + payload[0], payload[1], sid);
> + a->mem_state = payload[1];
> + } else {
> + a->mem_state = 0;
Just assign a->mem_state = payload[1] outside the conditional, as it
will be the same value.
> + }
> +
> + wake_up(&a->mem_wait);
> + break;
> + default:
> + dev_err(&adev->dev, "command[0x%x] not expecting rsp\n",
> + payload[0]);
> + break;
> + }
> + return 0;
> + }
Regards,
Bjorn
^ permalink raw reply
* [PATCH v7 2/5] clk: aspeed: Register core clocks
From: Benjamin Herrenschmidt @ 2018-01-02 5:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-3-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> This registers the core clocks; those which are required to calculate
> the rate of the timer peripheral so the system can load a clocksource
> driver.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v5:
> - Add Andrew's Reviewed-by
> v4:
> - Add defines to document the BIT() macros
> v3:
> - Fix ast2400 ahb calculation
> - Remove incorrect 'this is wrong' comment
> - Separate out clkin calc to be per platform
> - Support 48MHz clkin on ast2400
> ---
> drivers/clk/clk-aspeed.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 177 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 7a86ee08ea4f..5adedda82d26 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -13,7 +13,23 @@
>
> #define ASPEED_NUM_CLKS 35
>
> +#define ASPEED_RESET_CTRL 0x04
> +#define ASPEED_CLK_SELECTION 0x08
> +#define ASPEED_CLK_STOP_CTRL 0x0c
> +#define ASPEED_MPLL_PARAM 0x20
> +#define ASPEED_HPLL_PARAM 0x24
> +#define AST2500_HPLL_BYPASS_EN BIT(20)
> +#define AST2400_HPLL_STRAPPED BIT(18)
> +#define AST2400_HPLL_BYPASS_EN BIT(17)
> +#define ASPEED_MISC_CTRL 0x2c
> +#define UART_DIV13_EN BIT(12)
> #define ASPEED_STRAP 0x70
> +#define CLKIN_25MHZ_EN BIT(23)
> +#define AST2400_CLK_SOURCE_SEL BIT(18)
> +#define ASPEED_CLK_SELECTION_2 0xd8
> +
> +/* Globally visible clocks */
> +static DEFINE_SPINLOCK(aspeed_clk_lock);
>
> /* Keeps track of all clocks */
> static struct clk_hw_onecell_data *aspeed_clk_data;
> @@ -91,6 +107,160 @@ static const struct aspeed_gate_data aspeed_gates[] = {
> [ASPEED_CLK_GATE_LHCCLK] = { 28, -1, "lhclk-gate", "lhclk", 0 }, /* LPC master/LPC+ */
> };
>
> +static const struct clk_div_table ast2400_div_table[] = {
> + { 0x0, 2 },
> + { 0x1, 4 },
> + { 0x2, 6 },
> + { 0x3, 8 },
> + { 0x4, 10 },
> + { 0x5, 12 },
> + { 0x6, 14 },
> + { 0x7, 16 },
> + { 0 }
> +};
> +
> +static const struct clk_div_table ast2500_div_table[] = {
> + { 0x0, 4 },
> + { 0x1, 8 },
> + { 0x2, 12 },
> + { 0x3, 16 },
> + { 0x4, 20 },
> + { 0x5, 24 },
> + { 0x6, 28 },
> + { 0x7, 32 },
> + { 0 }
> +};
> +
> +static struct clk_hw *aspeed_ast2400_calc_pll(const char *name, u32 val)
> +{
> + unsigned int mult, div;
> +
> + if (val & AST2400_HPLL_BYPASS_EN) {
> + /* Pass through mode */
> + mult = div = 1;
> + } else {
> + /* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
> + u32 n = (val >> 5) & 0x3f;
> + u32 od = (val >> 4) & 0x1;
> + u32 d = val & 0xf;
> +
> + mult = (2 - od) * (n + 2);
> + div = d + 1;
> + }
> + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> + mult, div);
> +};
> +
> +static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
> +{
> + unsigned int mult, div;
> +
> + if (val & AST2500_HPLL_BYPASS_EN) {
> + /* Pass through mode */
> + mult = div = 1;
> + } else {
> + /* F = clkin * [(M+1) / (N+1)] / (P + 1) */
> + u32 p = (val >> 13) & 0x3f;
> + u32 m = (val >> 5) & 0xff;
> + u32 n = val & 0x1f;
> +
> + mult = (m + 1) / (n + 1);
> + div = p + 1;
> + }
> +
> + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> + mult, div);
> +}
> +
> +static void __init aspeed_ast2400_cc(struct regmap *map)
> +{
> + struct clk_hw *hw;
> + u32 val, freq, div;
> +
> + /*
> + * CLKIN is the crystal oscillator, 24, 48 or 25MHz selected by
> + * strapping
> + */
> + regmap_read(map, ASPEED_STRAP, &val);
> + if (val & CLKIN_25MHZ_EN)
> + freq = 25000000;
> + else if (val & AST2400_CLK_SOURCE_SEL)
> + freq = 48000000;
> + else
> + freq = 24000000;
> + hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
> + pr_debug("clkin @%u MHz\n", freq / 1000000);
> +
> + /*
> + * High-speed PLL clock derived from the crystal. This the CPU clock,
> + * and we assume that it is enabled
> + */
> + regmap_read(map, ASPEED_HPLL_PARAM, &val);
> + WARN(val & AST2400_HPLL_STRAPPED, "hpll is strapped not configured");
> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2400_calc_pll("hpll", val);
> +
> + /*
> + * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
> + * 00: Select CPU:AHB = 1:1
> + * 01: Select CPU:AHB = 2:1
> + * 10: Select CPU:AHB = 4:1
> + * 11: Select CPU:AHB = 3:1
> + */
> + regmap_read(map, ASPEED_STRAP, &val);
> + val = (val >> 10) & 0x3;
> + div = val + 1;
> + if (div == 3)
> + div = 4;
> + else if (div == 4)
> + div = 3;
> + hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> + aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> + /* APB clock clock selection register SCU08 (aka PCLK) */
> + hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
> + ast2400_div_table,
> + &aspeed_clk_lock);
> + aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +}
> +
> +static void __init aspeed_ast2500_cc(struct regmap *map)
> +{
> + struct clk_hw *hw;
> + u32 val, freq, div;
> +
> + /* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
> + regmap_read(map, ASPEED_STRAP, &val);
> + if (val & CLKIN_25MHZ_EN)
> + freq = 25000000;
> + else
> + freq = 24000000;
> + hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
> + pr_debug("clkin @%u MHz\n", freq / 1000000);
> +
> + /*
> + * High-speed PLL clock derived from the crystal. This the CPU clock,
> + * and we assume that it is enabled
> + */
> + regmap_read(map, ASPEED_HPLL_PARAM, &val);
> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2500_calc_pll("hpll", val);
> +
> + /* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
> + regmap_read(map, ASPEED_STRAP, &val);
> + val = (val >> 9) & 0x7;
> + WARN(val == 0, "strapping is zero: cannot determine ahb clock");
> + div = 2 * (val + 1);
> + hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> + aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> + /* APB clock clock selection register SCU08 (aka PCLK) */
> + regmap_read(map, ASPEED_CLK_SELECTION, &val);
> + val = (val >> 23) & 0x7;
> + div = 4 * (val + 1);
> + hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
> + aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +};
> +
> static void __init aspeed_cc_init(struct device_node *np)
> {
> struct regmap *map;
> @@ -132,6 +302,13 @@ static void __init aspeed_cc_init(struct device_node *np)
> return;
> }
>
> + if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
> + aspeed_ast2400_cc(map);
> + else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
> + aspeed_ast2500_cc(map);
> + else
> + pr_err("unknown platform, failed to add clocks\n");
> +
> aspeed_clk_data->num = ASPEED_NUM_CLKS;
> ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> if (ret)
^ permalink raw reply
* [PATCH v7 3/5] clk: aspeed: Add platform driver and register PLLs
From: Benjamin Herrenschmidt @ 2018-01-02 5:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-4-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> This registers a platform driver to set up all of the non-core clocks.
>
> The clocks that have configurable rates are now registered.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> --
> v6:
> - Add Andrew's reviewed-by
> v5:
> - Remove eclk configuration. We do not have enough information to
> correctly implement the mux and divisor, so it will have to be
> implemented in the future
> v4:
> - Add eclk div table to fix ast2500 calculation
> - Add defines to document the BIT() macros
> - Pass dev where we can when registering clocks
> - Check for errors when registering clk_hws
> v3:
> - Fix bclk and eclk calculation
> - Separate out ast2400 and ast25000 for pll calculation
> ---
> drivers/clk/clk-aspeed.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 5adedda82d26..cf5ea63feb31 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -5,6 +5,8 @@
> #include <linux/clk-provider.h>
> #include <linux/mfd/syscon.h>
> #include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> @@ -107,6 +109,18 @@ static const struct aspeed_gate_data aspeed_gates[] = {
> [ASPEED_CLK_GATE_LHCCLK] = { 28, -1, "lhclk-gate", "lhclk", 0 }, /* LPC master/LPC+ */
> };
>
> +static const struct clk_div_table ast2500_mac_div_table[] = {
> + { 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
> + { 0x1, 4 },
> + { 0x2, 6 },
> + { 0x3, 8 },
> + { 0x4, 10 },
> + { 0x5, 12 },
> + { 0x6, 14 },
> + { 0x7, 16 },
> + { 0 }
> +};
> +
> static const struct clk_div_table ast2400_div_table[] = {
> { 0x0, 2 },
> { 0x1, 4 },
> @@ -172,6 +186,122 @@ static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
> mult, div);
> }
>
> +struct aspeed_clk_soc_data {
> + const struct clk_div_table *div_table;
> + const struct clk_div_table *mac_div_table;
> + struct clk_hw *(*calc_pll)(const char *name, u32 val);
> +};
> +
> +static const struct aspeed_clk_soc_data ast2500_data = {
> + .div_table = ast2500_div_table,
> + .mac_div_table = ast2500_mac_div_table,
> + .calc_pll = aspeed_ast2500_calc_pll,
> +};
> +
> +static const struct aspeed_clk_soc_data ast2400_data = {
> + .div_table = ast2400_div_table,
> + .mac_div_table = ast2400_div_table,
> + .calc_pll = aspeed_ast2400_calc_pll,
> +};
> +
> +static int aspeed_clk_probe(struct platform_device *pdev)
> +{
> + const struct aspeed_clk_soc_data *soc_data;
> + struct device *dev = &pdev->dev;
> + struct regmap *map;
> + struct clk_hw *hw;
> + u32 val, rate;
> +
> + map = syscon_node_to_regmap(dev->of_node);
> + if (IS_ERR(map)) {
> + dev_err(dev, "no syscon regmap\n");
> + return PTR_ERR(map);
> + }
> +
> + /* SoC generations share common layouts but have different divisors */
> + soc_data = of_device_get_match_data(dev);
> + if (!soc_data) {
> + dev_err(dev, "no match data for platform\n");
> + return -EINVAL;
> + }
> +
> + /* UART clock div13 setting */
> + regmap_read(map, ASPEED_MISC_CTRL, &val);
> + if (val & UART_DIV13_EN)
> + rate = 24000000 / 13;
> + else
> + rate = 24000000;
> + /* TODO: Find the parent data for the uart clock */
> + hw = clk_hw_register_fixed_rate(dev, "uart", NULL, 0, rate);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
> +
> + /*
> + * Memory controller (M-PLL) PLL. This clock is configured by the
> + * bootloader, and is exposed to Linux as a read-only clock rate.
> + */
> + regmap_read(map, ASPEED_MPLL_PARAM, &val);
> + hw = soc_data->calc_pll("mpll", val);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_MPLL] = hw;
> +
> + /* SD/SDIO clock divider (TODO: There's a gate too) */
> + hw = clk_hw_register_divider_table(dev, "sdio", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> + soc_data->div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> +
> + /* MAC AHB bus clock divider */
> + hw = clk_hw_register_divider_table(dev, "mac", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
> + soc_data->mac_div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
> +
> + /* LPC Host (LHCLK) clock divider */
> + hw = clk_hw_register_divider_table(dev, "lhclk", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> + soc_data->div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
> +
> + /* P-Bus (BCLK) clock divider */
> + hw = clk_hw_register_divider_table(dev, "bclk", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION_2, 0, 2, 0,
> + soc_data->div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
> +
> + return 0;
> +};
> +
> +static const struct of_device_id aspeed_clk_dt_ids[] = {
> + { .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
> + { .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
> + { }
> +};
> +
> +static struct platform_driver aspeed_clk_driver = {
> + .probe = aspeed_clk_probe,
> + .driver = {
> + .name = "aspeed-clk",
> + .of_match_table = aspeed_clk_dt_ids,
> + .suppress_bind_attrs = true,
> + },
> +};
> +builtin_platform_driver(aspeed_clk_driver);
> +
> static void __init aspeed_ast2400_cc(struct regmap *map)
> {
> struct clk_hw *hw;
^ permalink raw reply
* [PATCH v7 4/5] clk: aspeed: Register gated clocks
From: Benjamin Herrenschmidt @ 2018-01-02 5:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-5-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> The majority of the clocks in the system are gates paired with a reset
> controller that holds the IP in reset.
>
> This borrows from clk_hw_register_gate, but registers two 'gates', one
> to control the clock enable register and the other to control the reset
> IP. This allows us to enforce the ordering:
>
> 1. Place IP in reset
> 2. Enable clock
> 3. Delay
> 4. Release reset
>
> There are some gates that do not have an associated reset; these are
> handled by using -1 as the index for the reset.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v7:
> - Use mdelay instead of msleep and remove the todo. Stephen points out:
> > No you can't sleep here. It needs to delay because this is inside
> > spinlock_irqsave.
> v5:
> - Add Andrew's Reviewed-by
> v4:
> - Drop useless 'disable clock' comment
> - Drop CLK_IS_BASIC flag
> - Fix 'there are a number of clocks...' comment
> - Pass device to clk registration functions
> - Check for errors when registering clk_hws
> v3:
> - Remove gates offset as gates are now at the start of the list
>
> mdelay
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/clk/clk-aspeed.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index cf5ea63feb31..dbd3c7774831 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -204,6 +204,106 @@ static const struct aspeed_clk_soc_data ast2400_data = {
> .calc_pll = aspeed_ast2400_calc_pll,
> };
>
> +static int aspeed_clk_enable(struct clk_hw *hw)
> +{
> + struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> + unsigned long flags;
> + u32 clk = BIT(gate->clock_idx);
> + u32 rst = BIT(gate->reset_idx);
> +
> + spin_lock_irqsave(gate->lock, flags);
> +
> + if (gate->reset_idx >= 0) {
> + /* Put IP in reset */
> + regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
> +
> + /* Delay 100us */
> + udelay(100);
> + }
> +
> + /* Enable clock */
> + regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, 0);
> +
> + if (gate->reset_idx >= 0) {
> + /* A delay of 10ms is specified by the ASPEED docs */
> + mdelay(10);
> +
> + /* Take IP out of reset */
> + regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
> + }
> +
> + spin_unlock_irqrestore(gate->lock, flags);
> +
> + return 0;
> +}
> +
> +static void aspeed_clk_disable(struct clk_hw *hw)
> +{
> + struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> + unsigned long flags;
> + u32 clk = BIT(gate->clock_idx);
> +
> + spin_lock_irqsave(gate->lock, flags);
> +
> + regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, clk);
> +
> + spin_unlock_irqrestore(gate->lock, flags);
> +}
> +
> +static int aspeed_clk_is_enabled(struct clk_hw *hw)
> +{
> + struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> + u32 clk = BIT(gate->clock_idx);
> + u32 reg;
> +
> + regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, ®);
> +
> + return (reg & clk) ? 0 : 1;
> +}
> +
> +static const struct clk_ops aspeed_clk_gate_ops = {
> + .enable = aspeed_clk_enable,
> + .disable = aspeed_clk_disable,
> + .is_enabled = aspeed_clk_is_enabled,
> +};
> +
> +static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> + const char *name, const char *parent_name, unsigned long flags,
> + struct regmap *map, u8 clock_idx, u8 reset_idx,
> + u8 clk_gate_flags, spinlock_t *lock)
> +{
> + struct aspeed_clk_gate *gate;
> + struct clk_init_data init;
> + struct clk_hw *hw;
> + int ret;
> +
> + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> + if (!gate)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + init.ops = &aspeed_clk_gate_ops;
> + init.flags = flags;
> + init.parent_names = parent_name ? &parent_name : NULL;
> + init.num_parents = parent_name ? 1 : 0;
> +
> + gate->map = map;
> + gate->clock_idx = clock_idx;
> + gate->reset_idx = reset_idx;
> + gate->flags = clk_gate_flags;
> + gate->lock = lock;
> + gate->hw.init = &init;
> +
> + hw = &gate->hw;
> + ret = clk_hw_register(dev, hw);
> + if (ret) {
> + kfree(gate);
> + hw = ERR_PTR(ret);
> + }
> +
> + return hw;
> +}
> +
> static int aspeed_clk_probe(struct platform_device *pdev)
> {
> const struct aspeed_clk_soc_data *soc_data;
> @@ -211,6 +311,7 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> struct regmap *map;
> struct clk_hw *hw;
> u32 val, rate;
> + int i;
>
> map = syscon_node_to_regmap(dev->of_node);
> if (IS_ERR(map)) {
> @@ -283,6 +384,35 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> return PTR_ERR(hw);
> aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
>
> + /*
> + * TODO: There are a number of clocks that not included in this driver
> + * as more information is required:
> + * D2-PLL
> + * D-PLL
> + * YCLK
> + * RGMII
> + * RMII
> + * UART[1..5] clock source mux
> + * Video Engine (ECLK) mux and clock divider
> + */
> +
> + for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
> + const struct aspeed_gate_data *gd = &aspeed_gates[i];
> +
> + hw = aspeed_clk_hw_register_gate(dev,
> + gd->name,
> + gd->parent_name,
> + gd->flags,
> + map,
> + gd->clock_idx,
> + gd->reset_idx,
> + CLK_GATE_SET_TO_DISABLE,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[i] = hw;
> + }
> +
> return 0;
> };
>
^ permalink raw reply
* [PATCH v7 5/5] clk: aspeed: Add reset controller
From: Benjamin Herrenschmidt @ 2018-01-02 5:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-6-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> There are some resets that are not associated with gates. These are
> represented by a reset controller.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v7:
> - Rebase on dt-bindings patch
> v5:
> - Add Andrew's Reviewed-by
> v3:
> - Add named initalisers for the reset defines
> - Add define for ADC
> ---
> drivers/clk/clk-aspeed.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index dbd3c7774831..6fb344730cea 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -8,6 +8,7 @@
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
>
> @@ -267,6 +268,68 @@ static const struct clk_ops aspeed_clk_gate_ops = {
> .is_enabled = aspeed_clk_is_enabled,
> };
>
> +/**
> + * struct aspeed_reset - Aspeed reset controller
> + * @map: regmap to access the containing system controller
> + * @rcdev: reset controller device
> + */
> +struct aspeed_reset {
> + struct regmap *map;
> + struct reset_controller_dev rcdev;
> +};
> +
> +#define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
> +
> +static const u8 aspeed_resets[] = {
> + [ASPEED_RESET_XDMA] = 25,
> + [ASPEED_RESET_MCTP] = 24,
> + [ASPEED_RESET_ADC] = 23,
> + [ASPEED_RESET_JTAG_MASTER] = 22,
> + [ASPEED_RESET_MIC] = 18,
> + [ASPEED_RESET_PWM] = 9,
> + [ASPEED_RESET_PCIVGA] = 8,
> + [ASPEED_RESET_I2C] = 2,
> + [ASPEED_RESET_AHB] = 1,
> +};
> +
> +static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> + u32 rst = BIT(aspeed_resets[id]);
> +
> + return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
> +}
> +
> +static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> + u32 rst = BIT(aspeed_resets[id]);
> +
> + return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
> +}
> +
> +static int aspeed_reset_status(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> + u32 val, rst = BIT(aspeed_resets[id]);
> + int ret;
> +
> + ret = regmap_read(ar->map, ASPEED_RESET_CTRL, &val);
> + if (ret)
> + return ret;
> +
> + return !!(val & rst);
> +}
> +
> +static const struct reset_control_ops aspeed_reset_ops = {
> + .assert = aspeed_reset_assert,
> + .deassert = aspeed_reset_deassert,
> + .status = aspeed_reset_status,
> +};
> +
> static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> const char *name, const char *parent_name, unsigned long flags,
> struct regmap *map, u8 clock_idx, u8 reset_idx,
> @@ -308,10 +371,11 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> {
> const struct aspeed_clk_soc_data *soc_data;
> struct device *dev = &pdev->dev;
> + struct aspeed_reset *ar;
> struct regmap *map;
> struct clk_hw *hw;
> u32 val, rate;
> - int i;
> + int i, ret;
>
> map = syscon_node_to_regmap(dev->of_node);
> if (IS_ERR(map)) {
> @@ -319,6 +383,22 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> return PTR_ERR(map);
> }
>
> + ar = devm_kzalloc(dev, sizeof(*ar), GFP_KERNEL);
> + if (!ar)
> + return -ENOMEM;
> +
> + ar->map = map;
> + ar->rcdev.owner = THIS_MODULE;
> + ar->rcdev.nr_resets = ARRAY_SIZE(aspeed_resets);
> + ar->rcdev.ops = &aspeed_reset_ops;
> + ar->rcdev.of_node = dev->of_node;
> +
> + ret = devm_reset_controller_register(dev, &ar->rcdev);
> + if (ret) {
> + dev_err(dev, "could not register reset controller\n");
> + return ret;
> + }
> +
> /* SoC generations share common layouts but have different divisors */
> soc_data = of_device_get_match_data(dev);
> if (!soc_data) {
^ permalink raw reply
* [PATCH] Input: misc: gpio_tilt: Delete driver
From: Dmitry Torokhov @ 2018-01-02 6:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <14171345.p7a3jPx4Si@phil>
On Wed, Dec 27, 2017 at 02:51:45PM +0100, Heiko Stuebner wrote:
> Hi Linus,
>
> Am Mittwoch, 27. Dezember 2017, 13:15:47 CET schrieb Linus Walleij:
> > This driver was merged in 2011 as a tool for detecting the orientation
> > of a screen. The device driver assumes board file setup using the
> > platform data from <linux/input/gpio_tilt.h>. But no boards in the
> > kernel tree defines this platform data.
> >
> > As I am faced with refactoring drivers to use GPIO descriptors and
> > pass decriptor tables from boards, or use the device tree device
> > drivers like these creates a serious problem: I cannot fix them and
> > cannot test them, not even compile-test them with a system actually
> > using it (no in-tree boardfile).
> >
> > I suggest to delete this driver and rewrite it using device tree if
> > it is still in use on actively maintained systems.
> >
> > I can also offer to rewrite it out of the blue using device tree if
> > someone promise to test it and help me iterate it.
> >
> > Cc: Heiko St?bner <heiko@sntech.de>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > Heiko: not meaning to be militant here, just contain the situation,
> > as stated: if you like the driver and can test it, I can reimplement
> > it from scratch using device tree.
>
> It seems that piece of hardware (gpio-connected orientation-sensors)
> was really only used in the one s3c24xx-based device I hacked on in 2011.
>
> I somehow lost focus from trying to do the s3c24xx devicetree migration
> when I started hacking on Rockchip stuff, so while I do have the devices
> still around, I don't think I'll find the time and energy trying to get a
> recent kernel to run on them anyway, so I'm fine with dropping the driver.
> It's simple enough to get reintroduced if someone really finds a device
> using it or time to redo the ereader support using devicetree.
>
> So long story short
> Acked-by: Heiko Stuebner <heiko@sntech.de>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH v2 0/4] arm64: defconfig: enable additional led triggers
From: Amit Kucheria @ 2018-01-02 7:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK8P3a0+4ACDucmNQ3OL_0UjJnK6d3JXOb6_z5jtMQ986uZkLQ@mail.gmail.com>
On Thu, Dec 21, 2017 at 8:46 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Dec 6, 2017 at 9:57 AM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>> (Adding Arnd)
>>
>> Now that the merge window rush has abated, can you please apply this
>> trivial series?
>>
>> On Mon, Nov 6, 2017 at 12:38 PM, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>>> This patchset enables the kernel panic and disk-activity trigger for LEDs
>>> and then enables the panic trigger for three 96Boards - DB410c, Hikey and
>>> Hikey960.
>>>
>>> DB410c and Hikey panic behaviour has been tested by triggering a panic
>>> through /proc/sysrq-trigger, while Hikey960 is only compile-tested.
>
> I applied all four now, but it would have been easier for me if you had either
> sent them to the platform maintainers, or to arm at kernel.org.
The platform maintainers are cc'ed but I guess nobody took them since
the patchset touched 3 different platforms and a common defconfig.
I'll remember to cc arm at kernel.org in the future but is there any
reason why this email address isn't listed in MAINTAINERS?
Thanks for applying the patches.
Wish you a Happy New Year.
Regards,
Amit
^ permalink raw reply
* [PATCH v7 02/11] thermal: armada: Use msleep for long delays
From: Miquel RAYNAL @ 2018-01-02 7:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101205537.GA29474@localhost.localdomain>
Hello Eduardo,
On Mon, 1 Jan 2018 12:55:39 -0800
Eduardo Valentin <edubezval@gmail.com> wrote:
> On Fri, Dec 22, 2017 at 05:14:04PM +0100, Miquel Raynal wrote:
> > From: Baruch Siach <baruch@tkos.co.il>
> >
> > Use msleep for long (> 10ms) delays, instead of the busy waiting
> > mdelay. All delays are called from the probe routine, where
> > scheduling is allowed.
> >
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> I am queueing this patch, however, ...
>
> > ---
> > drivers/thermal/armada_thermal.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 706d74798cbe..6c4af2622d4f
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -113,7 +113,7 @@ static void armada370_init_sensor(struct
> > platform_device *pdev, reg &= ~PMU_TDC0_START_CAL_MASK;
> > writel(reg, priv->control);
> >
> > - mdelay(10);
> > + msleep(10);
>
> I want to double check with you that msleep(10) is documented to reach
> up to 20ms, see:
>
>
> WARNING: msleep < 20ms can sleep for up to 20ms; see
> Documentation/timers/timers-howto.txt #43: FILE:
> drivers/thermal/armada_thermal.c:116:
> + msleep(10);
>
> WARNING: msleep < 20ms can sleep for up to 20ms; see
> Documentation/timers/timers-howto.txt #66: FILE:
> drivers/thermal/armada_thermal.c:146:
> + msleep(10);
>
> Just want to check that you are aware of this and that it won't cause
> troubles in the code flows changed in this patch. Driver is still in
> one piece, correct?
Thanks for queueing the series.
All of these delays are here to ensure that we do wait a minimum
amount of time to let the hardware be ready, IMHO waiting up to 20ms is
not an issue.
Best regards,
Miqu?l
>
> > }
> >
> > static void armada375_init_sensor(struct platform_device *pdev,
> > @@ -127,11 +127,11 @@ static void armada375_init_sensor(struct
> > platform_device *pdev, reg &= ~A375_HW_RESETn;
> >
> > writel(reg, priv->control + 4);
> > - mdelay(20);
> > + msleep(20);
> >
> > reg |= A375_HW_RESETn;
> > writel(reg, priv->control + 4);
> > - mdelay(50);
> > + msleep(50);
> > }
> >
> > static void armada380_init_sensor(struct platform_device *pdev,
> > @@ -143,7 +143,7 @@ static void armada380_init_sensor(struct
> > platform_device *pdev, if (!(reg & A380_HW_RESET)) {
> > reg |= A380_HW_RESET;
> > writel(reg, priv->control);
> > - mdelay(10);
> > + msleep(10);
> > }
> > }
> >
> > --
> > 2.11.0
> >
--
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [RFC PATCH v2] pciehp: fix a race between pciehp and removing operations by sysfs
From: Xiongfeng Wang @ 2018-01-02 7:52 UTC (permalink / raw)
To: linux-arm-kernel
From: Xiongfeng Wang <xiongfeng.wang@linaro.com>
When I run a stress test about pcie hotplug and removing operations by
sysfs, I got a hange task, and the following call trace is printed.
INFO: task kworker/0:2:4413 blocked for more than 120 seconds.
Tainted: P W O 4.12.0-rc1 #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/0:2 D 0 4413 2 0x00000000
Workqueue: pciehp-0 pciehp_power_thread
Call trace:
[<ffff0000080861d4>] __switch_to+0x94/0xa8
[<ffff000008bea9c0>] __schedule+0x1b0/0x708
[<ffff000008beaf58>] schedule+0x40/0xa4
[<ffff000008beb33c>] schedule_preempt_disabled+0x28/0x40
[<ffff000008bec1dc>] __mutex_lock.isra.8+0x148/0x50c
[<ffff000008bec5c4>] __mutex_lock_slowpath+0x24/0x30
[<ffff000008bec618>] mutex_lock+0x48/0x54
[<ffff0000084d8188>] pci_lock_rescan_remove+0x20/0x28
[<ffff0000084f87c0>] pciehp_unconfigure_device+0x54/0x1cc
[<ffff0000084f8260>] pciehp_disable_slot+0x4c/0xbc
[<ffff0000084f8370>] pciehp_power_thread+0xa0/0xb8
[<ffff0000080e9ce8>] process_one_work+0x13c/0x3f8
[<ffff0000080ea004>] worker_thread+0x60/0x3e4
[<ffff0000080f0814>] kthread+0x10c/0x138
[<ffff0000080836c0>] ret_from_fork+0x10/0x50
INFO: task bash:31732 blocked for more than 120 seconds.
Tainted: P W O 4.12.0-rc1 #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
bash D 0 31732 1 0x00000009
Call trace:
[<ffff0000080861d4>] __switch_to+0x94/0xa8
[<ffff000008bea9c0>] __schedule+0x1b0/0x708
[<ffff000008beaf58>] schedule+0x40/0xa4
[<ffff000008bee7b4>] schedule_timeout+0x1a0/0x340
[<ffff000008bebb88>] wait_for_common+0x108/0x1bc
[<ffff000008bebc64>] wait_for_completion+0x28/0x34
[<ffff0000080e7594>] flush_workqueue+0x130/0x488
[<ffff0000080e79b0>] drain_workqueue+0xc4/0x164
[<ffff0000080ec3cc>] destroy_workqueue+0x28/0x1f4
[<ffff0000084fa094>] pciehp_release_ctrl+0x34/0xe0
[<ffff0000084f75b0>] pciehp_remove+0x30/0x3c
[<ffff0000084f24d8>] pcie_port_remove_service+0x3c/0x54
[<ffff00000876b1e4>] device_release_driver_internal+0x150/0x1d0
[<ffff00000876b28c>] device_release_driver+0x28/0x34
[<ffff00000876a018>] bus_remove_device+0xe0/0x11c
[<ffff000008766348>] device_del+0x200/0x304
[<ffff00000876646c>] device_unregister+0x20/0x38
[<ffff0000084f2560>] remove_iter+0x44/0x54
[<ffff000008765230>] device_for_each_child+0x4c/0x90
[<ffff0000084f2c98>] pcie_port_device_remove+0x2c/0x48
[<ffff0000084f2f48>] pcie_portdrv_remove+0x60/0x6c
[<ffff0000084e3de4>] pci_device_remove+0x48/0x110
[<ffff00000876b1e4>] device_release_driver_internal+0x150/0x1d0
[<ffff00000876b28c>] device_release_driver+0x28/0x34
[<ffff0000084db028>] pci_stop_bus_device+0x9c/0xac
[<ffff0000084db190>] pci_stop_and_remove_bus_device_locked+0x24/0x3c
[<ffff0000084e5eb0>] remove_store+0x74/0x80
[<ffff000008764680>] dev_attr_store+0x44/0x5c
[<ffff0000082e7e1c>] sysfs_kf_write+0x5c/0x74
[<ffff0000082e7014>] kernfs_fop_write+0xcc/0x1dc
[<ffff0000082602e0>] __vfs_write+0x48/0x13c
[<ffff00000826174c>] vfs_write+0xa8/0x198
[<ffff000008262ce8>] SyS_write+0x54/0xb0
[<ffff000008083730>] el0_svc_naked+0x24/0x28
There is a race condition between these two kinds of operations.
When the Attention button on a PCIE slot is pressed, 5 seconds later,
pciehp_power_thread() will be scheduled on slot->wq. This function will
call pciehp_unconfigure_device(), which will try to get a global mutex
lock 'pci_rescan_remove_lock'.
At the same time, we remove the pcie port by sysfs, which results in
pci_stop_and_remove_bus_device_locked() called. This function will get
the global mutex lock 'pci_rescan_remove_lock', and then release the
struct 'ctrl', which will wait until the work_struct on slot->wq is
finished.
If pci_stop_and_remove_bus_device_locked() got the mutex lock, and
before it drains workqueue slot->wq, pciehp_power_thread() is scheduled
on slot->wq and tries to get the mutex lock but failed, so it will just
wait. Then pci_stop_and_remove_bus_device_locked() tries to drain workqueue
slot->wq and wait until work struct 'pciehp_power_thread()' is finished.
Then a hung_task occurs.
So this two kinds of operation, removing through attention buttion and
removing through /sys/devices/pci***/remove, should not be excuted at the
same time. This patch add a global variable to mark that one of these
operations is under processing. When this variable is set, if another
operation is requested, it will be rejected.
At first, I want to add a flag for each pci slot to record whether a
removing operation is under processing. When a bridge is being removed,
the flags of all the slots below the bridge need to be checked. But it
is hard for us to guarantee the atomic access. So I just use a global
flag.
This workaround method uses a global flag, which is not good for the code
framework and can't fix the race condition fully. But I can't figure out
a better way. I think we may need to reconstruct the code framework a lot
to fix this issue nicely. There are so many work struct created. Before
that, maybe we can use this patch as a temporary fix.
Signed-off-by: Xiongfeng Wang <xiongfeng.wang@linaro.org>
---
drivers/pci/hotplug/pciehp_ctrl.c | 7 +++++++
drivers/pci/hotplug/pciehp_hpc.c | 12 +++++++++++-
drivers/pci/pci-sysfs.c | 11 +++++++++--
drivers/pci/remove.c | 6 ++++++
include/linux/pci.h | 3 +++
5 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 83f3d4a..5680439 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -44,6 +44,7 @@ void pciehp_queue_interrupt_event(struct slot *p_slot, u32 event_type)
info = kmalloc(sizeof(*info), GFP_ATOMIC);
if (!info) {
ctrl_err(p_slot->ctrl, "dropped event %d (ENOMEM)\n", event_type);
+ slot_being_removed_rescanned = 0;
return;
}
@@ -188,6 +189,7 @@ static void pciehp_power_thread(struct work_struct *work)
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
mutex_unlock(&p_slot->lock);
+ slot_being_removed_rescanned = 0;
break;
case ENABLE_REQ:
mutex_lock(&p_slot->hotplug_lock);
@@ -198,6 +200,7 @@ static void pciehp_power_thread(struct work_struct *work)
mutex_lock(&p_slot->lock);
p_slot->state = STATIC_STATE;
mutex_unlock(&p_slot->lock);
+ slot_being_removed_rescanned = 0;
break;
default:
break;
@@ -216,6 +219,7 @@ static void pciehp_queue_power_work(struct slot *p_slot, int req)
if (!info) {
ctrl_err(p_slot->ctrl, "no memory to queue %s request\n",
(req == ENABLE_REQ) ? "poweron" : "poweroff");
+ slot_being_removed_rescanned = 0;
return;
}
info->p_slot = p_slot;
@@ -284,6 +288,7 @@ static void handle_button_press_event(struct slot *p_slot)
ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
slot_name(p_slot));
p_slot->state = STATIC_STATE;
+ slot_being_removed_rescanned = 0;
break;
case POWEROFF_STATE:
case POWERON_STATE:
@@ -294,10 +299,12 @@ static void handle_button_press_event(struct slot *p_slot)
*/
ctrl_info(ctrl, "Slot(%s): Button ignored\n",
slot_name(p_slot));
+ slot_being_removed_rescanned = 0;
break;
default:
ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
slot_name(p_slot), p_slot->state);
+ slot_being_removed_rescanned = 0;
break;
}
}
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 7bab060..7d1b559 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -622,7 +622,17 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
if (events & PCI_EXP_SLTSTA_ABP) {
ctrl_info(ctrl, "Slot(%s): Attention button pressed\n",
slot_name(slot));
- pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
+
+ if (!test_and_set_bit(0, &slot_being_removed_rescanned))
+ pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
+ else {
+ if (slot->state == BLINKINGOFF_STATE || slot->state == BLINKINGON_STATE)
+ pciehp_queue_interrupt_event(slot, INT_BUTTON_PRESS);
+ else
+ ctrl_info(ctrl, "Slot(%s): Slot operation failed because a remove or"
+ " rescan operation is under processing, please try later!\n",
+ slot_name(slot));
+ }
}
/*
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 06c7f0b..206162f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -489,8 +489,15 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
if (kstrtoul(buf, 0, &val) < 0)
return -EINVAL;
- if (val && device_remove_file_self(dev, attr))
- pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
+ if (val && device_remove_file_self(dev, attr)) {
+ if (!test_and_set_bit(0, &slot_being_removed_rescanned)) {
+ pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
+ slot_being_removed_rescanned = 0;
+ } else {
+ pr_info("Slot is being removed or rescanned, please try later!\n");
+ return -EPERM;
+ }
+ }
return count;
}
static struct device_attribute dev_remove_attr = __ATTR(remove,
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 2fa0dbd..c7c3e37 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -3,6 +3,12 @@
#include <linux/pci-aspm.h>
#include "pci.h"
+/*
+ * When a slot is being hotplug through Attention Button or being
+ * removed/rescanned through sysfs, this flag is set.
+ */
+unsigned long slot_being_removed_rescanned;
+
static void pci_free_resources(struct pci_dev *dev)
{
int i;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c170c92..85a6e75 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -854,6 +854,9 @@ enum pcie_bus_config_types {
/* Do NOT directly access these two variables, unless you are arch-specific PCI
* code, or PCI core code. */
extern struct list_head pci_root_buses; /* list of all known PCI buses */
+
+extern unsigned long slot_being_removed_rescanned;
+
/* Some device drivers need know if PCI is initiated */
int no_pci_devices(void);
--
1.7.12.4
^ permalink raw reply related
* [PATCH] ARM: dts: Delete bogus reference to the charlcd
From: Linus Walleij @ 2018-01-02 7:57 UTC (permalink / raw)
To: linux-arm-kernel
The EB MP board probably has a character LCD but the board manual does
not really state which IRQ it has assigned to this device. The invalid
assignment was a mistake by me during submission of the DTSI where I was
looking for the reference, didn't find it and didn't fill it in.
Delete this for now: it can probably be fixed but that requires access
to the actual board for some trial-and-error experiments.
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ARM SoC folks: please apply this directly for next to fix the DTS
compiler noise it is generating.
---
arch/arm/boot/dts/arm-realview-eb-mp.dtsi | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/arm/boot/dts/arm-realview-eb-mp.dtsi b/arch/arm/boot/dts/arm-realview-eb-mp.dtsi
index 7b8d90b7aeea..29b636fce23f 100644
--- a/arch/arm/boot/dts/arm-realview-eb-mp.dtsi
+++ b/arch/arm/boot/dts/arm-realview-eb-mp.dtsi
@@ -150,11 +150,6 @@
interrupts = <0 8 IRQ_TYPE_LEVEL_HIGH>;
};
-&charlcd {
- interrupt-parent = <&intc>;
- interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
-};
-
&serial0 {
interrupt-parent = <&intc>;
interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
--
2.14.3
^ permalink raw reply related
* [linux-sunxi] [PATCH v4 3/6] clk: sunxi-ng: add support for Allwinner A64 DE2 CCU
From: Chen-Yu Tsai @ 2018-01-02 8:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171230113043.30237-4-icenowy@aosc.io>
On Sat, Dec 30, 2017 at 7:30 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> Allwinner A64's DE2 needs to claim a section of SRAM (SRAM C) to work.
>
> Add support for it.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v4:
> - Use a struct to maintain both ccu desc and quirks as Chen-Yu Tsai
> suggested.
This made the patch slightly messy. Could you split it into two patches?
The first just adds the struct (without the sram_needed field) and migrates
everything to that. This patch should mention it is preperation for the
next patch, which adds the a field to the struct (otherwise it makes no
sense and just looks like churn.) The second patch will add the .sram_needed
field and support for the A64 DE2 CCU.
Thanks
ChenYu
^ permalink raw reply
* [PATCHv3 RESEND 1/3] Documentation: dt: memory: ti-emif: add edac support under emif
From: Tero Kristo @ 2018-01-02 8:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171226224819.6uekbnbd5tllaxb5@rob-hp-laptop>
On 27/12/17 00:48, Rob Herring wrote:
> On Fri, Dec 22, 2017 at 06:23:54PM +0200, Tero Kristo wrote:
>> Certain revisions of the TI EMIF IP contain ECC support in them. Reflect
>> this in the DT binding.
>
> "dt-bindings: edac: ..." is the preferred subject prefix.
>Err, I mean "dt-bindings: memory-controllers: ..."
I'll fix this.
>
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> ---
>> Just resending this patch, missed adding devicetree list on this previously
>> and it got lost.
>>
>> .../devicetree/bindings/memory-controllers/ti/emif.txt | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt b/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
>> index 0db6047..f56a347 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
>> @@ -3,12 +3,16 @@
>> EMIF - External Memory Interface - is an SDRAM controller used in
>> TI SoCs. EMIF supports, based on the IP revision, one or more of
>> DDR2/DDR3/LPDDR2 protocols. This binding describes a given instance
>> -of the EMIF IP and memory parts attached to it.
>> +of the EMIF IP and memory parts attached to it. Certain revisions
>> +of the EMIF IP controller also contain optional ECC support, which
>> +corrects one bit errors and detects two bit errors.
>>
>> Required properties:
>> - compatible : Should be of the form "ti,emif-<ip-rev>" where <ip-rev>
>> is the IP revision of the specific EMIF instance.
>> For am437x should be ti,emif-am4372.
>> + For dra7xx should be ti,emif-dra7xx.
>> + For k2x family, should be ti,emif-keystone.
>>
>> - phy-type : <u32> indicating the DDR phy type. Following are the
>> allowed values
>> @@ -42,6 +46,10 @@ Optional properties:
>> - hw-caps-temp-alert : Have this property if the controller
>> has capability for generating SDRAM temperature alerts
>>
>> +- interrupts : A list of interrupt specifiers for memory
>> + controller interrupts, if available. Required for EMIF instances
>> + that support ECC.
>
> Be explicit as to which compatibles have an interrupt. Is it really
> optional for for those controllers? The interrupt is in the h/w whether
> you use ECC or not.
It seems the interrupt property is actually required also for omap4,
omap5 emif based on the driver implementation, they use it for high temp
alert as the main feature. The interrupt exists on am3/am4 also but it
is not used at all there right now.
Would it be better just to make this a required property from binding
point of view and ignore the fact that am3/am4 do not really use it?
-Tero
>
>> +
>> Example:
>>
>> emif1: emif at 0x4c000000 {
>> @@ -54,3 +62,9 @@ emif1: emif at 0x4c000000 {
>> hw-caps-ll-interface;
>> hw-caps-temp-alert;
>> };
>> +
>> +emif1: emif at 4c000000 {
>> + compatible = "ti,emif-dra7";
>> + reg = <0x4c000000 0x200>;
>> + interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
>> +};
>> --
>> 1.9.1
>>
>> --
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply
* [linux-sunxi] [PATCH v4 1/6] ARM: sunxi: h3/h5: add simplefb nodes
From: Chen-Yu Tsai @ 2018-01-02 8:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171230113043.30237-2-icenowy@aosc.io>
On Sat, Dec 30, 2017 at 7:30 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> The H3/H5 SoCs have a HDMI output and a TV Composite output.
>
> Add simplefb nodes for these outputs.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v4:
> - Dropped extra clocks (bus clocks and HDMI DDC clocks), only keep the
> clocks that are needed to display framebuffer to the monitor.
Looks good. I assume you've tested this? It does continue to work
with the bus and DDC clocks disabled, right?
Thanks
ChenYu
^ permalink raw reply
* [linux-sunxi] [PATCH v4 1/6] ARM: sunxi: h3/h5: add simplefb nodes
From: Icenowy Zheng @ 2018-01-02 8:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGb2v64V7KYw5W95-E=b=Li13ZFXy7JakK4+Dx1dsrXXSJZ+wg@mail.gmail.com>
? 2018?1?2???? CST ??4:11:04?Chen-Yu Tsai ???
> On Sat, Dec 30, 2017 at 7:30 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> > The H3/H5 SoCs have a HDMI output and a TV Composite output.
> >
> > Add simplefb nodes for these outputs.
> >
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > ---
> > Changes in v4:
> > - Dropped extra clocks (bus clocks and HDMI DDC clocks), only keep the
> >
> > clocks that are needed to display framebuffer to the monitor.
>
> Looks good. I assume you've tested this? It does continue to work
> with the bus and DDC clocks disabled, right?
Yes. This patchset is tested in Orange Pi PC and SoPine w/ Baseboard "Model
A".
>
> Thanks
> ChenYu
^ permalink raw reply
* [linux-sunxi] [PATCH v4 5/6] arm64: allwinner: a64: add simplefb for A64 SoC
From: Chen-Yu Tsai @ 2018-01-02 8:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171230113043.30237-6-icenowy@aosc.io>
On Sat, Dec 30, 2017 at 7:30 PM, Icenowy Zheng <icenowy@aosc.io> wrote:
> The A64 SoC features two display pipelines, one has a LCD output, the
> other has a HDMI output.
>
> Add support for simplefb for these pipelines on A64 SoC.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v4:
> - Dropped extra clocks.
> - Added labels to the SimpleFB device tree nodes as boards may have
> extra regulator for display pipeline.
>
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index fb8ea7c414e1..d803c115d362 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -42,9 +42,11 @@
> * OTHER DEALINGS IN THE SOFTWARE.
> */
>
> +#include <dt-bindings/clock/sun8i-de2.h>
> #include <dt-bindings/clock/sun50i-a64-ccu.h>
> #include <dt-bindings/clock/sun8i-r-ccu.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/reset/sun8i-de2.h>
Nit: This isn't used anywhere. Please add it when DE2 DRM support is added.
ChenYu
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox