[PATCH net] bnxt_en: enable PTM function

[PATCH net] bnxt_en: enable PTM function

[Vadim Fedorenko]

The patch mentioned in Fixes missed one main point of implementing
proper PTM support. To make it fully operational it has to be explicitly
enabled. Add missing call in probe callback and disable it in teardown
callback.

Fixes: c470195b989f ("bnxt_en: Add PTP .getcrosststamp() interface to get device/host times")
Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 25099077fe4f..8a9c7646b53f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14978,6 +14978,7 @@ static void bnxt_unmap_bars(struct bnxt *bp, struct pci_dev *pdev)
 
 static void bnxt_cleanup_pci(struct bnxt *bp)
 {
+	pci_disable_ptm(bp->pdev);
 	bnxt_unmap_bars(bp, bp->pdev);
 	pci_release_regions(bp->pdev);
 	if (pci_is_enabled(bp->pdev))
@@ -15546,6 +15547,8 @@ static int bnxt_init_board(struct pci_dev *pdev, struct net_device *dev)
 		goto init_err_release;
 	}
 
+	pci_enable_ptm(pdev);
+
 	INIT_WORK(&bp->sp_task, bnxt_sp_task);
 	INIT_DELAYED_WORK(&bp->fw_reset_task, bnxt_fw_reset_task);
 
-- 
2.47.3

Re: [PATCH net] bnxt_en: enable PTM function

[Michael Chan]

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

On Thu, Aug 6, 2026 at 1:22 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> The patch mentioned in Fixes missed one main point of implementing
> proper PTM support. To make it fully operational it has to be explicitly
> enabled. Add missing call in probe callback and disable it in teardown
> callback.

We were actually discussing this internally just a few weeks ago.  I
think the FW always enables it and that's why pci_enable_ptm() is not
strictly required.  I'll let Pavan comment on this further.  Thanks.

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

Re: [PATCH net] bnxt_en: enable PTM function

[Vadim Fedorenko]

On 06.08.2026 21:35, Michael Chan wrote:
> On Thu, Aug 6, 2026 at 1:22 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> The patch mentioned in Fixes missed one main point of implementing
>> proper PTM support. To make it fully operational it has to be explicitly
>> enabled. Add missing call in probe callback and disable it in teardown
>> callback.
> 
> We were actually discussing this internally just a few weeks ago.  I
> think the FW always enables it and that's why pci_enable_ptm() is not
> strictly required.  I'll let Pavan comment on this further.  Thanks.

In my test even though firmware was acting as requestor, pcie_ptm_enabled()
returns false and disables setup of getcrosststamp callback. This happens
because kernel doesn't know that PTM is enabled.

Re: [PATCH net] bnxt_en: enable PTM function

[Pavan Chebbi]

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

On Fri, Aug 7, 2026 at 2:16 AM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 06.08.2026 21:35, Michael Chan wrote:
> > On Thu, Aug 6, 2026 at 1:22 PM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> The patch mentioned in Fixes missed one main point of implementing
> >> proper PTM support. To make it fully operational it has to be explicitly
> >> enabled. Add missing call in probe callback and disable it in teardown
> >> callback.
> >
> > We were actually discussing this internally just a few weeks ago.  I
> > think the FW always enables it and that's why pci_enable_ptm() is not
> > strictly required.  I'll let Pavan comment on this further.  Thanks.
>
> In my test even though firmware was acting as requestor, pcie_ptm_enabled()
> returns false and disables setup of getcrosststamp callback. This happens
> because kernel doesn't know that PTM is enabled.

I think the right thing to do would be to first check the capability
from FW (which actually decides whether we can support PTM on the
installed RC/platform) and then enable it in the kernel.
This is what we internally decided to do. Can you wait for us to
complete that work and send the patch?

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

Re: [PATCH net] bnxt_en: enable PTM function

[Vadim Fedorenko]

On 07/08/2026 13:51, Pavan Chebbi wrote:
> On Fri, Aug 7, 2026 at 2:16 AM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 06.08.2026 21:35, Michael Chan wrote:
>>> On Thu, Aug 6, 2026 at 1:22 PM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> The patch mentioned in Fixes missed one main point of implementing
>>>> proper PTM support. To make it fully operational it has to be explicitly
>>>> enabled. Add missing call in probe callback and disable it in teardown
>>>> callback.
>>>
>>> We were actually discussing this internally just a few weeks ago.  I
>>> think the FW always enables it and that's why pci_enable_ptm() is not
>>> strictly required.  I'll let Pavan comment on this further.  Thanks.
>>
>> In my test even though firmware was acting as requestor, pcie_ptm_enabled()
>> returns false and disables setup of getcrosststamp callback. This happens
>> because kernel doesn't know that PTM is enabled.
> 
> I think the right thing to do would be to first check the capability
> from FW (which actually decides whether we can support PTM on the
> installed RC/platform) and then enable it in the kernel.
> This is what we internally decided to do. Can you wait for us to
> complete that work and send the patch?


Could you please be a bit more specific on how FW decides? Does it
remove PTM capability in case it's not possible?

As you can see from the code of pci_enable_ptm(), sometimes parent
device needs PTM enabled as well before starting TLVs, will your FW be
aware of this?

What I'm trying to say is that it looks like you try to reinvent what
has already been implemented. pcie_ptm_enabled() is harmless in case the
platform cannot support it, and you are gating getcrosststamp() by FW
capabilities already - why do you need another check?

Re: [PATCH net] bnxt_en: enable PTM function

[Pavan Chebbi]

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

On Fri, Aug 7, 2026 at 7:30 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 07/08/2026 13:51, Pavan Chebbi wrote:
> > On Fri, Aug 7, 2026 at 2:16 AM Vadim Fedorenko
> > <vadim.fedorenko@linux.dev> wrote:
> >>
> >> On 06.08.2026 21:35, Michael Chan wrote:
> >>> On Thu, Aug 6, 2026 at 1:22 PM Vadim Fedorenko
> >>> <vadim.fedorenko@linux.dev> wrote:
> >>>>
> >>>> The patch mentioned in Fixes missed one main point of implementing
> >>>> proper PTM support. To make it fully operational it has to be explicitly
> >>>> enabled. Add missing call in probe callback and disable it in teardown
> >>>> callback.
> >>>
> >>> We were actually discussing this internally just a few weeks ago.  I
> >>> think the FW always enables it and that's why pci_enable_ptm() is not
> >>> strictly required.  I'll let Pavan comment on this further.  Thanks.
> >>
> >> In my test even though firmware was acting as requestor, pcie_ptm_enabled()
> >> returns false and disables setup of getcrosststamp callback. This happens
> >> because kernel doesn't know that PTM is enabled.
> >
> > I think the right thing to do would be to first check the capability
> > from FW (which actually decides whether we can support PTM on the
> > installed RC/platform) and then enable it in the kernel.
> > This is what we internally decided to do. Can you wait for us to
> > complete that work and send the patch?
>
>
> Could you please be a bit more specific on how FW decides? Does it
> remove PTM capability in case it's not possible?
>
> As you can see from the code of pci_enable_ptm(), sometimes parent
> device needs PTM enabled as well before starting TLVs, will your FW be
> aware of this?
>
> What I'm trying to say is that it looks like you try to reinvent what
> has already been implemented. pcie_ptm_enabled() is harmless in case the
> platform cannot support it, and you are gating getcrosststamp() by FW
> capabilities already - why do you need another check?

Ok I understand you now. Maybe it is fine to have
pcie_ptm_enabled()/disabled() called for this version of the driver.
Since FW currently always enables it, and these calls are harmless and
turn off .getcrosststamp on platforms that don't support PTM, both FW
and driver/kernel end up working in sync.
Only thing I would argue is should this be done in bnxt_ptp_init() and
clear()? Since as much as PTM is a PCIe feature, the cross timestamps
are tied with PTP.

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

Re: [PATCH net] bnxt_en: enable PTM function

[Vadim Fedorenko]

On 07/08/2026 17:13, Pavan Chebbi wrote:
> On Fri, Aug 7, 2026 at 7:30 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 07/08/2026 13:51, Pavan Chebbi wrote:
>>> On Fri, Aug 7, 2026 at 2:16 AM Vadim Fedorenko
>>> <vadim.fedorenko@linux.dev> wrote:
>>>>
>>>> On 06.08.2026 21:35, Michael Chan wrote:
>>>>> On Thu, Aug 6, 2026 at 1:22 PM Vadim Fedorenko
>>>>> <vadim.fedorenko@linux.dev> wrote:
>>>>>>
>>>>>> The patch mentioned in Fixes missed one main point of implementing
>>>>>> proper PTM support. To make it fully operational it has to be explicitly
>>>>>> enabled. Add missing call in probe callback and disable it in teardown
>>>>>> callback.
>>>>>
>>>>> We were actually discussing this internally just a few weeks ago.  I
>>>>> think the FW always enables it and that's why pci_enable_ptm() is not
>>>>> strictly required.  I'll let Pavan comment on this further.  Thanks.
>>>>
>>>> In my test even though firmware was acting as requestor, pcie_ptm_enabled()
>>>> returns false and disables setup of getcrosststamp callback. This happens
>>>> because kernel doesn't know that PTM is enabled.
>>>
>>> I think the right thing to do would be to first check the capability
>>> from FW (which actually decides whether we can support PTM on the
>>> installed RC/platform) and then enable it in the kernel.
>>> This is what we internally decided to do. Can you wait for us to
>>> complete that work and send the patch?
>>
>>
>> Could you please be a bit more specific on how FW decides? Does it
>> remove PTM capability in case it's not possible?
>>
>> As you can see from the code of pci_enable_ptm(), sometimes parent
>> device needs PTM enabled as well before starting TLVs, will your FW be
>> aware of this?
>>
>> What I'm trying to say is that it looks like you try to reinvent what
>> has already been implemented. pcie_ptm_enabled() is harmless in case the
>> platform cannot support it, and you are gating getcrosststamp() by FW
>> capabilities already - why do you need another check?
> 
> Ok I understand you now. Maybe it is fine to have
> pcie_ptm_enabled()/disabled() called for this version of the driver.
> Since FW currently always enables it, and these calls are harmless and
> turn off .getcrosststamp on platforms that don't support PTM, both FW
> and driver/kernel end up working in sync.
> Only thing I would argue is should this be done in bnxt_ptp_init() and
> clear()? Since as much as PTM is a PCIe feature, the cross timestamps
> are tied with PTP.

It makes sense to have PCIe things tied together, because PTM is PCIe
feature. Cross timestamps configuration are already in ptp area of the
driver, so no need to change anything around it

Re: [PATCH net] bnxt_en: enable PTM function

[Pavan Chebbi]

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

> > Ok I understand you now. Maybe it is fine to have
> > pcie_ptm_enabled()/disabled() called for this version of the driver.
> > Since FW currently always enables it, and these calls are harmless and
> > turn off .getcrosststamp on platforms that don't support PTM, both FW
> > and driver/kernel end up working in sync.
> > Only thing I would argue is should this be done in bnxt_ptp_init() and
> > clear()? Since as much as PTM is a PCIe feature, the cross timestamps
> > are tied with PTP.
>
> It makes sense to have PCIe things tied together, because PTM is PCIe
> feature. Cross timestamps configuration are already in ptp area of the
> driver, so no need to change anything around it

OK. Fine with me.
Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Thanks

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