RE: [PATCH v2 net-next 9/9] lan743x: Add PTP support
> > Can you clarify what is poor and what would be better? > > For example, should I change "X != 0" to "X ? true : false". > > Look at this: > lan743x_ptp_tx_timestamp_skb(tx->adapter, >buffer_info->skb, >(buffer_info->flags & > > TX_BUFFER_INFO_FLAG_IGNORE_SYNC) >!= 0); > > Can't you reduce > > (buffer_info->flags & TX_BUFFER_INFO_FLAG_IGNORE_SYNC) != 0 > > into a local variable: > > lan743x_ptp_tx_timestamp_skb(tx->adapter, buffer_info- > >skb, xyz); ? > > > So you mean PPS is not intended to generate a physical signal? > > Yes. > > > It is only intended to call ptp_clock_event? > > Yes. > > > I can configure the hardware to generate an interrupt each second and > > then call ptp_clock_event. Would that satisfy the pps requirements? > > Yes. > > > Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals? > > Yes. > > Thanks, > Richard Richard, Thank you for your quick answers. Bryan
Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support
On Wed, Jul 18, 2018 at 08:04:08PM +, bryan.whiteh...@microchip.com wrote:
> Thank you for your detailed feedback. I'm working on it now, but I feel it
> will take a little extra time to complete. Therefor I'm planning to remove
> PTP support from this patch series, and resubmit it in a new patch later.
Ok.
> > > + if (cleanup) {
> > > + lan743x_ptp_unrequest_tx_timestamp(tx->adapter);
> > > + dev_kfree_skb(buffer_info->skb);
> > > + } else {
> > > + lan743x_ptp_tx_timestamp_skb(tx->adapter,
> > > + buffer_info->skb,
> > > + (buffer_info->flags &
> > > +
> > TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
> > > + != 0);
> >
> > This is poor coding style. Please find a better way.
>
> Can you clarify what is poor and what would be better?
> For example, should I change "X != 0" to "X ? true : false".
Look at this:
lan743x_ptp_tx_timestamp_skb(tx->adapter,
buffer_info->skb,
(buffer_info->flags &
TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
!= 0);
Can't you reduce
(buffer_info->flags & TX_BUFFER_INFO_FLAG_IGNORE_SYNC) != 0
into a local variable:
lan743x_ptp_tx_timestamp_skb(tx->adapter, buffer_info->skb,
xyz);
?
> So you mean PPS is not intended to generate a physical signal?
Yes.
> It is only intended to call ptp_clock_event?
Yes.
> I can configure the hardware to generate an interrupt each second and then
> call
> ptp_clock_event. Would that satisfy the pps requirements?
Yes.
> Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals?
Yes.
Thanks,
Richard
RE: [PATCH v2 net-next 9/9] lan743x: Add PTP support
Hi Richard,
Thank you for your detailed feedback. I'm working on it now, but I feel it will
take a little extra time to complete. Therefor I'm planning to remove PTP
support from this patch series, and resubmit it in a new patch later.
I also have a few questions below.
> -Original Message-
> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: Thursday, July 12, 2018 11:32 PM
> To: Bryan Whitehead - C21958
> Cc: da...@davemloft.net; netdev@vger.kernel.org; UNGLinuxDriver
>
> Subject: Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support
>
...
> > + if (cleanup) {
> > + lan743x_ptp_unrequest_tx_timestamp(tx->adapter);
> > + dev_kfree_skb(buffer_info->skb);
> > + } else {
> > + lan743x_ptp_tx_timestamp_skb(tx->adapter,
> > +buffer_info->skb,
> > +(buffer_info->flags &
> > +
> TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
> > +!= 0);
>
> This is poor coding style. Please find a better way.
Can you clarify what is poor and what would be better?
For example, should I change "X != 0" to "X ? true : false".
> > +#ifdef CONFIG_PTP_1588_CLOCK
> > +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter) {
> > + struct lan743x_ptp *ptp = >ptp;
> > + u32 current_seconds = 0;
> > + u32 target_seconds = 0;
> > + u32 general_config = 0;
> > + int result = -ENODEV;
> > + int pps_bit = 0;
>
> So this function is really *not* implementing the PTP_CLK_REQ_PPS feature
> but rather the PTP_CLK_REQ_PEROUT with a period of once per second.
>
> PTP_CLK_REQ_PPS means placing a PPS event into the kernel's "hardpps"
> subsystem by calling ptp_clock_event().
>
> I'm sorry this isn't really documented. I should fix that.
>
> If you HW can output arbitrary signals, then you should implement
> PTP_CLK_REQ_PEROUT. In any case, you shouldn't advertise the
> ptp_clock_info.pps capability.
So you mean PPS is not intended to generate a physical signal?
It is only intended to call ptp_clock_event?
I can configure the hardware to generate an interrupt each second and then call
ptp_clock_event. Would that satisfy the pps requirements?
Regarding PTP_CLK_REQ_PEROUT. Is that intended for physical signals?
Thanks,
Bryan
Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support
On Thu, Jul 12, 2018 at 03:05:06PM -0400, Bryan Whitehead wrote:
> +static int lan743x_ethtool_get_ts_info(struct net_device *netdev,
> +struct ethtool_ts_info *ts_info)
> +{
> + struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> + ts_info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> +SOF_TIMESTAMPING_RX_SOFTWARE |
> +SOF_TIMESTAMPING_SOFTWARE |
> +SOF_TIMESTAMPING_TX_HARDWARE |
> +SOF_TIMESTAMPING_RX_HARDWARE |
> +SOF_TIMESTAMPING_RAW_HARDWARE;
> +#ifdef CONFIG_PTP_1588_CLOCK
No need for this ifdeferry - ptp_clock_index() already returns -1 in
that case.
> + if (adapter->ptp.ptp_clock)
> + ts_info->phc_index = ptp_clock_index(adapter->ptp.ptp_clock);
> + else
> + ts_info->phc_index = -1;
> +#else
> + ts_info->phc_index = -1;
> +#endif
> + ts_info->tx_types = BIT(HWTSTAMP_TX_OFF) |
> + BIT(HWTSTAMP_TX_ON);
> + ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> + BIT(HWTSTAMP_FILTER_ALL);
> + return 0;
> +}
> +
> @@ -690,6 +717,7 @@ const struct ethtool_ops lan743x_ethtool_ops = {
> .get_rxfh_indir_size = lan743x_ethtool_get_rxfh_indir_size,
> .get_rxfh = lan743x_ethtool_get_rxfh,
> .set_rxfh = lan743x_ethtool_set_rxfh,
> + .get_ts_info = lan743x_ethtool_get_ts_info,
> .get_eee = lan743x_ethtool_get_eee,
> .set_eee = lan743x_ethtool_set_eee,
> .get_link_ksettings = phy_ethtool_get_link_ksettings,
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 953b581..ca9ae49 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -267,6 +267,10 @@ static void lan743x_intr_shared_isr(void *context, u32
> int_sts, u32 flags)
> lan743x_intr_software_isr(adapter);
> int_sts &= ~INT_BIT_SW_GP_;
> }
> + if (int_sts & INT_BIT_1588_) {
> + lan743x_ptp_isr(adapter);
> + int_sts &= ~INT_BIT_1588_;
> + }
> }
> if (int_sts)
> lan743x_csr_write(adapter, INT_EN_CLR, int_sts);
> @@ -976,6 +980,7 @@ static void lan743x_phy_link_status_change(struct
> net_device *netdev)
> ksettings.base.duplex,
> local_advertisement,
> remote_advertisement);
> + lan743x_ptp_update_latency(adapter, ksettings.base.speed);
> }
> }
>
> @@ -1256,11 +1261,29 @@ static void lan743x_tx_release_desc(struct lan743x_tx
> *tx,
> buffer_info->dma_ptr = 0;
> buffer_info->buffer_length = 0;
> }
> - if (buffer_info->skb) {
> + if (!buffer_info->skb)
> + goto clear_active;
> +
> + if (!(buffer_info->flags &
> + TX_BUFFER_INFO_FLAG_TIMESTAMP_REQUESTED)) {
Bad line break.
> dev_kfree_skb(buffer_info->skb);
> - buffer_info->skb = NULL;
> + goto clear_skb;
> }
>
> + if (cleanup) {
> + lan743x_ptp_unrequest_tx_timestamp(tx->adapter);
> + dev_kfree_skb(buffer_info->skb);
> + } else {
> + lan743x_ptp_tx_timestamp_skb(tx->adapter,
> + buffer_info->skb,
> + (buffer_info->flags &
> + TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
> + != 0);
This is poor coding style. Please find a better way.
> + }
> +
> +clear_skb:
> + buffer_info->skb = NULL;
> +
> clear_active:
> buffer_info->flags &= ~TX_BUFFER_INFO_FLAG_ACTIVE;
>
> @@ -1321,10 +1344,25 @@ static int lan743x_tx_get_avail_desc(struct
> lan743x_tx *tx)
> return last_head - last_tail - 1;
> }
>
> +void lan743x_tx_set_timestamping_mode(struct lan743x_tx *tx,
> + bool enable_timestamping,
> + bool enable_onestep_sync)
> +{
> + if (enable_timestamping)
> + tx->ts_flags |= TX_TS_FLAG_TIMESTAMPING_ENABLED;
> + else
> + tx->ts_flags &= ~TX_TS_FLAG_TIMESTAMPING_ENABLED;
> + if (enable_onestep_sync)
> + tx->ts_flags |= TX_TS_FLAG_ONE_STEP_SYNC;
> + else
> + tx->ts_flags &= ~TX_TS_FLAG_ONE_STEP_SYNC;
> +}
> +
> static int lan743x_tx_frame_start(struct lan743x_tx *tx,
> unsigned char *first_buffer,
> unsigned int first_buffer_length,
> unsigned int frame_length,
> +
