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 <[email protected]>
---
drivers/ptp/ptp_ocelot.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++-
include/soc/mscc/ocelot.h | 3 ++
2 files changed, 98 insertions(+), 2 deletions(-)
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))
--
2.7.4
Hi Yangbo,
On Fri, 20 Mar 2020 at 12:42, Yangbo Lu <[email protected]> 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 <[email protected]>
> ---
Thanks,
-Vladimir
Hi Vladimir and Richard,
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Friday, March 20, 2020 9:21 PM
> To: Y.b. Lu <[email protected]>
> Cc: lkml <[email protected]>; netdev <[email protected]>;
> David S . Miller <[email protected]>; Richard Cochran
> <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins
>
> Hi Yangbo,
>
> On Fri, 20 Mar 2020 at 12:42, Yangbo Lu <[email protected]> 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 <[email protected]>
> > ---
>
> 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 <[email protected]> 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 <[email protected]>
> > ---
>
> Thanks,
> -Vladimir
--
/Horatiu
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 <[email protected]>
> Sent: Tuesday, March 24, 2020 9:08 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected]; David S . Miller
> <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> 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 <[email protected]>
> Sent: Tuesday, March 24, 2020 9:20 PM
> To: Y.b. Lu <[email protected]>
> Cc: Vladimir Oltean <[email protected]>; lkml
> <[email protected]>; netdev <[email protected]>; David S .
> Miller <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> 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 <[email protected]>
> Sent: Wednesday, March 25, 2020 9:16 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected]; David S . Miller
> <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> 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 <[email protected]>
> Sent: Wednesday, March 25, 2020 9:42 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected]; David S . Miller
> <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> 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 <[email protected]>
> Sent: Thursday, March 26, 2020 10:00 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected]; David S . Miller
> <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> 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 <[email protected]>
> Cc: [email protected]; [email protected]; David S . Miller
> <[email protected]>; Vladimir Oltean <[email protected]>;
> Claudiu Manoil <[email protected]>; Andrew Lunn <[email protected]>;
> Vivien Didelot <[email protected]>; Florian Fainelli
> <[email protected]>; Alexandre Belloni <[email protected]>;
> Microchip Linux Driver Support <[email protected]>
> Subject: RE: [PATCH 6/6] ptp_ocelot: support 4 programmable pins
>
> > -----Original Message-----
> > From: Richard Cochran <[email protected]>
> > Sent: Thursday, March 26, 2020 10:00 PM
> > To: Y.b. Lu <[email protected]>
> > Cc: [email protected]; [email protected]; David S . Miller
> > <[email protected]>; Vladimir Oltean <[email protected]>;
> > Claudiu Manoil <[email protected]>; Andrew Lunn
> <[email protected]>;
> > Vivien Didelot <[email protected]>; Florian Fainelli
> > <[email protected]>; Alexandre Belloni <[email protected]>;
> > Microchip Linux Driver Support <[email protected]>
> > 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