firmware: arm_scpi: Add hardware dependencies
PATCH 1/1 firmware: arm_scpi: Add hardware dependencies
With a name like that, I assume that the ARM SCPI protocol is only useful on the ARM architectures.
---
Please correct me if I'm wrong.
drivers/firmware/Kconfig | 1 + 1 file changed, 1 insertion(+)
drivers/firmware/Kconfig +1 -0
@@ -21,6 +21,7 @@ config ARM_PSCI_CHECKER config ARM_SCPI_PROTOCOL tristate "ARM System Control and Power Interface (SCPI) Message Protocol" + depends on ARM || ARM64 || COMPILE_TEST depends on MAILBOX help System Control and Power Interface (SCPI) Message Protocol is
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 10 lines · Jean Delvare
With a name like that, I assume that the ARM SCPI protocol is only useful on the ARM architectures.
Signed-off-by Jean Delvare <[email protected]>Fixes 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")Cc Sudeep Holla <[email protected]>Cc Jon Medhurst (Tixy) <[email protected]>---
Please correct me if I'm wrong.
I won't say you are wrong but the reason why it's named arm_scpi is because the protocol was developed by ARM. It doesnn't mean only ARM/ARM64 needs to use it, it can be used on any architecture for inter-processor communication using any communication technique (currently mailbox is the only supported in the driver)
Signature · 2 lines
Regards, Sudeep
Quoted 11 lines · Sudeep Holla
Quoted 9 lines · Jean Delvare
With a name like that, I assume that the ARM SCPI protocol is only useful on the ARM architectures.
Signed-off-by Jean Delvare <[email protected]>Fixes 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")Cc Sudeep Holla <[email protected]>Cc Jon Medhurst (Tixy) <[email protected]>---
Please correct me if I'm wrong.
Quoted 5 lines
I won't say you are wrong but the reason why it's named
arm_scpiis because the protocol was developed by ARM. It doesnn't mean only ARM/ARM64 needs to use it, it can be used on any architecture for inter-processor communication using any communication technique (currently mailbox is the only supported in the driver)
OK, thanks for the clarification. In practice, what other architectures are using it?
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 12 lines · Jean Delvare
Quoted 11 lines · Sudeep Holla
Quoted 9 lines · Jean Delvare
With a name like that, I assume that the ARM SCPI protocol is only useful on the ARM architectures.
Signed-off-by Jean Delvare <[email protected]>Fixes 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")Cc Sudeep Holla <[email protected]>Cc Jon Medhurst (Tixy) <[email protected]>---
Please correct me if I'm wrong.
Quoted 9 lines
Quoted 5 lines
I won't say you are wrong but the reason why it's named
arm_scpiis because the protocol was developed by ARM. It doesnn't mean only ARM/ARM64 needs to use it, it can be used on any architecture for inter-processor communication using any communication technique (currently mailbox is the only supported in the driver)OK, thanks for the clarification. In practice, what other architectures are using it?
None, hence I didn't say you are wrong ;). I am fine having the check if it breaks for any other architecture with COMPILE_TEST.
Also you have mentioned it fixes 8f1498c03d15, have you seen any regression with that commit ? If so, details in the commit would be good.
Signature · 2 lines
Regards, Sudeep
Quoted 19 lines · Sudeep Holla
Quoted 18 lines · Jean Delvare
Quoted 16 lines · Sudeep Holla
Quoted 9 lines · Jean Delvare
With a name like that, I assume that the ARM SCPI protocol is only useful on the ARM architectures.
Signed-off-by Jean Delvare <[email protected]>Fixes 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")Cc Sudeep Holla <[email protected]>Cc Jon Medhurst (Tixy) <[email protected]>---
Please correct me if I'm wrong.I won't say you are wrong but the reason why it's named
arm_scpiis because the protocol was developed by ARM. It doesnn't mean only ARM/ARM64 needs to use it, it can be used on any architecture for inter-processor communication using any communication technique (currently mailbox is the only supported in the driver)
Quoted 5 lines
Quoted 2 lines
OK, thanks for the clarification. In practice, what other architectures are using it?
None, hence I didn't say you are wrong ;). I am fine having the check if it breaks for any other architecture with
COMPILE_TEST.
Not sure what you mean here... The purpose of COMPILE_TEST is to allow limiting the scope of a driver withing hurting the build test coverage.
Quoted 3 lines
Also you have mentioned it fixes
8f1498c03d15, have you seen any regression with that commit ? If so, details in the commit would be good.
Before 8f1498c03d15, the dependency on ARM_MHU made the driver only visible on ARM kernels. Since 8f1498c03d15, the driver is proposed to all, which I think isn't correct. In that sense my proposed patch is fixing a (user-friendliness) regression. But nothing serious.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 20 lines · Jean Delvare
Quoted 19 lines · Sudeep Holla
Quoted 18 lines · Jean Delvare
Quoted 16 lines · Sudeep Holla
Quoted 9 lines · Jean Delvare
With a name like that, I assume that the ARM SCPI protocol is only useful on the ARM architectures.
Signed-off-by Jean Delvare <[email protected]>Fixes 8f1498c03d15 ("firmware: arm_scpi: make it depend on MAILBOX instead of")Cc Sudeep Holla <[email protected]>Cc Jon Medhurst (Tixy) <[email protected]>---
Please correct me if I'm wrong.I won't say you are wrong but the reason why it's named
arm_scpiis because the protocol was developed by ARM. It doesnn't mean only ARM/ARM64 needs to use it, it can be used on any architecture for inter-processor communication using any communication technique (currently mailbox is the only supported in the driver)
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
OK, thanks for the clarification. In practice, what other architectures are using it?
None, hence I didn't say you are wrong ;). I am fine having the check if it breaks for any other architecture with
COMPILE_TEST.Not sure what you mean here... The purpose of
COMPILE_TESTis to allow limiting the scope of a driver withing hurting the build test coverage.
No I agree with adding COMPILE_TEST just not ARM || ARM64
Quoted 7 lines
Quoted 3 lines
Also you have mentioned it fixes
8f1498c03d15, have you seen any regression with that commit ? If so, details in the commit would be good.Before
8f1498c03d15, the dependency onARM_MHUmade the driver only visible on ARM kernels. Since8f1498c03d15, the driver is proposed to all, which I think isn't correct.
I disagree here. It depends on mailbox as we use mailbox API. And it is now used on AmLogic Meson series of SoC. So it *is correct*.
Quoted 3 lines
In that sense my proposed patch is
fixing a (user-friendliness) regression. But nothing serious.
Can you elaborate ? What's that *user-friendliness regression* ? build/boot/... ? I just need more details.
Signature · 2 lines
Regards, Sudeep
Quoted 9 lines · Sudeep Holla
Quoted 6 lines · Jean Delvare
Quoted 2 lines · Sudeep Holla
None, hence I didn't say you are wrong ;). I am fine having the check if it breaks for any other architecture with
COMPILE_TEST.Not sure what you mean here... The purpose of
COMPILE_TESTis to allow limiting the scope of a driver withing hurting the build test coverage.No I agree with adding
COMPILE_TESTjust not ARM || ARM64
That doesn't make any sense. The only purpose of COMPILE_TEST is to be or'd with an architecture or platform dependency. If you disagree with adding an architecture or platform dependency, you can't agree with adding COMPILE_TEST.
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
Also you have mentioned it fixes
8f1498c03d15, have you seen any regression with that commit ? If so, details in the commit would be good.Before
8f1498c03d15, the dependency onARM_MHUmade the driver only visible on ARM kernels. Since8f1498c03d15, the driver is proposed to all, which I think isn't correct.I disagree here. It depends on mailbox as we use mailbox API. And it is now used on AmLogic Meson series of SoC. So it *is correct*.
I'm fine with depending on MAILBOX, I never proposed to change that. But Meson are ARM SoCs. So what's the problem with making the driver depend on ARM?
Quoted 7 lines
Quoted 2 lines
In that sense my proposed patch is
fixing a (user-friendliness) regression. But nothing serious.Can you elaborate ? What's that *user-friendliness regression* ? build/boot/... ? I just need more details.
From a distribution kernel maintainer point of view, having to answer
dozens of irrelevant questions with every kernel version update is highly user-unfriendly. If all drivers could have proper hardware dependencies from the start, it would be a lot easier.
And same holds for virtually everyone configuring his/her kernel. I'm building a kernel for my x86 desktop PC, "make oldconfig" shouldn't bother me with ARM drivers.
As a side note, there's no finger-pointing implied by Fixes: tags. They are only meant to help people backporting patches, so that they know if something they backported is later fixed up. It does not imply anything regarding how serious the problem was.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 9 lines · Jean Delvare
Quoted 8 lines · Sudeep Holla
Quoted 6 lines · Jean Delvare
Quoted 2 lines · Sudeep Holla
None, hence I didn't say you are wrong ;). I am fine having the check if it breaks for any other architecture with
COMPILE_TEST.Not sure what you mean here... The purpose of
COMPILE_TESTis to allow limiting the scope of a driver withing hurting the build test coverage.
Quoted 7 lines
Quoted 1 line
No I agree with adding
COMPILE_TESTjust not ARM || ARM64That doesn't make any sense. The only purpose of
COMPILE_TESTis to be or'd with an architecture or platform dependency. If you disagree with adding an architecture or platform dependency, you can't agree with addingCOMPILE_TEST.
OK. As I said I am fine with that. I just wanted to know the details. Also different maintainer have different opinion on how to use COMPILE_TEST. I don't have any strong opinion on that.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Also you have mentioned it fixes
8f1498c03d15, have you seen any regression with that commit ? If so, details in the commit would be good.Before
8f1498c03d15, the dependency onARM_MHUmade the driver only visible on ARM kernels. Since8f1498c03d15, the driver is proposed to all, which I think isn't correct.
Quoted 7 lines
Quoted 2 lines
I disagree here. It depends on mailbox as we use mailbox API. And it is now used on AmLogic Meson series of SoC. So it *is correct*.
I'm fine with depending on MAILBOX, I never proposed to change that. But Meson are ARM SoCs. So what's the problem with making the driver depend on ARM?
So I am not saying so, just fixes tag misled me. I assumed something was broken with that change.
Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
In that sense my proposed patch is
fixing a (user-friendliness) regression. But nothing serious.Can you elaborate ? What's that *user-friendliness regression* ? build/boot/... ? I just need more details.
From a distribution kernel maintainer point of view, having to answer dozens of irrelevant questions with every kernel version update is highly user-unfriendly. If all drivers could have proper hardware dependencies from the start, it would be a lot easier.
I agree, but generally I have seen suggestions no to add too much dependency if it's not required and that's why I wanted to learn the details.
Quoted 4 lines
And same holds for virtually everyone configuring his/her kernel. I'm building a kernel for my x86 desktop PC, "make oldconfig" shouldn't bother me with ARM drivers.
Agreed.
Quoted 4 lines
As a side note, there's no finger-pointing implied by Fixes: tags. They are only meant to help people backporting patches, so that they know if something they backported is later fixed up. It does not imply anything regarding how serious the problem was.
Yes, but sometimes it's taken for stable tree and I don't think patch is really fixing anything in that particular commit. e.g. what if x86 had MAILBOX enabled, that patch changes nothing. So definitely not stable material.
You can drop the fixes tag and add my ack. Please post it to [email protected], so that ARM SoC team can pick this up directly.
Signature · 2 lines
Regards, Sudeep
Hi Sudeep,
Quoted 2 lines · Sudeep Holla
Also different maintainer have different opinion on how to use
COMPILE_TEST. I don't have any strong opinion on that.
There's not much to discuss or disagree about COMPILE_TEST. It was introduced for one purpose and should be used for that one purpose:
restrict driver visibility to the relevant platforms without hindering the build test coverage.
Quoted 4 lines
(...)
I agree, but generally I have seen suggestions no to add too much dependency if it's not required and that's why I wanted to learn the details.
I think you are speaking about something different. You don't want to have dependencies on subsystems or options which your driver doesn't actually need, of course. But limiting drivers to their intended or actual architectures or platforms is a different kind of dependencies, so that rule doesn't apply.
Quoted 7 lines
Quoted 5 lines
(...)
As a side note, there's no finger-pointing implied by Fixes: tags. They are only meant to help people backporting patches, so that they know if something they backported is later fixed up. It does not imply anything regarding how serious the problem was.Yes, but sometimes it's taken for stable tree
Really? I would expect that only patches tagged with "Cc:
[email protected]", or explicitly sent to that list, are considered for stable trees. Greg, you don't pick commits for stable trees just because they have a "Fixes:" tag, do you?
Quoted 4 lines
and I don't think patch is
really fixing anything in that particular commit. e.g. what if x86 had MAILBOX enabled, that patch changes nothing. So definitely not stable material.
It's not fixing a build breakage, if that's what you were worried about. I agree it's not stable material and I did not Cc stable. For me, Cc: stable@ and Fixes: are not correlated.
Quoted 2 lines
You can drop the fixes tag and add my ack. Please post it to [email protected], so that ARM SoC team can pick this up directly.
I can't see this address in MAINTAINERS.
Signature · 2 lines
Jean Delvare SUSE L3 Support
Quoted 7 lines · Jean Delvare
Quoted 6 lines · Sudeep Holla
Quoted 5 lines
(...)
As a side note, there's no finger-pointing implied by Fixes: tags. They are only meant to help people backporting patches, so that they know if something they backported is later fixed up. It does not imply anything regarding how serious the problem was.
Quoted 6 lines
Quoted 1 line
Yes, but sometimes it's taken for stable tree
Really? I would expect that only patches tagged with "Cc:
[email protected]", or explicitly sent to that list, are considered for stable trees. Greg, you don't pick commits for stable trees just because they have a "Fixes:" tag, do you?
Sometimes I do, usually because the maintainer has forgotten to put the stable cc: (some subsystems are really bad about this), or because it fixes a patch that was previous backported to stable.
I also take other patches that have no markings at all for stable trees, depending on what they do, or who recommends them. I try to scan all commits for stuff that should be included that someone forgot to put a stable@ cc: on.
But, just because you put a Fixes: tag, does not mean it will always go into stable, because I do miss things at times. If you know it should go there, please always put the correct CC: tag.
thanks,
greg k-h
Quoted 3 lines · Jean Delvare
Hi Sudeep,
On Wed, 25 Jan 2017 15:15:45 +0000, Sudeep Holla wrote:
[...]
Quoted 9 lines
Quoted 4 lines
and I don't think patch is
really fixing anything in that particular commit. e.g. what if x86 had MAILBOX enabled, that patch changes nothing. So definitely not stable material.It's not fixing a build breakage, if that's what you were worried about. I agree it's not stable material and I did not Cc stable. For me, Cc: stable@ and Fixes: are not correlated.
I understand and I think Greg had replied which is much clear than what I was trying to tell very vaguely.
Quoted 5 lines
Quoted 2 lines
You can drop the fixes tag and add my ack. Please post it to [email protected], so that ARM SoC team can pick this up directly.
I can't see this address in
MAINTAINERS.
Ah yes, since I don't have any other patches I asked you to send it directly to the email where ARM SoC picks up the patches most of the time(the review still happens on the std. list, it's just one email for the group of maintainer where the co-ordinate so that it's not missed.
If you stick my ack and post there along with the mailing list, I will request them to pick it up directly instead of me sending pull request.
Signature · 2 lines
Regards, Sudeep