[PATCH net-next 0/3] dp83640: Increase support perout pins

This patch series increases the number of periodic output pins supported
on the dp83640 to 7, and allows for reprogramming the calibration pin.

Stefan Sørensen (3):
  ptp: Allow reassigning calibration pin function
  dp83640: Program pulsewidth2 values of perout triggers 0 and 1
  dp83640: Increase supported perout pins to 7

 drivers/net/phy/dp83640.c | 28 ++++++++++++++++++----------
 drivers/ptp/ptp_chardev.c |  9 ++-------
 2 files changed, 20 insertions(+), 17 deletions(-)

-- 
1.9.3

The ptp pin function programming does not allow calibration pin to change
function. This is problematic on hardware that uses the default calibration
pin for other purposes.

Removing this limitation does not impact calibration if userspace does not
reprogram the calibration pin.

Signed-off-by: Stefan Sørensen <redacted>
---
 drivers/net/phy/dp83640.c | 6 +++++-
 drivers/ptp/ptp_chardev.c | 9 ++-------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 6a999e6..73fada9 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c

@@ -594,7 +594,11 @@ static void recalibrate(struct dp83640_clock *clock)
 	u16 cal_gpio, cfg0, evnt, ptp_trig, trigger, val;
 
 	trigger = CAL_TRIGGER;
-	cal_gpio = gpio_tab[CALIBRATE_GPIO];
+	cal_gpio = 1 + ptp_find_pin(clock->ptp_clock, PTP_PF_PHYSYNC, 0);
+	if (cal_gpio < 1) {
+		pr_err("PHY calibration pin not avaible - PHY is not calibrated.");
+		return;
+	}
 
 	mutex_lock(&clock->extreg_lock);
 

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index 419056d..f8a7609 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c

@@ -86,17 +86,12 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 			return -EINVAL;
 		break;
 	case PTP_PF_PHYSYNC:
-		pr_err("sorry, cannot reassign the calibration pin\n");
-		return -EINVAL;
+		if (chan != 0)
+			return -EINVAL;
 	default:
 		return -EINVAL;
 	}
 
-	if (pin2->func == PTP_PF_PHYSYNC) {
-		pr_err("sorry, cannot reprogram the calibration pin\n");
-		return -EINVAL;
-	}
-
 	if (info->verify(info, pin, func, chan)) {
 		pr_err("driver cannot use function %u on pin %u\n", func, chan);
 		return -EOPNOTSUPP;

-- 
1.9.3

Periodic output triggers 0 and 1 of the dp83640 has a programmable
duty-cycle which is controlled by the Pulsewidth2 field of the trigger
data register.  This field is not documented in the datasheet, but it
is described in the "PHYTER Software Development Guide" section
3.1.4.1. Failing to set the field can result in unstable/no trigger
output.

Add programming of the Pulsewidth2 field, setting it to the same value
as the Pulsewidth field for a 50% duty cycle.

Signed-off-by: Stefan Sørensen <redacted>
---
 drivers/net/phy/dp83640.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 73fada9..0343b6c 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c

@@ -353,6 +353,11 @@ static int periodic_output(struct dp83640_clock *clock,
 	ext_write(0, phydev, PAGE4, PTP_TDR, sec >> 16);       /* sec[31:16] */
 	ext_write(0, phydev, PAGE4, PTP_TDR, pwidth & 0xffff); /* ns[15:0] */
 	ext_write(0, phydev, PAGE4, PTP_TDR, pwidth >> 16);    /* ns[31:16] */
+	/* Triggers 0 and 1 has programmable pulsewidth2 */
+	if(trigger < 2) {
+		ext_write(0, phydev, PAGE4, PTP_TDR, pwidth & 0xffff);
+		ext_write(0, phydev, PAGE4, PTP_TDR, pwidth >> 16);
+	}
 
 	/*enable trigger*/
 	val &= ~TRIG_LOAD;

-- 
1.9.3

This patch increases the number of supported periodic output pins from
1 to 7. The last pin is reserved for sync.

Signed-off-by: Stefan Sørensen <redacted>
---
 drivers/net/phy/dp83640.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 0343b6c..31ec766 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c

@@ -40,6 +40,7 @@
 #define LAYER2		0x01
 #define MAX_RXTS	64
 #define N_EXT_TS	6
+#define N_PER_OUT	7
 #define PSF_PTPVER	2
 #define PSF_EVNT	0x4000
 #define PSF_RX		0x2000

@@ -47,7 +48,6 @@
 #define EXT_EVENT	1
 #define CAL_EVENT	7
 #define CAL_TRIGGER	7
-#define PER_TRIGGER	6
 #define DP83640_N_PINS	12
 
 #define MII_DP83640_MICR 0x11

@@ -300,23 +300,22 @@ static u64 phy2txts(struct phy_txts *p)
 }
 
 static int periodic_output(struct dp83640_clock *clock,
-			   struct ptp_clock_request *clkreq, bool on)
+			   struct ptp_clock_request *clkreq, bool on,
+			   int trigger)
 {
 	struct dp83640_private *dp83640 = clock->chosen;
 	struct phy_device *phydev = dp83640->phydev;
 	u32 sec, nsec, pwidth;
-	u16 gpio, ptp_trig, trigger, val;
+	u16 gpio, ptp_trig, val;
 
 	if (on) {
-		gpio = 1 + ptp_find_pin(clock->ptp_clock, PTP_PF_PEROUT, 0);
+		gpio = 1 + ptp_find_pin(clock->ptp_clock, PTP_PF_PEROUT, trigger);
 		if (gpio < 1)
 			return -EINVAL;
 	} else {
 		gpio = 0;
 	}
 
-	trigger = PER_TRIGGER;
-
 	ptp_trig = TRIG_WR |
 		(trigger & TRIG_CSEL_MASK) << TRIG_CSEL_SHIFT |
 		(gpio & TRIG_GPIO_MASK) << TRIG_GPIO_SHIFT |

@@ -496,9 +495,9 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp,
 		return 0;
 
 	case PTP_CLK_REQ_PEROUT:
-		if (rq->perout.index != 0)
+		if (rq->perout.index >= N_PER_OUT)
 			return -EINVAL;
-		return periodic_output(clock, rq, on);
+		return periodic_output(clock, rq, on, rq->perout.index);
 
 	default:
 		break;

@@ -953,7 +952,7 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 	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_per_out	= N_PER_OUT;
 	clock->caps.n_pins	= DP83640_N_PINS;
 	clock->caps.pps		= 0;
 	clock->caps.adjfreq	= ptp_dp83640_adjfreq;

-- 
1.9.3

Hello.

On 06/25/2014 04:37 PM, Stefan Sørensen wrote:

Periodic output triggers 0 and 1 of the dp83640 has a programmable
duty-cycle which is controlled by the Pulsewidth2 field of the trigger
data register.  This field is not documented in the datasheet, but it
is described in the "PHYTER Software Development Guide" section
3.1.4.1. Failing to set the field can result in unstable/no trigger
output.

Add programming of the Pulsewidth2 field, setting it to the same value
as the Pulsewidth field for a 50% duty cycle.

Signed-off-by: Stefan Sørensen <redacted>
---
  drivers/net/phy/dp83640.c | 5 +++++
  1 file changed, 5 insertions(+)

    Please run your patches thru scripts/checkpatch.pl -- space is needed 
after *if*.

WBR, Sergei

On Wed, Jun 25, 2014 at 02:37:29PM +0200, Stefan Sørensen wrote:

The ptp pin function programming does not allow calibration pin to change
function. This is problematic on hardware that uses the default calibration
pin for other purposes.

Removing this limitation does not impact calibration if userspace does not
reprogram the calibration pin.

Reassigning the calibration function never makes sense, because it is
only used in the driver probe method.

Clobbering the calibration pin with another function only makes sense
if the hardware design has exactly one PHY.

Can you please add a check in the dp83640 verify method to enforce
these two constraints?

Thanks,
Richard

Hi Richard,

On Thu, Jun 26, 2014 at 7:21 AM, Richard Cochran
[off-list ref] wrote:

On Wed, Jun 25, 2014 at 02:37:29PM +0200, Stefan Sørensen wrote:

quoted

The ptp pin function programming does not allow calibration pin to change
function. This is problematic on hardware that uses the default calibration
pin for other purposes.

Removing this limitation does not impact calibration if userspace does not
reprogram the calibration pin.

Reassigning the calibration function never makes sense, because it is
only used in the driver probe method.

Yes, indeed, but isn't that a bug? I think the calibration should be
done again whenever the clock is loaded with a new value, i.e. in
ptp_dp83640_settime. See section 3.1 in [1]: "All subsequent settings
should use a step
adjustment or temporary rate adjustment, which should occur at each
PHY without introducing any error." This means, whenever we do
something else (directly write to the clock register), we must
recalibrate.

Clobbering the calibration pin with another function only makes sense
if the hardware design has exactly one PHY.

Or if the hardware has two PHYs that are not used at the same time. I
have a board that has two DP83640 on the same MII bus, one is used for
optical Ethernet, one for copper. Only one of the PHYs is powered up
at a time, the other one is in power down and MII isolate mode. In
such a case the calibration pin could be used for something else.

Christian

On Thu, Jun 26, 2014 at 8:14 AM, Christian Riesch
[off-list ref] wrote:

Yes, indeed, but isn't that a bug? I think the calibration should be
done again whenever the clock is loaded with a new value, i.e. in
ptp_dp83640_settime. See section 3.1 in [1]: "All subsequent settings
should use a step
adjustment or temporary rate adjustment, which should occur at each
PHY without introducing any error." This means, whenever we do
something else (directly write to the clock register), we must
recalibrate.

Forgot to paste the link:
[1] http://www.ti.com/lit/pdf/snla104

Sorry.

On Thu, Jun 26, 2014 at 08:14:58AM +0200, Christian Riesch wrote:

On Thu, Jun 26, 2014 at 7:21 AM, Richard Cochran [off-list ref] wrote:

quoted

Reassigning the calibration function never makes sense, because it is
only used in the driver probe method.

Yes, indeed, but isn't that a bug? I think the calibration should be
done again whenever the clock is loaded with a new value, i.e. in
ptp_dp83640_settime. See section 3.1 in [1]: "All subsequent settings
should use a step
adjustment or temporary rate adjustment, which should occur at each
PHY without introducing any error." This means, whenever we do
something else (directly write to the clock register), we must
recalibrate.

When we write the time, we use the broadcast address, and so the PHYs
receive the data at exactly the same time on the MDIO bus. But maybe
they would still need a new, fine calibration. Can you test this?

(I don't have a board with two phys.)

Thanks,
Richard