2019-01-23 15:40:39

by Richard Gong

[permalink] [raw]
Subject: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency

From: Richard Gong <[email protected]>

Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
can be built only on the platform that supports it.

Signed-off-by: Richard Gong <[email protected]>
---
drivers/firmware/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index f754578..cac16c4 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE

config INTEL_STRATIX10_SERVICE
tristate "Intel Stratix10 Service Layer"
- depends on HAVE_ARM_SMCCC
+ depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
default n
help
Intel Stratix10 service layer runs at privileged exception level,
--
2.7.4



2019-01-23 16:02:29

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency

On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
> From: Richard Gong <[email protected]>
>
> Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
> can be built only on the platform that supports it.
>
> Signed-off-by: Richard Gong <[email protected]>
> ---
> drivers/firmware/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index f754578..cac16c4 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
>
> config INTEL_STRATIX10_SERVICE
> tristate "Intel Stratix10 Service Layer"
> - depends on HAVE_ARM_SMCCC
> + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC

That's lame, what about building for testing?

And is this needed now, for 5.0-final, or can it wait for 5.1? What
changed to require this?

tahnks,

greg k-h

2019-01-23 16:39:13

by Alan Tull

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency

On Wed, Jan 23, 2019 at 10:00 AM Greg KH <[email protected]> wrote:

Hi Greg,

>
> On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
> > From: Richard Gong <[email protected]>
> >
> > Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
> > can be built only on the platform that supports it.
> >
> > Signed-off-by: Richard Gong <[email protected]>
> > ---
> > drivers/firmware/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index f754578..cac16c4 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
> >
> > config INTEL_STRATIX10_SERVICE
> > tristate "Intel Stratix10 Service Layer"
> > - depends on HAVE_ARM_SMCCC
> > + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
>
> That's lame, what about building for testing?

Do you mean this instead?

depends on (ARCH_STRATIX10 && HAVE_ARM_SMCCC) || COMPILE_TEST

>
> And is this needed now, for 5.0-final, or can it wait for 5.1?

This change will reduce kernel size for most arm64. It can go into
whichever kernel. We can resubmit allowing for COMPILE_TEST.

> What
> changed to require this?

Nothing, we just didn't catch this. We're used to having our own
defconfig. Not used to having a single defconfig that is shared.

Thanks for the review comments,
Alan

>
> tahnks,
>
> greg k-h

2019-01-23 16:45:45

by Dinh Nguyen

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency



On 1/23/19 10:37 AM, Alan Tull wrote:
> On Wed, Jan 23, 2019 at 10:00 AM Greg KH <[email protected]> wrote:
>
> Hi Greg,
>
>>
>> On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
>>> From: Richard Gong <[email protected]>
>>>
>>> Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
>>> can be built only on the platform that supports it.
>>>
>>> Signed-off-by: Richard Gong <[email protected]>
>>> ---
>>> drivers/firmware/Kconfig | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index f754578..cac16c4 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
>>>
>>> config INTEL_STRATIX10_SERVICE
>>> tristate "Intel Stratix10 Service Layer"
>>> - depends on HAVE_ARM_SMCCC
>>> + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
>>
>> That's lame, what about building for testing?
>
> Do you mean this instead?
>
> depends on (ARCH_STRATIX10 && HAVE_ARM_SMCCC) || COMPILE_TEST
>
>>
>> And is this needed now, for 5.0-final, or can it wait for 5.1?
>
> This change will reduce kernel size for most arm64. It can go into
> whichever kernel. We can resubmit allowing for COMPILE_TEST.
>

I don't see how this is true? ARM64 has a single defconfig and
ARCH_STRATIX10 is included in that defconfig. I don't see how adding
this dependency will reduce the kernel size for most arm64?

Dinh

2019-01-23 16:45:51

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency

On Wed, Jan 23, 2019 at 10:37:07AM -0600, Alan Tull wrote:
> On Wed, Jan 23, 2019 at 10:00 AM Greg KH <[email protected]> wrote:
>
> Hi Greg,
>
> >
> > On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
> > > From: Richard Gong <[email protected]>
> > >
> > > Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
> > > can be built only on the platform that supports it.
> > >
> > > Signed-off-by: Richard Gong <[email protected]>
> > > ---
> > > drivers/firmware/Kconfig | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > index f754578..cac16c4 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
> > >
> > > config INTEL_STRATIX10_SERVICE
> > > tristate "Intel Stratix10 Service Layer"
> > > - depends on HAVE_ARM_SMCCC
> > > + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
> >
> > That's lame, what about building for testing?
>
> Do you mean this instead?
>
> depends on (ARCH_STRATIX10 && HAVE_ARM_SMCCC) || COMPILE_TEST

Yes, that would be better, right?

> >
> > And is this needed now, for 5.0-final, or can it wait for 5.1?
>
> This change will reduce kernel size for most arm64. It can go into
> whichever kernel. We can resubmit allowing for COMPILE_TEST.

So it's not fixing a bug, but rather just allowing you to shrink an
image? If so, then 5.1 is good.

Please resubmit, after making sure it doesn't break the normal builds :)

thanks,

greg k-h

2019-01-23 16:53:04

by Richard Gong

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency

Hi Greg,

On 1/23/19 10:43 AM, Greg KH wrote:
> On Wed, Jan 23, 2019 at 10:37:07AM -0600, Alan Tull wrote:
>> On Wed, Jan 23, 2019 at 10:00 AM Greg KH <[email protected]> wrote:
>>
>> Hi Greg,
>>
>>>
>>> On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
>>>> From: Richard Gong <[email protected]>
>>>>
>>>> Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
>>>> can be built only on the platform that supports it.
>>>>
>>>> Signed-off-by: Richard Gong <[email protected]>
>>>> ---
>>>> drivers/firmware/Kconfig | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>>> index f754578..cac16c4 100644
>>>> --- a/drivers/firmware/Kconfig
>>>> +++ b/drivers/firmware/Kconfig
>>>> @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
>>>>
>>>> config INTEL_STRATIX10_SERVICE
>>>> tristate "Intel Stratix10 Service Layer"
>>>> - depends on HAVE_ARM_SMCCC
>>>> + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
>>>
>>> That's lame, what about building for testing?
>>
>> Do you mean this instead?
>>
>> depends on (ARCH_STRATIX10 && HAVE_ARM_SMCCC) || COMPILE_TEST
>
> Yes, that would be better, right?
>
>>>
>>> And is this needed now, for 5.0-final, or can it wait for 5.1?
>>
>> This change will reduce kernel size for most arm64. It can go into
>> whichever kernel. We can resubmit allowing for COMPILE_TEST.
>
> So it's not fixing a bug, but rather just allowing you to shrink an
> image? If so, then 5.1 is good.
>
> Please resubmit, after making sure it doesn't break the normal builds :)

Thanks for your review comments, I will resubmit the patch.
>
> thanks,
>
> greg k-h
>

2019-01-23 17:07:42

by Dinh Nguyen

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency



On 1/23/19 11:01 AM, Alan Tull wrote:
> On Wed, Jan 23, 2019 at 10:42 AM Dinh Nguyen <[email protected]> wrote:
>>
>>
>>
>> On 1/23/19 10:37 AM, Alan Tull wrote:
>>> On Wed, Jan 23, 2019 at 10:00 AM Greg KH <[email protected]> wrote:
>>>
>>> Hi Greg,
>>>
>>>>
>>>> On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
>>>>> From: Richard Gong <[email protected]>
>>>>>
>>>>> Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
>>>>> can be built only on the platform that supports it.
>>>>>
>>>>> Signed-off-by: Richard Gong <[email protected]>
>>>>> ---
>>>>> drivers/firmware/Kconfig | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>>>> index f754578..cac16c4 100644
>>>>> --- a/drivers/firmware/Kconfig
>>>>> +++ b/drivers/firmware/Kconfig
>>>>> @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
>>>>>
>>>>> config INTEL_STRATIX10_SERVICE
>>>>> tristate "Intel Stratix10 Service Layer"
>>>>> - depends on HAVE_ARM_SMCCC
>>>>> + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
>>>>
>>>> That's lame, what about building for testing?
>>>
>>> Do you mean this instead?
>>>
>>> depends on (ARCH_STRATIX10 && HAVE_ARM_SMCCC) || COMPILE_TEST
>>>
>>>>
>>>> And is this needed now, for 5.0-final, or can it wait for 5.1?
>>>
>>> This change will reduce kernel size for most arm64. It can go into
>>> whichever kernel. We can resubmit allowing for COMPILE_TEST.
>>>
>>
>> I don't see how this is true? ARM64 has a single defconfig and
>> ARCH_STRATIX10 is included in that defconfig. I don't see how adding
>> this dependency will reduce the kernel size for most arm64?
>
> Sorry, I wasn't clear. By default it will be built in since there's
> one arm64 defconfig. But adding ARCH_STRATIX10 dependency here
> supports users who turn off all the ARCH_* they don't care about in
> their own defconfig that won't get upstreamed.
>

Ah yes, forgot to consider that.

Dinh

2019-01-23 17:19:56

by Alan Tull

[permalink] [raw]
Subject: Re: [PATCHv1] firmware: intel_stratix10_service: add hardware dependency

On Wed, Jan 23, 2019 at 10:42 AM Dinh Nguyen <[email protected]> wrote:
>
>
>
> On 1/23/19 10:37 AM, Alan Tull wrote:
> > On Wed, Jan 23, 2019 at 10:00 AM Greg KH <[email protected]> wrote:
> >
> > Hi Greg,
> >
> >>
> >> On Wed, Jan 23, 2019 at 09:47:56AM -0600, [email protected] wrote:
> >>> From: Richard Gong <[email protected]>
> >>>
> >>> Add a Kconfig dependency to ensure Intel Stratix10 service layer driver
> >>> can be built only on the platform that supports it.
> >>>
> >>> Signed-off-by: Richard Gong <[email protected]>
> >>> ---
> >>> drivers/firmware/Kconfig | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> >>> index f754578..cac16c4 100644
> >>> --- a/drivers/firmware/Kconfig
> >>> +++ b/drivers/firmware/Kconfig
> >>> @@ -218,7 +218,7 @@ config FW_CFG_SYSFS_CMDLINE
> >>>
> >>> config INTEL_STRATIX10_SERVICE
> >>> tristate "Intel Stratix10 Service Layer"
> >>> - depends on HAVE_ARM_SMCCC
> >>> + depends on ARCH_STRATIX10 && HAVE_ARM_SMCCC
> >>
> >> That's lame, what about building for testing?
> >
> > Do you mean this instead?
> >
> > depends on (ARCH_STRATIX10 && HAVE_ARM_SMCCC) || COMPILE_TEST
> >
> >>
> >> And is this needed now, for 5.0-final, or can it wait for 5.1?
> >
> > This change will reduce kernel size for most arm64. It can go into
> > whichever kernel. We can resubmit allowing for COMPILE_TEST.
> >
>
> I don't see how this is true? ARM64 has a single defconfig and
> ARCH_STRATIX10 is included in that defconfig. I don't see how adding
> this dependency will reduce the kernel size for most arm64?

Sorry, I wasn't clear. By default it will be built in since there's
one arm64 defconfig. But adding ARCH_STRATIX10 dependency here
supports users who turn off all the ARCH_* they don't care about in
their own defconfig that won't get upstreamed.

>
> Dinh