Message-ID:
 <PAXPR04MB85101EBA548F05B21D87A7218851A@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Thu, 17 Jul 2025 08:42:15 +0000
From: Wei Fang <wei.fang@....com>
To: Frank Li <frank.li@....com>
CC: "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"richardcochran@...il.com" <richardcochran@...il.com>, Claudiu Manoil
	<claudiu.manoil@....com>, Vladimir Oltean <vladimir.oltean@....com>, Clark
 Wang <xiaoning.wang@....com>, "andrew+netdev@...n.ch"
	<andrew+netdev@...n.ch>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>,
	"shawnguo@...nel.org" <shawnguo@...nel.org>, "s.hauer@...gutronix.de"
	<s.hauer@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>, "F.S.
 Peng" <fushi.peng@....com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: RE: [PATCH v2 net-next 03/14] ptp: netc: add NETC Timer PTP driver
 support

> > +static u64 netc_timer_cnt_read(struct netc_timer *priv)
> > +{
> > +	u32 tmr_cnt_l, tmr_cnt_h;
> > +	u64 ns;
> > +
> > +	/* The user must read the TMR_CNC_L register first to get
> > +	 * correct 64-bit TMR_CNT_H/L counter values.
> > +	 */
> 
> Need comment about there are snapshot in side chip. So it is safe to
> read NETC_TMR_CNT_L then read NETC_TMR_CNT_H, otherwise
> NETC_TMR_CNT_L may change, during read NETC_TMR_CNT_H.
> 

Okay, I will improve the comment.

> > +static int netc_timer_pci_probe(struct pci_dev *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct netc_timer *priv;
> > +	int err, len;
> > +
> > +	pcie_flr(pdev);
> > +	err = pci_enable_device_mem(pdev);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "Failed to enable device\n");
> > +
> > +	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > +	if (err) {
> > +		dev_err(dev, "dma_set_mask_and_coherent() failed, err:%pe\n",
> > +			ERR_PTR(err));
> > +		goto disable_dev;
> > +	}
> 
> Needn't check return value for dma_set_mask_and_coherent() when mask >=
> 32
> It is never return fail.

Okay, I will remove the check.

> 
> use devm_add_action_or_reset() to avoid all goto here. then dev_err =>
> dev_err_probe().
> 

hm..., netdev does not encourage the use of auto-clean APIs such as "devm_".
https://elixir.bootlin.com/linux/v6.16-rc6/source/Documentation/process/maintainer-netdev.rst#L391

I can use helpers like devm_kzalloc() and devm_clk_xx(), but for me, using
devm_add_action_or_reset() does not bring much benefit.
> > +
> > +	err = pci_request_mem_regions(pdev, KBUILD_MODNAME);
> > +	if (err) {
> > +		dev_err(dev, "pci_request_regions() failed, err:%pe\n",
> > +			ERR_PTR(err));
> > +		goto disable_dev;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> 
> devm_kzalloc()
> 
> > +	if (!priv) {
> > +		err = -ENOMEM;
> > +		goto release_mem_regions;
> > +	}
> > +
> > +	priv->pdev = pdev;
> > +	len = pci_resource_len(pdev, NETC_TMR_REGS_BAR);
> > +	priv->base = ioremap(pci_resource_start(pdev, NETC_TMR_REGS_BAR),
> len);
> > +	if (!priv->base) {
> > +		err = -ENXIO;
> > +		dev_err(dev, "ioremap() failed\n");
> > +		goto free_priv;
> > +	}
> 
> pci_ioremap_bar()?
> 

Great, thanks.

> > +static int netc_timer_get_reference_clk_source(struct netc_timer *priv)
> > +{
> > +	struct device *dev = &priv->pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	const char *clk_name = NULL;
> > +	u64 ns = NSEC_PER_SEC;
> > +
> > +	/* Select NETC system clock as the reference clock by default */
> > +	priv->clk_select = NETC_TMR_SYSTEM_CLK;
> > +	priv->clk_freq = NETC_TMR_SYSCLK_333M;
> > +	priv->period = div_u64(ns << 32, priv->clk_freq);
> > +
> > +	if (!np)
> > +		return 0;
> > +
> > +	of_property_read_string(np, "clock-names", &clk_name);
> > +	if (!clk_name)
> > +		return 0;
> 
> Don't perfer parser this property by youself.
> 
> you can use devm_clk_bulk_get_all() clk_bulk_data have clock name, you
> can use it.
> 
> or use loop to try 3 time devm_clk_get(), first one return success is what
> you want.
> 
> > +
> > +	/* Update the clock source of the reference clock if the clock
> > +	 * name is specified in DTS node.
> > +	 */
> > +	if (!strcmp(clk_name, "system"))
> 
> strncmp();
> 
> "..." maybe longer than len of clc_name.
> 
> > +		priv->clk_select = NETC_TMR_SYSTEM_CLK;
> > +	else if (!strcmp(clk_name, "ccm_timer"))
> > +		priv->clk_select = NETC_TMR_CCM_TIMER1;
> > +	else if (!strcmp(clk_name, "ext_1588"))
> > +		priv->clk_select = NETC_TMR_EXT_OSC;
> > +	else
> > +		return -EINVAL;
> > +
> > +	priv->src_clk = devm_clk_get(dev, clk_name);
> > +	if (IS_ERR(priv->src_clk)) {
> > +		dev_err(dev, "Failed to get reference clock source\n");
> > +		return PTR_ERR(priv->src_clk);
> > +	}
> > +
> > +	priv->clk_freq = clk_get_rate(priv->src_clk);
> > +	priv->period = div_u64(ns << 32, priv->clk_freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int netc_timer_parse_dt(struct netc_timer *priv)
> > +{
> > +	return netc_timer_get_reference_clk_source(priv);
> > +}
> > +
> > +static irqreturn_t netc_timer_isr(int irq, void *data)
> > +{
> > +	struct netc_timer *priv = data;
> > +	u32 tmr_event, tmr_emask;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +
> > +	tmr_event = netc_timer_rd(priv, NETC_TMR_TEVENT);
> > +	tmr_emask = netc_timer_rd(priv, NETC_TMR_TEMASK);
> > +
> > +	tmr_event &= tmr_emask;
> > +	if (tmr_event & TMR_TEVENT_ALM1EN)
> > +		netc_timer_alarm_write(priv, NETC_TMR_DEFAULT_ALARM, 0);
> > +
> > +	if (tmr_event & TMR_TEVENT_ALM2EN)
> > +		netc_timer_alarm_write(priv, NETC_TMR_DEFAULT_ALARM, 1);
> > +
> > +	/* Clear interrupts status */
> > +	netc_timer_wr(priv, NETC_TMR_TEVENT, tmr_event);
> 
> clean irq status should be just after read it (before "tmr_event &= tmr_emask;)
> 

we only need to clear the status bits of enabled irqs (tmr_event &= tmr_emask),
Interrupts not enabled by tmr_emask will not generate interrupt. I can move
the clear irq status after " tmr_event &= tmr_emask;"

> otherwise the new irq maybe missed by netc_timer_alarm_write()


> 
> > +
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int netc_timer_init_msix_irq(struct netc_timer *priv)
> > +{
> > +	struct pci_dev *pdev = priv->pdev;
> > +	char irq_name[64];
> > +	int err, n;
> > +
> > +	n = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
> > +	if (n != 1) {
> > +		err = (n < 0) ? n : -EPERM;
> > +		dev_err(&pdev->dev, "pci_alloc_irq_vectors() failed\n");
> > +		return err;
> > +	}
> > +
> > +	priv->irq = pci_irq_vector(pdev, 0);
> > +	snprintf(irq_name, sizeof(irq_name), "ptp-netc %s", pci_name(pdev));
> > +	err = request_irq(priv->irq, netc_timer_isr, 0, irq_name, priv);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "request_irq() failed\n");
> > +		pci_free_irq_vectors(pdev);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void netc_timer_free_msix_irq(struct netc_timer *priv)
> > +{
> > +	struct pci_dev *pdev = priv->pdev;
> > +
> > +	disable_irq(priv->irq);
> > +	free_irq(priv->irq, priv);
> > +	pci_free_irq_vectors(pdev);
> > +}
> > +
> > +static int netc_timer_probe(struct pci_dev *pdev,
> > +			    const struct pci_device_id *id)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct netc_timer *priv;
> > +	int err;
> > +
> > +	err = netc_timer_pci_probe(pdev);
> > +	if (err)
> > +		return err;
> > +
> > +	priv = pci_get_drvdata(pdev);
> > +	err = netc_timer_parse_dt(priv);
> > +	if (err) {
> > +		dev_err(dev, "Failed to parse DT node\n");
> > +		goto timer_pci_remove;
> > +	}
> > +
> > +	priv->caps = netc_timer_ptp_caps;
> > +	priv->oclk_prsc = NETC_TMR_DEFAULT_PRSC;
> > +	priv->phc_index = -1; /* initialize it as an invalid index */
> > +	spin_lock_init(&priv->lock);
> > +
> > +	err = clk_prepare_enable(priv->src_clk);
> 
> use devm_ function
> 
> > +	if (err) {
> > +		dev_err(dev, "Failed to enable timer source clock\n");
> > +		goto timer_pci_remove;
> > +	}
> > +
> > +	err = netc_timer_init_msix_irq(priv);
> > +	if (err)
> > +		goto disable_clk;
> > +
> > +	netc_timer_init(priv);
> > +	priv->clock = ptp_clock_register(&priv->caps, dev);
> > +	if (IS_ERR(priv->clock)) {
> > +		err = PTR_ERR(priv->clock);
> > +		goto free_msix_irq;
> > +	}
> > +
> > +	priv->phc_index = ptp_clock_index(priv->clock);
> > +
> > +	return 0;
> > +
> > +free_msix_irq:
> > +	netc_timer_free_msix_irq(priv);
> > +disable_clk:
> > +	clk_disable_unprepare(priv->src_clk);
> > +timer_pci_remove:
> > +	netc_timer_pci_remove(pdev);
> 
> devm_add_action_or_reset() to simpify goto and netc_timer_remove().
> 
> > +
> > +	return err;
> > +}
> > +
> > +static void netc_timer_remove(struct pci_dev *pdev)
> > +{
> > +	struct netc_timer *priv = pci_get_drvdata(pdev);
> > +
> > +	ptp_clock_unregister(priv->clock);
> > +	netc_timer_free_msix_irq(priv);
> > +	clk_disable_unprepare(priv->src_clk);
> > +	netc_timer_pci_remove(pdev);
> > +}
> > +
> > +static const struct pci_device_id netc_timer_id_table[] = {
> > +	{ PCI_DEVICE(NETC_TMR_PCI_VENDOR, NETC_TMR_PCI_DEVID) },
> > +	{ 0, } /* End of table. */
> 
> just { }
> 
> needn't /* End of table. */
> 
> Frank
> > +};
> > +MODULE_DEVICE_TABLE(pci, netc_timer_id_table);
> > +
> > +static struct pci_driver netc_timer_driver = {
> > +	.name = KBUILD_MODNAME,
> > +	.id_table = netc_timer_id_table,
> > +	.probe = netc_timer_probe,
> > +	.remove = netc_timer_remove,
> > +};
> > +module_pci_driver(netc_timer_driver);
> > +
> > +MODULE_DESCRIPTION("NXP NETC Timer PTP Driver");
> > +MODULE_LICENSE("Dual BSD/GPL");
> > diff --git a/include/linux/fsl/netc_global.h b/include/linux/fsl/netc_global.h
> > index fdecca8c90f0..59c835e67ada 100644
> > --- a/include/linux/fsl/netc_global.h
> > +++ b/include/linux/fsl/netc_global.h
> > @@ -1,10 +1,11 @@
> >  /* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> > -/* Copyright 2024 NXP
> > +/* Copyright 2024-2025 NXP
> >   */
> >  #ifndef __NETC_GLOBAL_H
> >  #define __NETC_GLOBAL_H
> >
> >  #include <linux/io.h>
> > +#include <linux/pci.h>
> >
> >  static inline u32 netc_read(void __iomem *reg)
> >  {
> > @@ -16,4 +17,13 @@ static inline void netc_write(void __iomem *reg, u32
> val)
> >  	iowrite32(val, reg);
> >  }
> >
> > +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK_NETC)
> > +int netc_timer_get_phc_index(struct pci_dev *timer_pdev);
> > +#else
> > +static inline int netc_timer_get_phc_index(struct pci_dev *timer_pdev)
> > +{
> > +	return -ENODEV;
> > +}
> > +#endif
> > +
> >  #endif
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists