| Message ID | 20200320103726.32559-7-yangbo.lu@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Support programmable pins for Ocelot PTP driver | expand |
Hi Yangbo, On Fri, 20 Mar 2020 at 12:42, Yangbo Lu <yangbo.lu@nxp.com> wrote: > > Support 4 programmable pins for only one function periodic > signal for now. Since the hardware is not able to support > absolute start time, driver starts periodic signal immediately. > Are you absolutely sure it doesn't support absolute start time? Because that would mean it's pretty useless if the phase of the PTP clock signal is out of control. I tested your patch on the LS1028A-RDB board using the following commands: # Select PEROUT function and assign a channel to each of pins SWITCH_1588_DAT0 and SWITCH_1588_DAT1 echo '2 0' > /sys/class/ptp/ptp1/pins/switch_1588_dat0 echo '2 1' > /sys/class/ptp/ptp1/pins/switch_1588_dat1 # Generate pulses with 1 second period on channel 0 echo '0 0 0 1 0' > /sys/class/ptp/ptp1/period # Generate pulses with 1 second period on channel 1 echo '1 0 0 1 0' > /sys/class/ptp/ptp1/period And here is what I get: https://drive.google.com/open?id=1ErWufJL0TWv6hKDQdF1pRL5gn4hn4X-r So the periodic output really starts 'now' just like the print says, so the output from DAT0 is not even in sync with DAT1. > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > --- Thanks, -Vladimir
Hi Vladimir and Richard, > -----Original Message----- > From: Vladimir Oltean <olteanv@gmail.com> > Sent: Friday, March 20, 2020 9:21 PM > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: lkml <linux-kernel@vger.kernel.org>; netdev <netdev@vger.kernel.org>; > David S . Miller <davem@davemloft.net>; Richard Cochran > <richardcochran@gmail.com>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > Hi Yangbo, > > On Fri, 20 Mar 2020 at 12:42, Yangbo Lu <yangbo.lu@nxp.com> wrote: > > > > Support 4 programmable pins for only one function periodic > > signal for now. Since the hardware is not able to support > > absolute start time, driver starts periodic signal immediately. > > > > Are you absolutely sure it doesn't support absolute start time? > Because that would mean it's pretty useless if the phase of the PTP > clock signal is out of control. I'm absolutely sure that absolute start time is not supported for periodic clock unless reference manual is wrong. And I don’t think we need to consider phase for periodic clock which is with a specified period. But PPS is different. Pulse should be generated must after seconds increased. The waveform_high/low should be configurable for phase and pulse width if supported. This is supported by hardware but was not implemented by this patch. I was considering to add later. In my one previous patch, I was suggested to implement PPS with programmable pin periodic clock function. But I didn’t find how should PPS be implemented with periodic clock function after checking ptp driver. https://patchwork.ozlabs.org/patch/1215464/ Vladimir talked with me, for the special PPS case, we may consider, if (req.perout.period.sec ==1 && req.perout.period.nsec == 0) and configure WAVEFORM_LOW to be equal to req_perout.start.nsec. Richard, do you think is it ok? And another problem I am facing is, in .enable() callback (PTP_CLK_REQ_PEROUT request) I defined. /* * TODO: support disabling function * When ptp_disable_pinfunc() is to disable function, * it has already held pincfg_mux. * However ptp_find_pin() in .enable() called also needs * to hold pincfg_mux. * This causes dead lock. So, just return for function * disabling, and this needs fix-up. */ Hope some suggestions here. Thanks a lot. > > I tested your patch on the LS1028A-RDB board using the following commands: > > # Select PEROUT function and assign a channel to each of pins > SWITCH_1588_DAT0 and SWITCH_1588_DAT1 > echo '2 0' > /sys/class/ptp/ptp1/pins/switch_1588_dat0 > echo '2 1' > /sys/class/ptp/ptp1/pins/switch_1588_dat1 > # Generate pulses with 1 second period on channel 0 > echo '0 0 0 1 0' > /sys/class/ptp/ptp1/period > # Generate pulses with 1 second period on channel 1 > echo '1 0 0 1 0' > /sys/class/ptp/ptp1/period > > And here is what I get: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdrive.g > oogle.com%2Fopen%3Fid%3D1ErWufJL0TWv6hKDQdF1pRL5gn4hn4X-r& > data=02%7C01%7Cyangbo.lu%40nxp.com%7Cbd3e65bdaabb4999737d08d7c > cd17eee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63720307 > 2457124468&sdata=4D97D9ZoA%2FDJeSAN%2Fha4zNuZL6GwRLNxpNY > QiLsOsyM%3D&reserved=0 > > So the periodic output really starts 'now' just like the print says, > so the output from DAT0 is not even in sync with DAT1. > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > > --- > > Thanks, > -Vladimir
Hi Vladimir, The 03/20/2020 15:20, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Yangbo, > > On Fri, 20 Mar 2020 at 12:42, Yangbo Lu <yangbo.lu@nxp.com> wrote: > > > > Support 4 programmable pins for only one function periodic > > signal for now. Since the hardware is not able to support > > absolute start time, driver starts periodic signal immediately. > > > > Are you absolutely sure it doesn't support absolute start time? > Because that would mean it's pretty useless if the phase of the PTP > clock signal is out of control. It looks like there is no support for absolute start time. But you should be able to control the phase using the register PIN_WF_LOW_PERIOD. > > I tested your patch on the LS1028A-RDB board using the following commands: > > # Select PEROUT function and assign a channel to each of pins > SWITCH_1588_DAT0 and SWITCH_1588_DAT1 > echo '2 0' > /sys/class/ptp/ptp1/pins/switch_1588_dat0 > echo '2 1' > /sys/class/ptp/ptp1/pins/switch_1588_dat1 > # Generate pulses with 1 second period on channel 0 > echo '0 0 0 1 0' > /sys/class/ptp/ptp1/period > # Generate pulses with 1 second period on channel 1 > echo '1 0 0 1 0' > /sys/class/ptp/ptp1/period > > And here is what I get: > https://drive.google.com/open?id=1ErWufJL0TWv6hKDQdF1pRL5gn4hn4X-r > > So the periodic output really starts 'now' just like the print says, > so the output from DAT0 is not even in sync with DAT1. > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > > --- > > Thanks, > -Vladimir
On Fri, Mar 20, 2020 at 06:37:26PM +0800, Yangbo Lu wrote: > +static int ocelot_ptp_enable(struct ptp_clock_info *ptp, > + struct ptp_clock_request *rq, int on) > +{ > + struct ocelot *ocelot = container_of(ptp, struct ocelot, ptp_info); > + enum ocelot_ptp_pins ptp_pin; > + struct timespec64 ts; > + unsigned long flags; > + int pin = -1; > + u32 val; > + s64 ns; > + > + switch (rq->type) { > + case PTP_CLK_REQ_PEROUT: > + /* Reject requests with unsupported flags */ > + if (rq->perout.flags) > + return -EOPNOTSUPP; > + > + /* > + * TODO: support disabling function > + * When ptp_disable_pinfunc() is to disable function, > + * it has already held pincfg_mux. > + * However ptp_find_pin() in .enable() called also needs > + * to hold pincfg_mux. > + * This causes dead lock. So, just return for function > + * disabling, and this needs fix-up. What dead lock? When enable(PTP_CLK_REQ_PEROUT, on=0) is called, you don't need to call ptp_disable_pinfunc(). Just stop the periodic waveform generator. The assignment of function to pin remains unchanged. > + */ > + if (!on) > + break; > + > + pin = ptp_find_pin(ocelot->ptp_clock, PTP_PF_PEROUT, > + rq->perout.index); > + if (pin == 0) > + ptp_pin = PTP_PIN_0; > + else if (pin == 1) > + ptp_pin = PTP_PIN_1; > + else if (pin == 2) > + ptp_pin = PTP_PIN_2; > + else if (pin == 3) > + ptp_pin = PTP_PIN_3; > + else > + return -EINVAL; Return -EBUSY here instead. Thanks, Richard
On Tue, Mar 24, 2020 at 05:21:27AM +0000, Y.b. Lu wrote: > In my one previous patch, I was suggested to implement PPS with programmable pin periodic clock function. > But I didn’t find how should PPS be implemented with periodic clock function after checking ptp driver. > https://patchwork.ozlabs.org/patch/1215464/ Yes, for generating a 1-PPS output waveform, users call ioctl PTP_CLK_REQ_PEROUT with ptp_perout_request.period={1,0}. If your device can't control the start time, then it can accept an unspecified time of ptp_perout_request.start={0,0}. > Vladimir talked with me, for the special PPS case, we may consider, > if (req.perout.period.sec ==1 && req.perout.period.nsec == 0) and configure WAVEFORM_LOW to be equal to req_perout.start.nsec. > > Richard, do you think is it ok? Sound okay to me (but I don't know about WAVEFORM_LOW). > And another problem I am facing is, in .enable() callback (PTP_CLK_REQ_PEROUT request) I defined. > /* > * TODO: support disabling function > * When ptp_disable_pinfunc() is to disable function, > * it has already held pincfg_mux. > * However ptp_find_pin() in .enable() called also needs > * to hold pincfg_mux. > * This causes dead lock. So, just return for function > * disabling, and this needs fix-up. > */ > Hope some suggestions here. See my reply to the patch. Thanks, Richard
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Tuesday, March 24, 2020 9:08 PM > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; David S . Miller > <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > On Fri, Mar 20, 2020 at 06:37:26PM +0800, Yangbo Lu wrote: > > +static int ocelot_ptp_enable(struct ptp_clock_info *ptp, > > + struct ptp_clock_request *rq, int on) > > +{ > > + struct ocelot *ocelot = container_of(ptp, struct ocelot, ptp_info); > > + enum ocelot_ptp_pins ptp_pin; > > + struct timespec64 ts; > > + unsigned long flags; > > + int pin = -1; > > + u32 val; > > + s64 ns; > > + > > + switch (rq->type) { > > + case PTP_CLK_REQ_PEROUT: > > + /* Reject requests with unsupported flags */ > > + if (rq->perout.flags) > > + return -EOPNOTSUPP; > > + > > + /* > > + * TODO: support disabling function > > + * When ptp_disable_pinfunc() is to disable function, > > + * it has already held pincfg_mux. > > + * However ptp_find_pin() in .enable() called also needs > > + * to hold pincfg_mux. > > + * This causes dead lock. So, just return for function > > + * disabling, and this needs fix-up. > > What dead lock? > > When enable(PTP_CLK_REQ_PEROUT, on=0) is called, you don't need to > call ptp_disable_pinfunc(). Just stop the periodic waveform > generator. The assignment of function to pin remains unchanged. This happens when we try to change pin function through ptp_ioctl PTP_PIN_SETFUNC. When software holds pincfg_mux and calls ptp_set_pinfunc, it will disable the function previous assigned and the current function of current pin calling ptp_disable_pinfunc. The problem is the enable callback in ptp_disable_pinfunc may have to hold pincfg_mux for ptp_find_pin. The calling should be like this, ptp_set_pinfunc (hold pincfg_mux) ---> ptp_disable_pinfunc ---> .enable ---> ptp_find_pin (hold pincfg_mux) Thanks. > > > + */ > > + if (!on) > > + break; > > + > > + pin = ptp_find_pin(ocelot->ptp_clock, PTP_PF_PEROUT, > > + rq->perout.index); > > + if (pin == 0) > > + ptp_pin = PTP_PIN_0; > > + else if (pin == 1) > > + ptp_pin = PTP_PIN_1; > > + else if (pin == 2) > > + ptp_pin = PTP_PIN_2; > > + else if (pin == 3) > > + ptp_pin = PTP_PIN_3; > > + else > > + return -EINVAL; > > Return -EBUSY here instead. Thanks. Will modify it in next version. > > Thanks, > Richard
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Tuesday, March 24, 2020 9:20 PM > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: Vladimir Oltean <olteanv@gmail.com>; lkml > <linux-kernel@vger.kernel.org>; netdev <netdev@vger.kernel.org>; David S . > Miller <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > On Tue, Mar 24, 2020 at 05:21:27AM +0000, Y.b. Lu wrote: > > In my one previous patch, I was suggested to implement PPS with > programmable pin periodic clock function. > > But I didn’t find how should PPS be implemented with periodic clock > function after checking ptp driver. > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw > ork.ozlabs.org%2Fpatch%2F1215464%2F&data=02%7C01%7Cyangbo.lu > %40nxp.com%7Cbfdbd209ae014cd8484b08d7cff60c13%7C686ea1d3bc2b4c6 > fa92cd99c5c301635%7C0%7C0%7C637206527981191161&sdata=oy9m > T%2Bl69H%2BmpzM9T2kPXQNSMm5w%2FowLhzziUJX2gZc%3D&reserv > ed=0 > > Yes, for generating a 1-PPS output waveform, users call ioctl > PTP_CLK_REQ_PEROUT with ptp_perout_request.period={1,0}. > > If your device can't control the start time, then it can accept an > unspecified time of ptp_perout_request.start={0,0}. Get it. Thanks a lot. > > > Vladimir talked with me, for the special PPS case, we may consider, > > if (req.perout.period.sec ==1 && req.perout.period.nsec == 0) and configure > WAVEFORM_LOW to be equal to req_perout.start.nsec. > > > > Richard, do you think is it ok? > > Sound okay to me (but I don't know about WAVEFORM_LOW). Sorry. I should have explain more. There is a SYNC bit in Ocelot PTP hardware for PPS generation. WAFEFORM_LOW register could be used to adjust phase. RM says, "For the CLOCK action, the sync option makes the pin generate a single pulse, <WAFEFORM_LOW> nanoseconds after the time of day has increased the seconds. The pulse will get a width of <WAVEFORM_HIGH> nanoseconds." Then I will add PPS case in next version patch. Thanks. > > > And another problem I am facing is, in .enable() callback > (PTP_CLK_REQ_PEROUT request) I defined. > > /* > > * TODO: support disabling function > > * When ptp_disable_pinfunc() is to disable function, > > * it has already held pincfg_mux. > > * However ptp_find_pin() in .enable() called also needs > > * to hold pincfg_mux. > > * This causes dead lock. So, just return for function > > * disabling, and this needs fix-up. > > */ > > Hope some suggestions here. > > See my reply to the patch. > > Thanks, > Richard
On Fri, Mar 20, 2020 at 06:37:26PM +0800, Yangbo Lu wrote: > Support 4 programmable pins for only one function periodic > signal for now. For now? > +static int ocelot_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin, > + enum ptp_pin_function func, unsigned int chan) > +{ > + switch (func) { > + case PTP_PF_NONE: > + case PTP_PF_PEROUT: > + break; If the functions cannot be changed, then supporting the PTP_PIN_SETFUNC ioctl does not make sense! > + case PTP_PF_EXTTS: > + case PTP_PF_PHYSYNC: > + return -1; > + } > + return 0; > +} Thanks, Richard
On Wed, Mar 25, 2020 at 03:08:46AM +0000, Y.b. Lu wrote: > The calling should be like this, > ptp_set_pinfunc (hold pincfg_mux) > ---> ptp_disable_pinfunc > ---> .enable > ---> ptp_find_pin (hold pincfg_mux) I see. The call ptp_disable_pinfunc() --> .enable() is really ptp_disable_pinfunc() --> .enable(on=0) or disable. All of the other drivers (except mv88e6xxx which has a bug) avoid the deadlock by only calling ptp_find_pin() when invoked by .enable(on=1); Of course, that is horrible, and I am going to find a way to fix it. For now, maybe you can drop the "programmable pins" feature for your driver? After all, the pins are not programmable. Thanks, Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Wednesday, March 25, 2020 9:16 PM > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; David S . Miller > <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > On Fri, Mar 20, 2020 at 06:37:26PM +0800, Yangbo Lu wrote: > > Support 4 programmable pins for only one function periodic > > signal for now. > > For now? Yes. The pin on Ocelot/Felix supports both PTP_PF_PEROUT and PTP_PF_EXTTS functions. But the PTP_PF_EXTTS function should be implemented separately in Ocelot and Felix since hardware interrupt implementation is different on them. I am responsible for Felix. However I am facing some issue on PTP_PF_EXTTS function on hardware. It may take a long time to discuss internally. Thanks. > > > +static int ocelot_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin, > > + enum ptp_pin_function func, unsigned int chan) > > +{ > > + switch (func) { > > + case PTP_PF_NONE: > > + case PTP_PF_PEROUT: > > + break; > > If the functions cannot be changed, then supporting the > PTP_PIN_SETFUNC ioctl does not make sense! Did you mean the dead lock issue? Or you thought the pin supported only PTP_PF_PEROUT function in hardware? > > > + case PTP_PF_EXTTS: > > + case PTP_PF_PHYSYNC: > > + return -1; > > + } > > + return 0; > > +} > > Thanks, > Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Wednesday, March 25, 2020 9:42 PM > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; David S . Miller > <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > On Wed, Mar 25, 2020 at 03:08:46AM +0000, Y.b. Lu wrote: > > > The calling should be like this, > > ptp_set_pinfunc (hold pincfg_mux) > > ---> ptp_disable_pinfunc > > ---> .enable > > ---> ptp_find_pin (hold pincfg_mux) > > I see. The call > > ptp_disable_pinfunc() --> .enable() > > is really > > ptp_disable_pinfunc() --> .enable(on=0) > > or disable. > > All of the other drivers (except mv88e6xxx which has a bug) avoid the > deadlock by only calling ptp_find_pin() when invoked by .enable(on=1); > > Of course, that is horrible, and I am going to find a way to fix it. Thanks a lot. Do you think it is ok to move protection into ptp_set_pinfunc() to protect just pin_config accessing? ptp_disable_pinfunc() not touching pin_config could be out of protection. But it seems indeed total ptp_set_pinfunc() should be under protection... > > For now, maybe you can drop the "programmable pins" feature for your > driver? After all, the pins are not programmable. I still want to confirm, did you mean the deadlock issue? Or you thought the pin supports only PTP_PF_PEROUT in hardware? I could modify commit messages to indicate the pin supports both PTP_PF_PEROUT and PTP_PF_EXTTS, and PTP_PF_EXTTS support will be added in the future. Thanks a lot. > > Thanks, > Richard
On Thu, Mar 26, 2020 at 09:34:52AM +0000, Y.b. Lu wrote: > > Of course, that is horrible, and I am going to find a way to fix it. > > Thanks a lot. > Do you think it is ok to move protection into ptp_set_pinfunc() to protect just pin_config accessing? > ptp_disable_pinfunc() not touching pin_config could be out of protection. > But it seems indeed total ptp_set_pinfunc() should be under protection... Yes, and I have way to fix that. I will post a patch soon... > I could modify commit messages to indicate the pin supports both PTP_PF_PEROUT and PTP_PF_EXTTS, and PTP_PF_EXTTS support will be added in the future. Thanks for explaining. Since you do have programmable pin, please wait for my patch to fix the deadlock. Thanks, Richard
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Thursday, March 26, 2020 10:00 PM > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; David S . Miller > <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > On Thu, Mar 26, 2020 at 09:34:52AM +0000, Y.b. Lu wrote: > > > Of course, that is horrible, and I am going to find a way to fix it. > > > > Thanks a lot. > > Do you think it is ok to move protection into ptp_set_pinfunc() to protect > just pin_config accessing? > > ptp_disable_pinfunc() not touching pin_config could be out of protection. > > But it seems indeed total ptp_set_pinfunc() should be under protection... > > Yes, and I have way to fix that. I will post a patch soon... > > > I could modify commit messages to indicate the pin supports both > PTP_PF_PEROUT and PTP_PF_EXTTS, and PTP_PF_EXTTS support will be added > in the future. > > Thanks for explaining. Since you do have programmable pin, please > wait for my patch to fix the deadlock. Thanks a lot. Will wait your fix-up. Best regards, Yangbo Lu > > Thanks, > Richard
> -----Original Message----- > From: Y.b. Lu > Sent: Friday, March 27, 2020 1:48 PM > To: Richard Cochran <richardcochran@gmail.com> > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; David S . Miller > <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn <andrew@lunn.ch>; > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > Subject: RE: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > > -----Original Message----- > > From: Richard Cochran <richardcochran@gmail.com> > > Sent: Thursday, March 26, 2020 10:00 PM > > To: Y.b. Lu <yangbo.lu@nxp.com> > > Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; David S . Miller > > <davem@davemloft.net>; Vladimir Oltean <vladimir.oltean@nxp.com>; > > Claudiu Manoil <claudiu.manoil@nxp.com>; Andrew Lunn > <andrew@lunn.ch>; > > Vivien Didelot <vivien.didelot@gmail.com>; Florian Fainelli > > <f.fainelli@gmail.com>; Alexandre Belloni <alexandre.belloni@bootlin.com>; > > Microchip Linux Driver Support <UNGLinuxDriver@microchip.com> > > Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins > > > > On Thu, Mar 26, 2020 at 09:34:52AM +0000, Y.b. Lu wrote: > > > > Of course, that is horrible, and I am going to find a way to fix it. > > > > > > Thanks a lot. > > > Do you think it is ok to move protection into ptp_set_pinfunc() to protect > > just pin_config accessing? > > > ptp_disable_pinfunc() not touching pin_config could be out of protection. > > > But it seems indeed total ptp_set_pinfunc() should be under protection... > > > > Yes, and I have way to fix that. I will post a patch soon... > > > > > I could modify commit messages to indicate the pin supports both > > PTP_PF_PEROUT and PTP_PF_EXTTS, and PTP_PF_EXTTS support will be > added > > in the future. > > > > Thanks for explaining. Since you do have programmable pin, please > > wait for my patch to fix the deadlock. > > Thanks a lot. Will wait your fix-up. I see the fix-up was merged. Thanks Richard. 62582a7 ptp: Avoid deadlocks in the programmable pin code. I just sent out v2 patch-set based on that:) > > Best regards, > Yangbo Lu > > > > > Thanks, > > Richard
diff --git a/drivers/ptp/ptp_ocelot.c b/drivers/ptp/ptp_ocelot.c index 59420a7..299928e 100644 --- a/drivers/ptp/ptp_ocelot.c +++ b/drivers/ptp/ptp_ocelot.c @@ -164,26 +164,119 @@ static int ocelot_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm) return 0; } +static int ocelot_ptp_verify(struct ptp_clock_info *ptp, unsigned int pin, + enum ptp_pin_function func, unsigned int chan) +{ + switch (func) { + case PTP_PF_NONE: + case PTP_PF_PEROUT: + break; + case PTP_PF_EXTTS: + case PTP_PF_PHYSYNC: + return -1; + } + return 0; +} + +static int ocelot_ptp_enable(struct ptp_clock_info *ptp, + struct ptp_clock_request *rq, int on) +{ + struct ocelot *ocelot = container_of(ptp, struct ocelot, ptp_info); + enum ocelot_ptp_pins ptp_pin; + struct timespec64 ts; + unsigned long flags; + int pin = -1; + u32 val; + s64 ns; + + switch (rq->type) { + case PTP_CLK_REQ_PEROUT: + /* Reject requests with unsupported flags */ + if (rq->perout.flags) + return -EOPNOTSUPP; + + /* + * TODO: support disabling function + * When ptp_disable_pinfunc() is to disable function, + * it has already held pincfg_mux. + * However ptp_find_pin() in .enable() called also needs + * to hold pincfg_mux. + * This causes dead lock. So, just return for function + * disabling, and this needs fix-up. + */ + if (!on) + break; + + pin = ptp_find_pin(ocelot->ptp_clock, PTP_PF_PEROUT, + rq->perout.index); + if (pin == 0) + ptp_pin = PTP_PIN_0; + else if (pin == 1) + ptp_pin = PTP_PIN_1; + else if (pin == 2) + ptp_pin = PTP_PIN_2; + else if (pin == 3) + ptp_pin = PTP_PIN_3; + else + return -EINVAL; + + ts.tv_sec = rq->perout.period.sec; + ts.tv_nsec = rq->perout.period.nsec; + ns = timespec64_to_ns(&ts); + ns = ns >> 1; + if (ns > 0x3fffffff || ns <= 0x6) + return -EINVAL; + + spin_lock_irqsave(&ocelot->ptp_clock_lock, flags); + ocelot_write_rix(ocelot, ns, PTP_PIN_WF_LOW_PERIOD, ptp_pin); + ocelot_write_rix(ocelot, ns, PTP_PIN_WF_HIGH_PERIOD, ptp_pin); + + val = PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_CLOCK); + ocelot_write_rix(ocelot, val, PTP_PIN_CFG, ptp_pin); + spin_unlock_irqrestore(&ocelot->ptp_clock_lock, flags); + dev_warn(ocelot->dev, + "Starting periodic signal now! (absolute start time not supported)\n"); + break; + default: + return -EOPNOTSUPP; + } + return 0; +} + static struct ptp_clock_info ocelot_ptp_clock_info = { .owner = THIS_MODULE, .name = "ocelot ptp", .max_adj = 0x7fffffff, .n_alarm = 0, .n_ext_ts = 0, - .n_per_out = 0, - .n_pins = 0, + .n_per_out = OCELOT_PTP_PINS_NUM, + .n_pins = OCELOT_PTP_PINS_NUM, .pps = 0, .gettime64 = ocelot_ptp_gettime64, .settime64 = ocelot_ptp_settime64, .adjtime = ocelot_ptp_adjtime, .adjfine = ocelot_ptp_adjfine, + .verify = ocelot_ptp_verify, + .enable = ocelot_ptp_enable, }; int ocelot_init_timestamp(struct ocelot *ocelot) { struct ptp_clock *ptp_clock; + int i; ocelot->ptp_info = ocelot_ptp_clock_info; + + for (i = 0; i < OCELOT_PTP_PINS_NUM; i++) { + struct ptp_pin_desc *p = &ocelot->ptp_pins[i]; + + snprintf(p->name, sizeof(p->name), "switch_1588_dat%d", i); + p->index = i; + p->func = PTP_PF_NONE; + } + + ocelot->ptp_info.pin_config = &ocelot->ptp_pins[0]; + ptp_clock = ptp_clock_register(&ocelot->ptp_info, ocelot->dev); if (IS_ERR(ptp_clock)) return PTR_ERR(ptp_clock); diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index bcce278..db2fb14 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -92,6 +92,8 @@ #define OCELOT_SPEED_100 2 #define OCELOT_SPEED_10 3 +#define OCELOT_PTP_PINS_NUM 4 + #define TARGET_OFFSET 24 #define REG_MASK GENMASK(TARGET_OFFSET - 1, 0) #define REG(reg, offset) [reg & REG_MASK] = offset @@ -544,6 +546,7 @@ struct ocelot { struct mutex ptp_lock; /* Protects the PTP clock */ spinlock_t ptp_clock_lock; + struct ptp_pin_desc ptp_pins[OCELOT_PTP_PINS_NUM]; }; #define ocelot_read_ix(ocelot, reg, gi, ri) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
Support 4 programmable pins for only one function periodic signal for now. Since the hardware is not able to support absolute start time, driver starts periodic signal immediately. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> --- drivers/ptp/ptp_ocelot.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++- include/soc/mscc/ocelot.h | 3 ++ 2 files changed, 98 insertions(+), 2 deletions(-)