Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1674304 > unrolled thread

Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for  Keystone 2 DSPs Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-25 22:20 +0200
    Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for  Keystone 2 DSPs Suman Anna <s-anna@ti.com> - 2017-06-26 18:00 +0200
      Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for  Keystone 2 DSPs Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-26 22:10 +0200
        Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for  Keystone 2 DSPs Suman Anna <s-anna@ti.com> - 2017-06-26 22:30 +0200
          Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for  Keystone 2 DSPs Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-26 23:30 +0200

#1674304 — Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs

On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:

> +static int keystone_rproc_start(struct rproc *rproc)
> +{
> +	struct keystone_rproc *ksproc = rproc->priv;
> +	int ret;
> +
> +	INIT_WORK(&ksproc->workqueue, handle_event);
> +
> +	ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> +			  dev_name(ksproc->dev), ksproc);
> +	if (ret) {
> +		dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> +			ret);
> +		goto out;
> +	}
> +
> +	ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> +			  0, dev_name(ksproc->dev), ksproc);
> +	if (ret) {
> +		dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> +			ret);
> +		goto free_vring_irq;
> +	}

I do prefer that your request any resources during probe() and
potentially enable/disable them here. If below concern about using a
GPIO driver is cleared already I'll take it as is though.

[..]
> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct keystone_rproc *ksproc = rproc->priv;
> +
> +	if (WARN_ON(ksproc->kick_gpio < 0))
> +		return;
> +
> +	gpio_set_value(ksproc->kick_gpio, 1);
> +}
> +

This doesn't sound like a gpio-controller and the GPIO maintainer did
reject an attempt by me to use the GPIO framework to abstract a similar
thing. Do you already have this driver upstream or have you clarified
with the maintainer that the GPIO framework is an acceptable abstraction
for this?

It looks equivalent to the "APCS IPC" register found in Qualcomm
platforms, previously implemented through a syscon but in v4.13 being
pushed to being a mailbox driver.


Apart from this I think the series looks good.

Regards,
Bjorn

[toc] | [next] | [standalone]

#1674896

Hi Bjorn,

On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> 
>> +static int keystone_rproc_start(struct rproc *rproc)
>> +{
>> +	struct keystone_rproc *ksproc = rproc->priv;
>> +	int ret;
>> +
>> +	INIT_WORK(&ksproc->workqueue, handle_event);
>> +
>> +	ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>> +			  dev_name(ksproc->dev), ksproc);
>> +	if (ret) {
>> +		dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>> +			ret);
>> +		goto out;
>> +	}
>> +
>> +	ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>> +			  0, dev_name(ksproc->dev), ksproc);
>> +	if (ret) {
>> +		dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>> +			ret);
>> +		goto free_vring_irq;
>> +	}
> 
> I do prefer that your request any resources during probe() and
> potentially enable/disable them here. If below concern about using a
> GPIO driver is cleared already I'll take it as is though.
> 
> [..]
>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> +	struct keystone_rproc *ksproc = rproc->priv;
>> +
>> +	if (WARN_ON(ksproc->kick_gpio < 0))
>> +		return;
>> +
>> +	gpio_set_value(ksproc->kick_gpio, 1);
>> +}
>> +
> 
> This doesn't sound like a gpio-controller and the GPIO maintainer did
> reject an attempt by me to use the GPIO framework to abstract a similar
> thing. Do you already have this driver upstream or have you clarified
> with the maintainer that the GPIO framework is an acceptable abstraction
> for this?

Yeah, this has been upstream since quite some time. See commit
2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").

regards
Suman

> 
> It looks equivalent to the "APCS IPC" register found in Qualcomm
> platforms, previously implemented through a syscon but in v4.13 being
> pushed to being a mailbox driver.
> 
> 
> Apart from this I think the series looks good.
> 
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1675078

On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:

> Hi Bjorn,
> 
> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
> > On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
> > 
> >> +static int keystone_rproc_start(struct rproc *rproc)
> >> +{
> >> +	struct keystone_rproc *ksproc = rproc->priv;
> >> +	int ret;
> >> +
> >> +	INIT_WORK(&ksproc->workqueue, handle_event);
> >> +
> >> +	ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> >> +			  dev_name(ksproc->dev), ksproc);
> >> +	if (ret) {
> >> +		dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> >> +			ret);
> >> +		goto out;
> >> +	}
> >> +
> >> +	ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> >> +			  0, dev_name(ksproc->dev), ksproc);
> >> +	if (ret) {
> >> +		dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> >> +			ret);
> >> +		goto free_vring_irq;
> >> +	}
> > 
> > I do prefer that your request any resources during probe() and
> > potentially enable/disable them here. If below concern about using a
> > GPIO driver is cleared already I'll take it as is though.
> > 
> > [..]
> >> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> >> +{
> >> +	struct keystone_rproc *ksproc = rproc->priv;
> >> +
> >> +	if (WARN_ON(ksproc->kick_gpio < 0))
> >> +		return;
> >> +
> >> +	gpio_set_value(ksproc->kick_gpio, 1);
> >> +}
> >> +
> > 
> > This doesn't sound like a gpio-controller and the GPIO maintainer did
> > reject an attempt by me to use the GPIO framework to abstract a similar
> > thing. Do you already have this driver upstream or have you clarified
> > with the maintainer that the GPIO framework is an acceptable abstraction
> > for this?
> 
> Yeah, this has been upstream since quite some time. See commit
> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
> 

Okay, sounds good. I have merged the series.


I still would like to have resources allocated at probe() time, so I
would appreciate a follow up patch moving the request_irq()s to probe,
per above comment (but we can take that after v4.13).

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1675085

On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> 
>> Hi Bjorn,
>>
>> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
>>> On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:
>>>
>>>> +static int keystone_rproc_start(struct rproc *rproc)
>>>> +{
>>>> +	struct keystone_rproc *ksproc = rproc->priv;
>>>> +	int ret;
>>>> +
>>>> +	INIT_WORK(&ksproc->workqueue, handle_event);
>>>> +
>>>> +	ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
>>>> +			  dev_name(ksproc->dev), ksproc);
>>>> +	if (ret) {
>>>> +		dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
>>>> +			ret);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
>>>> +			  0, dev_name(ksproc->dev), ksproc);
>>>> +	if (ret) {
>>>> +		dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
>>>> +			ret);
>>>> +		goto free_vring_irq;
>>>> +	}
>>>
>>> I do prefer that your request any resources during probe() and
>>> potentially enable/disable them here. If below concern about using a
>>> GPIO driver is cleared already I'll take it as is though.
>>>
>>> [..]
>>>> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
>>>> +{
>>>> +	struct keystone_rproc *ksproc = rproc->priv;
>>>> +
>>>> +	if (WARN_ON(ksproc->kick_gpio < 0))
>>>> +		return;
>>>> +
>>>> +	gpio_set_value(ksproc->kick_gpio, 1);
>>>> +}
>>>> +
>>>
>>> This doesn't sound like a gpio-controller and the GPIO maintainer did
>>> reject an attempt by me to use the GPIO framework to abstract a similar
>>> thing. Do you already have this driver upstream or have you clarified
>>> with the maintainer that the GPIO framework is an acceptable abstraction
>>> for this?
>>
>> Yeah, this has been upstream since quite some time. See commit
>> 2134cb997f2f ("gpio: syscon: reuse for keystone 2 socs").
>>
> 
> Okay, sounds good. I have merged the series.
> 
> 
> I still would like to have resources allocated at probe() time, so I
> would appreciate a follow up patch moving the request_irq()s to probe,
> per above comment (but we can take that after v4.13).

OK thanks. This is a common theme across all the remoteproc drivers
supporting rpmsg, and I definitely need to disable them in probe since
the boot or the virtio/rpmsg devices are not guaranteed to be present in
the probe.

regards
Suman

[toc] | [prev] | [next] | [standalone]

#1675114

On Mon 26 Jun 13:20 PDT 2017, Suman Anna wrote:

> On 06/26/2017 03:06 PM, Bjorn Andersson wrote:
> > On Mon 26 Jun 08:54 PDT 2017, Suman Anna wrote:
> >> On 06/25/2017 03:15 PM, Bjorn Andersson wrote:
[..]
> > I still would like to have resources allocated at probe() time, so I
> > would appreciate a follow up patch moving the request_irq()s to probe,
> > per above comment (but we can take that after v4.13).
> 
> OK thanks. This is a common theme across all the remoteproc drivers
> supporting rpmsg, and I definitely need to disable them in probe since
> the boot or the virtio/rpmsg devices are not guaranteed to be present in
> the probe.
> 

We have a consistent struct rproc after rproc_alloc(). So before the
virtio device (rpmsg in your case) calls virtio_find_vqs() rvring->vq
will be NULL and rproc_vq_interrupt() is a nop.

So AFAICT it should be fine to register this at probe time and leave it
enabled...but there's a lot of moving parts involved here, so it's
possible that I'm missing something.


But in for both interrupts it's important to disable them before calling
rproc_free(), as this will free the memory passed to the interrupt
handler.

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web