Lan8841 has 10 GPIOs and it has 2 events(EVENT_A and EVENT_B). It is
possible to assigned the 2 events to any of the GPIOs, but a GPIO can
have only 1 event at a time.
These events are used to generate periodic signals. It is possible to
configure the length, the start time and the period of the signal by
configuring the event.
Currently the SW uses only EVENT_A to generate the perout.
These events are generated by comparing the target time with the PHC
time. In case the PHC time is changed to a value bigger than the target
time + reload time, then it would generate only 1 event and then it
would stop because target time + reload time is small than PHC time.
Therefore it is required to change also the target time every time when
the PHC is changed. The same will apply also when the PHC time is
changed to a smaller value.
This was tested using:
testptp -L 6,2
testptp -p 1000000000 -w 200000000
Signed-off-by: Horatiu Vultur <[email protected]>
---
drivers/net/phy/micrel.c | 363 +++++++++++++++++++++++++++++++++++++++
1 file changed, 363 insertions(+)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 2c84fccef4f64..ac818f1381942 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -318,6 +318,7 @@ struct kszphy_ptp_priv {
struct ptp_clock_info ptp_clock_info;
/* Lock for ptp_clock */
struct mutex ptp_lock;
+ struct ptp_pin_desc *pin_config;
};
struct kszphy_priv {
@@ -3665,6 +3666,9 @@ static int lan8841_hwtstamp(struct mii_timestamper *mii_ts, struct ifreq *ifr)
#define LAN8841_PTP_LTC_SET_NS_LO 266
#define LAN8841_PTP_CMD_CTL_PTP_LTC_LOAD BIT(4)
+static void lan8841_ptp_update_target(struct kszphy_ptp_priv *ptp_priv,
+ const struct timespec64 *ts);
+
static int lan8841_ptp_settime64(struct ptp_clock_info *ptp,
const struct timespec64 *ts)
{
@@ -3683,6 +3687,7 @@ static int lan8841_ptp_settime64(struct ptp_clock_info *ptp,
/* Set the command to load the LTC */
phy_write_mmd(phydev, 2, LAN8841_PTP_CMD_CTL,
LAN8841_PTP_CMD_CTL_PTP_LTC_LOAD);
+ lan8841_ptp_update_target(ptp_priv, ts);
mutex_unlock(&ptp_priv->ptp_lock);
return 0;
@@ -3803,6 +3808,12 @@ static int lan8841_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
}
mutex_unlock(&ptp_priv->ptp_lock);
+ /* Update the target clock */
+ ptp->gettime64(ptp, &ts);
+ mutex_lock(&ptp_priv->ptp_lock);
+ lan8841_ptp_update_target(ptp_priv, &ts);
+ mutex_unlock(&ptp_priv->ptp_lock);
+
return 0;
}
@@ -3839,6 +3850,337 @@ static int lan8841_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
return 0;
}
+static int lan8841_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;
+ default:
+ return -1;
+ }
+
+ return 0;
+}
+
+#define LAN8841_PTP_GPIO_NUM 10
+#define LAN8841_PTP_GPIO_MASK GENMASK(LAN8841_PTP_GPIO_NUM, 0)
+#define LAN8841_GPIO_EN 128
+#define LAN8841_GPIO_DIR 129
+#define LAN8841_GPIO_BUF 130
+
+static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
+{
+ struct phy_device *phydev = ptp_priv->phydev;
+ u16 tmp;
+
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
+ tmp &= ~BIT(pin);
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
+
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
+ tmp &= ~BIT(pin);
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
+
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
+ tmp &= ~BIT(pin);
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
+}
+
+static void lan8841_ptp_perout_on(struct kszphy_ptp_priv *ptp_priv, int pin)
+{
+ struct phy_device *phydev = ptp_priv->phydev;
+ u16 tmp;
+
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
+ tmp |= BIT(pin);
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
+
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
+ tmp |= BIT(pin);
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
+
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
+ tmp |= BIT(pin);
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
+}
+
+#define LAN8841_GPIO_DATA_SEL1 131
+#define LAN8841_GPIO_DATA_SEL2 132
+#define LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_MASK GENMASK(2, 0)
+#define LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_A 1
+#define LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_B 2
+#define LAN8841_PTP_GENERAL_CONFIG 257
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A BIT(1)
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_B BIT(3)
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK GENMASK(7, 4)
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_B_MASK GENMASK(11, 8)
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A 4
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_B 7
+
+#define LAN8841_EVENT_A 0
+#define LAN8841_EVENT_B 1
+
+static void lan8841_ptp_remove_event(struct kszphy_ptp_priv *ptp_priv, int pin,
+ u8 event)
+{
+ struct phy_device *phydev = ptp_priv->phydev;
+ u8 offset;
+ u16 tmp;
+
+ /* Not remove pin from the event. GPIO_DATA_SEL1 contains the GPIO
+ * pins 0-4 while GPIO_DATA_SEL2 contains GPIO pins 5-9, therefore
+ * depending on the pin, it requires to read a different register
+ */
+ if (pin < 5) {
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1);
+ offset = pin;
+ } else {
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2);
+ offset = pin - 5;
+ }
+ tmp &= ~(LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_MASK << (3 * offset));
+ if (pin < 5)
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1, tmp);
+ else
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2, tmp);
+
+ /* Disable the event */
+ tmp = phy_read_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG);
+ if (event == LAN8841_EVENT_A) {
+ tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
+ tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
+ } else {
+ tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
+ tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
+ }
+ phy_write_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG, tmp);
+}
+
+static void lan8841_ptp_enable_event(struct kszphy_ptp_priv *ptp_priv, int pin,
+ u8 event, int pulse_width)
+{
+ struct phy_device *phydev = ptp_priv->phydev;
+ u8 offset;
+ u16 tmp;
+
+ /* Enable the event */
+ tmp = phy_read_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG);
+ tmp |= event == LAN8841_EVENT_A ? LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A :
+ LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_B;
+ tmp &= event == LAN8841_EVENT_A ? ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK :
+ ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_B_MASK;
+ tmp |= event == LAN8841_EVENT_A ? pulse_width << LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A :
+ pulse_width << LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_B;
+ phy_write_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG, tmp);
+
+ /* Now connect the pin to the event. GPIO_DATA_SEL1 contains the GPIO
+ * pins 0-4 while GPIO_DATA_SEL2 contains GPIO pins 5-9, therefore
+ * depending on the pin, it requires to read a different register
+ */
+ if (pin < 5) {
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1);
+ offset = pin;
+ } else {
+ tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2);
+ offset = pin - 5;
+ }
+
+ if (event == LAN8841_EVENT_A)
+ tmp |= LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_A << (3 * offset);
+ else
+ tmp |= LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_B << (3 * offset);
+
+ if (pin < 5)
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1, tmp);
+ else
+ phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2, tmp);
+}
+
+#define LAN8841_PTP_LTC_TARGET_SEC_HI(event) ((event) == LAN8841_EVENT_A ? 278 : 288)
+#define LAN8841_PTP_LTC_TARGET_SEC_LO(event) ((event) == LAN8841_EVENT_A ? 279 : 289)
+#define LAN8841_PTP_LTC_TARGET_NS_HI(event) ((event) == LAN8841_EVENT_A ? 280 : 290)
+#define LAN8841_PTP_LTC_TARGET_NS_LO(event) ((event) == LAN8841_EVENT_A ? 281 : 291)
+
+static void lan8841_ptp_set_target(struct kszphy_ptp_priv *ptp_priv, u8 event,
+ s64 sec, u32 nsec)
+{
+ struct phy_device *phydev = ptp_priv->phydev;
+
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_SEC_HI(event),
+ upper_16_bits(sec));
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_SEC_LO(event),
+ lower_16_bits(sec));
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_NS_HI(event) & 0x3fff,
+ upper_16_bits(nsec));
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_NS_LO(event),
+ lower_16_bits(nsec));
+}
+
+#define LAN8841_BUFFER_TIME 2
+
+static void lan8841_ptp_update_target(struct kszphy_ptp_priv *ptp_priv,
+ const struct timespec64 *ts)
+{
+ lan8841_ptp_set_target(ptp_priv, LAN8841_EVENT_A,
+ ts->tv_sec + LAN8841_BUFFER_TIME, ts->tv_nsec);
+}
+
+#define LAN8841_PTP_LTC_TARGET_RELOAD_SEC_HI(event) ((event) == LAN8841_EVENT_A ? 282 : 292)
+#define LAN8841_PTP_LTC_TARGET_RELOAD_SEC_LO(event) ((event) == LAN8841_EVENT_A ? 283 : 293)
+#define LAN8841_PTP_LTC_TARGET_RELOAD_NS_HI(event) ((event) == LAN8841_EVENT_A ? 284 : 294)
+#define LAN8841_PTP_LTC_TARGET_RELOAD_NS_LO(event) ((event) == LAN8841_EVENT_A ? 285 : 295)
+
+static void lan8841_ptp_set_reload(struct kszphy_ptp_priv *ptp_priv, u8 event,
+ s64 sec, u32 nsec)
+{
+ struct phy_device *phydev = ptp_priv->phydev;
+
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_RELOAD_SEC_HI(event),
+ upper_16_bits(sec));
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_RELOAD_SEC_LO(event),
+ lower_16_bits(sec));
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_RELOAD_NS_HI(event) & 0x3fff,
+ upper_16_bits(nsec));
+ phy_write_mmd(phydev, 2, LAN8841_PTP_LTC_TARGET_RELOAD_NS_LO(event),
+ lower_16_bits(nsec));
+}
+
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_200MS 13
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100MS 12
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_50MS 11
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_10MS 10
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_5MS 9
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_1MS 8
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_500US 7
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100US 6
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_50US 5
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_10US 4
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_5US 3
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_1US 2
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_500NS 1
+#define LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100NS 0
+
+static int lan8841_ptp_perout(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
+ ptp_clock_info);
+ struct phy_device *phydev = ptp_priv->phydev;
+ struct timespec64 ts_on, ts_period;
+ s64 on_nsec, period_nsec;
+ int pulse_width;
+ int pin;
+
+ if (rq->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
+ return -EOPNOTSUPP;
+
+ pin = ptp_find_pin(ptp_priv->ptp_clock, PTP_PF_PEROUT, rq->perout.index);
+ if (pin == -1 || pin >= LAN8841_PTP_GPIO_NUM)
+ return -EINVAL;
+
+ if (!on) {
+ lan8841_ptp_perout_off(ptp_priv, pin);
+ lan8841_ptp_remove_event(ptp_priv, LAN8841_EVENT_A, pin);
+ return 0;
+ }
+
+ ts_on.tv_sec = rq->perout.on.sec;
+ ts_on.tv_nsec = rq->perout.on.nsec;
+ on_nsec = timespec64_to_ns(&ts_on);
+
+ ts_period.tv_sec = rq->perout.period.sec;
+ ts_period.tv_nsec = rq->perout.period.nsec;
+ period_nsec = timespec64_to_ns(&ts_period);
+
+ if (period_nsec < 200) {
+ phydev_warn(phydev,
+ "perout period too small, minimum is 200 nsec\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (on_nsec >= period_nsec) {
+ phydev_warn(phydev,
+ "pulse width must be smaller than period\n");
+ return -EINVAL;
+ }
+
+ switch (on_nsec) {
+ case 200000000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_200MS;
+ break;
+ case 100000000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100MS;
+ break;
+ case 50000000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_50MS;
+ break;
+ case 10000000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_10MS;
+ break;
+ case 5000000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_5MS;
+ break;
+ case 1000000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_1MS;
+ break;
+ case 500000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_500US;
+ break;
+ case 100000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100US;
+ break;
+ case 50000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_50US;
+ break;
+ case 10000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_10US;
+ break;
+ case 5000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_5US;
+ break;
+ case 1000:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_1US;
+ break;
+ case 500:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_500NS;
+ break;
+ case 100:
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100NS;
+ break;
+ default:
+ phydev_warn(phydev, "Use default duty cycle of 100ns\n");
+ pulse_width = LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100NS;
+ break;
+ }
+
+ mutex_lock(&ptp_priv->ptp_lock);
+ lan8841_ptp_set_target(ptp_priv, LAN8841_EVENT_A, rq->perout.start.sec,
+ rq->perout.start.nsec);
+ mutex_unlock(&ptp_priv->ptp_lock);
+
+ lan8841_ptp_set_reload(ptp_priv, LAN8841_EVENT_A, rq->perout.period.sec,
+ rq->perout.period.nsec);
+ lan8841_ptp_enable_event(ptp_priv, pin, LAN8841_EVENT_A, pulse_width);
+ lan8841_ptp_perout_on(ptp_priv, pin);
+
+ return 0;
+}
+
+static int lan8841_ptp_enable(struct ptp_clock_info *ptp,
+ struct ptp_clock_request *rq, int on)
+{
+ switch (rq->type) {
+ case PTP_CLK_REQ_PEROUT:
+ return lan8841_ptp_perout(ptp, rq, on);
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
static struct ptp_clock_info lan8841_ptp_clock_info = {
.owner = THIS_MODULE,
.name = "lan8841 ptp",
@@ -3847,6 +4189,10 @@ static struct ptp_clock_info lan8841_ptp_clock_info = {
.settime64 = lan8841_ptp_settime64,
.adjtime = lan8841_ptp_adjtime,
.adjfine = lan8841_ptp_adjfine,
+ .verify = lan8841_ptp_verify,
+ .enable = lan8841_ptp_enable,
+ .n_per_out = LAN8841_PTP_GPIO_NUM,
+ .n_pins = LAN8841_PTP_GPIO_NUM,
};
#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER 3
@@ -3874,7 +4220,24 @@ static int lan8841_probe(struct phy_device *phydev)
priv = phydev->priv;
ptp_priv = &priv->ptp_priv;
+ ptp_priv->pin_config = devm_kmalloc_array(&phydev->mdio.dev,
+ LAN8841_PTP_GPIO_NUM,
+ sizeof(*ptp_priv->pin_config),
+ GFP_KERNEL);
+ if (!ptp_priv->pin_config)
+ return -ENOMEM;
+
+ for (int i = 0; i < LAN8841_PTP_GPIO_NUM; ++i) {
+ struct ptp_pin_desc *p = &ptp_priv->pin_config[i];
+
+ memset(p, 0, sizeof(*p));
+ snprintf(p->name, sizeof(p->name), "pin%d", i);
+ p->index = i;
+ p->func = PTP_PF_NONE;
+ }
+
ptp_priv->ptp_clock_info = lan8841_ptp_clock_info;
+ ptp_priv->ptp_clock_info.pin_config = ptp_priv->pin_config;
ptp_priv->ptp_clock = ptp_clock_register(&ptp_priv->ptp_clock_info,
&phydev->mdio.dev);
if (IS_ERR(ptp_priv->ptp_clock)) {
--
2.38.0
On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> +{
> + struct phy_device *phydev = ptp_priv->phydev;
> + u16 tmp;
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> + tmp &= ~BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
Problem 1: doesn't check the return value of phy_read_mmd(), so a
spurious error results in an error code written back to the register.
Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
probably also makes sense to cache the mask. Thus, this whole thing
becomes:
u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
although I'm not sure why you need to mask off bits 15:11.
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> + tmp &= ~BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> + tmp &= ~BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
> +}
> +
> +static void lan8841_ptp_perout_on(struct kszphy_ptp_priv *ptp_priv, int pin)
> +{
> + struct phy_device *phydev = ptp_priv->phydev;
> + u16 tmp;
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> + tmp |= BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> + tmp |= BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> + tmp |= BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
Similar as above.
> +static void lan8841_ptp_remove_event(struct kszphy_ptp_priv *ptp_priv, int pin,
> + u8 event)
> +{
> + struct phy_device *phydev = ptp_priv->phydev;
> + u8 offset;
> + u16 tmp;
> +
> + /* Not remove pin from the event. GPIO_DATA_SEL1 contains the GPIO
> + * pins 0-4 while GPIO_DATA_SEL2 contains GPIO pins 5-9, therefore
> + * depending on the pin, it requires to read a different register
> + */
> + if (pin < 5) {
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1);
> + offset = pin;
> + } else {
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2);
> + offset = pin - 5;
> + }
> + tmp &= ~(LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_MASK << (3 * offset));
> + if (pin < 5)
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1, tmp);
> + else
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2, tmp);
This could be much simpler using phy_modify_mmd().
> +
> + /* Disable the event */
> + tmp = phy_read_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG);
> + if (event == LAN8841_EVENT_A) {
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> + } else {
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> + }
> + phy_write_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG, tmp);
Ditto... and the theme seems to continue throughout the rest of this
patch.
> +static int lan8841_ptp_perout(struct ptp_clock_info *ptp,
> + struct ptp_clock_request *rq, int on)
> +{
> + struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> + ptp_clock_info);
> + struct phy_device *phydev = ptp_priv->phydev;
> + struct timespec64 ts_on, ts_period;
> + s64 on_nsec, period_nsec;
> + int pulse_width;
> + int pin;
> +
> + if (rq->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
> + return -EOPNOTSUPP;
> +
> + pin = ptp_find_pin(ptp_priv->ptp_clock, PTP_PF_PEROUT, rq->perout.index);
> + if (pin == -1 || pin >= LAN8841_PTP_GPIO_NUM)
> + return -EINVAL;
> +
> + if (!on) {
> + lan8841_ptp_perout_off(ptp_priv, pin);
> + lan8841_ptp_remove_event(ptp_priv, LAN8841_EVENT_A, pin);
> + return 0;
> + }
> +
> + ts_on.tv_sec = rq->perout.on.sec;
> + ts_on.tv_nsec = rq->perout.on.nsec;
> + on_nsec = timespec64_to_ns(&ts_on);
> +
> + ts_period.tv_sec = rq->perout.period.sec;
> + ts_period.tv_nsec = rq->perout.period.nsec;
> + period_nsec = timespec64_to_ns(&ts_period);
> +
> + if (period_nsec < 200) {
> + phydev_warn(phydev,
> + "perout period too small, minimum is 200 nsec\n");
I'm not sure using the kernel log to print such things is a good idea,
especially without rate limiting.
> @@ -3874,7 +4220,24 @@ static int lan8841_probe(struct phy_device *phydev)
> priv = phydev->priv;
> ptp_priv = &priv->ptp_priv;
>
> + ptp_priv->pin_config = devm_kmalloc_array(&phydev->mdio.dev,
> + LAN8841_PTP_GPIO_NUM,
> + sizeof(*ptp_priv->pin_config),
> + GFP_KERNEL);
devm_kcalloc() to avoid the memset() below?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Feb 17, 2023 at 01:30:06PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> > +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u16 tmp;
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
>
> Problem 1: doesn't check the return value of phy_read_mmd(), so a
> spurious error results in an error code written back to the register.
>
> Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
> probably also makes sense to cache the mask. Thus, this whole thing
> becomes:
>
> u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
>
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
>
> although I'm not sure why you need to mask off bits 15:11.
Or even use phy_clear_bits_mmd(). There's also phy_set_bits_mmd() which I
think would also be useful elsewhere in this driver.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
The 02/17/2023 13:30, Russell King (Oracle) wrote:
Hi Russel,
>
> On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> > +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u16 tmp;
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
>
> Problem 1: doesn't check the return value of phy_read_mmd(), so a
> spurious error results in an error code written back to the register.
>
> Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
> probably also makes sense to cache the mask. Thus, this whole thing
> becomes:
>
> u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
>
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
Thanks for the review.
I will look at phy_modify_mmd and the other helper functions and try to
use them in the other perout functions.
>
> although I'm not sure why you need to mask off bits 15:11.
It is not necessary, only that those bits are marked as reserved.
>
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
> > +}
> > +
> > +static void lan8841_ptp_perout_on(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u16 tmp;
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > + tmp |= BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> > + tmp |= BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> > + tmp |= BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
>
> Similar as above.
>
> > +static void lan8841_ptp_remove_event(struct kszphy_ptp_priv *ptp_priv, int pin,
> > + u8 event)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u8 offset;
> > + u16 tmp;
> > +
> > + /* Not remove pin from the event. GPIO_DATA_SEL1 contains the GPIO
> > + * pins 0-4 while GPIO_DATA_SEL2 contains GPIO pins 5-9, therefore
> > + * depending on the pin, it requires to read a different register
> > + */
> > + if (pin < 5) {
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1);
> > + offset = pin;
> > + } else {
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2);
> > + offset = pin - 5;
> > + }
> > + tmp &= ~(LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_MASK << (3 * offset));
> > + if (pin < 5)
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1, tmp);
> > + else
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2, tmp);
>
> This could be much simpler using phy_modify_mmd().
>
> > +
> > + /* Disable the event */
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG);
> > + if (event == LAN8841_EVENT_A) {
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> > + } else {
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> > + }
> > + phy_write_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG, tmp);
>
> Ditto... and the theme seems to continue throughout the rest of this
> patch.
>
> > +static int lan8841_ptp_perout(struct ptp_clock_info *ptp,
> > + struct ptp_clock_request *rq, int on)
> > +{
> > + struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> > + ptp_clock_info);
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + struct timespec64 ts_on, ts_period;
> > + s64 on_nsec, period_nsec;
> > + int pulse_width;
> > + int pin;
> > +
> > + if (rq->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
> > + return -EOPNOTSUPP;
> > +
> > + pin = ptp_find_pin(ptp_priv->ptp_clock, PTP_PF_PEROUT, rq->perout.index);
> > + if (pin == -1 || pin >= LAN8841_PTP_GPIO_NUM)
> > + return -EINVAL;
> > +
> > + if (!on) {
> > + lan8841_ptp_perout_off(ptp_priv, pin);
> > + lan8841_ptp_remove_event(ptp_priv, LAN8841_EVENT_A, pin);
> > + return 0;
> > + }
> > +
> > + ts_on.tv_sec = rq->perout.on.sec;
> > + ts_on.tv_nsec = rq->perout.on.nsec;
> > + on_nsec = timespec64_to_ns(&ts_on);
> > +
> > + ts_period.tv_sec = rq->perout.period.sec;
> > + ts_period.tv_nsec = rq->perout.period.nsec;
> > + period_nsec = timespec64_to_ns(&ts_period);
> > +
> > + if (period_nsec < 200) {
> > + phydev_warn(phydev,
> > + "perout period too small, minimum is 200 nsec\n");
>
> I'm not sure using the kernel log to print such things is a good idea,
> especially without rate limiting.
I think it would be nice to have these warnings as it would be nice to
know why it fails. I will use pr_warn_ratelimited in the next version.
>
> > @@ -3874,7 +4220,24 @@ static int lan8841_probe(struct phy_device *phydev)
> > priv = phydev->priv;
> > ptp_priv = &priv->ptp_priv;
> >
> > + ptp_priv->pin_config = devm_kmalloc_array(&phydev->mdio.dev,
> > + LAN8841_PTP_GPIO_NUM,
> > + sizeof(*ptp_priv->pin_config),
> > + GFP_KERNEL);
>
> devm_kcalloc() to avoid the memset() below?
Good point. I will do that.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
--
/Horatiu
> +static void lan8841_ptp_update_target(struct kszphy_ptp_priv *ptp_priv,
> + const struct timespec64 *ts);
> +
Please avoid this. Move the code around so everything is in
order. Generally, i do such moves in an initial patch which only moves
code, making it easy to review.
Andrew
The 02/18/2023 01:55, Andrew Lunn wrote:
>
> > +static void lan8841_ptp_update_target(struct kszphy_ptp_priv *ptp_priv,
> > + const struct timespec64 *ts);
> > +
>
> Please avoid this. Move the code around so everything is in
> order. Generally, i do such moves in an initial patch which only moves
> code, making it easy to review.
Thanks for the suggestion. I will update this in the next version.
>
> Andrew
--
/Horatiu