| Message ID | 20220127102333.987195-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | net: lan966x: Add PTP Hardward Clock support | expand |
On Thu, Jan 27, 2022 at 11:23:28AM +0100, Horatiu Vultur wrote: > This patch adds the registers that will be used to configure the PHC in > the HW. See "This patch" in Documentation/process/submitting-patches.rst Thanks, Richard
On Thu, Jan 27, 2022 at 11:23:29AM +0100, Horatiu Vultur wrote: > The lan966x has 3 PHC. Enable each of them, for now all the > timestamping is happening on the first PHC. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> Acked-by: Richard Cochran <richardcochran@gmail.com>
On Thu, Jan 27, 2022 at 11:23:29AM +0100, Horatiu Vultur wrote: > +static int lan966x_ptp_phc_init(struct lan966x *lan966x, > + int index, > + struct ptp_clock_info *clock_info) > +{ > + struct lan966x_phc *phc = &lan966x->phc[index]; > + > + phc->info = *clock_info; > + phc->clock = ptp_clock_register(&phc->info, lan966x->dev); > + if (IS_ERR(phc->clock)) > + return PTR_ERR(phc->clock); > + > + phc->index = index; > + phc->lan966x = lan966x; > + > + /* PTP Rx stamping is always enabled. */ > + phc->hwtstamp_config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; Not true -- you allow it to be disabled in the next patch! Thanks, Richard > + > + return 0; > +}
On Thu, Jan 27, 2022 at 11:23:30AM +0100, Horatiu Vultur wrote: > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c > index 69d8f43e2b1b..9ff4d3fca5a1 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c > @@ -35,6 +35,90 @@ static u64 lan966x_ptp_get_nominal_value(void) > return res; > } > > +int lan966x_ptp_hwtstamp_set(struct lan966x_port *port, struct ifreq *ifr) > +{ > + struct lan966x *lan966x = port->lan966x; > + bool l2 = false, l4 = false; > + struct hwtstamp_config cfg; > + struct lan966x_phc *phc; > + > + /* For now don't allow to run ptp on ports that are part of a bridge, > + * because in case of transparent clock the HW will still forward the > + * frames, so there would be duplicate frames > + */ > + if (lan966x->bridge_mask & BIT(port->chip_port)) > + return -EINVAL; > + > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) > + return -EFAULT; > + > + switch (cfg.tx_type) { > + case HWTSTAMP_TX_ON: > + port->ptp_cmd = IFH_REW_OP_TWO_STEP_PTP; > + break; > + case HWTSTAMP_TX_ONESTEP_SYNC: > + port->ptp_cmd = IFH_REW_OP_ONE_STEP_PTP; > + break; > + case HWTSTAMP_TX_OFF: > + port->ptp_cmd = IFH_REW_OP_NOOP; > + break; > + default: > + return -ERANGE; > + } > + > + mutex_lock(&lan966x->ptp_lock); No need to lock stack variables. Move locking down to ... > + switch (cfg.rx_filter) { > + case HWTSTAMP_FILTER_NONE: > + break; > + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: > + l4 = true; > + break; > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > + l2 = true; > + break; > + case HWTSTAMP_FILTER_PTP_V2_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > + l2 = true; > + l4 = true; > + break; > + default: > + mutex_unlock(&lan966x->ptp_lock); > + return -ERANGE; > + } > + > + if (l2 && l4) > + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; > + else if (l2) > + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; > + else if (l4) > + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT; > + else > + cfg.rx_filter = HWTSTAMP_FILTER_NONE; > + > + /* Commit back the result & save it */ ... here > + phc = &lan966x->phc[LAN966X_PHC_PORT]; > + memcpy(&phc->hwtstamp_config, &cfg, sizeof(cfg)); > + mutex_unlock(&lan966x->ptp_lock); > + > + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0; > +} Thanks, Richard
The 01/27/2022 07:18, Richard Cochran wrote: Hi Richard, > > On Thu, Jan 27, 2022 at 11:23:28AM +0100, Horatiu Vultur wrote: > > This patch adds the registers that will be used to configure the PHC in > > the HW. > > See "This patch" in Documentation/process/submitting-patches.rst Yes, I will update this in the next version. > > Thanks, > Richard
The 01/27/2022 13:55, Richard Cochran wrote: > > On Thu, Jan 27, 2022 at 11:23:30AM +0100, Horatiu Vultur wrote: > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c > > index 69d8f43e2b1b..9ff4d3fca5a1 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c > > @@ -35,6 +35,90 @@ static u64 lan966x_ptp_get_nominal_value(void) > > return res; > > } > > > > +int lan966x_ptp_hwtstamp_set(struct lan966x_port *port, struct ifreq *ifr) > > +{ > > + struct lan966x *lan966x = port->lan966x; > > + bool l2 = false, l4 = false; > > + struct hwtstamp_config cfg; > > + struct lan966x_phc *phc; > > + > > + /* For now don't allow to run ptp on ports that are part of a bridge, > > + * because in case of transparent clock the HW will still forward the > > + * frames, so there would be duplicate frames > > + */ > > + if (lan966x->bridge_mask & BIT(port->chip_port)) > > + return -EINVAL; > > + > > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) > > + return -EFAULT; > > + > > + switch (cfg.tx_type) { > > + case HWTSTAMP_TX_ON: > > + port->ptp_cmd = IFH_REW_OP_TWO_STEP_PTP; > > + break; > > + case HWTSTAMP_TX_ONESTEP_SYNC: > > + port->ptp_cmd = IFH_REW_OP_ONE_STEP_PTP; > > + break; > > + case HWTSTAMP_TX_OFF: > > + port->ptp_cmd = IFH_REW_OP_NOOP; > > + break; > > + default: > > + return -ERANGE; > > + } > > + > > + mutex_lock(&lan966x->ptp_lock); > > No need to lock stack variables. Move locking down to ... Good catch, will do that. > > > + switch (cfg.rx_filter) { > > + case HWTSTAMP_FILTER_NONE: > > + break; > > + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: > > + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: > > + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: > > + l4 = true; > > + break; > > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > > + l2 = true; > > + break; > > + case HWTSTAMP_FILTER_PTP_V2_EVENT: > > + case HWTSTAMP_FILTER_PTP_V2_SYNC: > > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > > + l2 = true; > > + l4 = true; > > + break; > > + default: > > + mutex_unlock(&lan966x->ptp_lock); > > + return -ERANGE; > > + } > > + > > + if (l2 && l4) > > + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; > > + else if (l2) > > + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; > > + else if (l4) > > + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT; > > + else > > + cfg.rx_filter = HWTSTAMP_FILTER_NONE; > > + > > + /* Commit back the result & save it */ > > ... here > > > + phc = &lan966x->phc[LAN966X_PHC_PORT]; > > + memcpy(&phc->hwtstamp_config, &cfg, sizeof(cfg)); > > + mutex_unlock(&lan966x->ptp_lock); > > + > > + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0; > > +} > > Thanks, > Richard
The 01/27/2022 07:28, Richard Cochran wrote: > > On Thu, Jan 27, 2022 at 11:23:29AM +0100, Horatiu Vultur wrote: > > > +static int lan966x_ptp_phc_init(struct lan966x *lan966x, > > + int index, > > + struct ptp_clock_info *clock_info) > > +{ > > + struct lan966x_phc *phc = &lan966x->phc[index]; > > + > > + phc->info = *clock_info; > > + phc->clock = ptp_clock_register(&phc->info, lan966x->dev); > > + if (IS_ERR(phc->clock)) > > + return PTR_ERR(phc->clock); > > + > > + phc->index = index; > > + phc->lan966x = lan966x; > > + > > + /* PTP Rx stamping is always enabled. */ > > + phc->hwtstamp_config.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; > > Not true -- you allow it to be disabled in the next patch! Actually the other patch is wrong. The HW will timestamp all the frames. I will update the other patch in the next version. > > Thanks, > Richard > > > > + > > + return 0; > > +}