linux-kernel - Re: [RFC PATCH 2/3] soc: ti: Add wkup_m3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 12 Dec 2014 16:03:21 -0600
From:	Dave Gerlach <d-gerlach@...com>
To:	Arnd Bergmann <arnd@...db.de>,
	<linux-arm-kernel@...ts.infradead.org>
CC:	<linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>,
	<devicetree@...r.kernel.org>, Ohad Ben-Cohen <ohad@...ery.com>,
	Paul Walmsley <paul@...an.com>,
	Kevin Hilman <khilman@...aro.org>,
	Tony Lindgren <tony@...mide.com>,
	Benoit Cousson <bcousson@...libre.com>,
	Ohad Ben-Cohen <ohad@...ery.com>
Subject: Re: [RFC PATCH 2/3] soc: ti: Add wkup_m3_ipc driver

On 11/26/2014 03:51 PM, Arnd Bergmann wrote:
> On Wednesday 26 November 2014 15:38:09 Dave Gerlach wrote:
>> +
>> +static const struct wkup_m3_wakeup_src wakeups[] = {
>> +       {.irq_nr = 35,  .src = "USB0_PHY"},
>> +       {.irq_nr = 36,  .src = "USB1_PHY"},
>> +       {.irq_nr = 40,  .src = "I2C0"},
>> +       {.irq_nr = 41,  .src = "RTC Timer"},
>> +       {.irq_nr = 42,  .src = "RTC Alarm"},
>> +       {.irq_nr = 43,  .src = "Timer0"},
>> +       {.irq_nr = 44,  .src = "Timer1"},
>> +       {.irq_nr = 45,  .src = "UART"},
>> +       {.irq_nr = 46,  .src = "GPIO0"},
>> +       {.irq_nr = 48,  .src = "MPU_WAKE"},
>> +       {.irq_nr = 49,  .src = "WDT0"},
>> +       {.irq_nr = 50,  .src = "WDT1"},
>> +       {.irq_nr = 51,  .src = "ADC_TSC"},
>> +       {.irq_nr = 0,   .src = "Unknown"},
>> +};
>>
> 
> This seems awfully specific to some SoC version, and not aware of
> IRQ domains. It also seems to be only used in a dev_dbg statement,
> so I guess you could just kill this off entirely.

This is determined by the firmware in use on the remote processor and works for
both am335x and am437x. However it is not required information and I'd be fine
with taking it out.

> 
>> +static struct rproc *wkup_m3_get_rproc(struct device *dev)
>> +{
>> +       struct device_node *node;
>> +       struct rproc *rp;
>> +
>> +       node = of_parse_phandle(dev->of_node, "ti,rproc", 0);
>> +       if (!node)
>> +               return NULL;
>> +
>> +       dev = bus_find_device(&platform_bus_type, NULL, node, match);
>> +       if (!dev)
>> +               return NULL;
>> +
>> +       rp = dev_get_drvdata(dev);
>> +       return rp;
> 
> This is wrong on a number of levels. I suspect what you really want
> is an interface exported from drivers/remoteproc that looks up
> a 'struct rproc' and performs the necessary reference counting.
> 
> That one can just use of_find_node_by_phandle() to get to
> a device_node and use that to look up the rproc device in
> a linked list it maintains.

Added Ohad as I should have cc'd him in the first place..

Yes I agree that it's not the best solution. There used to be an
rproc_get/rproc_put api but that was removed, I'll look into adding
rproc_get_by_phandle into drivers/remoteproc as that would be ideal for this
situation and a cleaner way of doing it. Thanks for the comments.

Regards,
Dave

> 
> 	Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/