[PATCH] dp83640: Support a configurable number of periodic outputs

[PATCH] dp83640: Support a configurable number of periodic outputs

[Stefan Sørensen]

The driver is currently limited to a single periodic output.
This patch makes the number of peridodic output dynamic by
dropping the gpio_tab module parameter and adding cal_gpio,
perout_gpio_tab and extts_gpio_tabs parameters.

Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
---
 drivers/net/phy/dp83640.c | 70 ++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 40 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 547725f..0a54c79 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -38,15 +38,11 @@
 #define LAYER4		0x02
 #define LAYER2		0x01
 #define MAX_RXTS	64
-#define N_EXT_TS	6
 #define PSF_PTPVER	2
 #define PSF_EVNT	0x4000
 #define PSF_RX		0x2000
 #define PSF_TX		0x1000
-#define EXT_EVENT	1
-#define CAL_EVENT	7
-#define CAL_TRIGGER	7
-#define PER_TRIGGER	6
+#define N_EXT		8
 
 #define MII_DP83640_MICR 0x11
 #define MII_DP83640_MISR 0x12
@@ -148,30 +144,23 @@ struct dp83640_clock {
 
 /* globals */
 
-enum {
-	CALIBRATE_GPIO,
-	PEROUT_GPIO,
-	EXTTS0_GPIO,
-	EXTTS1_GPIO,
-	EXTTS2_GPIO,
-	EXTTS3_GPIO,
-	EXTTS4_GPIO,
-	EXTTS5_GPIO,
-	GPIO_TABLE_SIZE
-};
-
 static int chosen_phy = -1;
-static ushort gpio_tab[GPIO_TABLE_SIZE] = {
-	1, 2, 3, 4, 8, 9, 10, 11
-};
+static int cal_gpio = 1;
+static int perout_gpio_tab[N_EXT] = {2};
+static int n_perout = 1;
+static int extts_gpio_tab[N_EXT] = {3, 4, 8, 9, 10, 11};
+static int n_extts = 6;
 
 module_param(chosen_phy, int, 0444);
-module_param_array(gpio_tab, ushort, NULL, 0444);
+module_param(cal_gpio, int, 0444);
+module_param_array(perout_gpio_tab, int, &n_perout, 0444);
+module_param_array(extts_gpio_tab, int, &n_extts, 0444);
 
 MODULE_PARM_DESC(chosen_phy, \
 	"The address of the PHY to use for the ancillary clock features");
-MODULE_PARM_DESC(gpio_tab, \
-	"Which GPIO line to use for which purpose: cal,perout,extts1,...,extts6");
+MODULE_PARM_DESC(cal_gpio, "Which GPIO line to use for calibration");
+MODULE_PARM_DESC(perout_gpio_tab, "Which GPIO lines to use for periodic output");
+MODULE_PARM_DESC(extts_gpio_tab, "Which GPIO lines to use for external timestamping");
 
 /* a list of clocks and a mutex to protect it */
 static LIST_HEAD(phyter_clocks);
@@ -267,15 +256,16 @@ static u64 phy2txts(struct phy_txts *p)
 }
 
 static void periodic_output(struct dp83640_clock *clock,
-			    struct ptp_clock_request *clkreq, bool on)
+			    struct ptp_clock_request *clkreq,
+			    int index, bool on)
 {
 	struct dp83640_private *dp83640 = clock->chosen;
 	struct phy_device *phydev = dp83640->phydev;
 	u32 sec, nsec, period;
 	u16 gpio, ptp_trig, trigger, val;
 
-	gpio = on ? gpio_tab[PEROUT_GPIO] : 0;
-	trigger = PER_TRIGGER;
+	gpio = on ? perout_gpio_tab[index] : 0;
+	trigger = n_extts + index;
 
 	ptp_trig = TRIG_WR |
 		(trigger & TRIG_CSEL_MASK) << TRIG_CSEL_SHIFT |
@@ -430,12 +420,12 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp,
 	switch (rq->type) {
 	case PTP_CLK_REQ_EXTTS:
 		index = rq->extts.index;
-		if (index < 0 || index >= N_EXT_TS)
+		if (index < 0 || index >= n_extts)
 			return -EINVAL;
-		event_num = EXT_EVENT + index;
+		event_num = index;
 		evnt = EVNT_WR | (event_num & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
 		if (on) {
-			gpio_num = gpio_tab[EXTTS0_GPIO + index];
+			gpio_num = extts_gpio_tab[index];
 			evnt |= (gpio_num & EVNT_GPIO_MASK) << EVNT_GPIO_SHIFT;
 			evnt |= EVNT_RISE;
 		}
@@ -443,9 +433,10 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp,
 		return 0;
 
 	case PTP_CLK_REQ_PEROUT:
-		if (rq->perout.index != 0)
+		index = rq->perout.index;
+		if (index < 0 || index >= n_perout)
 			return -EINVAL;
-		periodic_output(clock, rq, on);
+		periodic_output(clock, rq, index, on);
 		return 0;
 
 	default:
@@ -538,10 +529,9 @@ static void recalibrate(struct dp83640_clock *clock)
 	struct list_head *this;
 	struct dp83640_private *tmp;
 	struct phy_device *master = clock->chosen->phydev;
-	u16 cal_gpio, cfg0, evnt, ptp_trig, trigger, val;
+	u16 cfg0, evnt, ptp_trig, trigger, val;
 
-	trigger = CAL_TRIGGER;
-	cal_gpio = gpio_tab[CALIBRATE_GPIO];
+	trigger = n_extts + n_perout;
 
 	mutex_lock(&clock->extreg_lock);
 
@@ -564,7 +554,7 @@ static void recalibrate(struct dp83640_clock *clock)
 	 * enable an event timestamp
 	 */
 	evnt = EVNT_WR | EVNT_RISE | EVNT_SINGLE;
-	evnt |= (CAL_EVENT & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
+	evnt |= (trigger & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
 	evnt |= (cal_gpio & EVNT_GPIO_MASK) << EVNT_GPIO_SHIFT;
 
 	list_for_each(this, &clock->phylist) {
@@ -642,7 +632,7 @@ static void recalibrate(struct dp83640_clock *clock)
 
 static inline u16 exts_chan_to_edata(int ch)
 {
-	return 1 << ((ch + EXT_EVENT) * 2);
+	return 1 << ((ch) * 2);
 }
 
 static int decode_evnt(struct dp83640_private *dp83640,
@@ -676,14 +666,14 @@ static int decode_evnt(struct dp83640_private *dp83640,
 		parsed = words + 2;
 	} else {
 		parsed = words + 1;
-		i = ((ests >> EVNT_NUM_SHIFT) & EVNT_NUM_MASK) - EXT_EVENT;
+		i = ((ests >> EVNT_NUM_SHIFT) & EVNT_NUM_MASK);
 		ext_status = exts_chan_to_edata(i);
 	}
 
 	event.type = PTP_CLOCK_EXTTS;
 	event.timestamp = phy2txts(&dp83640->edata);
 
-	for (i = 0; i < N_EXT_TS; i++) {
+	for (i = 0; i < n_extts; i++) {
 		if (ext_status & exts_chan_to_edata(i)) {
 			event.index = i;
 			ptp_clock_event(dp83640->clock->ptp_clock, &event);
@@ -889,8 +879,8 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 	sprintf(clock->caps.name, "dp83640 timer");
 	clock->caps.max_adj	= 1953124;
 	clock->caps.n_alarm	= 0;
-	clock->caps.n_ext_ts	= N_EXT_TS;
-	clock->caps.n_per_out	= 1;
+	clock->caps.n_ext_ts	= n_extts;
+	clock->caps.n_per_out	= n_perout;
 	clock->caps.pps		= 0;
 	clock->caps.adjfreq	= ptp_dp83640_adjfreq;
 	clock->caps.adjtime	= ptp_dp83640_adjtime;
-- 
1.8.5.3

Re: [PATCH] dp83640: Support a configurable number of periodic outputs

[Richard Cochran]

On Tue, Feb 04, 2014 at 09:52:22AM +0100, Stefan Sørensen wrote:
> The driver is currently limited to a single periodic output.
> This patch makes the number of peridodic output dynamic by
> dropping the gpio_tab module parameter and adding cal_gpio,
> perout_gpio_tab and extts_gpio_tabs parameters.

...

>  module_param(chosen_phy, int, 0444);
> -module_param_array(gpio_tab, ushort, NULL, 0444);
> +module_param(cal_gpio, int, 0444);
> +module_param_array(perout_gpio_tab, int, &n_perout, 0444);
> +module_param_array(extts_gpio_tab, int, &n_extts, 0444);
>  
>  MODULE_PARM_DESC(chosen_phy, \
>  	"The address of the PHY to use for the ancillary clock features");
> -MODULE_PARM_DESC(gpio_tab, \
> -	"Which GPIO line to use for which purpose: cal,perout,extts1,...,extts6");
> +MODULE_PARM_DESC(cal_gpio, "Which GPIO line to use for calibration");
> +MODULE_PARM_DESC(perout_gpio_tab, "Which GPIO lines to use for periodic output");
> +MODULE_PARM_DESC(extts_gpio_tab, "Which GPIO lines to use for external timestamping");

Modules parameters are surely easiest (for the developer), but perhaps
the time has come for the "right way."

Currently there is no interface for configuring the GPIOs used by PHC
devices, and last year I was going to add the GPIOs to the igb
driver. The conclusion of the discussion was that module parameters
are bad for this, but ethtool is good.

  http://www.spinics.net/lists/netdev/msg237692.html

[ I did not follow through to come up with an ethtool way of configuring
  the igb pins. ]

Even though it is more work, I think the way forward is to invent a
way to let the user configure this kind of thing via ethtool. Would
you care to take a stab at this?

Thanks,
Richard

Re: [PATCH] dp83640: Support a configurable number of periodic outputs

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

On Tue, 2014-02-04 at 10:29 +0100, Richard Cochran wrote:
> On Tue, Feb 04, 2014 at 09:52:22AM +0100, Stefan Sørensen wrote:
> > The driver is currently limited to a single periodic output.
> > This patch makes the number of peridodic output dynamic by
> > dropping the gpio_tab module parameter and adding cal_gpio,
> > perout_gpio_tab and extts_gpio_tabs parameters.
> 
> ...
> 
> >  module_param(chosen_phy, int, 0444);
> > -module_param_array(gpio_tab, ushort, NULL, 0444);
> > +module_param(cal_gpio, int, 0444);
> > +module_param_array(perout_gpio_tab, int, &n_perout, 0444);
> > +module_param_array(extts_gpio_tab, int, &n_extts, 0444);
> >  
> >  MODULE_PARM_DESC(chosen_phy, \
> >  	"The address of the PHY to use for the ancillary clock features");
> > -MODULE_PARM_DESC(gpio_tab, \
> > -	"Which GPIO line to use for which purpose: cal,perout,extts1,...,extts6");
> > +MODULE_PARM_DESC(cal_gpio, "Which GPIO line to use for calibration");
> > +MODULE_PARM_DESC(perout_gpio_tab, "Which GPIO lines to use for periodic output");
> > +MODULE_PARM_DESC(extts_gpio_tab, "Which GPIO lines to use for external timestamping");
> 
> Modules parameters are surely easiest (for the developer), but perhaps
> the time has come for the "right way."
> 
> Currently there is no interface for configuring the GPIOs used by PHC
> devices, and last year I was going to add the GPIOs to the igb
> driver. The conclusion of the discussion was that module parameters
> are bad for this, but ethtool is good.
> 
>   http://www.spinics.net/lists/netdev/msg237692.html
> 
> [ I did not follow through to come up with an ethtool way of configuring
>   the igb pins. ]
> 
> Even though it is more work, I think the way forward is to invent a
> way to let the user configure this kind of thing via ethtool. Would
> you care to take a stab at this?

It seems to me that this is board configuration, not user configuration,
so it should be defined in DT or ACPI or whatever.

Are there boards which expose multiple GPIOs for users to plug together
PPS signals?

Ben.

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] dp83640: Support a configurable number of periodic outputs

[Sørensen, Stefan]

On Tue, 2014-02-04 at 10:53 +0000, Ben Hutchings wrote:
> > Modules parameters are surely easiest (for the developer), but perhaps
> > the time has come for the "right way."
> > 
> > Currently there is no interface for configuring the GPIOs used by PHC
> > devices, and last year I was going to add the GPIOs to the igb
> > driver. The conclusion of the discussion was that module parameters
> > are bad for this, but ethtool is good.
> > 
> >   http://www.spinics.net/lists/netdev/msg237692.html
> > 
> > [ I did not follow through to come up with an ethtool way of configuring
> >   the igb pins. ]
> > 
> > Even though it is more work, I think the way forward is to invent a
> > way to let the user configure this kind of thing via ethtool. Would
> > you care to take a stab at this?
> 
> It seems to me that this is board configuration, not user configuration,
> so it should be defined in DT or ACPI or whatever.
> 
> Are there boards which expose multiple GPIOs for users to plug together
> PPS signals?

I would definitely prefer the DT approach. On all the platforms I have
used, the GPIO configuration has been set in stone by the HW. But there
might be some dev boards or the like that has them configurable.

Stefan

Re: [PATCH] dp83640: Support a configurable number of periodic outputs

[Richard Cochran]

On Tue, Feb 04, 2014 at 03:24:27PM +0000, Sørensen, Stefan wrote:
> 
> I would definitely prefer the DT approach. On all the platforms I have
> used, the GPIO configuration has been set in stone by the HW. But there
> might be some dev boards or the like that has them configurable.

I think DT is fine for the initial configuration, but the ethtool way
will still be needed in at least two cases.

1. Platform has no DT at all.
2. User wants to change the function at run time.

Thanks,
Richard

Re: [PATCH] dp83640: Support a configurable number of periodic outputs

[Sørensen, Stefan]

On Tue, 2014-02-04 at 16:43 +0100, Richard Cochran wrote:
> On Tue, Feb 04, 2014 at 03:24:27PM +0000, Sørensen, Stefan wrote:
> > 
> > I would definitely prefer the DT approach. On all the platforms I have
> > used, the GPIO configuration has been set in stone by the HW. But there
> > might be some dev boards or the like that has them configurable.
> 
> I think DT is fine for the initial configuration, but the ethtool way
> will still be needed in at least two cases.
> 
> 1. Platform has no DT at all.
> 2. User wants to change the function at run time.

Will all PTP clocks be associated with an ethernet device? If not,
wouldn't a PTP ioctl be better than ethtool? 

Stefan

Re: [PATCH] dp83640: Support a configurable number of periodic outputs

[Richard Cochran]

On Wed, Feb 05, 2014 at 07:40:17AM +0000, Sørensen, Stefan wrote:
> 
> Will all PTP clocks be associated with an ethernet device? If not,
> wouldn't a PTP ioctl be better than ethtool? 

Hm, maybe. I don't have a strong preference. I'll try and put together
an idea of how the ioctl might look like.

Thanks,
Richard

RE: [PATCH] dp83640: Support a configurable number of periodic outputs

[Manfred Rudigier]

>From: netdev-owner@vger.kernel.org [mailto:netdev-
>owner@vger.kernel.org] On Behalf Of Richard Cochran
>Sent: Friday, February 07, 2014 6:49 AM
>To: Sørensen, Stefan
>Cc: ben@decadent.org.uk; netdev@vger.kernel.org
>Subject: Re: [PATCH] dp83640: Support a configurable number of periodic
>outputs
>
>On Wed, Feb 05, 2014 at 07:40:17AM +0000, Sørensen, Stefan wrote:
>>
>> Will all PTP clocks be associated with an ethernet device? If not,
>> wouldn't a PTP ioctl be better than ethtool?
>
>Hm, maybe. I don't have a strong preference. I'll try and put together an idea
>of how the ioctl might look like.

At least on the Freescale P2020 CPU there is just one PTP clock for 3 
Ethernet devices. But since the PTP registers are shared between all three 
Ethernet ports, an ethtool approach would still work, but seems somehow strange
for me.

Regards,
Manfred