diff mbox

[v7,8/8] PCC: Enable PCC only when needed

Message ID f88f374b4e09f23d5601a2081d1d3d9322e3caf5.1436464513.git.ashwin.chaugule@linaro.org (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Ashwin Chaugule July 9, 2015, 6:04 p.m. UTC 
CPPC is the first client to make use of the PCC Mailbox channel. So
enable it only when CPPC is also enabled.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Reviewed-by: Al Stone <al.stone@linaro.org>
---
 drivers/mailbox/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sudeep Holla July 20, 2015, 2:22 p.m. UTC | #1 
On 09/07/15 19:04, Ashwin Chaugule wrote:
> CPPC is the first client to make use of the PCC Mailbox channel. So
> enable it only when CPPC is also enabled.
>
This sounds like a reverse dependency to me. So if there's some client
unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 20, 2015, 10:04 p.m. UTC | #2 
On Monday, July 20, 2015 03:22:37 PM Sudeep Holla wrote:
> 
> On 09/07/15 19:04, Ashwin Chaugule wrote:
> > CPPC is the first client to make use of the PCC Mailbox channel. So
> > enable it only when CPPC is also enabled.
> >
> This sounds like a reverse dependency to me. So if there's some client
> unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC ?

No.  The other client will need to select PCC too.

I requested that change, because I'm slightly bothered by the fact that we
build code used by no one by default.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla July 21, 2015, 9:23 a.m. UTC | #3 
On 20/07/15 23:04, Rafael J. Wysocki wrote:
> On Monday, July 20, 2015 03:22:37 PM Sudeep Holla wrote:
>>
>> On 09/07/15 19:04, Ashwin Chaugule wrote:
>>> CPPC is the first client to make use of the PCC Mailbox channel. So
>>> enable it only when CPPC is also enabled.
>>>
>> This sounds like a reverse dependency to me. So if there's some client
>> unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC ?
>
> No.  The other client will need to select PCC too.

Yes the PCC users/clients selecting PCC is fine and that's already
done(i.e. ACPI_CPPC_LIB selects PCC). I still don't understand the need
for this change, also how will other clients possibly select PCC which
now depends on CPPC_LIB ? e.g. if we have

config ACPI_XYZ_LIB
	select PCC

config ACPI_XYZ
	select ACPI_XYZ_LIB

Won't this shout warning: (ACPI_XYZ_LIB && ACPI_CPPC_LIB) selects PCC
which has unmet direct dependencies (MAILBOX && ACPI && ACPI_CPPC_LIB)
if ACPI_CPPC_LIB is not selected ?

OK, for now we enable ACPI_CPPC_LIB on ARM64 and not on x86. When x86
has a PCC client how will that select PCC without ACPI_CPPC_LIB. Sorry
if I am missing to understand something.

>
> I requested that change, because I'm slightly bothered by the fact that we
> build code used by no one by default.
>

I understand, but will keeping them default off should suffice ? No ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 21, 2015, 2:34 p.m. UTC | #4 
Hi Sudeep,

On Tue, Jul 21, 2015 at 11:23 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 20/07/15 23:04, Rafael J. Wysocki wrote:
>>
>> On Monday, July 20, 2015 03:22:37 PM Sudeep Holla wrote:
>>>
>>>
>>> On 09/07/15 19:04, Ashwin Chaugule wrote:
>>>>
>>>> CPPC is the first client to make use of the PCC Mailbox channel. So
>>>> enable it only when CPPC is also enabled.
>>>>
>>> This sounds like a reverse dependency to me. So if there's some client
>>> unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC
>>> ?
>>
>>
>> No.  The other client will need to select PCC too.
>
>
> Yes the PCC users/clients selecting PCC is fine and that's already
> done(i.e. ACPI_CPPC_LIB selects PCC). I still don't understand the need
> for this change, also how will other clients possibly select PCC which
> now depends on CPPC_LIB ? e.g. if we have
>
> config ACPI_XYZ_LIB
>         select PCC
>
> config ACPI_XYZ
>         select ACPI_XYZ_LIB
>
> Won't this shout warning: (ACPI_XYZ_LIB && ACPI_CPPC_LIB) selects PCC
> which has unmet direct dependencies (MAILBOX && ACPI && ACPI_CPPC_LIB)
> if ACPI_CPPC_LIB is not selected ?

That depends on the "depends on" clauses used.  Selecting itself
doesn't cause any dependencies to appear.

> OK, for now we enable ACPI_CPPC_LIB on ARM64 and not on x86. When x86
> has a PCC client how will that select PCC without ACPI_CPPC_LIB. Sorry
> if I am missing to understand something.

Presumably, the new feature will have a Kconfig option associated with
it and it will do "select PCC" too.

>>
>> I requested that change, because I'm slightly bothered by the fact that we
>> build code used by no one by default.
>>
>
> I understand, but will keeping them default off should suffice ? No ?

Yes, it should.  But is it the case now?

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla July 21, 2015, 3:28 p.m. UTC | #5 
Hi Rafael,

On 21/07/15 15:34, Rafael J. Wysocki wrote:
> Hi Sudeep,
>
> On Tue, Jul 21, 2015 at 11:23 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 20/07/15 23:04, Rafael J. Wysocki wrote:
>>>
>>> On Monday, July 20, 2015 03:22:37 PM Sudeep Holla wrote:
>>>>
>>>>
>>>> On 09/07/15 19:04, Ashwin Chaugule wrote:
>>>>>
>>>>> CPPC is the first client to make use of the PCC Mailbox channel. So
>>>>> enable it only when CPPC is also enabled.
>>>>>
>>>> This sounds like a reverse dependency to me. So if there's some client
>>>> unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC
>>>> ?
>>>
>>>
>>> No.  The other client will need to select PCC too.
>>
>>
>> Yes the PCC users/clients selecting PCC is fine and that's already
>> done(i.e. ACPI_CPPC_LIB selects PCC). I still don't understand the need
>> for this change, also how will other clients possibly select PCC which
>> now depends on CPPC_LIB ? e.g. if we have
>>
>> config ACPI_XYZ_LIB
>>          select PCC
>>
>> config ACPI_XYZ
>>          select ACPI_XYZ_LIB
>>
>> Won't this shout warning: (ACPI_XYZ_LIB && ACPI_CPPC_LIB) selects PCC
>> which has unmet direct dependencies (MAILBOX && ACPI && ACPI_CPPC_LIB)
>> if ACPI_CPPC_LIB is not selected ?
>
> That depends on the "depends on" clauses used.  Selecting itself
> doesn't cause any dependencies to appear.
>

Agreed and I am absolutely fine with that. But if you look at this 
patch, it does

config PCC
	bool "Platform Communication Channel Driver"
	depends on ACPI && ACPI_CPPC_LIB

I am fine with ACPI_CPPC_LIB selecting PCC which is already done in
earlier patch. I am against making PCC depend on ACPI_CPPC_LIB.

Lets assume it's fine to do that. Now if another feature XYZ as in my
example say on x86 selects PCC then you will *get warnings* as we will
not able to select PCC without selecting ACPI_CPPC_LIB after this patch
and x86 will never select ACPI_CPPC_LIB. I did test something like my
XYZ example and it did trigger the warning as I suspected.

I am sorry either I am missing to understand again or unable to express
the problem here :(

>> OK, for now we enable ACPI_CPPC_LIB on ARM64 and not on x86. When x86
>> has a PCC client how will that select PCC without ACPI_CPPC_LIB. Sorry
>> if I am missing to understand something.
>
> Presumably, the new feature will have a Kconfig option associated with
> it and it will do "select PCC" too.
>

As I say I am fine with that and no arguments there.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki July 22, 2015, 1:28 a.m. UTC | #6 
On Tuesday, July 21, 2015 04:28:43 PM Sudeep Holla wrote:
> Hi Rafael,
> 
> On 21/07/15 15:34, Rafael J. Wysocki wrote:
> > Hi Sudeep,
> >
> > On Tue, Jul 21, 2015 at 11:23 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>
> >>
> >> On 20/07/15 23:04, Rafael J. Wysocki wrote:
> >>>
> >>> On Monday, July 20, 2015 03:22:37 PM Sudeep Holla wrote:
> >>>>
> >>>>
> >>>> On 09/07/15 19:04, Ashwin Chaugule wrote:
> >>>>>
> >>>>> CPPC is the first client to make use of the PCC Mailbox channel. So
> >>>>> enable it only when CPPC is also enabled.
> >>>>>
> >>>> This sounds like a reverse dependency to me. So if there's some client
> >>>> unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC
> >>>> ?
> >>>
> >>>
> >>> No.  The other client will need to select PCC too.
> >>
> >>
> >> Yes the PCC users/clients selecting PCC is fine and that's already
> >> done(i.e. ACPI_CPPC_LIB selects PCC). I still don't understand the need
> >> for this change, also how will other clients possibly select PCC which
> >> now depends on CPPC_LIB ? e.g. if we have
> >>
> >> config ACPI_XYZ_LIB
> >>          select PCC
> >>
> >> config ACPI_XYZ
> >>          select ACPI_XYZ_LIB
> >>
> >> Won't this shout warning: (ACPI_XYZ_LIB && ACPI_CPPC_LIB) selects PCC
> >> which has unmet direct dependencies (MAILBOX && ACPI && ACPI_CPPC_LIB)
> >> if ACPI_CPPC_LIB is not selected ?
> >
> > That depends on the "depends on" clauses used.  Selecting itself
> > doesn't cause any dependencies to appear.
> >
> 
> Agreed and I am absolutely fine with that. But if you look at this 
> patch, it does
> 
> config PCC
> 	bool "Platform Communication Channel Driver"
> 	depends on ACPI && ACPI_CPPC_LIB

My bad, I've evidently overlooked that.

If PPC is selected from ACPI_CPPC_LIB, the "depends on" above is
obviously not needed.

> I am fine with ACPI_CPPC_LIB selecting PCC which is already done in
> earlier patch. I am against making PCC depend on ACPI_CPPC_LIB.

OK, makes sense.  Sorry for the misunderstanding.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla July 22, 2015, 8:59 a.m. UTC | #7 
On 22/07/15 02:28, Rafael J. Wysocki wrote:
> On Tuesday, July 21, 2015 04:28:43 PM Sudeep Holla wrote:
>> Hi Rafael,
>>
>> On 21/07/15 15:34, Rafael J. Wysocki wrote:
>>> Hi Sudeep,
>>>
>>> On Tue, Jul 21, 2015 at 11:23 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>>
>>>>
>>>> On 20/07/15 23:04, Rafael J. Wysocki wrote:

[..]

>>>>>
>>>>>
>>>>> No.  The other client will need to select PCC too.
>>>>
>>>>
>>>> Yes the PCC users/clients selecting PCC is fine and that's already
>>>> done(i.e. ACPI_CPPC_LIB selects PCC). I still don't understand the need
>>>> for this change, also how will other clients possibly select PCC which
>>>> now depends on CPPC_LIB ? e.g. if we have
>>>>
>>>> config ACPI_XYZ_LIB
>>>>           select PCC
>>>>
>>>> config ACPI_XYZ
>>>>           select ACPI_XYZ_LIB
>>>>
>>>> Won't this shout warning: (ACPI_XYZ_LIB && ACPI_CPPC_LIB) selects PCC
>>>> which has unmet direct dependencies (MAILBOX && ACPI && ACPI_CPPC_LIB)
>>>> if ACPI_CPPC_LIB is not selected ?
>>>
>>> That depends on the "depends on" clauses used.  Selecting itself
>>> doesn't cause any dependencies to appear.
>>>
>>
>> Agreed and I am absolutely fine with that. But if you look at this
>> patch, it does
>>
>> config PCC
>> 	bool "Platform Communication Channel Driver"
>> 	depends on ACPI && ACPI_CPPC_LIB
>
> My bad, I've evidently overlooked that.
>
> If PPC is selected from ACPI_CPPC_LIB, the "depends on" above is
> obviously not needed.
>

Thanks for the confirmation.

>> I am fine with ACPI_CPPC_LIB selecting PCC which is already done in
>> earlier patch. I am against making PCC depend on ACPI_CPPC_LIB.
>
> OK, makes sense.  Sorry for the misunderstanding.
>

It's okay, I just wanted to make sure that I am not missing to
understand some kind of dependency.

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ashwin Chaugule Aug. 3, 2015, 5:35 p.m. UTC | #8 
On 21 July 2015 at 21:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, July 21, 2015 04:28:43 PM Sudeep Holla wrote:
>> Hi Rafael,
>>
>> On 21/07/15 15:34, Rafael J. Wysocki wrote:
>> > Hi Sudeep,
>> >
>> > On Tue, Jul 21, 2015 at 11:23 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> >>
>> >>
>> >> On 20/07/15 23:04, Rafael J. Wysocki wrote:
>> >>>
>> >>> On Monday, July 20, 2015 03:22:37 PM Sudeep Holla wrote:
>> >>>>
>> >>>>
>> >>>> On 09/07/15 19:04, Ashwin Chaugule wrote:
>> >>>>>
>> >>>>> CPPC is the first client to make use of the PCC Mailbox channel. So
>> >>>>> enable it only when CPPC is also enabled.
>> >>>>>
>> >>>> This sounds like a reverse dependency to me. So if there's some client
>> >>>> unrelated to CPPC using PCC, CPPC_LIB needs to be selected to enable PCC
>> >>>> ?
>> >>>
>> >>>
>> >>> No.  The other client will need to select PCC too.
>> >>
>> >>
>> >> Yes the PCC users/clients selecting PCC is fine and that's already
>> >> done(i.e. ACPI_CPPC_LIB selects PCC). I still don't understand the need
>> >> for this change, also how will other clients possibly select PCC which
>> >> now depends on CPPC_LIB ? e.g. if we have
>> >>
>> >> config ACPI_XYZ_LIB
>> >>          select PCC
>> >>
>> >> config ACPI_XYZ
>> >>          select ACPI_XYZ_LIB
>> >>
>> >> Won't this shout warning: (ACPI_XYZ_LIB && ACPI_CPPC_LIB) selects PCC
>> >> which has unmet direct dependencies (MAILBOX && ACPI && ACPI_CPPC_LIB)
>> >> if ACPI_CPPC_LIB is not selected ?
>> >
>> > That depends on the "depends on" clauses used.  Selecting itself
>> > doesn't cause any dependencies to appear.
>> >
>>
>> Agreed and I am absolutely fine with that. But if you look at this
>> patch, it does
>>
>> config PCC
>>       bool "Platform Communication Channel Driver"
>>       depends on ACPI && ACPI_CPPC_LIB
>
> My bad, I've evidently overlooked that.
>
> If PPC is selected from ACPI_CPPC_LIB, the "depends on" above is
> obviously not needed.

Thanks Sudeep.
So I'll take this dependency out and default PCC to 'n'. Think that
should work for all cases. Agree?

Thanks,
Ashwin.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla Aug. 4, 2015, 2:53 p.m. UTC | #9 
On 03/08/15 18:35, Ashwin Chaugule wrote:
> On 21 July 2015 at 21:28, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Tuesday, July 21, 2015 04:28:43 PM Sudeep Holla wrote:
>>> Hi Rafael,
>>>

[...]

>>>
>>> Agreed and I am absolutely fine with that. But if you look at this
>>> patch, it does
>>>
>>> config PCC
>>>        bool "Platform Communication Channel Driver"
>>>        depends on ACPI && ACPI_CPPC_LIB
>>
>> My bad, I've evidently overlooked that.
>>
>> If PPC is selected from ACPI_CPPC_LIB, the "depends on" above is
>> obviously not needed.
>
> Thanks Sudeep.
> So I'll take this dependency out and default PCC to 'n'. Think that
> should work for all cases. Agree?
>

Yes

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84b0a2d..f2c30cc 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -45,7 +45,7 @@  config OMAP_MBOX_KFIFO_SIZE
 
 config PCC
 	bool "Platform Communication Channel Driver"
-	depends on ACPI
+	depends on ACPI && ACPI_CPPC_LIB
 	help
 	  ACPI 5.0+ spec defines a generic mode of communication
 	  between the OS and a platform such as the BMC. This medium