| Message ID | 1409081738-5602-2-git-send-email-ashwin.chaugule@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote: > The PCC (Platform Communication Channel) is a generic > mailbox to be used by PCC clients such as CPPC, RAS > and MPST as defined in the ACPI 5.0+ spec. This patch > modifies the Mailbox framework to lookup PCC mailbox > controllers and channels such that PCC drivers can work > with or without ACPI support in the kernel. > > Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org> > --- > drivers/mailbox/mailbox.c | 118 ++++++++++++++++++++++++++++++++++--- > include/linux/mailbox_client.h | 6 ++ > include/linux/mailbox_controller.h | 1 + > 3 files changed, 118 insertions(+), 7 deletions(-) Based on the patch description (adding support for a particular kind of mailbox) I'd expect to see a new driver or helper library being added to drivers/mailbox rather than changes in the mailbox core. If changes in the core are needed to support this I'd expect to see them broken out as separate patches. > +static struct mbox_controller * > +mbox_find_pcc_controller(char *name) > +{ > + struct mbox_controller *mbox; > + list_for_each_entry(mbox, &mbox_cons, node) { > + if (mbox->name) > + if (!strcmp(mbox->name, name)) > + return mbox; > + } > + > + return NULL; > +} This doesn't look particularly PCC specific? > /* Sanity check */ > - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) > + > + /* > + * PCC clients and controllers are currently not backed by > + * platform device structures. > + */ > +#ifndef CONFIG_PCC > + if (!mbox->dev) > + return -EINVAL; > +#endif It seems better to make this consistent - either enforce it all the time or don't enforce it.
Hi Mark, On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote: > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote: > >> The PCC (Platform Communication Channel) is a generic >> mailbox to be used by PCC clients such as CPPC, RAS >> and MPST as defined in the ACPI 5.0+ spec. This patch >> modifies the Mailbox framework to lookup PCC mailbox >> controllers and channels such that PCC drivers can work >> with or without ACPI support in the kernel. >> >> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org> >> --- >> drivers/mailbox/mailbox.c | 118 ++++++++++++++++++++++++++++++++++--- >> include/linux/mailbox_client.h | 6 ++ >> include/linux/mailbox_controller.h | 1 + >> 3 files changed, 118 insertions(+), 7 deletions(-) > > Based on the patch description (adding support for a particular kind of > mailbox) I'd expect to see a new driver or helper library being added to > drivers/mailbox rather than changes in the mailbox core. If changes in > the core are needed to support this I'd expect to see them broken out as > separate patches. [..] > >> +static struct mbox_controller * >> +mbox_find_pcc_controller(char *name) >> +{ >> + struct mbox_controller *mbox; >> + list_for_each_entry(mbox, &mbox_cons, node) { >> + if (mbox->name) >> + if (!strcmp(mbox->name, name)) >> + return mbox; >> + } >> + >> + return NULL; >> +} > > This doesn't look particularly PCC specific? Call this mbox_find_controller_by_name() instead? > >> /* Sanity check */ >> - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) >> + >> + /* >> + * PCC clients and controllers are currently not backed by >> + * platform device structures. >> + */ >> +#ifndef CONFIG_PCC >> + if (!mbox->dev) >> + return -EINVAL; >> +#endif > > It seems better to make this consistent - either enforce it all the time > or don't enforce it. So this is where it got really messy. We're trying to create a "device" out of something that isn't. The PCCT, which is used as a mailbox controller here, is a table and not a peripheral device. To treat this as a device (without faking it by manually putting together a struct device), would require adding a DSDT entry which is really a wrong place for it. Are there examples today where drivers manually create a struct driver and struct device and match them internally? (i.e. w/o using the generic driver subsystem) The main reason why I thought this Mailbox framework looked useful (after you pointed me to it) for PCC was due to its async notification features. But thats easy and small enough to add to the PCC driver itself. We can also add a generic controller lookup mechanism in the PCC driver for anyone who doesn't want to use ACPI. I think thats a much cleaner way to handle PCC support. Adding PCC as a generic mailbox controller is turning out to be more messier that we'd originally imagined. Cheers, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote: > On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote: > >> +static struct mbox_controller * > >> +mbox_find_pcc_controller(char *name) > >> +{ > >> + struct mbox_controller *mbox; > >> + list_for_each_entry(mbox, &mbox_cons, node) { > >> + if (mbox->name) > >> + if (!strcmp(mbox->name, name)) > >> + return mbox; > >> + } > >> + > >> + return NULL; > >> +} > > This doesn't look particularly PCC specific? > Call this mbox_find_controller_by_name() instead? That certainly looks like what it's doing. Probably also make the name that gets passed in const while you're at it. > >> /* Sanity check */ > >> - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) > >> + > >> + /* > >> + * PCC clients and controllers are currently not backed by > >> + * platform device structures. > >> + */ > >> +#ifndef CONFIG_PCC > >> + if (!mbox->dev) > >> + return -EINVAL; > >> +#endif > > It seems better to make this consistent - either enforce it all the time > > or don't enforce it. > So this is where it got really messy. We're trying to create a The messiness is orthogonal to my comment here - either it's legal to request a mailbox without a device or it isn't, it shouldn't depend on a random kernel configuration option for a particular mailbox driver which it is. > "device" out of something that isn't. The PCCT, which is used as a > mailbox controller here, is a table and not a peripheral device. To > treat this as a device (without faking it by manually putting together > a struct device), would require adding a DSDT entry which is really a > wrong place for it. Are there examples today where drivers manually > create a struct driver and struct device and match them internally? > (i.e. w/o using the generic driver subsystem) Arguably that's what things like cpufreq end up doing, though people tend to just shove a device into DT. Are you sure there isn't any device at all in ACPI that you could hang this off, looking at my desktop I see rather a lot of apparently synthetic ACPI devices with names starting LNX including for example LNXSYSTM:00? > The main reason why I thought this Mailbox framework looked useful > (after you pointed me to it) for PCC was due to its async notification > features. But thats easy and small enough to add to the PCC driver > itself. We can also add a generic controller lookup mechanism in the > PCC driver for anyone who doesn't want to use ACPI. I think thats a > much cleaner way to handle PCC support. Adding PCC as a generic > mailbox controller is turning out to be more messier that we'd > originally imagined. If PCC is described by ACPI tables how would non-ACPI users be able to use it?
Hi Mark, On 27 August 2014 15:09, Mark Brown <broonie@kernel.org> wrote: > On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote: >> On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote: >> > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote: >> >> /* Sanity check */ >> >> - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) >> >> + >> >> + /* >> >> + * PCC clients and controllers are currently not backed by >> >> + * platform device structures. >> >> + */ >> >> +#ifndef CONFIG_PCC >> >> + if (!mbox->dev) >> >> + return -EINVAL; >> >> +#endif > >> > It seems better to make this consistent - either enforce it all the time >> > or don't enforce it. > >> So this is where it got really messy. We're trying to create a > > The messiness is orthogonal to my comment here - either it's legal to > request a mailbox without a device or it isn't, it shouldn't depend on a > random kernel configuration option for a particular mailbox driver which > it is. > Fair enough. This was just to show that PCC is unfortunately not a good candidate as a generic mailbox controller. >> "device" out of something that isn't. The PCCT, which is used as a >> mailbox controller here, is a table and not a peripheral device. To >> treat this as a device (without faking it by manually putting together >> a struct device), would require adding a DSDT entry which is really a >> wrong place for it. Are there examples today where drivers manually >> create a struct driver and struct device and match them internally? >> (i.e. w/o using the generic driver subsystem) > > Arguably that's what things like cpufreq end up doing, though people > tend to just shove a device into DT. Are you sure there isn't any > device at all in ACPI that you could hang this off, looking at my > desktop I see rather a lot of apparently synthetic ACPI devices with > names starting LNX including for example LNXSYSTM:00? Those are special HIDs defined in the ACPI spec and none of those can be used to back a device for the PCCT itself, since they're unrelated to the PCC protocol. The PCCT is defined in the spec as a separate table and if supported, should be visible in your system in the PCCT.dsl file. Just for the sake of the Mailbox framework, trying to represent the PCCT (which is a table) as a mailbox controller device, would require registering a special HID. That in turn would make an otherwise OS agnostic thing very Linux specific. > >> The main reason why I thought this Mailbox framework looked useful >> (after you pointed me to it) for PCC was due to its async notification >> features. But thats easy and small enough to add to the PCC driver >> itself. We can also add a generic controller lookup mechanism in the >> PCC driver for anyone who doesn't want to use ACPI. I think thats a >> much cleaner way to handle PCC support. Adding PCC as a generic >> mailbox controller is turning out to be more messier that we'd >> originally imagined. > > If PCC is described by ACPI tables how would non-ACPI users be able to > use it? Whoever wants to do that, would need to come up with DT bindings that describe something similar to the PCCT contents. They could possibly ignore the ACPI specific bits like signature, asl compiler details etc. (which are only used by ACPI table parsers) and provide the rest of it. Its essentially an array of structures that point to various shared memory regions, each of which is owned by a PCC client and shared with the firmware. The intercommunication between client and firmware is via a doorbell, which is also described in these entries and can be implemented as an SGI or similar. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 27 August 2014 20:09:02 Mark Brown wrote: > On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote: > > On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote: > > > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote: > > > >> +static struct mbox_controller * > > >> +mbox_find_pcc_controller(char *name) > > >> +{ > > >> + struct mbox_controller *mbox; > > >> + list_for_each_entry(mbox, &mbox_cons, node) { > > >> + if (mbox->name) > > >> + if (!strcmp(mbox->name, name)) > > >> + return mbox; > > >> + } > > >> + > > >> + return NULL; > > >> +} > > > > This doesn't look particularly PCC specific? > > > Call this mbox_find_controller_by_name() instead? > > That certainly looks like what it's doing. Probably also make the name > that gets passed in const while you're at it. The mailbox API intentionally does not have an interface for that: you are supposed to get a reference to an mbox controller from a phandle or similar, not by knowing the name of the controller. Unfortunately, the three patches that Ashwin posted don't have a caller for this function, so I don't know what it's actually used for. Why do we need this function for pcc, and what are the names that can be passed here? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 27, 2014 at 05:49:53PM -0400, Ashwin Chaugule wrote: > On 27 August 2014 15:09, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote: > > The messiness is orthogonal to my comment here - either it's legal to > > request a mailbox without a device or it isn't, it shouldn't depend on a > > random kernel configuration option for a particular mailbox driver which > > it is. > Fair enough. This was just to show that PCC is unfortunately not a > good candidate as a generic mailbox controller. That seems to be a very big leap... > >> "device" out of something that isn't. The PCCT, which is used as a > >> mailbox controller here, is a table and not a peripheral device. To > >> treat this as a device (without faking it by manually putting together > >> a struct device), would require adding a DSDT entry which is really a > >> wrong place for it. Are there examples today where drivers manually > >> create a struct driver and struct device and match them internally? > >> (i.e. w/o using the generic driver subsystem) > > Arguably that's what things like cpufreq end up doing, though people > > tend to just shove a device into DT. Are you sure there isn't any > > device at all in ACPI that you could hang this off, looking at my > > desktop I see rather a lot of apparently synthetic ACPI devices with > > names starting LNX including for example LNXSYSTM:00? > Those are special HIDs defined in the ACPI spec and none of those can > be used to back a device for the PCCT itself, since they're unrelated > to the PCC protocol. The PCCT is defined in the spec as a separate > table and if supported, should be visible in your system in the > PCCT.dsl file. Just for the sake of the Mailbox framework, trying to > represent the PCCT (which is a table) as a mailbox controller device, > would require registering a special HID. That in turn would make an > otherwise OS agnostic thing very Linux specific. OK, but then there's always the option of just having some code that runs on init and instantiates a device if it sees the appropriate thing in the ACPI tables in a similar manner to how HIDs are handled. It's a small amount of work but it will generally make life easier if there is a struct device. > > If PCC is described by ACPI tables how would non-ACPI users be able to > > use it? > Whoever wants to do that, would need to come up with DT bindings that > describe something similar to the PCCT contents. They could possibly > ignore the ACPI specific bits like signature, asl compiler details > etc. (which are only used by ACPI table parsers) and provide the rest > of it. Its essentially an array of structures that point to various > shared memory regions, each of which is owned by a PCC client and > shared with the firmware. The intercommunication between client and > firmware is via a doorbell, which is also described in these entries > and can be implemented as an SGI or similar. Of course most likely such a binding would involve creating a device that owns the mailboxes so this'd be fairly straightforward...
On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote: > On Wednesday 27 August 2014 20:09:02 Mark Brown wrote: > > That certainly looks like what it's doing. Probably also make the name > > that gets passed in const while you're at it. > The mailbox API intentionally does not have an interface for > that: you are supposed to get a reference to an mbox controller > from a phandle or similar, not by knowing the name of the controller. Right, and what he's trying to work around here is that ACPI has chosen to provide a generic binding for some mailboxes which isn't associated with anything we represent as a device and he doesn't want to provide that device as a Linux virtual thing. > Unfortunately, the three patches that Ashwin posted don't have a > caller for this function, so I don't know what it's actually used for. > Why do we need this function for pcc, and what are the names that > can be passed here? AFAICT the names he's interested in will be defined by the ACPI specs. It does seem like we should be providing a device for the controller and then either using references in ACPI to look it up if they exist or a lookup function for this particular namespace that goes and fetches the device we created and looks up in its context.
Hi Arnd, On 28 August 2014 04:39, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 27 August 2014 20:09:02 Mark Brown wrote: >> On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote: >> > On 27 August 2014 06:27, Mark Brown <broonie@kernel.org> wrote: >> > > On Tue, Aug 26, 2014 at 03:35:36PM -0400, Ashwin Chaugule wrote: >> >> > >> +static struct mbox_controller * >> > >> +mbox_find_pcc_controller(char *name) >> > >> +{ >> > >> + struct mbox_controller *mbox; >> > >> + list_for_each_entry(mbox, &mbox_cons, node) { >> > >> + if (mbox->name) >> > >> + if (!strcmp(mbox->name, name)) >> > >> + return mbox; >> > >> + } >> > >> + >> > >> + return NULL; >> > >> +} >> >> > > This doesn't look particularly PCC specific? >> >> > Call this mbox_find_controller_by_name() instead? >> >> That certainly looks like what it's doing. Probably also make the name >> that gets passed in const while you're at it. > > The mailbox API intentionally does not have an interface for > that: you are supposed to get a reference to an mbox controller > from a phandle or similar, not by knowing the name of the controller. This snippet is based off of your suggestions. [1] [2] :) > > Unfortunately, the three patches that Ashwin posted don't have a > caller for this function, mbox_find_pcc_controller() is called from pcc_mbox_request_channel() which is in this patch. > so I don't know what it's actually used for. > Why do we need this function for pcc, and what are the names that > can be passed here? > > Arnd [1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265395.html [2] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/266528.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 28 August 2014 06:10, Mark Brown <broonie@kernel.org> wrote: > On Wed, Aug 27, 2014 at 05:49:53PM -0400, Ashwin Chaugule wrote: >> On 27 August 2014 15:09, Mark Brown <broonie@kernel.org> wrote: >> > On Wed, Aug 27, 2014 at 09:07:15AM -0400, Ashwin Chaugule wrote: >> >> "device" out of something that isn't. The PCCT, which is used as a >> >> mailbox controller here, is a table and not a peripheral device. To >> >> treat this as a device (without faking it by manually putting together >> >> a struct device), would require adding a DSDT entry which is really a >> >> wrong place for it. Are there examples today where drivers manually >> >> create a struct driver and struct device and match them internally? >> >> (i.e. w/o using the generic driver subsystem) > >> > Arguably that's what things like cpufreq end up doing, though people >> > tend to just shove a device into DT. Are you sure there isn't any >> > device at all in ACPI that you could hang this off, looking at my >> > desktop I see rather a lot of apparently synthetic ACPI devices with >> > names starting LNX including for example LNXSYSTM:00? > >> Those are special HIDs defined in the ACPI spec and none of those can >> be used to back a device for the PCCT itself, since they're unrelated >> to the PCC protocol. The PCCT is defined in the spec as a separate >> table and if supported, should be visible in your system in the >> PCCT.dsl file. Just for the sake of the Mailbox framework, trying to >> represent the PCCT (which is a table) as a mailbox controller device, >> would require registering a special HID. That in turn would make an >> otherwise OS agnostic thing very Linux specific. > > OK, but then there's always the option of just having some code that > runs on init and instantiates a device if it sees the appropriate thing > in the ACPI tables in a similar manner to how HIDs are handled. It's a > small amount of work but it will generally make life easier if there is > a struct device. I need to give this a try. AFAICS, the HIDs get their device created by the driver subsystem. This would require manually creating one and I didn't see existing examples. Thinking of a table as a device (virtual/real) seemed weird to me, especially since we're seemingly only using it for ref counts here. But it sounds like this is an acceptable thing. > >> > If PCC is described by ACPI tables how would non-ACPI users be able to >> > use it? > >> Whoever wants to do that, would need to come up with DT bindings that >> describe something similar to the PCCT contents. They could possibly >> ignore the ACPI specific bits like signature, asl compiler details >> etc. (which are only used by ACPI table parsers) and provide the rest >> of it. Its essentially an array of structures that point to various >> shared memory regions, each of which is owned by a PCC client and >> shared with the firmware. The intercommunication between client and >> firmware is via a doorbell, which is also described in these entries >> and can be implemented as an SGI or similar. > > Of course most likely such a binding would involve creating a device > that owns the mailboxes so this'd be fairly straightforward... With DT, yes, it seems to be a bit more straightforward. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 August 2014 06:15, Mark Brown <broonie@kernel.org> wrote: > On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote: >> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote: > >> > That certainly looks like what it's doing. Probably also make the name >> > that gets passed in const while you're at it. > >> The mailbox API intentionally does not have an interface for >> that: you are supposed to get a reference to an mbox controller >> from a phandle or similar, not by knowing the name of the controller. > > Right, and what he's trying to work around here is that ACPI has chosen > to provide a generic binding for some mailboxes which isn't associated > with anything we represent as a device and he doesn't want to provide > that device as a Linux virtual thing. Just the idea of a table as a device, when it doesn't do any power management, hotplug or anything like a device seemed strange. But I'm open to ideas if we find a good solution. Its highly possible that I'm not seeing it the way you are because the driver subsystem internals are fairly new to me. :) Suppose we create a platform_device for the PCCT (mailbox controller) and another one for the PCC client (mailbox client). How should the PCC client(s) identify the mailbox controller without passing a name? In DT, the "mboxes" field in the client DT entry is all strings with mailbox controller names. The "index" in mbox_request_channel() picks up one set of strings. How should this work with PCC? Should we use the PCC client platform_device->dev->platform_data to store mailbox controller strings? > >> Unfortunately, the three patches that Ashwin posted don't have a >> caller for this function, so I don't know what it's actually used for. >> Why do we need this function for pcc, and what are the names that >> can be passed here? > > AFAICT the names he's interested in will be defined by the ACPI specs. > It does seem like we should be providing a device for the controller and > then either using references in ACPI to look it up if they exist or a > lookup function for this particular namespace that goes and fetches the > device we created and looks up in its context. What is the comparison in this lookup function? A string or a struct device pointer? If it is the latter, how does the client get the reference to the controller struct device? One way would be to register the PCCT as a platform_device and the PCC client as its platform_driver. But I think that will restrict the number of PCC clients to who ever matches first. I suspect this is not what you're implying, so I'd appreciate some more help. Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, Arnd, On 28 August 2014 16:34, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > On 28 August 2014 06:15, Mark Brown <broonie@kernel.org> wrote: >> On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote: >>> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote: >> >>> > That certainly looks like what it's doing. Probably also make the name >>> > that gets passed in const while you're at it. >> >>> The mailbox API intentionally does not have an interface for >>> that: you are supposed to get a reference to an mbox controller >>> from a phandle or similar, not by knowing the name of the controller. >> >> Right, and what he's trying to work around here is that ACPI has chosen >> to provide a generic binding for some mailboxes which isn't associated >> with anything we represent as a device and he doesn't want to provide >> that device as a Linux virtual thing. > > Just the idea of a table as a device, when it doesn't do any power > management, hotplug or anything like a device seemed strange. But I'm > open to ideas if we find a good solution. Its highly possible that I'm > not seeing it the way you are because the driver subsystem internals > are fairly new to me. :) > > Suppose we create a platform_device for the PCCT (mailbox controller) > and another one for the PCC client (mailbox client). How should the > PCC client(s) identify the mailbox controller without passing a name? > In DT, the "mboxes" field in the client DT entry is all strings with > mailbox controller names. The "index" in mbox_request_channel() picks > up one set of strings. How should this work with PCC? Should we use > the PCC client platform_device->dev->platform_data to store mailbox > controller strings? > >> >>> Unfortunately, the three patches that Ashwin posted don't have a >>> caller for this function, so I don't know what it's actually used for. >>> Why do we need this function for pcc, and what are the names that >>> can be passed here? >> >> AFAICT the names he's interested in will be defined by the ACPI specs. >> It does seem like we should be providing a device for the controller and >> then either using references in ACPI to look it up if they exist or a >> lookup function for this particular namespace that goes and fetches the >> device we created and looks up in its context. > > What is the comparison in this lookup function? A string or a struct > device pointer? If it is the latter, how does the client get the > reference to the controller struct device? One way would be to > register the PCCT as a platform_device and the PCC client as its > platform_driver. But I think that will restrict the number of PCC > clients to who ever matches first. I suspect this is not what you're > implying, so I'd appreciate some more help. I dont see a way to create a lookup table for PCC without storing the name of the controller somewhere. The suggestion of creating a platform device for the controller and client led to restricting only one client to the controller. Can you please suggest how to move this forward? Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 02 September 2014 14:16:42 Ashwin Chaugule wrote: > On 28 August 2014 16:34, Ashwin Chaugule <ashwin.chaugule@linaro.org> wrote: > > On 28 August 2014 06:15, Mark Brown <broonie@kernel.org> wrote: > >> On Thu, Aug 28, 2014 at 10:39:01AM +0200, Arnd Bergmann wrote: > >>> On Wednesday 27 August 2014 20:09:02 Mark Brown wrote: > >> > >>> > That certainly looks like what it's doing. Probably also make the name > >>> > that gets passed in const while you're at it. > >> > >>> The mailbox API intentionally does not have an interface for > >>> that: you are supposed to get a reference to an mbox controller > >>> from a phandle or similar, not by knowing the name of the controller. > >> > >> Right, and what he's trying to work around here is that ACPI has chosen > >> to provide a generic binding for some mailboxes which isn't associated > >> with anything we represent as a device and he doesn't want to provide > >> that device as a Linux virtual thing. > > > > Just the idea of a table as a device, when it doesn't do any power > > management, hotplug or anything like a device seemed strange. But I'm > > open to ideas if we find a good solution. Its highly possible that I'm > > not seeing it the way you are because the driver subsystem internals > > are fairly new to me. > > > > Suppose we create a platform_device for the PCCT (mailbox controller) > > and another one for the PCC client (mailbox client). How should the > > PCC client(s) identify the mailbox controller without passing a name? > > In DT, the "mboxes" field in the client DT entry is all strings with > > mailbox controller names. No, it's not a string at all, it's a phandle, which is more like a pointer. We intentionally never match by a global string at all, because those might not be unique. > > The "index" in mbox_request_channel() picks > > up one set of strings. How should this work with PCC? Should we use > > the PCC client platform_device->dev->platform_data to store mailbox > > controller strings? I didn't think there was more than one PCC provider, why do you even need a string? For the general case in ACPI, there should be a similar way of looking up mailbox providers to what we have in DT, but if I understand you correctly, the PCC specification does not allow that. Using platform_data would no be helpful, because there is no platform code to fill that out on ACPI based systems. > >>> Unfortunately, the three patches that Ashwin posted don't have a > >>> caller for this function, so I don't know what it's actually used for. > >>> Why do we need this function for pcc, and what are the names that > >>> can be passed here? > >> > >> AFAICT the names he's interested in will be defined by the ACPI specs. > >> It does seem like we should be providing a device for the controller and > >> then either using references in ACPI to look it up if they exist or a > >> lookup function for this particular namespace that goes and fetches the > >> device we created and looks up in its context. > > > > What is the comparison in this lookup function? A string or a struct > > device pointer? If it is the latter, how does the client get the > > reference to the controller struct device? One way would be to > > register the PCCT as a platform_device and the PCC client as its > > platform_driver. But I think that will restrict the number of PCC > > clients to who ever matches first. I suspect this is not what you're > > implying, so I'd appreciate some more help. > > I dont see a way to create a lookup table for PCC without storing the > name of the controller somewhere. The suggestion of creating a > platform device for the controller and client led to restricting only > one client to the controller. Can you please suggest how to move this > forward? I've forgotten the details, but I thought we had already worked it out when we discussed it the last time. What is the information available to the client driver? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On 2 September 2014 15:22, Arnd Bergmann <arnd@arndb.de> wrote: >> > Suppose we create a platform_device for the PCCT (mailbox controller) >> > and another one for the PCC client (mailbox client). How should the >> > PCC client(s) identify the mailbox controller without passing a name? >> > In DT, the "mboxes" field in the client DT entry is all strings with >> > mailbox controller names. > > No, it's not a string at all, it's a phandle, which is more like a > pointer. We intentionally never match by a global string at all, > because those might not be unique. Ok. With PCC, I dont see a way to do this sort of pointer matching. > >> > The "index" in mbox_request_channel() picks >> > up one set of strings. How should this work with PCC? Should we use >> > the PCC client platform_device->dev->platform_data to store mailbox >> > controller strings? > > I didn't think there was more than one PCC provider, why do you even > need a string? > > For the general case in ACPI, there should be a similar way of looking > up mailbox providers to what we have in DT, but if I understand you > correctly, the PCC specification does not allow that. Right. At least not in a way DT does. PCC clients know if something needs to be written/read via PCC mailbox and can identify a PCC subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely identified/defined in the spec. #define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10 So we could use this ID instead of a string and use that to look up the PCC controller for a PCC client. > > Using platform_data would no be helpful, because there is no platform > code to fill that out on ACPI based systems. Right. So the question is how do we work around the "mbox->dev" and "client->dev" expectations in the Mailbox framework for PCC, given that these tables aren't backed by "struct devices" ? Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote: > > Using platform_data would no be helpful, because there is no platform > > code to fill that out on ACPI based systems. > Right. So the question is how do we work around the "mbox->dev" and > "client->dev" expectations in the Mailbox framework for PCC, given > that these tables aren't backed by "struct devices" ? As previously suggested just looking things up in the context of a device created to represent the PCC controller seems fine; clients know they're using PCC so can just call a PCC specific API which hides the mechanics from them (for example, using a global variable to store the device).
On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote: > > > >> > The "index" in mbox_request_channel() picks > >> > up one set of strings. How should this work with PCC? Should we use > >> > the PCC client platform_device->dev->platform_data to store mailbox > >> > controller strings? > > > > I didn't think there was more than one PCC provider, why do you even > > need a string? > > > > For the general case in ACPI, there should be a similar way of looking > > up mailbox providers to what we have in DT, but if I understand you > > correctly, the PCC specification does not allow that. > > Right. At least not in a way DT does. PCC clients know if something > needs to be written/read via PCC mailbox and can identify a PCC > subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely > identified/defined in the spec. > > #define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10 > > So we could use this ID instead of a string and use that to look up > the PCC controller for a PCC client. I didn't realize this was the case. Does that mean we can treat pcc as a linearly accessible address space the way we do for system memory, pci-config etc? If that works, we should probably just have a regmap for it rather than expose the mailbox API to client drivers. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 03, 2014 at 01:23:21PM +0200, Arnd Bergmann wrote: > On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote: > > Right. At least not in a way DT does. PCC clients know if something > > needs to be written/read via PCC mailbox and can identify a PCC > > subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely > > identified/defined in the spec. > > #define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10 > > So we could use this ID instead of a string and use that to look up > > the PCC controller for a PCC client. > I didn't realize this was the case. Does that mean we can treat > pcc as a linearly accessible address space the way we do for > system memory, pci-config etc? > If that works, we should probably just have a regmap for it rather > than expose the mailbox API to client drivers. A regmap doesn't seem to map very well here - as far as I can tell the addresses referred to are mailboxes rather than registers or memory addresses. I could be misunderstanding though.
On Wednesday 03 September 2014 15:49:21 Mark Brown wrote: > On Wed, Sep 03, 2014 at 01:23:21PM +0200, Arnd Bergmann wrote: > > On Tuesday 02 September 2014 16:15:05 Ashwin Chaugule wrote: > > > > Right. At least not in a way DT does. PCC clients know if something > > > needs to be written/read via PCC mailbox and can identify a PCC > > > subspace. (i.e. Mailbox channel). The PCC mailbox is uniquely > > > identified/defined in the spec. > > > > #define ACPI_ADR_SPACE_PLATFORM_COMM (acpi_adr_space_type) 10 > > > > So we could use this ID instead of a string and use that to look up > > > the PCC controller for a PCC client. > > > I didn't realize this was the case. Does that mean we can treat > > pcc as a linearly accessible address space the way we do for > > system memory, pci-config etc? > > > If that works, we should probably just have a regmap for it rather > > than expose the mailbox API to client drivers. > > A regmap doesn't seem to map very well here - as far as I can tell the > addresses referred to are mailboxes rather than registers or memory > addresses. I could be misunderstanding though. No, I think you are right. Nevermind then. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote: > On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote: > >> > Using platform_data would no be helpful, because there is no platform >> > code to fill that out on ACPI based systems. > >> Right. So the question is how do we work around the "mbox->dev" and >> "client->dev" expectations in the Mailbox framework for PCC, given >> that these tables aren't backed by "struct devices" ? > > As previously suggested just looking things up in the context of a > device created to represent the PCC controller seems fine; clients know > they're using PCC so can just call a PCC specific API which hides the > mechanics from them (for example, using a global variable to store the > device). IIUC, this means, either modifying the existing mbox_controller_register() to know when a PCC mbox is being added, or have another PCC specific API for registering as a mailbox? Then, in the PCC specific API that requests a PCC channel, depending on what we do in the registration path, this function can pick out the PCC mailbox pointer and try_module_get(mbox->dev..). Since this is PCC specific anyway, we don't need the client->dev argument at all. Did I understand you correctly? Thanks, Ashwin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 03 September 2014 11:23:21 Ashwin Chaugule wrote: > On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote: > > On Tue, Sep 02, 2014 at 04:15:05PM -0400, Ashwin Chaugule wrote: > > > >> > Using platform_data would no be helpful, because there is no platform > >> > code to fill that out on ACPI based systems. > > > >> Right. So the question is how do we work around the "mbox->dev" and > >> "client->dev" expectations in the Mailbox framework for PCC, given > >> that these tables aren't backed by "struct devices" ? > > > > As previously suggested just looking things up in the context of a > > device created to represent the PCC controller seems fine; clients know > > they're using PCC so can just call a PCC specific API which hides the > > mechanics from them (for example, using a global variable to store the > > device). > > IIUC, this means, either modifying the existing > mbox_controller_register() to know when a PCC mbox is being added, or > have another PCC specific API for registering as a mailbox? Then, in > the PCC specific API that requests a PCC channel, depending on what we > do in the registration path, this function can pick out the PCC > mailbox pointer and try_module_get(mbox->dev..). Since this is PCC > specific anyway, we don't need the client->dev argument at all. Did I > understand you correctly? Yes, I think this would be reasonable. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote: > On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote: > > As previously suggested just looking things up in the context of a > > device created to represent the PCC controller seems fine; clients know > > they're using PCC so can just call a PCC specific API which hides the > > mechanics from them (for example, using a global variable to store the > > device). > IIUC, this means, either modifying the existing > mbox_controller_register() to know when a PCC mbox is being added, or > have another PCC specific API for registering as a mailbox? Then, in Why would you have to do that - can't the PCC specific API manage to hide this?
On Wednesday 03 September 2014 16:36:01 Mark Brown wrote: > On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote: > > On 2 September 2014 19:03, Mark Brown <broonie@kernel.org> wrote: > > > > As previously suggested just looking things up in the context of a > > > device created to represent the PCC controller seems fine; clients know > > > they're using PCC so can just call a PCC specific API which hides the > > > mechanics from them (for example, using a global variable to store the > > > device). > > > IIUC, this means, either modifying the existing > > mbox_controller_register() to know when a PCC mbox is being added, or > > have another PCC specific API for registering as a mailbox? Then, in > > Why would you have to do that - can't the PCC specific API manage to > hide this? I think that's what Ashwin meant as the 'or' part of the sentence above. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 03, 2014 at 05:41:20PM +0200, Arnd Bergmann wrote: > On Wednesday 03 September 2014 16:36:01 Mark Brown wrote: > > On Wed, Sep 03, 2014 at 11:23:21AM -0400, Ashwin Chaugule wrote: > > > IIUC, this means, either modifying the existing > > > mbox_controller_register() to know when a PCC mbox is being added, or > > > have another PCC specific API for registering as a mailbox? Then, in > > Why would you have to do that - can't the PCC specific API manage to > > hide this? > I think that's what Ashwin meant as the 'or' part of the sentence above. So it is.
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c index 63ecc17..09ad488 100644 --- a/drivers/mailbox/mailbox.c +++ b/drivers/mailbox/mailbox.c @@ -356,6 +356,14 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index) } EXPORT_SYMBOL_GPL(mbox_request_channel); +static void inline free_channel(struct mbox_chan *chan) +{ + chan->cl = NULL; + chan->active_req = NULL; + if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) + chan->txdone_method = TXDONE_BY_POLL; +} + /** * mbox_free_channel - The client relinquishes control of a mailbox * channel by this call. @@ -369,14 +377,9 @@ void mbox_free_channel(struct mbox_chan *chan) return; chan->mbox->ops->shutdown(chan); - /* The queued TX requests are simply aborted, no callbacks are made */ spin_lock_irqsave(&chan->lock, flags); - chan->cl = NULL; - chan->active_req = NULL; - if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK)) - chan->txdone_method = TXDONE_BY_POLL; - + free_channel(chan); module_put(chan->mbox->dev->driver->owner); spin_unlock_irqrestore(&chan->lock, flags); } @@ -394,6 +397,97 @@ of_mbox_index_xlate(struct mbox_controller *mbox, return &mbox->chans[ind]; } +#ifdef CONFIG_PCC +/* + * The PCC (Platform Communication Channel) is + * defined in the ACPI 5.0+ spec. It is a generic + * mailbox interface between an OS and a Platform + * such as a BMC. The PCCT (Mailbox controller) has + * its own ACPI specific way to describe PCC clients + * and their subspace ids (Mailbox channels/clients). + * + * The following API is added such that PCC + * drivers continue to work with this Mailbox + * framework with or without ACPI. + */ + +static struct mbox_controller * +mbox_find_pcc_controller(char *name) +{ + struct mbox_controller *mbox; + list_for_each_entry(mbox, &mbox_cons, node) { + if (mbox->name) + if (!strcmp(mbox->name, name)) + return mbox; + } + + return NULL; +} + +struct mbox_chan * +pcc_mbox_request_channel(struct mbox_client *cl, + char *name, unsigned chan_id) +{ + struct mbox_controller *mbox; + struct mbox_chan *pcc_chan; + unsigned long flags; + int ret; + + mutex_lock(&con_mutex); + mbox = mbox_find_pcc_controller(name); + + if (!mbox) { + pr_err("PCC mbox %s not found.\n", name); + mutex_unlock(&con_mutex); + return ERR_PTR(-ENODEV); + } + + pcc_chan = &mbox->chans[chan_id]; + + spin_lock_irqsave(&pcc_chan->lock, flags); + pcc_chan->msg_free = 0; + pcc_chan->msg_count = 0; + pcc_chan->active_req = NULL; + pcc_chan->cl = cl; + init_completion(&pcc_chan->tx_complete); + + if (pcc_chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone) + pcc_chan->txdone_method |= TXDONE_BY_ACK; + + spin_unlock_irqrestore(&pcc_chan->lock, flags); + + ret = pcc_chan->mbox->ops->startup(pcc_chan); + if (ret) { + pr_err("Unable to startup the PCC channel (%d)\n", ret); + mbox_free_channel(pcc_chan); + mutex_unlock(&con_mutex); + return ERR_PTR(-ENODEV); + } + + mutex_unlock(&con_mutex); + + return pcc_chan; +} + +EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); + +void pcc_mbox_free_channel(struct mbox_chan *chan) +{ + unsigned long flags; + + if (!chan || !chan->cl) + return; + + chan->mbox->ops->shutdown(chan); + + spin_lock_irqsave(&chan->lock, flags); + free_channel(chan); + spin_unlock_irqrestore(&chan->lock, flags); +} +EXPORT_SYMBOL_GPL(pcc_mbox_free_channel); + +#endif + /** * mbox_controller_register - Register the mailbox controller * @mbox: Pointer to the mailbox controller. @@ -405,7 +499,17 @@ int mbox_controller_register(struct mbox_controller *mbox) int i, txdone; /* Sanity check */ - if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans) + + /* + * PCC clients and controllers are currently not backed by + * platform device structures. + */ +#ifndef CONFIG_PCC + if (!mbox->dev) + return -EINVAL; +#endif + + if (!mbox || !mbox->ops || !mbox->num_chans) return -EINVAL; if (mbox->txdone_irq) diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h index 307d9ca..6a78df0 100644 --- a/include/linux/mailbox_client.h +++ b/include/linux/mailbox_client.h @@ -37,6 +37,12 @@ struct mbox_client { void (*tx_done)(struct mbox_client *cl, void *mssg, int r); }; +#ifdef CONFIG_PCC +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *c, + char *name, unsigned int chan_id); +void pcc_mbox_free_channel(struct mbox_chan *chan); /* may sleep */ +#endif + struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index); int mbox_send_message(struct mbox_chan *chan, void *mssg); void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */ diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h index 9ee195b..9f0ae42 100644 --- a/include/linux/mailbox_controller.h +++ b/include/linux/mailbox_controller.h @@ -81,6 +81,7 @@ struct mbox_controller { unsigned txpoll_period; struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox, const struct of_phandle_args *sp); + char *name; /* Internal to API */ struct timer_list poll; unsigned period;
The PCC (Platform Communication Channel) is a generic mailbox to be used by PCC clients such as CPPC, RAS and MPST as defined in the ACPI 5.0+ spec. This patch modifies the Mailbox framework to lookup PCC mailbox controllers and channels such that PCC drivers can work with or without ACPI support in the kernel. Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org> --- drivers/mailbox/mailbox.c | 118 ++++++++++++++++++++++++++++++++++--- include/linux/mailbox_client.h | 6 ++ include/linux/mailbox_controller.h | 1 + 3 files changed, 118 insertions(+), 7 deletions(-)