| Message ID | CAJ5Y-eYN6NfFdKAkv5yrHTdeRNWJfvgPVL61EjZdgN0DYP8zpA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 20 June 2014 17:43:05 Ashwin Chaugule wrote: > > > > Note that "device" here doesn't have to mean a platform device that > > is instantiated from DSDT, it can be any mailbox provider that is > > registered in an arbitrary way, as long as you have a method to map > > back from the (consumer-device, name-string) tuple back to the > > (provider, channel) tuple. I have read your patch again now and noticed > > that you actually tried to do this, but unfortunately you got it > > wrong by requiring the consumer to fill out the name of the provider > > in the request. You can't do that, because it's not generic enough > > to support devices that can be reused, and it means that drivers > > using the API are never portable between DT and ACPI. You have to > > get rid of the "ctrl_name" field in the mbox_client structure and > > change the lookup to be based only on cd->dev and cl->chan_name, > > using whatever tables you have available in ACPI. > > I think you looked at the previous version of the patch. I'm attaching > the latest version here again FWIW. In this version, I removed the > "ctrl_name" field and rely on the cl->chan_name to provide the info as > described in Jassi' original patch. > > > linux/mailbox_client.h > > 18 * struct mbox_client - User of a mailbox > 19 * @dev: The client device > 20 * @chan_name: The "controller:channel" this client wants > > Instead of dev, I added a name string to the mbox controller > structure. So now the client gets its channel by requesting > "controller:channel" where controller should match with mbox->name and > channel becomes an index into mbox->chans[]. > Right, I looked at the wrong version, sorry about that. However, it seems you still make the same mistake here: The name that gets passed as chan_name in the mailbox API is a local identifier that is supposed to be interpreted for the client device and used to look up a pointer to the mailbox device and channel. If you require drivers to put global data (e.g. the mbox->name, or the channel number) in there, it's impossible to write a driver that works on both DT and ACPI. If you want to use the mbox_request_channel() interface from a driver, you need some form of lookup table in the ACPI data to do the conversion. The alternative would be not to use mbox_request_channel() at all for now, but to add a new interface that can only be used PCC and that matches by ID but is independent of the use of ACPI or DT, something like: struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl, char *name, unsigned chan_id, struct mbox_chan **chan) { struct mbox_controller *mbox; mbox = mbox_find_pcc_controller(name, ...); *chan = &mbox->chans[chan_id]; return init_channel(*chan, cl); } This would mean that we'd have to special-case "pcc" users, which is not very nice, but at least it would work on both DT and ACPI, and a future ACPI version could still add support for the mailbox API later. Arnd
Hi Arnd, On 21 June 2014 05:34, Arnd Bergmann <arnd@arndb.de> wrote: >> >> linux/mailbox_client.h >> >> 18 * struct mbox_client - User of a mailbox >> 19 * @dev: The client device >> 20 * @chan_name: The "controller:channel" this client wants Please correct me if I'm mistaken, but I think this comment in the header is confusing. It gives the impression that the user is expected to fill in this field as "controller name: channel id". But, looking at an example of a DT based mbox client [1] , that doesnt seem to be the case. And "chan_name" is compared with "mbox-names", which seems to contain a list of Channel names. The mailbox is then identified by a separate DT binding : "mbox", which has the mailbox name and the channel id. So shouldnt this comment not say anything about the "controller" and the DT binding should be changed to "channel-names", instead of "mbox-names" to keep things consistent? >> >> Instead of dev, I added a name string to the mbox controller >> structure. So now the client gets its channel by requesting >> "controller:channel" where controller should match with mbox->name and >> channel becomes an index into mbox->chans[]. >> > > Right, I looked at the wrong version, sorry about that. No problem. Many thanks for the review. > > However, it seems you still make the same mistake here: The name that > gets passed as chan_name in the mailbox API is a local identifier > that is supposed to be interpreted for the client device and used > to look up a pointer to the mailbox device and channel. If you require > drivers to put global data (e.g. the mbox->name, or the channel > number) in there, it's impossible to write a driver that works on > both DT and ACPI. If you want to use the mbox_request_channel() > interface from a driver, you need some form of lookup table in > the ACPI data to do the conversion. Fair point. The more I think about this, it seems that if we want to use the mailbox framework for ACPI kernels, we should have a PCC specific bypass, something like the one you suggested below. The ACPI spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC clients defined as well; CPPC, MPST and RASF. Each of these have their own ACPI tables and so they dont require special DSDT entries. Moreover, these PCC client drivers will be very ACPI specific anyway. So, trying to emulate DT like mbox controller-client matching in ACPI at this point is rather pointless. It will require creating dummy DSDT entries for the PCC mailbox controller and PCC mailbox clients which have their own well defined ACPI tables (and so dont belong in the OS agnostic DSDT) and then coming up with customized Device Specific Methods (DSMs) for mbox clients to refer to mbox controllers. The other alternative is to skip the mailbox framework altogether. One thing to note is that the PCC driver and its clients should work on X86, ARMv8 and any other platform that has ACPI support. Currently the Mailbox framework looks platform agnostic but is tied to DT, so it may not work well for everyone. But like I mentioned early on, the framework provides for async notification and queuing which is useful for PCC, so I'd prefer the PCC specific bypass option. > > The alternative would be not to use mbox_request_channel() at all > for now, but to add a new interface that can only be used PCC and > that matches by ID but is independent of the use of ACPI or DT, > something like: > > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl, > char *name, unsigned chan_id, > struct mbox_chan **chan) > { > struct mbox_controller *mbox; > mbox = mbox_find_pcc_controller(name, ...); > > *chan = &mbox->chans[chan_id]; > return init_channel(*chan, cl); > } > > This would mean that we'd have to special-case "pcc" users, which is > not very nice, but at least it would work on both DT and ACPI, > and a future ACPI version could still add support for the mailbox > API later. I'll play around with this idea a bit and see how it looks. Cheers, Ashwin [1] - https://github.com/hackerspace/rpi-linux/commit/cd0b9584cbedf46812cfd220ba47d80e86b8b7ea
On Monday 23 June 2014 14:25:26 Ashwin Chaugule wrote: > Hi Arnd, > > On 21 June 2014 05:34, Arnd Bergmann <arnd@arndb.de> wrote: > >> > >> linux/mailbox_client.h > >> > >> 18 * struct mbox_client - User of a mailbox > >> 19 * @dev: The client device > >> 20 * @chan_name: The "controller:channel" this client wants > > Please correct me if I'm mistaken, but I think this comment in the > header is confusing. Yes, definitely. Thanks for pointing that out. > It gives the impression that the user is expected > to fill in this field as "controller name: channel id". But, looking > at an example of a DT based mbox client [1] , that doesnt seem to be > the case. And "chan_name" is compared with "mbox-names", which seems > to contain a list of Channel names. The mailbox is then identified by > a separate DT binding : "mbox", which has the mailbox name and the > channel id. So shouldnt this comment not say anything about the > "controller" and the DT binding should be changed to "channel-names", > instead of "mbox-names" to keep things consistent? The comment should be changed, but the property name is good the way it is. We follow the exact same pattern we have for registers, interrupts, dma-channels, etc. > > However, it seems you still make the same mistake here: The name that > > gets passed as chan_name in the mailbox API is a local identifier > > that is supposed to be interpreted for the client device and used > > to look up a pointer to the mailbox device and channel. If you require > > drivers to put global data (e.g. the mbox->name, or the channel > > number) in there, it's impossible to write a driver that works on > > both DT and ACPI. If you want to use the mbox_request_channel() > > interface from a driver, you need some form of lookup table in > > the ACPI data to do the conversion. > > Fair point. The more I think about this, it seems that if we want to > use the mailbox framework for ACPI kernels, we should have a PCC > specific bypass, something like the one you suggested below. The ACPI > spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC > clients defined as well; CPPC, MPST and RASF. Each of these have their > own ACPI tables and so they dont require special DSDT entries. Ok, I see. Can you describe what data is in these tables? > Moreover, these PCC client drivers will be very ACPI specific anyway. > So, trying to emulate DT like mbox controller-client matching in ACPI > at this point is rather pointless. It will require creating dummy DSDT > entries for the PCC mailbox controller and PCC mailbox clients which > have their own well defined ACPI tables (and so dont belong in the OS > agnostic DSDT) and then coming up with customized Device Specific > Methods (DSMs) for mbox clients to refer to mbox controllers. Actually you wouldn't necessarily need DSDT entries, the ACPI core could just call platform_device_create() to instantiate the devices based on the PCC tables. > The other alternative is to skip the mailbox framework altogether. One > thing to note is that the PCC driver and its clients should work on > X86, ARMv8 and any other platform that has ACPI support. Currently the > Mailbox framework looks platform agnostic but is tied to DT, so it may > not work well for everyone. But like I mentioned early on, the > framework provides for async notification and queuing which is useful > for PCC, so I'd prefer the PCC specific bypass option. The mailbox API should still work fine without DT, it would be easy enough to add a lookup mechanism for architectures that create their own platform devices from hardcoded kernel structures, or from ACPI tables that are meant to emulate the DT bindings on embedded x86. But treating PCC special probably does make most sense here, at least the lookup path. > > The alternative would be not to use mbox_request_channel() at all > > for now, but to add a new interface that can only be used PCC and > > that matches by ID but is independent of the use of ACPI or DT, > > something like: > > > > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl, > > char *name, unsigned chan_id, > > struct mbox_chan **chan) > > { > > struct mbox_controller *mbox; > > mbox = mbox_find_pcc_controller(name, ...); > > > > *chan = &mbox->chans[chan_id]; > > return init_channel(*chan, cl); > > } > > > > This would mean that we'd have to special-case "pcc" users, which is > > not very nice, but at least it would work on both DT and ACPI, > > and a future ACPI version could still add support for the mailbox > > API later. > > I'll play around with this idea a bit and see how it looks. > Ok, thanks for looking into this. Arnd
Hello, On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote: >> Fair point. The more I think about this, it seems that if we want to >> use the mailbox framework for ACPI kernels, we should have a PCC >> specific bypass, something like the one you suggested below. The ACPI >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC >> clients defined as well; CPPC, MPST and RASF. Each of these have their >> own ACPI tables and so they dont require special DSDT entries. > > Ok, I see. Can you describe what data is in these tables? For CPPC, its a field for version number, number of entries and then followed by a bunch of PCC entries that have the following structure: 51 struct pcc_register_resource { 52 u8 descriptor; 53 u16 length; 54 u8 space_id; 55 u8 bit_width; 56 u8 bit_offset; 57 u8 access_size; 58 u64 address; 59 } __attribute__ ((packed)); These essentially describe the PCC register space to be used by the respective protocol. e.g. CPPC uses these to exchange CPU performance metrics between the OS and the firmware. I believe MPST and RASF also follow the same format. > >> Moreover, these PCC client drivers will be very ACPI specific anyway. >> So, trying to emulate DT like mbox controller-client matching in ACPI >> at this point is rather pointless. It will require creating dummy DSDT >> entries for the PCC mailbox controller and PCC mailbox clients which >> have their own well defined ACPI tables (and so dont belong in the OS >> agnostic DSDT) and then coming up with customized Device Specific >> Methods (DSMs) for mbox clients to refer to mbox controllers. > > Actually you wouldn't necessarily need DSDT entries, the ACPI core could > just call platform_device_create() to instantiate the devices based on > the PCC tables. > >> The other alternative is to skip the mailbox framework altogether. One >> thing to note is that the PCC driver and its clients should work on >> X86, ARMv8 and any other platform that has ACPI support. Currently the >> Mailbox framework looks platform agnostic but is tied to DT, so it may >> not work well for everyone. But like I mentioned early on, the >> framework provides for async notification and queuing which is useful >> for PCC, so I'd prefer the PCC specific bypass option. > > The mailbox API should still work fine without DT, it would be easy > enough to add a lookup mechanism for architectures that create their > own platform devices from hardcoded kernel structures, or from ACPI > tables that are meant to emulate the DT bindings on embedded x86. Right, a generic lookup method would be useful. I think we should probably revisit this option when/if there are ACPI cases which use anything other than the PCC mailbox controller. > > But treating PCC special probably does make most sense here, at > least the lookup path. Agreed. > >> > The alternative would be not to use mbox_request_channel() at all >> > for now, but to add a new interface that can only be used PCC and >> > that matches by ID but is independent of the use of ACPI or DT, >> > something like: >> > >> > struct mbox_chan *pcc_mbox_get_channel(struct mbox_client *cl, >> > char *name, unsigned chan_id, >> > struct mbox_chan **chan) >> > { >> > struct mbox_controller *mbox; >> > mbox = mbox_find_pcc_controller(name, ...); >> > >> > *chan = &mbox->chans[chan_id]; >> > return init_channel(*chan, cl); >> > } >> > >> > This would mean that we'd have to special-case "pcc" users, which is >> > not very nice, but at least it would work on both DT and ACPI, >> > and a future ACPI version could still add support for the mailbox >> > API later. >> >> I'll play around with this idea a bit and see how it looks. >> > > Ok, thanks for looking into this. Cheers, Ashwin
On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote: > Hello, > > On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote: > >> Fair point. The more I think about this, it seems that if we want to > >> use the mailbox framework for ACPI kernels, we should have a PCC > >> specific bypass, something like the one you suggested below. The ACPI > >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC > >> clients defined as well; CPPC, MPST and RASF. Each of these have their > >> own ACPI tables and so they dont require special DSDT entries. > > > > Ok, I see. Can you describe what data is in these tables? > > For CPPC, its a field for version number, number of entries and then > followed by a bunch of PCC entries that have the following structure: > > 51 struct pcc_register_resource { > 52 u8 descriptor; > 53 u16 length; > 54 u8 space_id; > 55 u8 bit_width; > 56 u8 bit_offset; > 57 u8 access_size; > 58 u64 address; > 59 } __attribute__ ((packed)); > > These essentially describe the PCC register space to be used by the > respective protocol. e.g. CPPC uses these to exchange CPU performance > metrics between the OS and the firmware. > I believe MPST and RASF also follow the same format. Interesting. So I guess it's one entry per client of the PCC? How exactly does the client know which index to use in this table? Arnd
On 23 June 2014 16:21, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote: >> Hello, >> >> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote: >> >> Fair point. The more I think about this, it seems that if we want to >> >> use the mailbox framework for ACPI kernels, we should have a PCC >> >> specific bypass, something like the one you suggested below. The ACPI >> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC >> >> clients defined as well; CPPC, MPST and RASF. Each of these have their >> >> own ACPI tables and so they dont require special DSDT entries. >> > >> > Ok, I see. Can you describe what data is in these tables? >> >> For CPPC, its a field for version number, number of entries and then >> followed by a bunch of PCC entries that have the following structure: >> >> 51 struct pcc_register_resource { >> 52 u8 descriptor; >> 53 u16 length; >> 54 u8 space_id; >> 55 u8 bit_width; >> 56 u8 bit_offset; >> 57 u8 access_size; >> 58 u64 address; >> 59 } __attribute__ ((packed)); >> >> These essentially describe the PCC register space to be used by the >> respective protocol. e.g. CPPC uses these to exchange CPU performance >> metrics between the OS and the firmware. >> I believe MPST and RASF also follow the same format. > > Interesting. So I guess it's one entry per client of the PCC? How > exactly does the client know which index to use in this table? Nah. Just when you'd think - surely there cant be any more 4 letter ACPI acronyms. ;) CPPC is one PCC client by itself. There is a table called PCCT, which describes a list of all PCC subspaces. Each subspace is like a shared mem region dedicated to a PCC client. e.g. a platform may choose subspace id 1 for CPPC. The CPPC has its own table(s) (in case of CPPC, it is a table per CPU), which lists the PCC registers using the structure above. These registers reside in the dedicated PCC subspace. The PCC client drivers will probe their own tables and parse the PCC register entries from within. e.g. the CPPC driver will look for the CPC table (per CPU) and parse its PCC registers. Each PCC register entry reuses the "access_size" field to indicate a PCC subspace index. This is the index into the PCCT list of subspaces. From here the client can get the base address of its shared mem region and the doorbell semantics for communication. Cheers, Ashwin
On Monday 23 June 2014 17:27:21 Ashwin Chaugule wrote: > On 23 June 2014 16:21, Arnd Bergmann <arnd@arndb.de> wrote: > > On Monday 23 June 2014 15:46:08 Ashwin Chaugule wrote: > >> Hello, > >> > >> On 23 June 2014 15:10, Arnd Bergmann <arnd@arndb.de> wrote: > >> >> Fair point. The more I think about this, it seems that if we want to > >> >> use the mailbox framework for ACPI kernels, we should have a PCC > >> >> specific bypass, something like the one you suggested below. The ACPI > >> >> spec defines PCC as the only "mailbox" like mechanism. There are 3 PCC > >> >> clients defined as well; CPPC, MPST and RASF. Each of these have their > >> >> own ACPI tables and so they dont require special DSDT entries. > >> > > >> > Ok, I see. Can you describe what data is in these tables? > >> > >> For CPPC, its a field for version number, number of entries and then > >> followed by a bunch of PCC entries that have the following structure: > >> > >> 51 struct pcc_register_resource { > >> 52 u8 descriptor; > >> 53 u16 length; > >> 54 u8 space_id; > >> 55 u8 bit_width; > >> 56 u8 bit_offset; > >> 57 u8 access_size; > >> 58 u64 address; > >> 59 } __attribute__ ((packed)); > >> > >> These essentially describe the PCC register space to be used by the > >> respective protocol. e.g. CPPC uses these to exchange CPU performance > >> metrics between the OS and the firmware. > >> I believe MPST and RASF also follow the same format. > > > > Interesting. So I guess it's one entry per client of the PCC? How > > exactly does the client know which index to use in this table? > > Nah. Just when you'd think - surely there cant be any more 4 letter > ACPI acronyms. ;) > > CPPC is one PCC client by itself. Ok, I see. > There is a table called PCCT, which describes a list of all PCC > subspaces. Each subspace is like a shared mem region dedicated to a > PCC client. e.g. a platform may choose subspace id 1 for CPPC. The > CPPC has its own table(s) (in case of CPPC, it is a table per CPU), > which lists the PCC registers using the structure above. These > registers reside in the dedicated PCC subspace. > > The PCC client drivers will probe their own tables and parse the PCC > register entries from within. e.g. the CPPC driver will look for the > CPC table (per CPU) and parse its PCC registers. Each PCC register > entry reuses the "access_size" field to indicate a PCC subspace index. > This is the index into the PCCT list of subspaces. From here the > client can get the base address of its shared mem region and the > doorbell semantics for communication. I think a model that's closer to the mailbox subsystem would imply that the common mailbox code (or the pcc driver itself) parses the PCCT table, while the slave driver only passes an index. Or even better, the slave driver would only pass a device pointer, from which the pcc driver can find the pcc_register_resource in the corresponding ACPI table. The name of that table can be the string you pass down to the mailbox API. I suspect there is some issue that makes this all break down though, but that would be a portable way to do this for both DT and ACPI: If we wanted to use DT with the same driver, we would put the name of the table containing pcc_register_resource into the mbox-names property, and that could get used to look up a reference to the pcc device, and to the other data that you have in pcc_register_resource and PCCT. Arnd
Hi Arnd, On 06/24/2014 10:18 AM, Arnd Bergmann wrote: > > I think a model that's closer to the mailbox subsystem would imply > that the common mailbox code (or the pcc driver itself) parses the > PCCT table, while the slave driver only passes an index. This is pretty close to what I have here: The PCC mailbox version: https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/mbox-pcc-review Here the pcc driver in /drivers/acpi/pcc.c is the PCC mailbox controller which parses the PCCT and /drivers/acpi/pcc-test.c is a sample PCC client that sends dummy PCC reads/writes. The PCC + CPPC non-mailbox version: https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/pcc-cppc-dev Here, the CPPC is the PCC client driver which parses the CPC tables. The mailbox conversion for this stuff is a WIP. But it should give an idea of how PCC and PCC clients would work. > Or even better, the slave driver would only pass a device pointer, > from which the pcc driver can find the pcc_register_resource in > the corresponding ACPI table. The name of that table can be the > string you pass down to the mailbox API. I suspect there is some > issue that makes this all break down though, but that would be > a portable way to do this for both DT and ACPI: > > If we wanted to use DT with the same driver, we would put the > name of the table containing pcc_register_resource into the > mbox-names property, and that could get used to look up a > reference to the pcc device, and to the other data that you > have in pcc_register_resource and PCCT. So I dont think we should worry about the PCC clients being used in the DT case, since the PCC and its client specification is very ACPI centric and platforms that want to use these drivers will need an ACPI based firmware anyway. Which is why I think having a separate PCC specific mbox API makes sense. Something like what you suggested should work well for ACPI based platforms. struct mbox_controller * mbox_find_pcc_controller(char *name) { list_for_each_entry(mbox, &mbox_cons, node) { if (mbox->name) if (!strncmp(mbox->name, name)) return mbox; } return -ENODEV; } int pcc_mbox_get_channel(struct mbox_client *cl, char *name, unsigned chan_id, struct mbox_chan **chan) { struct mbox_controller *mbox; mbox = mbox_find_pcc_controller(name); if (!mbox) { pr_err("PCC mbox %s not found.\n", name); return -ENODEV; } *chan = &mbox->chans[chan_id]; return init_channel(*chan, cl); } Cheers, Ashwin
On Tuesday 24 June 2014 13:55:42 Ashwin Chaugule wrote: > > Hi Arnd, > > On 06/24/2014 10:18 AM, Arnd Bergmann wrote: > > > > > I think a model that's closer to the mailbox subsystem would imply > > that the common mailbox code (or the pcc driver itself) parses the > > PCCT table, while the slave driver only passes an index. > > This is pretty close to what I have here: > > The PCC mailbox version: > > https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/mbox-pcc-review > > > Here the pcc driver in /drivers/acpi/pcc.c is the PCC mailbox controller > which parses the PCCT and /drivers/acpi/pcc-test.c is a sample PCC > client that sends dummy PCC reads/writes. > > > The PCC + CPPC non-mailbox version: > > https://git.linaro.org/people/ashwin.chaugule/leg-kernel.git/shortlog/refs/heads/pcc-cppc-dev > > Here, the CPPC is the PCC client driver which parses the CPC tables. The > mailbox conversion for this stuff is a WIP. But it should give an idea > of how PCC and PCC clients would work. Ok, but unfortunately it seems that there is no way for the CPPC to tell the PCC driver to pull the index out of the CPPC tables as far as I can tell. > > Or even better, the slave driver would only pass a device pointer, > > from which the pcc driver can find the pcc_register_resource in > > the corresponding ACPI table. The name of that table can be the > > string you pass down to the mailbox API. I suspect there is some > > issue that makes this all break down though, but that would be > > a portable way to do this for both DT and ACPI: > > > > If we wanted to use DT with the same driver, we would put the > > name of the table containing pcc_register_resource into the > > mbox-names property, and that could get used to look up a > > reference to the pcc device, and to the other data that you > > have in pcc_register_resource and PCCT. > > So I dont think we should worry about the PCC clients being used in the > DT case, since the PCC and its client specification is very ACPI centric > and platforms that want to use these drivers will need an ACPI based > firmware anyway. Which is why I think having a separate PCC specific > mbox API makes sense. I think we should be prepared to add any feature that exists in ACPI also for DT if the need arises, even if we don't expect it to be necessary. There are a number of reasons why you might want to use the drivers with DT, e.g. board bringup (before firmware is available), or to use some features of a SoC that cannot be represented in ACPI but that may be useful for a special-purpose appliance. > Something like what you suggested should work well for ACPI based platforms. > > struct mbox_controller * > mbox_find_pcc_controller(char *name) > { > list_for_each_entry(mbox, &mbox_cons, node) { > if (mbox->name) > if (!strncmp(mbox->name, name)) > return mbox; > } > > return -ENODEV; > } > > int pcc_mbox_get_channel(struct mbox_client *cl, > char *name, unsigned chan_id, > struct mbox_chan **chan) > { > struct mbox_controller *mbox; > mbox = mbox_find_pcc_controller(name); > > if (!mbox) { > pr_err("PCC mbox %s not found.\n", name); > return -ENODEV; > } > > *chan = &mbox->chans[chan_id]; > return init_channel(*chan, cl); > } Yes, that seems fine, and it will just work with DT as well if we need that. Arnd
From 6fe6e583f0b23b08643a4a85545a9a5338b9b1a0 Mon Sep 17 00:00:00 2001 From: Ashwin Chaugule <ashwin.chaugule@linaro.org> Date: Wed, 11 Jun 2014 16:09:35 -0400 Subject: [PATCH] Mailbox: Add support for ACPI The current mailbox framework only supports DT based bindings. Add another mechanism for mailbox clients to register with mailbox controllers and request for specific mailbox channels. This enables usage of the mailbox framework on kernels with ACPI support. Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org> --- drivers/mailbox/mailbox.c | 177 ++++++++++++++++++++++++++----------- include/linux/mailbox_client.h | 2 +- include/linux/mailbox_controller.h | 1 + 3 files changed, 129 insertions(+), 51 deletions(-) diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index d83d12c..e2704f5 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -17,6 +17,7 @@ #include <linux/err.h> #include <linux/module.h> #include <linux/device.h> +#include <linux/acpi.h> #include <linux/mailbox_client.h> #include <linux/mailbox_controller.h> @@ -273,100 +274,166 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg) } EXPORT_SYMBOL_GPL(mbox_send_message); -/** - * mbox_request_channel - Request a mailbox channel. - * @cl: Identity of the client requesting the channel. - * - * The Client specifies its requirements and capabilities while asking for - * a mailbox channel by name. It can't be called from atomic context. - * The channel is exclusively allocated and can't be used by another - * client before the owner calls mbox_free_channel. - * After assignment, any packet received on this channel will be - * handed over to the client via the 'rx_callback'. - * - * Return: Pointer to the channel assigned to the client if successful. - * ERR_PTR for request failure. - */ -struct mbox_chan *mbox_request_channel(struct mbox_client *cl) +static int init_channel(struct mbox_chan *chan, + struct mbox_client *cl) +{ + unsigned long flags; + int ret; + + if (!chan) { + pr_err("No mailbox channel specified\n"); + return -EINVAL; + } + + spin_lock_irqsave(&chan->lock, flags); + chan->msg_free = 0; + chan->msg_count = 0; + chan->active_req = NULL; + chan->cl = cl; + + if (!cl->tx_tout) /* wait for ever */ + cl->tx_tout = msecs_to_jiffies(3600000); + else + cl->tx_tout = msecs_to_jiffies(cl->tx_tout); + if (chan->txdone_method == TXDONE_BY_POLL + && cl->knows_txdone) + chan->txdone_method |= TXDONE_BY_ACK; + spin_unlock_irqrestore(&chan->lock, flags); + + ret = chan->mbox->ops->startup(chan); + if (ret) { + pr_err("Unable to startup the chan\n"); + mbox_free_channel(chan); + chan = ERR_PTR(ret); + } + + return ret; +} + +static int get_acpi_mbox_chan(struct mbox_client *cl, + struct mbox_chan **chan) +{ + struct mbox_controller *mbox; + int chan_id, ret, len; + char *chan_ptr; + + if (!cl->chan_name) + return -ENODEV; + + list_for_each_entry(mbox, &mbox_cons, node) { + if (mbox->name) { + /* + * The cl->chan_name has the format => controller:channel + * as described in mailbox_client.h + */ + + len = strlen(mbox->name); + chan_ptr = cl->chan_name + len + 1; + + ret = kstrtou32(cl->chan_name, 0, &chan_id); + + if (ret < 0) { + pr_err("Err while parsing mailbox:%s channel idx\n", + mbox->name); + continue; + } + + if (!strncmp(cl->chan_name, mbox->name, len)) { + *chan = &mbox->chans[chan_id]; + return init_channel(*chan, cl); + } + } + } + + return -ENODEV; +} + +static int get_of_mbox_chan(struct mbox_client *cl, + struct mbox_chan **chan) { struct device *dev = cl->dev; struct mbox_controller *mbox; struct of_phandle_args spec; - struct mbox_chan *chan; - unsigned long flags; int count, i, ret; if (!dev || !dev->of_node) { pr_err("%s: No owner device node\n", __func__); - return ERR_PTR(-ENODEV); + return -ENODEV; } count = of_property_count_strings(dev->of_node, "mbox-names"); if (count < 0) { pr_err("%s: mbox-names property of node '%s' missing\n", __func__, dev->of_node->full_name); - return ERR_PTR(-ENODEV); + return -ENODEV; } - - mutex_lock(&con_mutex); - - ret = -ENODEV; for (i = 0; i < count; i++) { const char *s; if (of_property_read_string_index(dev->of_node, - "mbox-names", i, &s)) + "mbox-names", i, &s)) continue; if (strcmp(cl->chan_name, s)) continue; if (of_parse_phandle_with_args(dev->of_node, - "mbox", "#mbox-cells", i, &spec)) + "mbox", "#mbox-cells", i, &spec)) continue; - chan = NULL; list_for_each_entry(mbox, &mbox_cons, node) if (mbox->dev->of_node == spec.np) { - chan = mbox->of_xlate(mbox, &spec); + *chan = mbox->of_xlate(mbox, &spec); break; } of_node_put(spec.np); - if (!chan) + if (!(*chan)) continue; ret = -EBUSY; - if (!chan->cl && try_module_get(mbox->dev->driver->owner)) + if (!(*chan)->cl && acpi_disabled && + try_module_get(mbox->dev->driver->owner)) break; } if (i == count) { mutex_unlock(&con_mutex); - return ERR_PTR(ret); + return ret; } - spin_lock_irqsave(&chan->lock, flags); - chan->msg_free = 0; - chan->msg_count = 0; - chan->active_req = NULL; - chan->cl = cl; - if (!cl->tx_tout) /* wait for ever */ - cl->tx_tout = msecs_to_jiffies(3600000); + return init_channel(*chan, cl); +} + +/** + * mbox_request_channel - Request a mailbox channel. + * @cl: Identity of the client requesting the channel. + * + * The Client specifies its requirements and capabilities while asking for + * a mailbox channel by name. It can't be called from atomic context. + * The channel is exclusively allocated and can't be used by another + * client before the owner calls mbox_free_channel. + * After assignment, any packet received on this channel will be + * handed over to the client via the 'rx_callback'. + * + * Return: Pointer to the channel assigned to the client if successful. + * ERR_PTR for request failure. + */ +struct mbox_chan *mbox_request_channel(struct mbox_client *cl) +{ + struct mbox_chan *chan = NULL; + int ret; + + mutex_lock(&con_mutex); + + if (acpi_disabled) + ret = get_of_mbox_chan(cl, &chan); else - cl->tx_tout = msecs_to_jiffies(cl->tx_tout); - if (chan->txdone_method == TXDONE_BY_POLL - && cl->knows_txdone) - chan->txdone_method |= TXDONE_BY_ACK; - spin_unlock_irqrestore(&chan->lock, flags); + ret = get_acpi_mbox_chan(cl, &chan); - ret = chan->mbox->ops->startup(chan); - if (ret) { - pr_err("Unable to startup the chan\n"); - mbox_free_channel(chan); - chan = ERR_PTR(ret); - } + if (ret) + pr_err("No mailbox channels found\n"); mutex_unlock(&con_mutex); return chan; @@ -394,7 +461,9 @@ void mbox_free_channel(struct mbox_chan *chan) if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) chan->txdone_method = TXDONE_BY_POLL; - module_put(chan->mbox->dev->driver->owner); + if (chan->mbox->dev) + module_put(chan->mbox->dev->driver->owner); + spin_unlock_irqrestore(&chan->lock, flags); } EXPORT_SYMBOL_GPL(mbox_free_channel); @@ -422,7 +491,15 @@ int mbox_controller_register(struct mbox_controller *mbox) int i, txdone; /* Sanity check */ - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) + if (!mbox || !mbox->ops || !mbox->num_chans) + return -EINVAL; + + /* + * For ACPI platforms, to get mbox->dev, we'd need to + * have a fake meaningless entry in the DSDT for the + * mailbox controller. + */ + if (acpi_disabled && !mbox->dev) return -EINVAL; if (mbox->txdone_irq) diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h index bbac2d2..716fbae 100644 --- a/include/linux/mailbox_client.h +++ b/include/linux/mailbox_client.h @@ -29,7 +29,7 @@ struct mbox_chan; */ struct mbox_client { struct device *dev; - const char *chan_name; + char *chan_name; void (*rx_callback)(struct mbox_client *cl, void *mssg); void (*tx_done)(struct mbox_client *cl, void *mssg, enum mbox_result r); bool tx_block; diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index cf81e80..06476ef 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -78,6 +78,7 @@ struct mbox_controller { unsigned period; /* Hook to add to the global controller list */ struct list_head node; + char *name; } __aligned(32); /* -- 1.8.3.2