[PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Desmond Yan]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

Add kconfig for enabling bcm-vk driver to support the viper chip.
Default is off and only explicitly configured that the viper chip
would be discovered by the driver and loaded for usage.

Tested-by: Desmond Yan <desmond.yan@broadcom.com>
Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>
---
 drivers/misc/bcm-vk/Kconfig      | 10 ++++++++++
 drivers/misc/bcm-vk/bcm_vk_dev.c |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
index 68a972772b99..43910a378163 100644
--- a/drivers/misc/bcm-vk/Kconfig
+++ b/drivers/misc/bcm-vk/Kconfig
@@ -16,6 +16,16 @@ config BCM_VK
 
 	  If unsure, say N.
 
+config BCM_VK_VIPER
+	bool "VK driver for VIPER"
+	depends on BCM_VK
+	help
+	  Turn on to enable using the vk driver for the viper variant.
+	  Used for development environments where viper uses VK driver
+	  and VK firmware rather than an alternate firmware and driver.
+
+	  If unsure, say N.
+
 config BCM_VK_TTY
 	bool "Enable tty ports on a Broadcom VK Accelerator device"
 	depends on TTY
diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
index ad639ee85b2a..41dbfa4fa4a5 100644
--- a/drivers/misc/bcm-vk/bcm_vk_dev.c
+++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
@@ -1633,7 +1633,9 @@ static void bcm_vk_shutdown(struct pci_dev *pdev)
 
 static const struct pci_device_id bcm_vk_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_VALKYRIE), },
+#if defined(CONFIG_BCM_VK_VIPER)
 	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_VIPER), },
+#endif
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, bcm_vk_ids);
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Greg Kroah-Hartman]

On Fri, Feb 18, 2022 at 12:08:11PM -0800, Desmond Yan wrote:
> Add kconfig for enabling bcm-vk driver to support the viper chip.
> Default is off and only explicitly configured that the viper chip
> would be discovered by the driver and loaded for usage.
> 
> Tested-by: Desmond Yan <desmond.yan@broadcom.com>
> Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>
> ---
>  drivers/misc/bcm-vk/Kconfig      | 10 ++++++++++
>  drivers/misc/bcm-vk/bcm_vk_dev.c |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> index 68a972772b99..43910a378163 100644
> --- a/drivers/misc/bcm-vk/Kconfig
> +++ b/drivers/misc/bcm-vk/Kconfig
> @@ -16,6 +16,16 @@ config BCM_VK
>  
>  	  If unsure, say N.
>  
> +config BCM_VK_VIPER
> +	bool "VK driver for VIPER"
> +	depends on BCM_VK
> +	help
> +	  Turn on to enable using the vk driver for the viper variant.
> +	  Used for development environments where viper uses VK driver
> +	  and VK firmware rather than an alternate firmware and driver.
> +
> +	  If unsure, say N.
> +
>  config BCM_VK_TTY
>  	bool "Enable tty ports on a Broadcom VK Accelerator device"
>  	depends on TTY
> diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
> index ad639ee85b2a..41dbfa4fa4a5 100644
> --- a/drivers/misc/bcm-vk/bcm_vk_dev.c
> +++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
> @@ -1633,7 +1633,9 @@ static void bcm_vk_shutdown(struct pci_dev *pdev)
>  
>  static const struct pci_device_id bcm_vk_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_VALKYRIE), },
> +#if defined(CONFIG_BCM_VK_VIPER)
>  	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_VIPER), },
> +#endif

Why do you need a new config option just to add a new device id?  Why
would you ever NOT want this option enabled as there is no other driver
for this hardware?

thanks,

greg k-h

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Desmond Yan (APD)]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

Greg,

This is to allow the customer to use their own driver, by default,
which is their preferred option.

thx
Des


On Fri, Feb 18, 2022 at 11:33 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Feb 18, 2022 at 12:08:11PM -0800, Desmond Yan wrote:
> > Add kconfig for enabling bcm-vk driver to support the viper chip.
> > Default is off and only explicitly configured that the viper chip
> > would be discovered by the driver and loaded for usage.
> >
> > Tested-by: Desmond Yan <desmond.yan@broadcom.com>
> > Signed-off-by: Desmond Yan <desmond.yan@broadcom.com>
> > ---
> >  drivers/misc/bcm-vk/Kconfig      | 10 ++++++++++
> >  drivers/misc/bcm-vk/bcm_vk_dev.c |  2 ++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
> > index 68a972772b99..43910a378163 100644
> > --- a/drivers/misc/bcm-vk/Kconfig
> > +++ b/drivers/misc/bcm-vk/Kconfig
> > @@ -16,6 +16,16 @@ config BCM_VK
> >
> >         If unsure, say N.
> >
> > +config BCM_VK_VIPER
> > +     bool "VK driver for VIPER"
> > +     depends on BCM_VK
> > +     help
> > +       Turn on to enable using the vk driver for the viper variant.
> > +       Used for development environments where viper uses VK driver
> > +       and VK firmware rather than an alternate firmware and driver.
> > +
> > +       If unsure, say N.
> > +
> >  config BCM_VK_TTY
> >       bool "Enable tty ports on a Broadcom VK Accelerator device"
> >       depends on TTY
> > diff --git a/drivers/misc/bcm-vk/bcm_vk_dev.c b/drivers/misc/bcm-vk/bcm_vk_dev.c
> > index ad639ee85b2a..41dbfa4fa4a5 100644
> > --- a/drivers/misc/bcm-vk/bcm_vk_dev.c
> > +++ b/drivers/misc/bcm-vk/bcm_vk_dev.c
> > @@ -1633,7 +1633,9 @@ static void bcm_vk_shutdown(struct pci_dev *pdev)
> >
> >  static const struct pci_device_id bcm_vk_ids[] = {
> >       { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_VALKYRIE), },
> > +#if defined(CONFIG_BCM_VK_VIPER)
> >       { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_VIPER), },
> > +#endif
>
> Why do you need a new config option just to add a new device id?  Why
> would you ever NOT want this option enabled as there is no other driver
> for this hardware?
>
> thanks,
>
> greg k-h

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Greg Kroah-Hartman]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Tue, Feb 22, 2022 at 07:41:04AM -0800, Desmond Yan (APD) wrote:
> Greg,
> 
> This is to allow the customer to use their own driver, by default,
> which is their preferred option.

We do not care about out-of-tree code, and do not add Kconfig options
just for that, sorry.

Why not just add the out-of-tree driver here to the in-kernel code?
What is preventing that from happening today?

thanks,

greg k-h

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Desmond yan]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]


Greg,

Please see inlined.

On 2/22/22 09:13, Greg Kroah-Hartman wrote:
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
> 
> A: No.
> Q: Should I include quotations after my reply?
> 
> 
> http://daringfireball.net/2007/07/on_top
> 
> On Tue, Feb 22, 2022 at 07:41:04AM -0800, Desmond Yan (APD) wrote:
>> Greg,
>>
>> This is to allow the customer to use their own driver, by default,
>> which is their preferred option.
> 
> We do not care about out-of-tree code, and do not add Kconfig options
> just for that, sorry.
> 
> Why not just add the out-of-tree driver here to the in-kernel code?
> What is preventing that from happening today?

Using the bcm_vk driver for viper is only for proof-of-concept, and is 
not aimed to be deployed for production.  In parallel, customer is 
developing their own production-ready driver and would probably upstream 
it whenever the appropriate time comes.

thx.

des
> 
> thanks,
> 
> greg k-h

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Greg Kroah-Hartman]

On Tue, Feb 22, 2022 at 09:14:02PM -0800, Desmond yan wrote:
> 
> Greg,
> 
> Please see inlined.
> 
> On 2/22/22 09:13, Greg Kroah-Hartman wrote:
> > A: http://en.wikipedia.org/wiki/Top_post
> > Q: Were do I find info about this thing called top-posting?
> > A: Because it messes up the order in which people normally read text.
> > Q: Why is top-posting such a bad thing?
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> > 
> > A: No.
> > Q: Should I include quotations after my reply?
> > 
> > 
> > http://daringfireball.net/2007/07/on_top
> > 
> > On Tue, Feb 22, 2022 at 07:41:04AM -0800, Desmond Yan (APD) wrote:
> > > Greg,
> > > 
> > > This is to allow the customer to use their own driver, by default,
> > > which is their preferred option.
> > 
> > We do not care about out-of-tree code, and do not add Kconfig options
> > just for that, sorry.
> > 
> > Why not just add the out-of-tree driver here to the in-kernel code?
> > What is preventing that from happening today?
> 
> Using the bcm_vk driver for viper is only for proof-of-concept, and is not
> aimed to be deployed for production.  In parallel, customer is developing
> their own production-ready driver and would probably upstream it whenever
> the appropriate time comes.

Ok, so let us not add a new config option only for one person working on
writing their own driver.  Let's just add the device id as there are
more people than just that one person wanting to use their hardware with
the existing drivers.

Please resubmit this without the Kconfig change and I will be glad to
take it.

thanks,

greg k-h

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Scott Branden]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

Hi Greg,

On 2022-02-22 23:26, Greg Kroah-Hartman wrote:
> On Tue, Feb 22, 2022 at 09:14:02PM -0800, Desmond yan wrote:
>>
>> Greg,
>>
>> Please see inlined.
>>
>> On 2/22/22 09:13, Greg Kroah-Hartman wrote:
>>> A: http://en.wikipedia.org/wiki/Top_post
>>> Q: Were do I find info about this thing called top-posting?
>>> A: Because it messes up the order in which people normally read text.
>>> Q: Why is top-posting such a bad thing?
>>> A: Top-posting.
>>> Q: What is the most annoying thing in e-mail?
>>>
>>> A: No.
>>> Q: Should I include quotations after my reply?
>>>
>>>
>>> http://daringfireball.net/2007/07/on_top
>>>
>>> On Tue, Feb 22, 2022 at 07:41:04AM -0800, Desmond Yan (APD) wrote:
>>>> Greg,
>>>>
>>>> This is to allow the customer to use their own driver, by default,
>>>> which is their preferred option.
>>>
>>> We do not care about out-of-tree code, and do not add Kconfig options
>>> just for that, sorry.
>>>
>>> Why not just add the out-of-tree driver here to the in-kernel code?
>>> What is preventing that from happening today?
>>
>> Using the bcm_vk driver for viper is only for proof-of-concept, and is not
>> aimed to be deployed for production.  In parallel, customer is developing
>> their own production-ready driver and would probably upstream it whenever
>> the appropriate time comes.
> 
> Ok, so let us not add a new config option only for one person working on
> writing their own driver.  Let's just add the device id as there are
> more people than just that one person wanting to use their hardware with
> the existing drivers.
The bcm_vk driver is the official driver for Valkyrie.  For Viper, the 
bcm_vk driver is only for proof of concept, testing, and demonstration 
purposes. For production, the new driver in development will be used.

To support such configuration, we need to have the Viper pci id 
configurable and off by default in the bcm_vk driver to allow the new 
production driver to be developed against the upstream kernel and drivers.
> 
> Please resubmit this without the Kconfig change and I will be glad to
> take it.
I don't understand what there would be to submit?  The Viper pci id is 
already upstreamed into the bcm_vk driver.  The sole purpose of this 
patch is to add the Kconfig option and have it off by default.
> 
> thanks,
> 
> greg k-h

Regards,
Scott

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Greg Kroah-Hartman]

On Wed, Feb 23, 2022 at 09:59:48AM -0800, Scott Branden wrote:
> Hi Greg,
> 
> On 2022-02-22 23:26, Greg Kroah-Hartman wrote:
> > On Tue, Feb 22, 2022 at 09:14:02PM -0800, Desmond yan wrote:
> > > 
> > > Greg,
> > > 
> > > Please see inlined.
> > > 
> > > On 2/22/22 09:13, Greg Kroah-Hartman wrote:
> > > > A: http://en.wikipedia.org/wiki/Top_post
> > > > Q: Were do I find info about this thing called top-posting?
> > > > A: Because it messes up the order in which people normally read text.
> > > > Q: Why is top-posting such a bad thing?
> > > > A: Top-posting.
> > > > Q: What is the most annoying thing in e-mail?
> > > > 
> > > > A: No.
> > > > Q: Should I include quotations after my reply?
> > > > 
> > > > 
> > > > http://daringfireball.net/2007/07/on_top
> > > > 
> > > > On Tue, Feb 22, 2022 at 07:41:04AM -0800, Desmond Yan (APD) wrote:
> > > > > Greg,
> > > > > 
> > > > > This is to allow the customer to use their own driver, by default,
> > > > > which is their preferred option.
> > > > 
> > > > We do not care about out-of-tree code, and do not add Kconfig options
> > > > just for that, sorry.
> > > > 
> > > > Why not just add the out-of-tree driver here to the in-kernel code?
> > > > What is preventing that from happening today?
> > > 
> > > Using the bcm_vk driver for viper is only for proof-of-concept, and is not
> > > aimed to be deployed for production.  In parallel, customer is developing
> > > their own production-ready driver and would probably upstream it whenever
> > > the appropriate time comes.
> > 
> > Ok, so let us not add a new config option only for one person working on
> > writing their own driver.  Let's just add the device id as there are
> > more people than just that one person wanting to use their hardware with
> > the existing drivers.
> The bcm_vk driver is the official driver for Valkyrie.  For Viper, the
> bcm_vk driver is only for proof of concept, testing, and demonstration
> purposes. For production, the new driver in development will be used.
> 
> To support such configuration, we need to have the Viper pci id configurable
> and off by default in the bcm_vk driver to allow the new production driver
> to be developed against the upstream kernel and drivers.
> > 
> > Please resubmit this without the Kconfig change and I will be glad to
> > take it.
> I don't understand what there would be to submit?  The Viper pci id is
> already upstreamed into the bcm_vk driver.  The sole purpose of this patch
> is to add the Kconfig option and have it off by default.

Ah, I missed that it was disabling the id.  That's even worse.  No,
sorry, not going to take this upstream.  Again, we do not add changes to
the kernel (and especially not Kconfig options) for out-of-tree code.

You know this...

thanks,

greg k-h

Re: [PATCH v1 1/1] misc: bcm-vk: add kconfig for supporting viper chip

[Scott Branden]

[-- Attachment #1: Type: text/plain, Size: 2941 bytes --]



On 2022-02-23 10:27, Greg Kroah-Hartman wrote:
> On Wed, Feb 23, 2022 at 09:59:48AM -0800, Scott Branden wrote:
>> Hi Greg,
>>
>> On 2022-02-22 23:26, Greg Kroah-Hartman wrote:
>>> On Tue, Feb 22, 2022 at 09:14:02PM -0800, Desmond yan wrote:
>>>>
>>>> Greg,
>>>>
>>>> Please see inlined.
>>>>
>>>> On 2/22/22 09:13, Greg Kroah-Hartman wrote:
>>>>> A: http://en.wikipedia.org/wiki/Top_post
>>>>> Q: Were do I find info about this thing called top-posting?
>>>>> A: Because it messes up the order in which people normally read text.
>>>>> Q: Why is top-posting such a bad thing?
>>>>> A: Top-posting.
>>>>> Q: What is the most annoying thing in e-mail?
>>>>>
>>>>> A: No.
>>>>> Q: Should I include quotations after my reply?
>>>>>
>>>>>
>>>>> http://daringfireball.net/2007/07/on_top
>>>>>
>>>>> On Tue, Feb 22, 2022 at 07:41:04AM -0800, Desmond Yan (APD) wrote:
>>>>>> Greg,
>>>>>>
>>>>>> This is to allow the customer to use their own driver, by default,
>>>>>> which is their preferred option.
>>>>>
>>>>> We do not care about out-of-tree code, and do not add Kconfig options
>>>>> just for that, sorry.
>>>>>
>>>>> Why not just add the out-of-tree driver here to the in-kernel code?
>>>>> What is preventing that from happening today?
>>>>
>>>> Using the bcm_vk driver for viper is only for proof-of-concept, and is not
>>>> aimed to be deployed for production.  In parallel, customer is developing
>>>> their own production-ready driver and would probably upstream it whenever
>>>> the appropriate time comes.
>>>
>>> Ok, so let us not add a new config option only for one person working on
>>> writing their own driver.  Let's just add the device id as there are
>>> more people than just that one person wanting to use their hardware with
>>> the existing drivers.
>> The bcm_vk driver is the official driver for Valkyrie.  For Viper, the
>> bcm_vk driver is only for proof of concept, testing, and demonstration
>> purposes. For production, the new driver in development will be used.
>>
>> To support such configuration, we need to have the Viper pci id configurable
>> and off by default in the bcm_vk driver to allow the new production driver
>> to be developed against the upstream kernel and drivers.
>>>
>>> Please resubmit this without the Kconfig change and I will be glad to
>>> take it.
>> I don't understand what there would be to submit?  The Viper pci id is
>> already upstreamed into the bcm_vk driver.  The sole purpose of this patch
>> is to add the Kconfig option and have it off by default.
> 
> Ah, I missed that it was disabling the id.  That's even worse.  No,
> sorry, not going to take this upstream.  Again, we do not add changes to
> the kernel (and especially not Kconfig options) for out-of-tree code.
Well, will you accept a patch to remove the Viper PCI id from upstream 
instead then?  bcm_vk driver should not support Viper PCI id in official 
systems.
> 
> You know this...
> 
> thanks,
> 
> greg k-h

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]