From: "Nithin Nayak Sujir" <nsujir@broadcom•com>
To: "Richard Cochran" <richardcochran@gmail•com>
Cc: "Michael Chan" <mchan@broadcom•com>,
davem@davemloft•net, netdev@vger•kernel.org
Subject: Re: [PATCH 1/4 net-next] tg3: PTP - Add header definitions, initialization and hw access functions.
Date: Mon, 3 Dec 2012 13:49:29 -0800 [thread overview]
Message-ID: <50BD1E69.4040208@broadcom.com> (raw)
In-Reply-To: <20121203182338.GA2531@netboy.at.omicron.at>
Hi Richard,
Thanks for your comments.
On 12/03/2012 10:23 AM, Richard Cochran wrote:
> Please put me on CC for any PTP related patches.
> I have a few comments, below.
>
> On Sun, Dec 02, 2012 at 07:42:48PM -0800, Michael Chan wrote:
>> From: Matt Carlson <mcarlson@broadcom•com>
>>
>> This patch adds code to register/unregister the ptp clock and write
>> the reference clock. If a chip reset is performed, the hwclock is
>> reinitialized with the adjusted kernel time
>>
>> Signed-off-by: Nithin Nayak Sujir <nsujir@broadcom•com>
>> Signed-off-by: Michael Chan <mchan@broadcom•com>
>> ---
>> drivers/net/ethernet/broadcom/Kconfig | 1 +
>> drivers/net/ethernet/broadcom/tg3.c | 84 +++++++++++++++++++++++++++++++--
>> drivers/net/ethernet/broadcom/tg3.h | 60 ++++++++++++++++++++++-
>> 3 files changed, 137 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
>> index 4bd416b..f552673 100644
>> --- a/drivers/net/ethernet/broadcom/Kconfig
>> +++ b/drivers/net/ethernet/broadcom/Kconfig
>> @@ -102,6 +102,7 @@ config TIGON3
>> depends on PCI
>> select PHYLIB
>> select HWMON
>> + select PTP_1588_CLOCK
>> ---help---
>> This driver supports Broadcom Tigon3 based gigabit Ethernet cards.
>>
>> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
>> index 5cc976d..38047a9 100644
>> --- a/drivers/net/ethernet/broadcom/tg3.c
>> +++ b/drivers/net/ethernet/broadcom/tg3.c
>> @@ -54,6 +54,9 @@
>> #include <asm/byteorder.h>
>> #include <linux/uaccess.h>
>>
>> +#include <uapi/linux/net_tstamp.h>
>> +#include <linux/ptp_clock_kernel.h>
>> +
>> #ifdef CONFIG_SPARC
>> #include <asm/idprom.h>
>> #include <asm/prom.h>
>> @@ -5516,6 +5519,57 @@ static int tg3_setup_phy(struct tg3 *tp, int force_reset)
>> return err;
>> }
>>
>> +static void tg3_refclk_write(struct tg3 *tp, u64 newval)
>> +{
>> + tw32(TG3_EAV_REF_CLCK_CTL, TG3_EAV_REF_CLCK_CTL_STOP);
>> + tw32(TG3_EAV_REF_CLCK_LSB, newval & 0xffffffff);
>> + tw32(TG3_EAV_REF_CLCK_MSB, newval >> 32);
>> + tw32_f(TG3_EAV_REF_CLCK_CTL, TG3_EAV_REF_CLCK_CTL_RESUME);
>> +}
>> +
>> +static const struct ptp_clock_info tg3_ptp_caps = {
>> + .owner = THIS_MODULE,
>> + .name = "",
> Please use a static name here, something related to the driver, like
> "tigon3 clock" perhaps. There used to be drivers doing other things
> with this, but now the kernel doc from ptp_clock_kernel.h says:
>
> @name: A short "friendly name" to identify the clock and to
> help distinguish PHY based devices from MAC based ones.
> The string is not meant to be a unique id.
>
>> + .max_adj = 0,
>> + .n_alarm = 0,
>> + .n_ext_ts = 0,
>> + .n_per_out = 0,
>> + .pps = 0,
> You have left the methods as a bunch of NULL pointers. This will not
> fly, since someone might land on this commit during a bisect. In
> general, every patch in a series should result in a working kernel.
>
>> +};
>> +
>> +static void tg3_ptp_init(struct tg3 *tp)
>> +{
>> + if (!tg3_flag(tp, PTP_CAPABLE))
>> + return;
>> +
>> + /* Initialize the hardware clock to the system time. */
>> + tg3_refclk_write(tp, ktime_to_ns(ktime_get_real()));
>> + tp->ptp_adjust = 0;
>> +
>> + tp->ptp_info = tg3_ptp_caps;
>> + strncpy(tp->ptp_info.name, tp->dev->name, IFNAMSIZ);
>> +}
>> +
>> +static void tg3_ptp_resume(struct tg3 *tp)
>> +{
>> + if (!tg3_flag(tp, PTP_CAPABLE))
>> + return;
>> +
>> + tg3_refclk_write(tp, ktime_to_ns(ktime_get_real()) + tp->ptp_adjust);
>> + tp->ptp_adjust = 0;
>> +}
>> +
>> +static void tg3_ptp_fini(struct tg3 *tp)
>> +{
>> + if (!tg3_flag(tp, PTP_CAPABLE) ||
>> + !tp->ptp_clock)
> Overzealous line break.
>
>> + return;
>> +
>> + ptp_clock_unregister(tp->ptp_clock);
>> + tp->ptp_clock = NULL;
>> + tp->ptp_adjust = 0;
>> +}
>> +
>> static inline int tg3_irq_sync(struct tg3 *tp)
>> {
>> return tp->irq_sync;
>> @@ -6527,6 +6581,8 @@ static inline void tg3_netif_stop(struct tg3 *tp)
>>
>> static inline void tg3_netif_start(struct tg3 *tp)
>> {
>> + tg3_ptp_resume(tp);
>> +
>> /* NOTE: unconditional netif_tx_wake_all_queues is only
>> * appropriate so long as all callers are assured to
>> * have free tx slots (such as after tg3_init_hw)
>> @@ -10364,7 +10420,8 @@ static void tg3_ints_fini(struct tg3 *tp)
>> tg3_flag_clear(tp, ENABLE_TSS);
>> }
>>
>> -static int tg3_start(struct tg3 *tp, bool reset_phy, bool test_irq)
>> +static int tg3_start(struct tg3 *tp, bool reset_phy, bool test_irq,
>> + bool init)
>> {
>> struct net_device *dev = tp->dev;
>> int i, err;
>> @@ -10443,6 +10500,12 @@ static int tg3_start(struct tg3 *tp, bool reset_phy, bool test_irq)
>> tg3_flag_set(tp, INIT_COMPLETE);
>> tg3_enable_ints(tp);
>>
>> + if (init)
>> + tg3_ptp_init(tp);
>> + else
>> + tg3_ptp_resume(tp);
>> +
>> +
>> tg3_full_unlock(tp);
>>
>> netif_tx_start_all_queues(dev);
>> @@ -10540,11 +10603,19 @@ static int tg3_open(struct net_device *dev)
>>
>> tg3_full_unlock(tp);
>>
>> - err = tg3_start(tp, true, true);
>> + err = tg3_start(tp, true, true, true);
>> if (err) {
>> tg3_frob_aux_power(tp, false);
>> pci_set_power_state(tp->pdev, PCI_D3hot);
>> }
>> +
>> + if (tg3_flag(tp, PTP_CAPABLE)) {
>> + tp->ptp_clock = ptp_clock_register(&tp->ptp_info,
>> + &tp->pdev->dev);
>> + if (IS_ERR(tp->ptp_clock))
>> + tp->ptp_clock = NULL;
>> + }
>> +
>> return err;
>> }
>>
>> @@ -10552,6 +10623,8 @@ static int tg3_close(struct net_device *dev)
>> {
>> struct tg3 *tp = netdev_priv(dev);
>>
>> + tg3_ptp_fini(tp);
>> +
>> tg3_stop(tp);
>>
>> /* Clear stats across close / open calls */
>> @@ -11454,7 +11527,7 @@ static int tg3_set_channels(struct net_device *dev,
>>
>> tg3_carrier_off(tp);
>>
>> - tg3_start(tp, true, false);
>> + tg3_start(tp, true, false, false);
>>
>> return 0;
>> }
>> @@ -12507,7 +12580,6 @@ static void tg3_self_test(struct net_device *dev, struct ethtool_test *etest,
>> }
>>
>> tg3_full_lock(tp, irq_sync);
>> -
>> tg3_halt(tp, RESET_KIND_SUSPEND, 1);
>> err = tg3_nvram_lock(tp);
>> tg3_halt_cpu(tp, RX_CPU_BASE);
>> @@ -16598,8 +16670,8 @@ static void tg3_io_resume(struct pci_dev *pdev)
>> tg3_full_lock(tp, 0);
>> tg3_flag_set(tp, INIT_COMPLETE);
>> err = tg3_restart_hw(tp, 1);
>> - tg3_full_unlock(tp);
>> if (err) {
>> + tg3_full_unlock(tp);
> This is hunk or the next one somehow related to the PTP code?
> If not, they it should go into their own patch.
>
Yes, they are related. tg3_netif_start() now calls tg3_ptp_resume() to
reinitialize the hwclock after a chip reset. This should be inside a
full_lock(). This hunk moves the tg3_full_unlock() below tg3_netif_start(). It
also brings netif_device_attach() and tg3_timer_start() inside the lock() but
this seems to be ok since other places already do that.
I will fix the other comments in v2.
>> netdev_err(netdev, "Cannot restart hardware after reset.\n");
>> goto done;
>> }
>> @@ -16610,6 +16682,8 @@ static void tg3_io_resume(struct pci_dev *pdev)
>>
>> tg3_netif_start(tp);
>>
>> + tg3_full_unlock(tp);
>> +
>> tg3_phy_start(tp);
>>
>> done:
> Thanks,
> Richard
>
prev parent reply other threads:[~2012-12-03 21:49 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-03 3:42 [PATCH 1/4 net-next] tg3: PTP - Add header definitions, initialization and hw access functions Michael Chan
2012-12-03 3:42 ` [PATCH 2/4 net-next] tg3: PTP - Implement the ptp api and ethtool functions Michael Chan
2012-12-03 3:42 ` [PATCH 3/4 net-next] tg3: PTP - Add the hardware timestamp ioctl Michael Chan
2012-12-03 3:42 ` [PATCH 4/4 net-next] tg3: PTP - Enable the timestamping feature in hardware and fill skb tx/rx timestamps Michael Chan
2012-12-03 19:41 ` [PATCH 3/4 net-next] tg3: PTP - Add the hardware timestamp ioctl Richard Cochran
2012-12-03 18:51 ` [PATCH 2/4 net-next] tg3: PTP - Implement the ptp api and ethtool functions Richard Cochran
2012-12-03 21:52 ` Nithin Nayak Sujir
2012-12-04 7:53 ` Richard Cochran
2012-12-03 18:23 ` [PATCH 1/4 net-next] tg3: PTP - Add header definitions, initialization and hw access functions Richard Cochran
2012-12-03 21:49 ` Nithin Nayak Sujir [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50BD1E69.4040208@broadcom.com \
--to=nsujir@broadcom$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=mchan@broadcom$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=richardcochran@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox