* [PATCH 0/9] net: ethernet: ti: cpts: update and fixes
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
Hi,
It is preparation serie intended to clean up and optimize TI CPTS driver to
facilitate further integration with other TI's SoCs like Keystone 2.
It also include some non critical fixes:
net: ethernet: ti: exclude cpts from build when disabled
net: ethernet: ti: cpts: fix overflow check period
net: ethernet: ti: cpts: clean up event list if event pool is empty
Grygorii Strashko (7):
net: ethernet: ti: exclude cpts from build when disabled
net: ethernet: ti: cpsw: minimize direct access to struct cpts
net: ethernet: ti: cpts: rework initialization/deinitialization
net: ethernet: ti: cpts: move dt props parsing to cpts driver
net: ethernet: ti: cpts: calc mult and shift from refclk freq
net: ethernet: ti: cpts: fix overflow check period
net: ethernet: ti: cpts: switch to readl/writel_relaxed()
WingMan Kwok (2):
net: ethernet: ti: cpts: add return value to tx and rx timestamp
funcitons
net: ethernet: ti: cpts: clean up event list if event pool is empty
Documentation/devicetree/bindings/net/cpsw.txt | 4 +-
drivers/net/ethernet/ti/Makefile | 3 +-
drivers/net/ethernet/ti/cpsw.c | 83 ++++----
drivers/net/ethernet/ti/cpsw.h | 2 -
drivers/net/ethernet/ti/cpts.c | 256 ++++++++++++++++++-------
drivers/net/ethernet/ti/cpts.h | 93 +++++++--
6 files changed, 319 insertions(+), 122 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 0/9] net: ethernet: ti: cpts: update and fixes
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
Hi,
It is preparation serie intended to clean up and optimize TI CPTS driver to
facilitate further integration with other TI's SoCs like Keystone 2.
It also include some non critical fixes:
net: ethernet: ti: exclude cpts from build when disabled
net: ethernet: ti: cpts: fix overflow check period
net: ethernet: ti: cpts: clean up event list if event pool is empty
Grygorii Strashko (7):
net: ethernet: ti: exclude cpts from build when disabled
net: ethernet: ti: cpsw: minimize direct access to struct cpts
net: ethernet: ti: cpts: rework initialization/deinitialization
net: ethernet: ti: cpts: move dt props parsing to cpts driver
net: ethernet: ti: cpts: calc mult and shift from refclk freq
net: ethernet: ti: cpts: fix overflow check period
net: ethernet: ti: cpts: switch to readl/writel_relaxed()
WingMan Kwok (2):
net: ethernet: ti: cpts: add return value to tx and rx timestamp
funcitons
net: ethernet: ti: cpts: clean up event list if event pool is empty
Documentation/devicetree/bindings/net/cpsw.txt | 4 +-
drivers/net/ethernet/ti/Makefile | 3 +-
drivers/net/ethernet/ti/cpsw.c | 83 ++++----
drivers/net/ethernet/ti/cpsw.h | 2 -
drivers/net/ethernet/ti/cpts.c | 256 ++++++++++++++++++-------
drivers/net/ethernet/ti/cpts.h | 93 +++++++--
6 files changed, 319 insertions(+), 122 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 1/9] net: ethernet: ti: exclude cpts from build when disabled
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
TI CPTS feature declared as optional, but cpts.c module is
included in build always.
Exclude cpts.c from build when CPTS is disabled in Kconfig and
optimize usage of CONFIG_TI_CPTS.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/Makefile | 3 ++-
drivers/net/ethernet/ti/cpsw.c | 21 ++++++++++++++++-----
drivers/net/ethernet/ti/cpts.c | 8 --------
drivers/net/ethernet/ti/cpts.h | 14 ++++++++++++--
4 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index d420d94..1e7c10b 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -12,8 +12,9 @@ obj-$(CONFIG_TI_DAVINCI_MDIO) += davinci_mdio.o
obj-$(CONFIG_TI_DAVINCI_CPDMA) += davinci_cpdma.o
obj-$(CONFIG_TI_CPSW_PHY_SEL) += cpsw-phy-sel.o
obj-$(CONFIG_TI_CPSW_ALE) += cpsw_ale.o
+obj-$(CONFIG_TI_CPTS) += cpts.o
obj-$(CONFIG_TI_CPSW) += ti_cpsw.o
-ti_cpsw-y := cpsw.o cpts.o
+ti_cpsw-y := cpsw.o
obj-$(CONFIG_TI_KEYSTONE_NETCP) += keystone_netcp.o
keystone_netcp-y := netcp_core.o
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c6cff3d..b743bb1d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1514,7 +1514,6 @@ fail:
}
#ifdef CONFIG_TI_CPTS
-
static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
{
struct cpsw_slave *slave = &cpsw->slaves[cpsw->data.active_slave];
@@ -1661,7 +1660,16 @@ static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
}
+#else
+static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
+{
+ return -EOPNOTSUPP;
+}
+static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
+{
+ return -EOPNOTSUPP;
+}
#endif /*CONFIG_TI_CPTS*/
static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
@@ -1674,12 +1682,10 @@ static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
return -EINVAL;
switch (cmd) {
-#ifdef CONFIG_TI_CPTS
case SIOCSHWTSTAMP:
return cpsw_hwtstamp_set(dev, req);
case SIOCGHWTSTAMP:
return cpsw_hwtstamp_get(dev, req);
-#endif
}
if (!cpsw->slaves[slave_no].phy)
@@ -1935,10 +1941,10 @@ static void cpsw_set_msglevel(struct net_device *ndev, u32 value)
priv->msg_enable = value;
}
+#ifdef CONFIG_TI_CPTS
static int cpsw_get_ts_info(struct net_device *ndev,
struct ethtool_ts_info *info)
{
-#ifdef CONFIG_TI_CPTS
struct cpsw_common *cpsw = ndev_to_cpsw(ndev);
info->so_timestamping =
@@ -1955,7 +1961,12 @@ static int cpsw_get_ts_info(struct net_device *ndev,
info->rx_filters =
(1 << HWTSTAMP_FILTER_NONE) |
(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+ return 0;
+}
#else
+static int cpsw_get_ts_info(struct net_device *ndev,
+ struct ethtool_ts_info *info)
+{
info->so_timestamping =
SOF_TIMESTAMPING_TX_SOFTWARE |
SOF_TIMESTAMPING_RX_SOFTWARE |
@@ -1963,9 +1974,9 @@ static int cpsw_get_ts_info(struct net_device *ndev,
info->phc_index = -1;
info->tx_types = 0;
info->rx_filters = 0;
-#endif
return 0;
}
+#endif
static int cpsw_get_settings(struct net_device *ndev,
struct ethtool_cmd *ecmd)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 85a55b4..aaab08e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -31,8 +31,6 @@
#include "cpts.h"
-#ifdef CONFIG_TI_CPTS
-
#define cpts_read32(c, r) __raw_readl(&c->reg->r)
#define cpts_write32(c, v, r) __raw_writel(v, &c->reg->r)
@@ -350,12 +348,9 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
skb_tstamp_tx(skb, &ssh);
}
-#endif /*CONFIG_TI_CPTS*/
-
int cpts_register(struct device *dev, struct cpts *cpts,
u32 mult, u32 shift)
{
-#ifdef CONFIG_TI_CPTS
int err, i;
unsigned long flags;
@@ -391,18 +386,15 @@ int cpts_register(struct device *dev, struct cpts *cpts,
schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
cpts->phc_index = ptp_clock_index(cpts->clock);
-#endif
return 0;
}
void cpts_unregister(struct cpts *cpts)
{
-#ifdef CONFIG_TI_CPTS
if (cpts->clock) {
ptp_clock_unregister(cpts->clock);
cancel_delayed_work_sync(&cpts->overflow_work);
}
if (cpts->refclk)
cpts_clk_release(cpts);
-#endif
}
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 69a46b9..a68780d 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -130,6 +130,8 @@ struct cpts {
#ifdef CONFIG_TI_CPTS
void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+void cpts_unregister(struct cpts *cpts);
#else
static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
@@ -137,9 +139,17 @@ static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
}
+
+static inline int
+cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+{
+ return 0;
+}
+
+static inline void cpts_unregister(struct cpts *cpts)
+{
+}
#endif
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
-void cpts_unregister(struct cpts *cpts);
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 1/9] net: ethernet: ti: exclude cpts from build when disabled
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
TI CPTS feature declared as optional, but cpts.c module is
included in build always.
Exclude cpts.c from build when CPTS is disabled in Kconfig and
optimize usage of CONFIG_TI_CPTS.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/Makefile | 3 ++-
drivers/net/ethernet/ti/cpsw.c | 21 ++++++++++++++++-----
drivers/net/ethernet/ti/cpts.c | 8 --------
drivers/net/ethernet/ti/cpts.h | 14 ++++++++++++--
4 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/ti/Makefile b/drivers/net/ethernet/ti/Makefile
index d420d94..1e7c10b 100644
--- a/drivers/net/ethernet/ti/Makefile
+++ b/drivers/net/ethernet/ti/Makefile
@@ -12,8 +12,9 @@ obj-$(CONFIG_TI_DAVINCI_MDIO) += davinci_mdio.o
obj-$(CONFIG_TI_DAVINCI_CPDMA) += davinci_cpdma.o
obj-$(CONFIG_TI_CPSW_PHY_SEL) += cpsw-phy-sel.o
obj-$(CONFIG_TI_CPSW_ALE) += cpsw_ale.o
+obj-$(CONFIG_TI_CPTS) += cpts.o
obj-$(CONFIG_TI_CPSW) += ti_cpsw.o
-ti_cpsw-y := cpsw.o cpts.o
+ti_cpsw-y := cpsw.o
obj-$(CONFIG_TI_KEYSTONE_NETCP) += keystone_netcp.o
keystone_netcp-y := netcp_core.o
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c6cff3d..b743bb1d 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1514,7 +1514,6 @@ fail:
}
#ifdef CONFIG_TI_CPTS
-
static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
{
struct cpsw_slave *slave = &cpsw->slaves[cpsw->data.active_slave];
@@ -1661,7 +1660,16 @@ static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
}
+#else
+static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
+{
+ return -EOPNOTSUPP;
+}
+static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
+{
+ return -EOPNOTSUPP;
+}
#endif /*CONFIG_TI_CPTS*/
static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
@@ -1674,12 +1682,10 @@ static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
return -EINVAL;
switch (cmd) {
-#ifdef CONFIG_TI_CPTS
case SIOCSHWTSTAMP:
return cpsw_hwtstamp_set(dev, req);
case SIOCGHWTSTAMP:
return cpsw_hwtstamp_get(dev, req);
-#endif
}
if (!cpsw->slaves[slave_no].phy)
@@ -1935,10 +1941,10 @@ static void cpsw_set_msglevel(struct net_device *ndev, u32 value)
priv->msg_enable = value;
}
+#ifdef CONFIG_TI_CPTS
static int cpsw_get_ts_info(struct net_device *ndev,
struct ethtool_ts_info *info)
{
-#ifdef CONFIG_TI_CPTS
struct cpsw_common *cpsw = ndev_to_cpsw(ndev);
info->so_timestamping =
@@ -1955,7 +1961,12 @@ static int cpsw_get_ts_info(struct net_device *ndev,
info->rx_filters =
(1 << HWTSTAMP_FILTER_NONE) |
(1 << HWTSTAMP_FILTER_PTP_V2_EVENT);
+ return 0;
+}
#else
+static int cpsw_get_ts_info(struct net_device *ndev,
+ struct ethtool_ts_info *info)
+{
info->so_timestamping =
SOF_TIMESTAMPING_TX_SOFTWARE |
SOF_TIMESTAMPING_RX_SOFTWARE |
@@ -1963,9 +1974,9 @@ static int cpsw_get_ts_info(struct net_device *ndev,
info->phc_index = -1;
info->tx_types = 0;
info->rx_filters = 0;
-#endif
return 0;
}
+#endif
static int cpsw_get_settings(struct net_device *ndev,
struct ethtool_cmd *ecmd)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 85a55b4..aaab08e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -31,8 +31,6 @@
#include "cpts.h"
-#ifdef CONFIG_TI_CPTS
-
#define cpts_read32(c, r) __raw_readl(&c->reg->r)
#define cpts_write32(c, v, r) __raw_writel(v, &c->reg->r)
@@ -350,12 +348,9 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
skb_tstamp_tx(skb, &ssh);
}
-#endif /*CONFIG_TI_CPTS*/
-
int cpts_register(struct device *dev, struct cpts *cpts,
u32 mult, u32 shift)
{
-#ifdef CONFIG_TI_CPTS
int err, i;
unsigned long flags;
@@ -391,18 +386,15 @@ int cpts_register(struct device *dev, struct cpts *cpts,
schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
cpts->phc_index = ptp_clock_index(cpts->clock);
-#endif
return 0;
}
void cpts_unregister(struct cpts *cpts)
{
-#ifdef CONFIG_TI_CPTS
if (cpts->clock) {
ptp_clock_unregister(cpts->clock);
cancel_delayed_work_sync(&cpts->overflow_work);
}
if (cpts->refclk)
cpts_clk_release(cpts);
-#endif
}
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 69a46b9..a68780d 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -130,6 +130,8 @@ struct cpts {
#ifdef CONFIG_TI_CPTS
void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+void cpts_unregister(struct cpts *cpts);
#else
static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
@@ -137,9 +139,17 @@ static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
}
+
+static inline int
+cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+{
+ return 0;
+}
+
+static inline void cpts_unregister(struct cpts *cpts)
+{
+}
#endif
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
-void cpts_unregister(struct cpts *cpts);
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 2/9] net: ethernet: ti: cpsw: minimize direct access to struct cpts
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
This will provide more flexibility in changing CPTS internals and also
required for further changes.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 28 +++++++++++++++-------------
drivers/net/ethernet/ti/cpts.h | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index b743bb1d..9b900f0 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1481,7 +1481,7 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
}
if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
- cpsw->cpts->tx_enable)
+ cpts_is_tx_enabled(cpsw->cpts))
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
skb_tx_timestamp(skb);
@@ -1519,7 +1519,8 @@ static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
struct cpsw_slave *slave = &cpsw->slaves[cpsw->data.active_slave];
u32 ts_en, seq_id;
- if (!cpsw->cpts->tx_enable && !cpsw->cpts->rx_enable) {
+ if (!cpts_is_tx_enabled(cpsw->cpts) &&
+ !cpts_is_rx_enabled(cpsw->cpts)) {
slave_write(slave, 0, CPSW1_TS_CTL);
return;
}
@@ -1527,10 +1528,10 @@ static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
seq_id = (30 << CPSW_V1_SEQ_ID_OFS_SHIFT) | ETH_P_1588;
ts_en = EVENT_MSG_BITS << CPSW_V1_MSG_TYPE_OFS;
- if (cpsw->cpts->tx_enable)
+ if (cpts_is_tx_enabled(cpsw->cpts))
ts_en |= CPSW_V1_TS_TX_EN;
- if (cpsw->cpts->rx_enable)
+ if (cpts_is_rx_enabled(cpsw->cpts))
ts_en |= CPSW_V1_TS_RX_EN;
slave_write(slave, ts_en, CPSW1_TS_CTL);
@@ -1553,20 +1554,20 @@ static void cpsw_hwtstamp_v2(struct cpsw_priv *priv)
case CPSW_VERSION_2:
ctrl &= ~CTRL_V2_ALL_TS_MASK;
- if (cpsw->cpts->tx_enable)
+ if (cpts_is_tx_enabled(cpsw->cpts))
ctrl |= CTRL_V2_TX_TS_BITS;
- if (cpsw->cpts->rx_enable)
+ if (cpts_is_rx_enabled(cpsw->cpts))
ctrl |= CTRL_V2_RX_TS_BITS;
break;
case CPSW_VERSION_3:
default:
ctrl &= ~CTRL_V3_ALL_TS_MASK;
- if (cpsw->cpts->tx_enable)
+ if (cpts_is_tx_enabled(cpsw->cpts))
ctrl |= CTRL_V3_TX_TS_BITS;
- if (cpsw->cpts->rx_enable)
+ if (cpts_is_rx_enabled(cpsw->cpts))
ctrl |= CTRL_V3_RX_TS_BITS;
break;
}
@@ -1602,7 +1603,7 @@ static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
- cpts->rx_enable = 0;
+ cpts_rx_enable(cpts, 0);
break;
case HWTSTAMP_FILTER_ALL:
case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
@@ -1618,14 +1619,14 @@ static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
case HWTSTAMP_FILTER_PTP_V2_EVENT:
case HWTSTAMP_FILTER_PTP_V2_SYNC:
case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
- cpts->rx_enable = 1;
+ cpts_rx_enable(cpts, 1);
cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
break;
default:
return -ERANGE;
}
- cpts->tx_enable = cfg.tx_type == HWTSTAMP_TX_ON;
+ cpts_tx_enable(cpts, cfg.tx_type == HWTSTAMP_TX_ON);
switch (cpsw->version) {
case CPSW_VERSION_1:
@@ -1654,8 +1655,9 @@ static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
return -EOPNOTSUPP;
cfg.flags = 0;
- cfg.tx_type = cpts->tx_enable ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
- cfg.rx_filter = (cpts->rx_enable ?
+ cfg.tx_type = cpts_is_tx_enabled(cpts) ?
+ HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+ cfg.rx_filter = (cpts_is_rx_enabled(cpts) ?
HWTSTAMP_FILTER_PTP_V2_EVENT : HWTSTAMP_FILTER_NONE);
return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index a68780d..fec753c 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -132,6 +132,29 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
void cpts_unregister(struct cpts *cpts);
+
+static inline void cpts_rx_enable(struct cpts *cpts, int enable)
+{
+ if (cpts)
+ cpts->rx_enable = enable;
+}
+
+static inline bool cpts_is_rx_enabled(struct cpts *cpts)
+{
+ return cpts && !!cpts->rx_enable;
+}
+
+static inline void cpts_tx_enable(struct cpts *cpts, int enable)
+{
+ if (cpts)
+ cpts->tx_enable = enable;
+}
+
+static inline bool cpts_is_tx_enabled(struct cpts *cpts)
+{
+ return cpts && !!cpts->tx_enable;
+}
+
#else
static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
@@ -149,6 +172,24 @@ cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
static inline void cpts_unregister(struct cpts *cpts)
{
}
+
+static inline void cpts_rx_enable(struct cpts *cpts, int enable)
+{
+}
+
+static inline bool cpts_is_rx_enabled(struct cpts *cpts)
+{
+ return false;
+}
+
+static inline void cpts_tx_enable(struct cpts *cpts, int enable)
+{
+}
+
+static inline bool cpts_is_tx_enabled(struct cpts *cpts)
+{
+ return false;
+}
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 2/9] net: ethernet: ti: cpsw: minimize direct access to struct cpts
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
This will provide more flexibility in changing CPTS internals and also
required for further changes.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 28 +++++++++++++++-------------
drivers/net/ethernet/ti/cpts.h | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index b743bb1d..9b900f0 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1481,7 +1481,7 @@ static netdev_tx_t cpsw_ndo_start_xmit(struct sk_buff *skb,
}
if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
- cpsw->cpts->tx_enable)
+ cpts_is_tx_enabled(cpsw->cpts))
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
skb_tx_timestamp(skb);
@@ -1519,7 +1519,8 @@ static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
struct cpsw_slave *slave = &cpsw->slaves[cpsw->data.active_slave];
u32 ts_en, seq_id;
- if (!cpsw->cpts->tx_enable && !cpsw->cpts->rx_enable) {
+ if (!cpts_is_tx_enabled(cpsw->cpts) &&
+ !cpts_is_rx_enabled(cpsw->cpts)) {
slave_write(slave, 0, CPSW1_TS_CTL);
return;
}
@@ -1527,10 +1528,10 @@ static void cpsw_hwtstamp_v1(struct cpsw_common *cpsw)
seq_id = (30 << CPSW_V1_SEQ_ID_OFS_SHIFT) | ETH_P_1588;
ts_en = EVENT_MSG_BITS << CPSW_V1_MSG_TYPE_OFS;
- if (cpsw->cpts->tx_enable)
+ if (cpts_is_tx_enabled(cpsw->cpts))
ts_en |= CPSW_V1_TS_TX_EN;
- if (cpsw->cpts->rx_enable)
+ if (cpts_is_rx_enabled(cpsw->cpts))
ts_en |= CPSW_V1_TS_RX_EN;
slave_write(slave, ts_en, CPSW1_TS_CTL);
@@ -1553,20 +1554,20 @@ static void cpsw_hwtstamp_v2(struct cpsw_priv *priv)
case CPSW_VERSION_2:
ctrl &= ~CTRL_V2_ALL_TS_MASK;
- if (cpsw->cpts->tx_enable)
+ if (cpts_is_tx_enabled(cpsw->cpts))
ctrl |= CTRL_V2_TX_TS_BITS;
- if (cpsw->cpts->rx_enable)
+ if (cpts_is_rx_enabled(cpsw->cpts))
ctrl |= CTRL_V2_RX_TS_BITS;
break;
case CPSW_VERSION_3:
default:
ctrl &= ~CTRL_V3_ALL_TS_MASK;
- if (cpsw->cpts->tx_enable)
+ if (cpts_is_tx_enabled(cpsw->cpts))
ctrl |= CTRL_V3_TX_TS_BITS;
- if (cpsw->cpts->rx_enable)
+ if (cpts_is_rx_enabled(cpsw->cpts))
ctrl |= CTRL_V3_RX_TS_BITS;
break;
}
@@ -1602,7 +1603,7 @@ static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
switch (cfg.rx_filter) {
case HWTSTAMP_FILTER_NONE:
- cpts->rx_enable = 0;
+ cpts_rx_enable(cpts, 0);
break;
case HWTSTAMP_FILTER_ALL:
case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
@@ -1618,14 +1619,14 @@ static int cpsw_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
case HWTSTAMP_FILTER_PTP_V2_EVENT:
case HWTSTAMP_FILTER_PTP_V2_SYNC:
case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
- cpts->rx_enable = 1;
+ cpts_rx_enable(cpts, 1);
cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
break;
default:
return -ERANGE;
}
- cpts->tx_enable = cfg.tx_type == HWTSTAMP_TX_ON;
+ cpts_tx_enable(cpts, cfg.tx_type == HWTSTAMP_TX_ON);
switch (cpsw->version) {
case CPSW_VERSION_1:
@@ -1654,8 +1655,9 @@ static int cpsw_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
return -EOPNOTSUPP;
cfg.flags = 0;
- cfg.tx_type = cpts->tx_enable ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
- cfg.rx_filter = (cpts->rx_enable ?
+ cfg.tx_type = cpts_is_tx_enabled(cpts) ?
+ HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+ cfg.rx_filter = (cpts_is_rx_enabled(cpts) ?
HWTSTAMP_FILTER_PTP_V2_EVENT : HWTSTAMP_FILTER_NONE);
return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index a68780d..fec753c 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -132,6 +132,29 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
void cpts_unregister(struct cpts *cpts);
+
+static inline void cpts_rx_enable(struct cpts *cpts, int enable)
+{
+ if (cpts)
+ cpts->rx_enable = enable;
+}
+
+static inline bool cpts_is_rx_enabled(struct cpts *cpts)
+{
+ return cpts && !!cpts->rx_enable;
+}
+
+static inline void cpts_tx_enable(struct cpts *cpts, int enable)
+{
+ if (cpts)
+ cpts->tx_enable = enable;
+}
+
+static inline bool cpts_is_tx_enabled(struct cpts *cpts)
+{
+ return cpts && !!cpts->tx_enable;
+}
+
#else
static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
@@ -149,6 +172,24 @@ cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
static inline void cpts_unregister(struct cpts *cpts)
{
}
+
+static inline void cpts_rx_enable(struct cpts *cpts, int enable)
+{
+}
+
+static inline bool cpts_is_rx_enabled(struct cpts *cpts)
+{
+ return false;
+}
+
+static inline void cpts_tx_enable(struct cpts *cpts, int enable)
+{
+}
+
+static inline bool cpts_is_tx_enabled(struct cpts *cpts)
+{
+ return false;
+}
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) is pretty entangled and
has some issues, like:
- ptp clock registered before spinlock, which is protecting it, and
before timecounter and cyclecounter initialization;
- CPTS ref_clk requested using devm API while cpts_register() is
called from .ndo_open(), as result additional checks required;
- CPTS ref_clk is prepared, but never unprepared;
- CPTS is not disabled even when unregistered..
Hence, make things simpler and fix above issues by adding
cpts_create()/cpts_release() which should be called from
.probe()/.remove() respectively and move all static initialization
there. Clean up and update cpts_register/unregister() so PTP clock is
registered the last and unregistered first. In addition, this change
allows to clean up cpts.h for the case when CPTS is disabled.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 24 ++++----
drivers/net/ethernet/ti/cpts.c | 125 ++++++++++++++++++++++++++---------------
drivers/net/ethernet/ti/cpts.h | 26 +++++++--
3 files changed, 113 insertions(+), 62 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 9b900f0..dfd5707 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1406,9 +1406,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
if (ret < 0)
goto err_cleanup;
- if (cpts_register(cpsw->dev, cpsw->cpts,
- cpsw->data.cpts_clock_mult,
- cpsw->data.cpts_clock_shift))
+ if (cpts_register(cpsw->cpts))
dev_err(priv->dev, "error registering cpts device\n");
}
@@ -2551,6 +2549,7 @@ static int cpsw_probe(struct platform_device *pdev)
struct cpdma_params dma_params;
struct cpsw_ale_params ale_params;
void __iomem *ss_regs;
+ void __iomem *cpts_regs;
struct resource *res, *ss_res;
const struct of_device_id *of_id;
struct gpio_descs *mode;
@@ -2575,12 +2574,6 @@ static int cpsw_probe(struct platform_device *pdev)
priv->dev = &ndev->dev;
priv->msg_enable = netif_msg_init(debug_level, CPSW_DEBUG);
cpsw->rx_packet_max = max(rx_packet_max, 128);
- cpsw->cpts = devm_kzalloc(&pdev->dev, sizeof(struct cpts), GFP_KERNEL);
- if (!cpsw->cpts) {
- dev_err(&pdev->dev, "error allocating cpts\n");
- ret = -ENOMEM;
- goto clean_ndev_ret;
- }
mode = devm_gpiod_get_array_optional(&pdev->dev, "mode", GPIOD_OUT_LOW);
if (IS_ERR(mode)) {
@@ -2669,7 +2662,7 @@ static int cpsw_probe(struct platform_device *pdev)
switch (cpsw->version) {
case CPSW_VERSION_1:
cpsw->host_port_regs = ss_regs + CPSW1_HOST_PORT_OFFSET;
- cpsw->cpts->reg = ss_regs + CPSW1_CPTS_OFFSET;
+ cpts_regs = ss_regs + CPSW1_CPTS_OFFSET;
cpsw->hw_stats = ss_regs + CPSW1_HW_STATS;
dma_params.dmaregs = ss_regs + CPSW1_CPDMA_OFFSET;
dma_params.txhdp = ss_regs + CPSW1_STATERAM_OFFSET;
@@ -2683,7 +2676,7 @@ static int cpsw_probe(struct platform_device *pdev)
case CPSW_VERSION_3:
case CPSW_VERSION_4:
cpsw->host_port_regs = ss_regs + CPSW2_HOST_PORT_OFFSET;
- cpsw->cpts->reg = ss_regs + CPSW2_CPTS_OFFSET;
+ cpts_regs = ss_regs + CPSW2_CPTS_OFFSET;
cpsw->hw_stats = ss_regs + CPSW2_HW_STATS;
dma_params.dmaregs = ss_regs + CPSW2_CPDMA_OFFSET;
dma_params.txhdp = ss_regs + CPSW2_STATERAM_OFFSET;
@@ -2749,6 +2742,14 @@ static int cpsw_probe(struct platform_device *pdev)
goto clean_dma_ret;
}
+ cpsw->cpts = cpts_create(cpsw->dev, cpts_regs,
+ cpsw->data.cpts_clock_mult,
+ cpsw->data.cpts_clock_shift);
+ if (IS_ERR(cpsw->cpts)) {
+ ret = PTR_ERR(cpsw->cpts);
+ goto clean_ale_ret;
+ }
+
ndev->irq = platform_get_irq(pdev, 1);
if (ndev->irq < 0) {
dev_err(priv->dev, "error getting irq resource\n");
@@ -2857,6 +2858,7 @@ static int cpsw_remove(struct platform_device *pdev)
unregister_netdev(cpsw->slaves[1].ndev);
unregister_netdev(ndev);
+ cpts_release(cpsw->cpts);
cpsw_ale_destroy(cpsw->ale);
cpdma_ctlr_destroy(cpsw->dma);
of_platform_depopulate(&pdev->dev);
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index aaab08e..a46478e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -228,22 +228,6 @@ static void cpts_overflow_check(struct work_struct *work)
schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
}
-static void cpts_clk_init(struct device *dev, struct cpts *cpts)
-{
- cpts->refclk = devm_clk_get(dev, "cpts");
- if (IS_ERR(cpts->refclk)) {
- dev_err(dev, "Failed to get cpts refclk\n");
- cpts->refclk = NULL;
- return;
- }
- clk_prepare_enable(cpts->refclk);
-}
-
-static void cpts_clk_release(struct cpts *cpts)
-{
- clk_disable(cpts->refclk);
-}
-
static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
u16 ts_seqid, u8 ts_msgtype)
{
@@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
u64 ns;
struct skb_shared_hwtstamps *ssh;
- if (!cpts->rx_enable)
+ if (!cpts || !cpts->rx_enable)
return;
ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
if (!ns)
@@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
u64 ns;
struct skb_shared_hwtstamps ssh;
- if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
+ if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
return;
ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
if (!ns)
@@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
skb_tstamp_tx(skb, &ssh);
}
-int cpts_register(struct device *dev, struct cpts *cpts,
- u32 mult, u32 shift)
+int cpts_register(struct cpts *cpts)
{
int err, i;
- unsigned long flags;
- cpts->info = cpts_info;
- cpts->clock = ptp_clock_register(&cpts->info, dev);
- if (IS_ERR(cpts->clock)) {
- err = PTR_ERR(cpts->clock);
- cpts->clock = NULL;
- return err;
- }
- spin_lock_init(&cpts->lock);
-
- cpts->cc.read = cpts_systim_read;
- cpts->cc.mask = CLOCKSOURCE_MASK(32);
- cpts->cc_mult = mult;
- cpts->cc.mult = mult;
- cpts->cc.shift = shift;
+ if (!cpts)
+ return -EINVAL;
INIT_LIST_HEAD(&cpts->events);
INIT_LIST_HEAD(&cpts->pool);
for (i = 0; i < CPTS_MAX_EVENTS; i++)
list_add(&cpts->pool_data[i].list, &cpts->pool);
- cpts_clk_init(dev, cpts);
+ clk_enable(cpts->refclk);
+
cpts_write32(cpts, CPTS_EN, control);
cpts_write32(cpts, TS_PEND_EN, int_enable);
- spin_lock_irqsave(&cpts->lock, flags);
+ cpts->cc.mult = cpts->cc_mult;
timecounter_init(&cpts->tc, &cpts->cc, ktime_to_ns(ktime_get_real()));
- spin_unlock_irqrestore(&cpts->lock, flags);
-
- INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
- schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+ cpts->info = cpts_info;
+ cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
+ if (IS_ERR(cpts->clock)) {
+ err = PTR_ERR(cpts->clock);
+ cpts->clock = NULL;
+ goto err_ptp;
+ }
cpts->phc_index = ptp_clock_index(cpts->clock);
+
+ schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
return 0;
+
+err_ptp:
+ clk_disable(cpts->refclk);
+ return err;
}
void cpts_unregister(struct cpts *cpts)
{
- if (cpts->clock) {
- ptp_clock_unregister(cpts->clock);
- cancel_delayed_work_sync(&cpts->overflow_work);
+ if (!cpts)
+ return;
+
+ if (WARN_ON(!cpts->clock))
+ return;
+
+ cancel_delayed_work_sync(&cpts->overflow_work);
+
+ ptp_clock_unregister(cpts->clock);
+ cpts->clock = NULL;
+
+ cpts_write32(cpts, 0, int_enable);
+ cpts_write32(cpts, 0, control);
+
+ clk_disable(cpts->refclk);
+}
+
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+ u32 mult, u32 shift)
+{
+ struct cpts *cpts;
+
+ if (!regs || !dev)
+ return ERR_PTR(-EINVAL);
+
+ cpts = devm_kzalloc(dev, sizeof(*cpts), GFP_KERNEL);
+ if (!cpts)
+ return ERR_PTR(-ENOMEM);
+
+ cpts->dev = dev;
+ cpts->reg = (struct cpsw_cpts __iomem *)regs;
+ spin_lock_init(&cpts->lock);
+ INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
+
+ cpts->refclk = devm_clk_get(dev, "cpts");
+ if (IS_ERR(cpts->refclk)) {
+ dev_err(dev, "Failed to get cpts refclk\n");
+ return ERR_PTR(PTR_ERR(cpts->refclk));
}
- if (cpts->refclk)
- cpts_clk_release(cpts);
+
+ clk_prepare(cpts->refclk);
+
+ cpts->cc.read = cpts_systim_read;
+ cpts->cc.mask = CLOCKSOURCE_MASK(32);
+ cpts->cc.shift = shift;
+ cpts->cc_mult = mult;
+
+ return cpts;
+}
+
+void cpts_release(struct cpts *cpts)
+{
+ if (!cpts)
+ return;
+
+ if (!cpts->refclk)
+ return;
+
+ clk_unprepare(cpts->refclk);
}
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index fec753c..0c02f48 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -20,6 +20,8 @@
#ifndef _TI_CPTS_H_
#define _TI_CPTS_H_
+#ifdef CONFIG_TI_CPTS
+
#include <linux/clk.h>
#include <linux/clkdev.h>
#include <linux/clocksource.h>
@@ -108,10 +110,10 @@ struct cpts_event {
};
struct cpts {
+ struct device *dev;
struct cpsw_cpts __iomem *reg;
int tx_enable;
int rx_enable;
-#ifdef CONFIG_TI_CPTS
struct ptp_clock_info info;
struct ptp_clock *clock;
spinlock_t lock; /* protects time registers */
@@ -124,14 +126,15 @@ struct cpts {
struct list_head events;
struct list_head pool;
struct cpts_event pool_data[CPTS_MAX_EVENTS];
-#endif
};
-#ifdef CONFIG_TI_CPTS
void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+int cpts_register(struct cpts *cpts);
void cpts_unregister(struct cpts *cpts);
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+ u32 mult, u32 shift);
+void cpts_release(struct cpts *cpts);
static inline void cpts_rx_enable(struct cpts *cpts, int enable)
{
@@ -156,6 +159,8 @@ static inline bool cpts_is_tx_enabled(struct cpts *cpts)
}
#else
+struct cpts;
+
static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
}
@@ -163,8 +168,19 @@ static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
}
+static inline
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+ u32 mult, u32 shift)
+{
+ return NULL;
+}
+
+static inline void cpts_release(struct cpts *cpts)
+{
+}
+
static inline int
-cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+cpts_register(struct cpts *cpts)
{
return 0;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) is pretty entangled and
has some issues, like:
- ptp clock registered before spinlock, which is protecting it, and
before timecounter and cyclecounter initialization;
- CPTS ref_clk requested using devm API while cpts_register() is
called from .ndo_open(), as result additional checks required;
- CPTS ref_clk is prepared, but never unprepared;
- CPTS is not disabled even when unregistered..
Hence, make things simpler and fix above issues by adding
cpts_create()/cpts_release() which should be called from
.probe()/.remove() respectively and move all static initialization
there. Clean up and update cpts_register/unregister() so PTP clock is
registered the last and unregistered first. In addition, this change
allows to clean up cpts.h for the case when CPTS is disabled.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 24 ++++----
drivers/net/ethernet/ti/cpts.c | 125 ++++++++++++++++++++++++++---------------
drivers/net/ethernet/ti/cpts.h | 26 +++++++--
3 files changed, 113 insertions(+), 62 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 9b900f0..dfd5707 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1406,9 +1406,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
if (ret < 0)
goto err_cleanup;
- if (cpts_register(cpsw->dev, cpsw->cpts,
- cpsw->data.cpts_clock_mult,
- cpsw->data.cpts_clock_shift))
+ if (cpts_register(cpsw->cpts))
dev_err(priv->dev, "error registering cpts device\n");
}
@@ -2551,6 +2549,7 @@ static int cpsw_probe(struct platform_device *pdev)
struct cpdma_params dma_params;
struct cpsw_ale_params ale_params;
void __iomem *ss_regs;
+ void __iomem *cpts_regs;
struct resource *res, *ss_res;
const struct of_device_id *of_id;
struct gpio_descs *mode;
@@ -2575,12 +2574,6 @@ static int cpsw_probe(struct platform_device *pdev)
priv->dev = &ndev->dev;
priv->msg_enable = netif_msg_init(debug_level, CPSW_DEBUG);
cpsw->rx_packet_max = max(rx_packet_max, 128);
- cpsw->cpts = devm_kzalloc(&pdev->dev, sizeof(struct cpts), GFP_KERNEL);
- if (!cpsw->cpts) {
- dev_err(&pdev->dev, "error allocating cpts\n");
- ret = -ENOMEM;
- goto clean_ndev_ret;
- }
mode = devm_gpiod_get_array_optional(&pdev->dev, "mode", GPIOD_OUT_LOW);
if (IS_ERR(mode)) {
@@ -2669,7 +2662,7 @@ static int cpsw_probe(struct platform_device *pdev)
switch (cpsw->version) {
case CPSW_VERSION_1:
cpsw->host_port_regs = ss_regs + CPSW1_HOST_PORT_OFFSET;
- cpsw->cpts->reg = ss_regs + CPSW1_CPTS_OFFSET;
+ cpts_regs = ss_regs + CPSW1_CPTS_OFFSET;
cpsw->hw_stats = ss_regs + CPSW1_HW_STATS;
dma_params.dmaregs = ss_regs + CPSW1_CPDMA_OFFSET;
dma_params.txhdp = ss_regs + CPSW1_STATERAM_OFFSET;
@@ -2683,7 +2676,7 @@ static int cpsw_probe(struct platform_device *pdev)
case CPSW_VERSION_3:
case CPSW_VERSION_4:
cpsw->host_port_regs = ss_regs + CPSW2_HOST_PORT_OFFSET;
- cpsw->cpts->reg = ss_regs + CPSW2_CPTS_OFFSET;
+ cpts_regs = ss_regs + CPSW2_CPTS_OFFSET;
cpsw->hw_stats = ss_regs + CPSW2_HW_STATS;
dma_params.dmaregs = ss_regs + CPSW2_CPDMA_OFFSET;
dma_params.txhdp = ss_regs + CPSW2_STATERAM_OFFSET;
@@ -2749,6 +2742,14 @@ static int cpsw_probe(struct platform_device *pdev)
goto clean_dma_ret;
}
+ cpsw->cpts = cpts_create(cpsw->dev, cpts_regs,
+ cpsw->data.cpts_clock_mult,
+ cpsw->data.cpts_clock_shift);
+ if (IS_ERR(cpsw->cpts)) {
+ ret = PTR_ERR(cpsw->cpts);
+ goto clean_ale_ret;
+ }
+
ndev->irq = platform_get_irq(pdev, 1);
if (ndev->irq < 0) {
dev_err(priv->dev, "error getting irq resource\n");
@@ -2857,6 +2858,7 @@ static int cpsw_remove(struct platform_device *pdev)
unregister_netdev(cpsw->slaves[1].ndev);
unregister_netdev(ndev);
+ cpts_release(cpsw->cpts);
cpsw_ale_destroy(cpsw->ale);
cpdma_ctlr_destroy(cpsw->dma);
of_platform_depopulate(&pdev->dev);
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index aaab08e..a46478e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -228,22 +228,6 @@ static void cpts_overflow_check(struct work_struct *work)
schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
}
-static void cpts_clk_init(struct device *dev, struct cpts *cpts)
-{
- cpts->refclk = devm_clk_get(dev, "cpts");
- if (IS_ERR(cpts->refclk)) {
- dev_err(dev, "Failed to get cpts refclk\n");
- cpts->refclk = NULL;
- return;
- }
- clk_prepare_enable(cpts->refclk);
-}
-
-static void cpts_clk_release(struct cpts *cpts)
-{
- clk_disable(cpts->refclk);
-}
-
static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
u16 ts_seqid, u8 ts_msgtype)
{
@@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
u64 ns;
struct skb_shared_hwtstamps *ssh;
- if (!cpts->rx_enable)
+ if (!cpts || !cpts->rx_enable)
return;
ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
if (!ns)
@@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
u64 ns;
struct skb_shared_hwtstamps ssh;
- if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
+ if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
return;
ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
if (!ns)
@@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
skb_tstamp_tx(skb, &ssh);
}
-int cpts_register(struct device *dev, struct cpts *cpts,
- u32 mult, u32 shift)
+int cpts_register(struct cpts *cpts)
{
int err, i;
- unsigned long flags;
- cpts->info = cpts_info;
- cpts->clock = ptp_clock_register(&cpts->info, dev);
- if (IS_ERR(cpts->clock)) {
- err = PTR_ERR(cpts->clock);
- cpts->clock = NULL;
- return err;
- }
- spin_lock_init(&cpts->lock);
-
- cpts->cc.read = cpts_systim_read;
- cpts->cc.mask = CLOCKSOURCE_MASK(32);
- cpts->cc_mult = mult;
- cpts->cc.mult = mult;
- cpts->cc.shift = shift;
+ if (!cpts)
+ return -EINVAL;
INIT_LIST_HEAD(&cpts->events);
INIT_LIST_HEAD(&cpts->pool);
for (i = 0; i < CPTS_MAX_EVENTS; i++)
list_add(&cpts->pool_data[i].list, &cpts->pool);
- cpts_clk_init(dev, cpts);
+ clk_enable(cpts->refclk);
+
cpts_write32(cpts, CPTS_EN, control);
cpts_write32(cpts, TS_PEND_EN, int_enable);
- spin_lock_irqsave(&cpts->lock, flags);
+ cpts->cc.mult = cpts->cc_mult;
timecounter_init(&cpts->tc, &cpts->cc, ktime_to_ns(ktime_get_real()));
- spin_unlock_irqrestore(&cpts->lock, flags);
-
- INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
- schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+ cpts->info = cpts_info;
+ cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
+ if (IS_ERR(cpts->clock)) {
+ err = PTR_ERR(cpts->clock);
+ cpts->clock = NULL;
+ goto err_ptp;
+ }
cpts->phc_index = ptp_clock_index(cpts->clock);
+
+ schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
return 0;
+
+err_ptp:
+ clk_disable(cpts->refclk);
+ return err;
}
void cpts_unregister(struct cpts *cpts)
{
- if (cpts->clock) {
- ptp_clock_unregister(cpts->clock);
- cancel_delayed_work_sync(&cpts->overflow_work);
+ if (!cpts)
+ return;
+
+ if (WARN_ON(!cpts->clock))
+ return;
+
+ cancel_delayed_work_sync(&cpts->overflow_work);
+
+ ptp_clock_unregister(cpts->clock);
+ cpts->clock = NULL;
+
+ cpts_write32(cpts, 0, int_enable);
+ cpts_write32(cpts, 0, control);
+
+ clk_disable(cpts->refclk);
+}
+
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+ u32 mult, u32 shift)
+{
+ struct cpts *cpts;
+
+ if (!regs || !dev)
+ return ERR_PTR(-EINVAL);
+
+ cpts = devm_kzalloc(dev, sizeof(*cpts), GFP_KERNEL);
+ if (!cpts)
+ return ERR_PTR(-ENOMEM);
+
+ cpts->dev = dev;
+ cpts->reg = (struct cpsw_cpts __iomem *)regs;
+ spin_lock_init(&cpts->lock);
+ INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
+
+ cpts->refclk = devm_clk_get(dev, "cpts");
+ if (IS_ERR(cpts->refclk)) {
+ dev_err(dev, "Failed to get cpts refclk\n");
+ return ERR_PTR(PTR_ERR(cpts->refclk));
}
- if (cpts->refclk)
- cpts_clk_release(cpts);
+
+ clk_prepare(cpts->refclk);
+
+ cpts->cc.read = cpts_systim_read;
+ cpts->cc.mask = CLOCKSOURCE_MASK(32);
+ cpts->cc.shift = shift;
+ cpts->cc_mult = mult;
+
+ return cpts;
+}
+
+void cpts_release(struct cpts *cpts)
+{
+ if (!cpts)
+ return;
+
+ if (!cpts->refclk)
+ return;
+
+ clk_unprepare(cpts->refclk);
}
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index fec753c..0c02f48 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -20,6 +20,8 @@
#ifndef _TI_CPTS_H_
#define _TI_CPTS_H_
+#ifdef CONFIG_TI_CPTS
+
#include <linux/clk.h>
#include <linux/clkdev.h>
#include <linux/clocksource.h>
@@ -108,10 +110,10 @@ struct cpts_event {
};
struct cpts {
+ struct device *dev;
struct cpsw_cpts __iomem *reg;
int tx_enable;
int rx_enable;
-#ifdef CONFIG_TI_CPTS
struct ptp_clock_info info;
struct ptp_clock *clock;
spinlock_t lock; /* protects time registers */
@@ -124,14 +126,15 @@ struct cpts {
struct list_head events;
struct list_head pool;
struct cpts_event pool_data[CPTS_MAX_EVENTS];
-#endif
};
-#ifdef CONFIG_TI_CPTS
void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+int cpts_register(struct cpts *cpts);
void cpts_unregister(struct cpts *cpts);
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+ u32 mult, u32 shift);
+void cpts_release(struct cpts *cpts);
static inline void cpts_rx_enable(struct cpts *cpts, int enable)
{
@@ -156,6 +159,8 @@ static inline bool cpts_is_tx_enabled(struct cpts *cpts)
}
#else
+struct cpts;
+
static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
}
@@ -163,8 +168,19 @@ static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
}
+static inline
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+ u32 mult, u32 shift)
+{
+ return NULL;
+}
+
+static inline void cpts_release(struct cpts *cpts)
+{
+}
+
static inline int
-cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+cpts_register(struct cpts *cpts)
{
return 0;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
Move DT properties parsing into CPTS driver to simplify consumer's
code and CPTS driver porting on other SoC in the future
(like Keystone 2).
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 16 +---------------
drivers/net/ethernet/ti/cpsw.h | 2 --
drivers/net/ethernet/ti/cpts.c | 29 ++++++++++++++++++++++++++---
drivers/net/ethernet/ti/cpts.h | 5 +++--
4 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index dfd5707..3db8fec 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2311,18 +2311,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
}
data->active_slave = prop;
- if (of_property_read_u32(node, "cpts_clock_mult", &prop)) {
- dev_err(&pdev->dev, "Missing cpts_clock_mult property in the DT.\n");
- return -EINVAL;
- }
- data->cpts_clock_mult = prop;
-
- if (of_property_read_u32(node, "cpts_clock_shift", &prop)) {
- dev_err(&pdev->dev, "Missing cpts_clock_shift property in the DT.\n");
- return -EINVAL;
- }
- data->cpts_clock_shift = prop;
-
data->slave_data = devm_kzalloc(&pdev->dev, data->slaves
* sizeof(struct cpsw_slave_data),
GFP_KERNEL);
@@ -2742,9 +2730,7 @@ static int cpsw_probe(struct platform_device *pdev)
goto clean_dma_ret;
}
- cpsw->cpts = cpts_create(cpsw->dev, cpts_regs,
- cpsw->data.cpts_clock_mult,
- cpsw->data.cpts_clock_shift);
+ cpsw->cpts = cpts_create(cpsw->dev, cpts_regs, cpsw->dev->of_node);
if (IS_ERR(cpsw->cpts)) {
ret = PTR_ERR(cpsw->cpts);
goto clean_ale_ret;
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 16b54c6..6c3037a 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -31,8 +31,6 @@ struct cpsw_platform_data {
u32 channels; /* number of cpdma channels (symmetric) */
u32 slaves; /* number of slave cpgmac ports */
u32 active_slave; /* time stamping, ethtool and SIOCGMIIPHY slave */
- u32 cpts_clock_mult; /* convert input clock ticks to nanoseconds */
- u32 cpts_clock_shift; /* convert input clock ticks to nanoseconds */
u32 ale_entries; /* ale table size */
u32 bd_ram_size; /*buffer descriptor ram size */
u32 mac_control; /* Mac control register */
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index a46478e..1ee64c6 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -388,10 +388,31 @@ void cpts_unregister(struct cpts *cpts)
clk_disable(cpts->refclk);
}
+static int cpts_of_parse(struct cpts *cpts, struct device_node *node)
+{
+ int ret = -EINVAL;
+ u32 prop;
+
+ if (of_property_read_u32(node, "cpts_clock_mult", &prop))
+ goto of_error;
+ cpts->cc_mult = prop;
+
+ if (of_property_read_u32(node, "cpts_clock_shift", &prop))
+ goto of_error;
+ cpts->cc.shift = prop;
+
+ return 0;
+
+of_error:
+ dev_err(cpts->dev, "CPTS: Missing property in the DT.\n");
+ return ret;
+}
+
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
- u32 mult, u32 shift)
+ struct device_node *node)
{
struct cpts *cpts;
+ int ret;
if (!regs || !dev)
return ERR_PTR(-EINVAL);
@@ -405,6 +426,10 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
spin_lock_init(&cpts->lock);
INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
+ ret = cpts_of_parse(cpts, node);
+ if (ret)
+ return ERR_PTR(ret);
+
cpts->refclk = devm_clk_get(dev, "cpts");
if (IS_ERR(cpts->refclk)) {
dev_err(dev, "Failed to get cpts refclk\n");
@@ -415,8 +440,6 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
cpts->cc.read = cpts_systim_read;
cpts->cc.mask = CLOCKSOURCE_MASK(32);
- cpts->cc.shift = shift;
- cpts->cc_mult = mult;
return cpts;
}
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 0c02f48..a865193 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -27,6 +27,7 @@
#include <linux/clocksource.h>
#include <linux/device.h>
#include <linux/list.h>
+#include <linux/of.h>
#include <linux/ptp_clock_kernel.h>
#include <linux/skbuff.h>
#include <linux/timecounter.h>
@@ -133,7 +134,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
int cpts_register(struct cpts *cpts);
void cpts_unregister(struct cpts *cpts);
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
- u32 mult, u32 shift);
+ struct device_node *node);
void cpts_release(struct cpts *cpts);
static inline void cpts_rx_enable(struct cpts *cpts, int enable)
@@ -170,7 +171,7 @@ static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
static inline
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
- u32 mult, u32 shift)
+ struct device_node *node)
{
return NULL;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
Move DT properties parsing into CPTS driver to simplify consumer's
code and CPTS driver porting on other SoC in the future
(like Keystone 2).
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpsw.c | 16 +---------------
drivers/net/ethernet/ti/cpsw.h | 2 --
drivers/net/ethernet/ti/cpts.c | 29 ++++++++++++++++++++++++++---
drivers/net/ethernet/ti/cpts.h | 5 +++--
4 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index dfd5707..3db8fec 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2311,18 +2311,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
}
data->active_slave = prop;
- if (of_property_read_u32(node, "cpts_clock_mult", &prop)) {
- dev_err(&pdev->dev, "Missing cpts_clock_mult property in the DT.\n");
- return -EINVAL;
- }
- data->cpts_clock_mult = prop;
-
- if (of_property_read_u32(node, "cpts_clock_shift", &prop)) {
- dev_err(&pdev->dev, "Missing cpts_clock_shift property in the DT.\n");
- return -EINVAL;
- }
- data->cpts_clock_shift = prop;
-
data->slave_data = devm_kzalloc(&pdev->dev, data->slaves
* sizeof(struct cpsw_slave_data),
GFP_KERNEL);
@@ -2742,9 +2730,7 @@ static int cpsw_probe(struct platform_device *pdev)
goto clean_dma_ret;
}
- cpsw->cpts = cpts_create(cpsw->dev, cpts_regs,
- cpsw->data.cpts_clock_mult,
- cpsw->data.cpts_clock_shift);
+ cpsw->cpts = cpts_create(cpsw->dev, cpts_regs, cpsw->dev->of_node);
if (IS_ERR(cpsw->cpts)) {
ret = PTR_ERR(cpsw->cpts);
goto clean_ale_ret;
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 16b54c6..6c3037a 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -31,8 +31,6 @@ struct cpsw_platform_data {
u32 channels; /* number of cpdma channels (symmetric) */
u32 slaves; /* number of slave cpgmac ports */
u32 active_slave; /* time stamping, ethtool and SIOCGMIIPHY slave */
- u32 cpts_clock_mult; /* convert input clock ticks to nanoseconds */
- u32 cpts_clock_shift; /* convert input clock ticks to nanoseconds */
u32 ale_entries; /* ale table size */
u32 bd_ram_size; /*buffer descriptor ram size */
u32 mac_control; /* Mac control register */
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index a46478e..1ee64c6 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -388,10 +388,31 @@ void cpts_unregister(struct cpts *cpts)
clk_disable(cpts->refclk);
}
+static int cpts_of_parse(struct cpts *cpts, struct device_node *node)
+{
+ int ret = -EINVAL;
+ u32 prop;
+
+ if (of_property_read_u32(node, "cpts_clock_mult", &prop))
+ goto of_error;
+ cpts->cc_mult = prop;
+
+ if (of_property_read_u32(node, "cpts_clock_shift", &prop))
+ goto of_error;
+ cpts->cc.shift = prop;
+
+ return 0;
+
+of_error:
+ dev_err(cpts->dev, "CPTS: Missing property in the DT.\n");
+ return ret;
+}
+
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
- u32 mult, u32 shift)
+ struct device_node *node)
{
struct cpts *cpts;
+ int ret;
if (!regs || !dev)
return ERR_PTR(-EINVAL);
@@ -405,6 +426,10 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
spin_lock_init(&cpts->lock);
INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
+ ret = cpts_of_parse(cpts, node);
+ if (ret)
+ return ERR_PTR(ret);
+
cpts->refclk = devm_clk_get(dev, "cpts");
if (IS_ERR(cpts->refclk)) {
dev_err(dev, "Failed to get cpts refclk\n");
@@ -415,8 +440,6 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
cpts->cc.read = cpts_systim_read;
cpts->cc.mask = CLOCKSOURCE_MASK(32);
- cpts->cc.shift = shift;
- cpts->cc_mult = mult;
return cpts;
}
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 0c02f48..a865193 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -27,6 +27,7 @@
#include <linux/clocksource.h>
#include <linux/device.h>
#include <linux/list.h>
+#include <linux/of.h>
#include <linux/ptp_clock_kernel.h>
#include <linux/skbuff.h>
#include <linux/timecounter.h>
@@ -133,7 +134,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
int cpts_register(struct cpts *cpts);
void cpts_unregister(struct cpts *cpts);
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
- u32 mult, u32 shift);
+ struct device_node *node);
void cpts_release(struct cpts *cpts);
static inline void cpts_rx_enable(struct cpts *cpts, int enable)
@@ -170,7 +171,7 @@ static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
static inline
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
- u32 mult, u32 shift)
+ struct device_node *node)
{
return NULL;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 5/9] net: ethernet: ti: cpts: add return value to tx and rx timestamp funcitons
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
From: WingMan Kwok <w-kwok2@ti.com>
Added return values in tx and rx timestamp funcitons facilitate the
possibililies of timestamping by CPSW modules other than CPTS, such as
packet accelerator on Keystone 2 devices.
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 16 ++++++++++------
drivers/net/ethernet/ti/cpts.h | 11 +++++++----
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 1ee64c6..970d4e2 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -302,34 +302,38 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
return ns;
}
-void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
u64 ns;
struct skb_shared_hwtstamps *ssh;
if (!cpts || !cpts->rx_enable)
- return;
+ return -EPERM;
ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
if (!ns)
- return;
+ return -ENOENT;
ssh = skb_hwtstamps(skb);
memset(ssh, 0, sizeof(*ssh));
ssh->hwtstamp = ns_to_ktime(ns);
+
+ return 0;
}
-void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
u64 ns;
struct skb_shared_hwtstamps ssh;
if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
- return;
+ return -EPERM;
ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
if (!ns)
- return;
+ return -ENOENT;
memset(&ssh, 0, sizeof(ssh));
ssh.hwtstamp = ns_to_ktime(ns);
skb_tstamp_tx(skb, &ssh);
+
+ return 0;
}
int cpts_register(struct cpts *cpts)
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index a865193..47026ec 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -129,8 +129,8 @@ struct cpts {
struct cpts_event pool_data[CPTS_MAX_EVENTS];
};
-void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
int cpts_register(struct cpts *cpts);
void cpts_unregister(struct cpts *cpts);
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
@@ -162,11 +162,14 @@ static inline bool cpts_is_tx_enabled(struct cpts *cpts)
#else
struct cpts;
-static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+static inline int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
+ return -EOPNOTSUPP;
}
-static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+
+static inline int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
+ return -EOPNOTSUPP;
}
static inline
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 5/9] net: ethernet: ti: cpts: add return value to tx and rx timestamp funcitons
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
From: WingMan Kwok <w-kwok2@ti.com>
Added return values in tx and rx timestamp funcitons facilitate the
possibililies of timestamping by CPSW modules other than CPTS, such as
packet accelerator on Keystone 2 devices.
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 16 ++++++++++------
drivers/net/ethernet/ti/cpts.h | 11 +++++++----
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 1ee64c6..970d4e2 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -302,34 +302,38 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
return ns;
}
-void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
u64 ns;
struct skb_shared_hwtstamps *ssh;
if (!cpts || !cpts->rx_enable)
- return;
+ return -EPERM;
ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
if (!ns)
- return;
+ return -ENOENT;
ssh = skb_hwtstamps(skb);
memset(ssh, 0, sizeof(*ssh));
ssh->hwtstamp = ns_to_ktime(ns);
+
+ return 0;
}
-void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
u64 ns;
struct skb_shared_hwtstamps ssh;
if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
- return;
+ return -EPERM;
ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
if (!ns)
- return;
+ return -ENOENT;
memset(&ssh, 0, sizeof(ssh));
ssh.hwtstamp = ns_to_ktime(ns);
skb_tstamp_tx(skb, &ssh);
+
+ return 0;
}
int cpts_register(struct cpts *cpts)
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index a865193..47026ec 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -129,8 +129,8 @@ struct cpts {
struct cpts_event pool_data[CPTS_MAX_EVENTS];
};
-void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
+int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
int cpts_register(struct cpts *cpts);
void cpts_unregister(struct cpts *cpts);
struct cpts *cpts_create(struct device *dev, void __iomem *regs,
@@ -162,11 +162,14 @@ static inline bool cpts_is_tx_enabled(struct cpts *cpts)
#else
struct cpts;
-static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+static inline int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
+ return -EOPNOTSUPP;
}
-static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
+
+static inline int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
{
+ return -EOPNOTSUPP;
}
static inline
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
From: WingMan Kwok <w-kwok2@ti.com>
When a CPTS user does not exit gracefully by disabling cpts
timestamping and leaving a joined multicast group, the system
continues to receive and timestamps the ptp packets which eventually
occupy all the event list entries. When this happns, the added code
tries to remove some list entries which are expired.
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 970d4e2..ff8bb85 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)
return -1;
}
+static int cpts_event_list_clean_up(struct cpts *cpts)
+{
+ struct list_head *this, *next;
+ struct cpts_event *event;
+ int removed = 0;
+
+ list_for_each_safe(this, next, &cpts->events) {
+ event = list_entry(this, struct cpts_event, list);
+ if (event_expired(event)) {
+ list_del_init(&event->list);
+ list_add(&event->list, &cpts->pool);
+ ++removed;
+ }
+ }
+ return removed;
+}
+
/*
* Returns zero if matching event type was found.
*/
static int cpts_fifo_read(struct cpts *cpts, int match)
{
int i, type = -1;
+ int removed;
u32 hi, lo;
struct cpts_event *event;
for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
if (cpts_fifo_pop(cpts, &hi, &lo))
break;
+
if (list_empty(&cpts->pool)) {
- pr_err("cpts: event pool is empty\n");
- return -1;
+ removed = cpts_event_list_clean_up(cpts);
+ if (!removed) {
+ dev_err(cpts->dev,
+ "cpts: event pool is empty\n");
+ return -1;
+ }
+ dev_dbg(cpts->dev,
+ "cpts: event pool cleaned up %d\n", removed);
}
+
event = list_first_entry(&cpts->pool, struct cpts_event, list);
event->tmo = jiffies + 2;
event->high = hi;
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
From: WingMan Kwok <w-kwok2@ti.com>
When a CPTS user does not exit gracefully by disabling cpts
timestamping and leaving a joined multicast group, the system
continues to receive and timestamps the ptp packets which eventually
occupy all the event list entries. When this happns, the added code
tries to remove some list entries which are expired.
Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 970d4e2..ff8bb85 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)
return -1;
}
+static int cpts_event_list_clean_up(struct cpts *cpts)
+{
+ struct list_head *this, *next;
+ struct cpts_event *event;
+ int removed = 0;
+
+ list_for_each_safe(this, next, &cpts->events) {
+ event = list_entry(this, struct cpts_event, list);
+ if (event_expired(event)) {
+ list_del_init(&event->list);
+ list_add(&event->list, &cpts->pool);
+ ++removed;
+ }
+ }
+ return removed;
+}
+
/*
* Returns zero if matching event type was found.
*/
static int cpts_fifo_read(struct cpts *cpts, int match)
{
int i, type = -1;
+ int removed;
u32 hi, lo;
struct cpts_event *event;
for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
if (cpts_fifo_pop(cpts, &hi, &lo))
break;
+
if (list_empty(&cpts->pool)) {
- pr_err("cpts: event pool is empty\n");
- return -1;
+ removed = cpts_event_list_clean_up(cpts);
+ if (!removed) {
+ dev_err(cpts->dev,
+ "cpts: event pool is empty\n");
+ return -1;
+ }
+ dev_dbg(cpts->dev,
+ "cpts: event pool cleaned up %d\n", removed);
}
+
event = list_first_entry(&cpts->pool, struct cpts_event, list);
event->tmo = jiffies + 2;
event->high = hi;
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
The cyclecounter mult and shift values can be calculated based on the
CPTS rftclk frequency and timekeepnig framework provides required algos
and API's.
Hence, calc mult and shift basing on CPTS rftclk frequency if both
cpts_clock_shift and cpts_clock_mult properties are not provided in DT
(the basis of calculation algorithm is borrowed from
__clocksource_update_freq_scale()). After this change cpts_clock_shift
and cpts_clock_mult DT properties will become optional.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Documentation/devicetree/bindings/net/cpsw.txt | 4 +-
drivers/net/ethernet/ti/cpts.c | 56 +++++++++++++++++++++++---
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 5ad439f..88f81c7 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -20,8 +20,6 @@ Required properties:
- slaves : Specifies number for slaves
- active_slave : Specifies the slave to use for time stamping,
ethtool and SIOCGMIIPHY
-- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
-- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
Optional properties:
- ti,hwmods : Must be "cpgmac0"
@@ -35,6 +33,8 @@ Optional properties:
For example in dra72x-evm, pcf gpio has to be
driven low so that cpsw slave 0 and phy data
lines are connected via mux.
+- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
+- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
Slave Properties:
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index ff8bb85..8046a21 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts)
clk_disable(cpts->refclk);
}
+static void cpts_calc_mult_shift(struct cpts *cpts)
+{
+ u64 maxsec;
+ u32 freq;
+ u32 mult;
+ u32 shift;
+ u64 ns;
+ u64 frac;
+
+ if (cpts->cc_mult || cpts->cc.shift)
+ return;
+
+ freq = clk_get_rate(cpts->refclk);
+
+ /* Calc the maximum number of seconds which we can run before
+ * wrapping around.
+ */
+ maxsec = cpts->cc.mask;
+ do_div(maxsec, freq);
+ if (!maxsec)
+ maxsec = 1;
+ else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
+ maxsec = 600;
+
+ clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
+
+ cpts->cc_mult = mult;
+ cpts->cc.mult = mult;
+ cpts->cc.shift = shift;
+ /* Check calculations and inform if not precise */
+ frac = 0;
+ ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
+
+ dev_info(cpts->dev,
+ "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",
+ freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
+}
+
static int cpts_of_parse(struct cpts *cpts, struct device_node *node)
{
int ret = -EINVAL;
u32 prop;
- if (of_property_read_u32(node, "cpts_clock_mult", &prop))
- goto of_error;
- cpts->cc_mult = prop;
+ cpts->cc_mult = 0;
+ if (!of_property_read_u32(node, "cpts_clock_mult", &prop))
+ cpts->cc_mult = prop;
- if (of_property_read_u32(node, "cpts_clock_shift", &prop))
- goto of_error;
- cpts->cc.shift = prop;
+ cpts->cc.shift = 0;
+ if (!of_property_read_u32(node, "cpts_clock_shift", &prop))
+ cpts->cc.shift = prop;
+
+ if ((cpts->cc_mult && !cpts->cc.shift) ||
+ (!cpts->cc_mult && cpts->cc.shift))
+ goto of_error;
return 0;
@@ -471,6 +513,8 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
cpts->cc.read = cpts_systim_read;
cpts->cc.mask = CLOCKSOURCE_MASK(32);
+ cpts_calc_mult_shift(cpts);
+
return cpts;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
The cyclecounter mult and shift values can be calculated based on the
CPTS rftclk frequency and timekeepnig framework provides required algos
and API's.
Hence, calc mult and shift basing on CPTS rftclk frequency if both
cpts_clock_shift and cpts_clock_mult properties are not provided in DT
(the basis of calculation algorithm is borrowed from
__clocksource_update_freq_scale()). After this change cpts_clock_shift
and cpts_clock_mult DT properties will become optional.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Documentation/devicetree/bindings/net/cpsw.txt | 4 +-
drivers/net/ethernet/ti/cpts.c | 56 +++++++++++++++++++++++---
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 5ad439f..88f81c7 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -20,8 +20,6 @@ Required properties:
- slaves : Specifies number for slaves
- active_slave : Specifies the slave to use for time stamping,
ethtool and SIOCGMIIPHY
-- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
-- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
Optional properties:
- ti,hwmods : Must be "cpgmac0"
@@ -35,6 +33,8 @@ Optional properties:
For example in dra72x-evm, pcf gpio has to be
driven low so that cpsw slave 0 and phy data
lines are connected via mux.
+- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
+- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
Slave Properties:
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index ff8bb85..8046a21 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts)
clk_disable(cpts->refclk);
}
+static void cpts_calc_mult_shift(struct cpts *cpts)
+{
+ u64 maxsec;
+ u32 freq;
+ u32 mult;
+ u32 shift;
+ u64 ns;
+ u64 frac;
+
+ if (cpts->cc_mult || cpts->cc.shift)
+ return;
+
+ freq = clk_get_rate(cpts->refclk);
+
+ /* Calc the maximum number of seconds which we can run before
+ * wrapping around.
+ */
+ maxsec = cpts->cc.mask;
+ do_div(maxsec, freq);
+ if (!maxsec)
+ maxsec = 1;
+ else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
+ maxsec = 600;
+
+ clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
+
+ cpts->cc_mult = mult;
+ cpts->cc.mult = mult;
+ cpts->cc.shift = shift;
+ /* Check calculations and inform if not precise */
+ frac = 0;
+ ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
+
+ dev_info(cpts->dev,
+ "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",
+ freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
+}
+
static int cpts_of_parse(struct cpts *cpts, struct device_node *node)
{
int ret = -EINVAL;
u32 prop;
- if (of_property_read_u32(node, "cpts_clock_mult", &prop))
- goto of_error;
- cpts->cc_mult = prop;
+ cpts->cc_mult = 0;
+ if (!of_property_read_u32(node, "cpts_clock_mult", &prop))
+ cpts->cc_mult = prop;
- if (of_property_read_u32(node, "cpts_clock_shift", &prop))
- goto of_error;
- cpts->cc.shift = prop;
+ cpts->cc.shift = 0;
+ if (!of_property_read_u32(node, "cpts_clock_shift", &prop))
+ cpts->cc.shift = prop;
+
+ if ((cpts->cc_mult && !cpts->cc.shift) ||
+ (!cpts->cc_mult && cpts->cc.shift))
+ goto of_error;
return 0;
@@ -471,6 +513,8 @@ struct cpts *cpts_create(struct device *dev, void __iomem *regs,
cpts->cc.read = cpts_systim_read;
cpts->cc.mask = CLOCKSOURCE_MASK(32);
+ cpts_calc_mult_shift(cpts);
+
return cpts;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
The CPTS drivers uses 8sec period for overflow checking with
assumption that CPTS rftclk will not exceed 500MHz. But that's not
true on some TI's platforms (Kesytone 2). As result, it is possible that
CPTS counter will overflow more than once between two readings.
Hence, fix it by selecting overflow check period dynamically as
max_sec_before_overflow/2, where
max_sec_before_overflow = max_counter_val / rftclk_freq.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 16 +++++++++++-----
drivers/net/ethernet/ti/cpts.h | 4 +---
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 8046a21..cbe0974 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -251,7 +251,7 @@ static void cpts_overflow_check(struct work_struct *work)
cpts_write32(cpts, TS_PEND_EN, int_enable);
cpts_ptp_gettime(&cpts->info, &ts);
pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
- schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+ schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
}
static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
@@ -391,7 +391,7 @@ int cpts_register(struct cpts *cpts)
}
cpts->phc_index = ptp_clock_index(cpts->clock);
- schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+ schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
return 0;
err_ptp:
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
u64 ns;
u64 frac;
- if (cpts->cc_mult || cpts->cc.shift)
- return;
-
freq = clk_get_rate(cpts->refclk);
/* Calc the maximum number of seconds which we can run before
@@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
maxsec = 600;
+ /* Calc overflow check period (maxsec / 2) */
+ cpts->ov_check_period = (HZ * maxsec) / 2;
+ dev_info(cpts->dev, "cpts: overflow check period %lu\n",
+ cpts->ov_check_period);
+
+ if (cpts->cc_mult || cpts->cc.shift)
+ return;
+
clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
cpts->cc_mult = mult;
cpts->cc.mult = mult;
cpts->cc.shift = shift;
+
/* Check calculations and inform if not precise */
frac = 0;
ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 47026ec..e0e4a62b 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -97,9 +97,6 @@ enum {
CPTS_EV_TX, /* Ethernet Transmit Event */
};
-/* This covers any input clock up to about 500 MHz. */
-#define CPTS_OVERFLOW_PERIOD (HZ * 8)
-
#define CPTS_FIFO_DEPTH 16
#define CPTS_MAX_EVENTS 32
@@ -127,6 +124,7 @@ struct cpts {
struct list_head events;
struct list_head pool;
struct cpts_event pool_data[CPTS_MAX_EVENTS];
+ unsigned long ov_check_period;
};
int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
The CPTS drivers uses 8sec period for overflow checking with
assumption that CPTS rftclk will not exceed 500MHz. But that's not
true on some TI's platforms (Kesytone 2). As result, it is possible that
CPTS counter will overflow more than once between two readings.
Hence, fix it by selecting overflow check period dynamically as
max_sec_before_overflow/2, where
max_sec_before_overflow = max_counter_val / rftclk_freq.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 16 +++++++++++-----
drivers/net/ethernet/ti/cpts.h | 4 +---
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 8046a21..cbe0974 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -251,7 +251,7 @@ static void cpts_overflow_check(struct work_struct *work)
cpts_write32(cpts, TS_PEND_EN, int_enable);
cpts_ptp_gettime(&cpts->info, &ts);
pr_debug("cpts overflow check at %lld.%09lu\n", ts.tv_sec, ts.tv_nsec);
- schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+ schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
}
static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
@@ -391,7 +391,7 @@ int cpts_register(struct cpts *cpts)
}
cpts->phc_index = ptp_clock_index(cpts->clock);
- schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
+ schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
return 0;
err_ptp:
@@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
u64 ns;
u64 frac;
- if (cpts->cc_mult || cpts->cc.shift)
- return;
-
freq = clk_get_rate(cpts->refclk);
/* Calc the maximum number of seconds which we can run before
@@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
maxsec = 600;
+ /* Calc overflow check period (maxsec / 2) */
+ cpts->ov_check_period = (HZ * maxsec) / 2;
+ dev_info(cpts->dev, "cpts: overflow check period %lu\n",
+ cpts->ov_check_period);
+
+ if (cpts->cc_mult || cpts->cc.shift)
+ return;
+
clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
cpts->cc_mult = mult;
cpts->cc.mult = mult;
cpts->cc.shift = shift;
+
/* Check calculations and inform if not precise */
frac = 0;
ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index 47026ec..e0e4a62b 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -97,9 +97,6 @@ enum {
CPTS_EV_TX, /* Ethernet Transmit Event */
};
-/* This covers any input clock up to about 500 MHz. */
-#define CPTS_OVERFLOW_PERIOD (HZ * 8)
-
#define CPTS_FIFO_DEPTH 16
#define CPTS_MAX_EVENTS 32
@@ -127,6 +124,7 @@ struct cpts {
struct list_head events;
struct list_head pool;
struct cpts_event pool_data[CPTS_MAX_EVENTS];
+ unsigned long ov_check_period;
};
int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 9/9] net: ethernet: ti: cpts: switch to readl/writel_relaxed()
2016-09-14 13:02 ` Grygorii Strashko
@ 2016-09-14 13:02 ` Grygorii Strashko
-1 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
Switch to readl/writel_relaxed() APIs, because The CPTS IP is reused
on Keystone 2 SoCs where LE/BE modes are supported.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index cbe0974..0226582 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -31,8 +31,8 @@
#include "cpts.h"
-#define cpts_read32(c, r) __raw_readl(&c->reg->r)
-#define cpts_write32(c, v, r) __raw_writel(v, &c->reg->r)
+#define cpts_read32(c, r) readl_relaxed(&c->reg->r)
+#define cpts_write32(c, v, r) writel_relaxed(v, &c->reg->r)
static int event_expired(struct cpts_event *event)
{
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* [PATCH 9/9] net: ethernet: ti: cpts: switch to readl/writel_relaxed()
@ 2016-09-14 13:02 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 13:02 UTC (permalink / raw)
To: David S. Miller, netdev, Mugunthan V N, Richard Cochran
Cc: Sekhar Nori, linux-kernel, linux-omap, WingMan Kwok, Grygorii Strashko
Switch to readl/writel_relaxed() APIs, because The CPTS IP is reused
on Keystone 2 SoCs where LE/BE modes are supported.
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/ethernet/ti/cpts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index cbe0974..0226582 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -31,8 +31,8 @@
#include "cpts.h"
-#define cpts_read32(c, r) __raw_readl(&c->reg->r)
-#define cpts_write32(c, v, r) __raw_writel(v, &c->reg->r)
+#define cpts_read32(c, r) readl_relaxed(&c->reg->r)
+#define cpts_write32(c, v, r) writel_relaxed(v, &c->reg->r)
static int event_expired(struct cpts_event *event)
{
--
2.9.3
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 13:02 ` Grygorii Strashko
(?)
@ 2016-09-14 13:52 ` Richard Cochran
2016-09-14 20:10 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 13:52 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> u64 ns;
> struct skb_shared_hwtstamps *ssh;
>
> - if (!cpts->rx_enable)
> + if (!cpts || !cpts->rx_enable)
> return;
This function is in the hot path, and you have added a pointless new
test. Don't do that.
> ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
> if (!ns)
> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> u64 ns;
> struct skb_shared_hwtstamps ssh;
>
> - if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> + if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> return;
Same here.
> ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
> if (!ns)
> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> skb_tstamp_tx(skb, &ssh);
> }
>
> -int cpts_register(struct device *dev, struct cpts *cpts,
> - u32 mult, u32 shift)
> +int cpts_register(struct cpts *cpts)
> {
> int err, i;
> - unsigned long flags;
>
> - cpts->info = cpts_info;
> - cpts->clock = ptp_clock_register(&cpts->info, dev);
> - if (IS_ERR(cpts->clock)) {
> - err = PTR_ERR(cpts->clock);
> - cpts->clock = NULL;
> - return err;
> - }
> - spin_lock_init(&cpts->lock);
> -
> - cpts->cc.read = cpts_systim_read;
> - cpts->cc.mask = CLOCKSOURCE_MASK(32);
> - cpts->cc_mult = mult;
> - cpts->cc.mult = mult;
> - cpts->cc.shift = shift;
> + if (!cpts)
> + return -EINVAL;
Not hot path, but still silly. The caller should never pass NULL.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver
2016-09-14 13:02 ` Grygorii Strashko
(?)
@ 2016-09-14 13:55 ` Richard Cochran
2016-09-14 19:45 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 13:55 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:26PM +0300, Grygorii Strashko wrote:
> Move DT properties parsing into CPTS driver to simplify consumer's
> code and CPTS driver porting on other SoC in the future
> (like Keystone 2).
And just who is the consumer?
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/net/ethernet/ti/cpsw.c | 16 +---------------
> drivers/net/ethernet/ti/cpsw.h | 2 --
> drivers/net/ethernet/ti/cpts.c | 29 ++++++++++++++++++++++++++---
> drivers/net/ethernet/ti/cpts.h | 5 +++--
> 4 files changed, 30 insertions(+), 22 deletions(-)
You have more (+) than (-). I wouldn't call that a simplification.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 5/9] net: ethernet: ti: cpts: add return value to tx and rx timestamp funcitons
2016-09-14 13:02 ` Grygorii Strashko
(?)
@ 2016-09-14 14:00 ` Richard Cochran
-1 siblings, 0 replies; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 14:00 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:27PM +0300, Grygorii Strashko wrote:
> From: WingMan Kwok <w-kwok2@ti.com>
>
> Added return values in tx and rx timestamp funcitons facilitate the
> possibililies of timestamping by CPSW modules other than CPTS, such as
> packet accelerator on Keystone 2 devices.
I'm sorry, this is totally bogus.
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/net/ethernet/ti/cpts.c | 16 ++++++++++------
> drivers/net/ethernet/ti/cpts.h | 11 +++++++----
> 2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index 1ee64c6..970d4e2 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -302,34 +302,38 @@ static u64 cpts_find_ts(struct cpts *cpts, struct sk_buff *skb, int ev_type)
> return ns;
> }
>
> -void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> +int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> {
> u64 ns;
> struct skb_shared_hwtstamps *ssh;
>
> if (!cpts || !cpts->rx_enable)
> - return;
> + return -EPERM;
EPERM? Come on.
> ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
> if (!ns)
> - return;
> + return -ENOENT;
> ssh = skb_hwtstamps(skb);
> memset(ssh, 0, sizeof(*ssh));
> ssh->hwtstamp = ns_to_ktime(ns);
> +
> + return 0;
> }
>
> -void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> +int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> {
> u64 ns;
> struct skb_shared_hwtstamps ssh;
>
> if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> - return;
> + return -EPERM;
> ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
> if (!ns)
> - return;
> + return -ENOENT;
> memset(&ssh, 0, sizeof(ssh));
> ssh.hwtstamp = ns_to_ktime(ns);
> skb_tstamp_tx(skb, &ssh);
> +
> + return 0;
> }
>
> int cpts_register(struct cpts *cpts)
> diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
> index a865193..47026ec 100644
> --- a/drivers/net/ethernet/ti/cpts.h
> +++ b/drivers/net/ethernet/ti/cpts.h
> @@ -129,8 +129,8 @@ struct cpts {
> struct cpts_event pool_data[CPTS_MAX_EVENTS];
> };
>
> -void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> -void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> +int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> +int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> int cpts_register(struct cpts *cpts);
> void cpts_unregister(struct cpts *cpts);
> struct cpts *cpts_create(struct device *dev, void __iomem *regs,
> @@ -162,11 +162,14 @@ static inline bool cpts_is_tx_enabled(struct cpts *cpts)
> #else
> struct cpts;
>
> -static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> +static inline int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> {
> + return -EOPNOTSUPP;
You are planning to check in the hot path if a compile time feature is
enabled?
Brilliant stuff.
> }
> -static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> +
> +static inline int cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
> {
> + return -EOPNOTSUPP;
> }
>
> static inline
> --
> 2.9.3
>
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty
2016-09-14 13:02 ` Grygorii Strashko
(?)
@ 2016-09-14 14:14 ` Richard Cochran
2016-09-14 19:54 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 14:14 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
> From: WingMan Kwok <w-kwok2@ti.com>
>
> When a CPTS user does not exit gracefully by disabling cpts
> timestamping and leaving a joined multicast group, the system
> continues to receive and timestamps the ptp packets which eventually
> occupy all the event list entries. When this happns, the added code
> tries to remove some list entries which are expired.
>
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index 970d4e2..ff8bb85 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)
> return -1;
> }
>
> +static int cpts_event_list_clean_up(struct cpts *cpts)
5 words, that is quite a mouth full. How about this instead?
static int cpts_purge_events(struct cpts *cpts);
> +{
> + struct list_head *this, *next;
> + struct cpts_event *event;
> + int removed = 0;
> +
> + list_for_each_safe(this, next, &cpts->events) {
> + event = list_entry(this, struct cpts_event, list);
> + if (event_expired(event)) {
> + list_del_init(&event->list);
> + list_add(&event->list, &cpts->pool);
> + ++removed;
> + }
> + }
> + return removed;
> +}
> +
> /*
> * Returns zero if matching event type was found.
> */
> static int cpts_fifo_read(struct cpts *cpts, int match)
> {
> int i, type = -1;
> + int removed;
No need for another variable, just change the return code above to
return removed ? 0 : -1;
and then you have ...
> u32 hi, lo;
> struct cpts_event *event;
>
> for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
> if (cpts_fifo_pop(cpts, &hi, &lo))
> break;
> +
> if (list_empty(&cpts->pool)) {
> - pr_err("cpts: event pool is empty\n");
> - return -1;
> + removed = cpts_event_list_clean_up(cpts);
> + if (!removed) {
> + dev_err(cpts->dev,
> + "cpts: event pool is empty\n");
> + return -1;
> + }
if (cpts_purge_events(cpts)) {
dev_err(cpts->dev, "cpts: event pool empty\n");
return -1;
}
Notice how I avoided the ugly line break?
> + dev_dbg(cpts->dev,
> + "cpts: event pool cleaned up %d\n", removed);
> }
> +
> event = list_first_entry(&cpts->pool, struct cpts_event, list);
> event->tmo = jiffies + 2;
> event->high = hi;
> --
> 2.9.3
>
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 13:02 ` Grygorii Strashko
(?)
@ 2016-09-14 14:22 ` Richard Cochran
2016-09-14 19:59 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 14:22 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
> @@ -35,6 +33,8 @@ Optional properties:
> For example in dra72x-evm, pcf gpio has to be
> driven low so that cpsw slave 0 and phy data
> lines are connected via mux.
> +- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
> +- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
You should explain to the reader how these will be calculated when the
properties are missing.
> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index ff8bb85..8046a21 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts)
> clk_disable(cpts->refclk);
> }
>
> +static void cpts_calc_mult_shift(struct cpts *cpts)
> +{
> + u64 maxsec;
> + u32 freq;
> + u32 mult;
> + u32 shift;
> + u64 ns;
> + u64 frac;
Why so many new lines? This isn't good style. Please combine
variables of the same type into one line and sort the lists
alphabetically.
> + if (cpts->cc_mult || cpts->cc.shift)
> + return;
> +
> + freq = clk_get_rate(cpts->refclk);
> +
> + /* Calc the maximum number of seconds which we can run before
> + * wrapping around.
> + */
> + maxsec = cpts->cc.mask;
> + do_div(maxsec, freq);
> + if (!maxsec)
> + maxsec = 1;
> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
> + maxsec = 600;
> +
> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
> +
> + cpts->cc_mult = mult;
> + cpts->cc.mult = mult;
> + cpts->cc.shift = shift;
> + /* Check calculations and inform if not precise */
Contrary to this comment, you are not making any kind of judgment
about whether the calculations are precise or not.
> + frac = 0;
> + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
> +
> + dev_info(cpts->dev,
> + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",
> + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
> +}
> +
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 13:02 ` Grygorii Strashko
(?)
@ 2016-09-14 14:25 ` Richard Cochran
2016-09-14 20:03 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 14:25 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
> u64 ns;
> u64 frac;
>
> - if (cpts->cc_mult || cpts->cc.shift)
> - return;
> -
> freq = clk_get_rate(cpts->refclk);
>
> /* Calc the maximum number of seconds which we can run before
This hunk has nothing to do with $subject.
> @@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
> else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
> maxsec = 600;
>
> + /* Calc overflow check period (maxsec / 2) */
> + cpts->ov_check_period = (HZ * maxsec) / 2;
> + dev_info(cpts->dev, "cpts: overflow check period %lu\n",
> + cpts->ov_check_period);
> +
> + if (cpts->cc_mult || cpts->cc.shift)
> + return;
> +
> clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>
> cpts->cc_mult = mult;
> cpts->cc.mult = mult;
> cpts->cc.shift = shift;
> +
Nor does this.
Thanks,
Richard
> /* Check calculations and inform if not precise */
> frac = 0;
> ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
> diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
> index 47026ec..e0e4a62b 100644
> --- a/drivers/net/ethernet/ti/cpts.h
> +++ b/drivers/net/ethernet/ti/cpts.h
> @@ -97,9 +97,6 @@ enum {
> CPTS_EV_TX, /* Ethernet Transmit Event */
> };
>
> -/* This covers any input clock up to about 500 MHz. */
> -#define CPTS_OVERFLOW_PERIOD (HZ * 8)
> -
> #define CPTS_FIFO_DEPTH 16
> #define CPTS_MAX_EVENTS 32
>
> @@ -127,6 +124,7 @@ struct cpts {
> struct list_head events;
> struct list_head pool;
> struct cpts_event pool_data[CPTS_MAX_EVENTS];
> + unsigned long ov_check_period;
> };
>
> int cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver
2016-09-14 13:55 ` Richard Cochran
@ 2016-09-14 19:45 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 19:45 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 04:55 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:26PM +0300, Grygorii Strashko wrote:
>> Move DT properties parsing into CPTS driver to simplify consumer's
>> code and CPTS driver porting on other SoC in the future
>> (like Keystone 2).
>
> And just who is the consumer?
Now CPSW, and Keystone 2 netcp in the future.
>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/net/ethernet/ti/cpsw.c | 16 +---------------
>> drivers/net/ethernet/ti/cpsw.h | 2 --
>> drivers/net/ethernet/ti/cpts.c | 29 ++++++++++++++++++++++++++---
>> drivers/net/ethernet/ti/cpts.h | 5 +++--
>> 4 files changed, 30 insertions(+), 22 deletions(-)
>
> You have more (+) than (-). I wouldn't call that a simplification.
With this change It will not be required to add the same DT parsing code
in Keystone 2 netcp driver, so overall number of lines will be reduced.
As I mentioned in cover letter - this is preparation step and this series
indented help and simplify CPTS integration with few TI net drivers
(now cpsw, soon KS2 netcp), because similar CPTS IP is integrated
in two different TI's SoC families (and might be in more the future).
I've not posted KS2 patches here, because of two reasons:
- they are still under internal review
- I hate big patch series
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver
@ 2016-09-14 19:45 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 19:45 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 04:55 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:26PM +0300, Grygorii Strashko wrote:
>> Move DT properties parsing into CPTS driver to simplify consumer's
>> code and CPTS driver porting on other SoC in the future
>> (like Keystone 2).
>
> And just who is the consumer?
Now CPSW, and Keystone 2 netcp in the future.
>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/net/ethernet/ti/cpsw.c | 16 +---------------
>> drivers/net/ethernet/ti/cpsw.h | 2 --
>> drivers/net/ethernet/ti/cpts.c | 29 ++++++++++++++++++++++++++---
>> drivers/net/ethernet/ti/cpts.h | 5 +++--
>> 4 files changed, 30 insertions(+), 22 deletions(-)
>
> You have more (+) than (-). I wouldn't call that a simplification.
With this change It will not be required to add the same DT parsing code
in Keystone 2 netcp driver, so overall number of lines will be reduced.
As I mentioned in cover letter - this is preparation step and this series
indented help and simplify CPTS integration with few TI net drivers
(now cpsw, soon KS2 netcp), because similar CPTS IP is integrated
in two different TI's SoC families (and might be in more the future).
I've not posted KS2 patches here, because of two reasons:
- they are still under internal review
- I hate big patch series
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty
2016-09-14 14:14 ` Richard Cochran
@ 2016-09-14 19:54 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 19:54 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 05:14 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
>> From: WingMan Kwok <w-kwok2@ti.com>
>>
>> When a CPTS user does not exit gracefully by disabling cpts
>> timestamping and leaving a joined multicast group, the system
>> continues to receive and timestamps the ptp packets which eventually
>> occupy all the event list entries. When this happns, the added code
>> tries to remove some list entries which are expired.
>>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
>> index 970d4e2..ff8bb85 100644
>> --- a/drivers/net/ethernet/ti/cpts.c
>> +++ b/drivers/net/ethernet/ti/cpts.c
>> @@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)
>> return -1;
>> }
>>
>> +static int cpts_event_list_clean_up(struct cpts *cpts)
>
> 5 words, that is quite a mouth full. How about this instead?
>
> static int cpts_purge_events(struct cpts *cpts);
>
>> +{
>> + struct list_head *this, *next;
>> + struct cpts_event *event;
>> + int removed = 0;
>> +
>> + list_for_each_safe(this, next, &cpts->events) {
>> + event = list_entry(this, struct cpts_event, list);
>> + if (event_expired(event)) {
>> + list_del_init(&event->list);
>> + list_add(&event->list, &cpts->pool);
>> + ++removed;
>> + }
>> + }
>> + return removed;
>> +}
>> +
>> /*
>> * Returns zero if matching event type was found.
>> */
>> static int cpts_fifo_read(struct cpts *cpts, int match)
>> {
>> int i, type = -1;
>> + int removed;
>
> No need for another variable, just change the return code above to
>
> return removed ? 0 : -1;
>
> and then you have ...
>
>> u32 hi, lo;
>> struct cpts_event *event;
>>
>> for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
>> if (cpts_fifo_pop(cpts, &hi, &lo))
>> break;
>> +
>> if (list_empty(&cpts->pool)) {
>> - pr_err("cpts: event pool is empty\n");
>> - return -1;
>> + removed = cpts_event_list_clean_up(cpts);
>> + if (!removed) {
>> + dev_err(cpts->dev,
>> + "cpts: event pool is empty\n");
>> + return -1;
>> + }
>
> if (cpts_purge_events(cpts)) {
> dev_err(cpts->dev, "cpts: event pool empty\n");
> return -1;
> }
>
> Notice how I avoided the ugly line break?
>
>> + dev_dbg(cpts->dev,
>> + "cpts: event pool cleaned up %d\n", removed);
>> }
>> +
>> event = list_first_entry(&cpts->pool, struct cpts_event, list);
>> event->tmo = jiffies + 2;
>> event->high = hi;
>> --
>> 2.9.3
>>
>
NP here - will update. Thanks for you review.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty
@ 2016-09-14 19:54 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 19:54 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 05:14 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
>> From: WingMan Kwok <w-kwok2@ti.com>
>>
>> When a CPTS user does not exit gracefully by disabling cpts
>> timestamping and leaving a joined multicast group, the system
>> continues to receive and timestamps the ptp packets which eventually
>> occupy all the event list entries. When this happns, the added code
>> tries to remove some list entries which are expired.
>>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
>> index 970d4e2..ff8bb85 100644
>> --- a/drivers/net/ethernet/ti/cpts.c
>> +++ b/drivers/net/ethernet/ti/cpts.c
>> @@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, u32 *low)
>> return -1;
>> }
>>
>> +static int cpts_event_list_clean_up(struct cpts *cpts)
>
> 5 words, that is quite a mouth full. How about this instead?
>
> static int cpts_purge_events(struct cpts *cpts);
>
>> +{
>> + struct list_head *this, *next;
>> + struct cpts_event *event;
>> + int removed = 0;
>> +
>> + list_for_each_safe(this, next, &cpts->events) {
>> + event = list_entry(this, struct cpts_event, list);
>> + if (event_expired(event)) {
>> + list_del_init(&event->list);
>> + list_add(&event->list, &cpts->pool);
>> + ++removed;
>> + }
>> + }
>> + return removed;
>> +}
>> +
>> /*
>> * Returns zero if matching event type was found.
>> */
>> static int cpts_fifo_read(struct cpts *cpts, int match)
>> {
>> int i, type = -1;
>> + int removed;
>
> No need for another variable, just change the return code above to
>
> return removed ? 0 : -1;
>
> and then you have ...
>
>> u32 hi, lo;
>> struct cpts_event *event;
>>
>> for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
>> if (cpts_fifo_pop(cpts, &hi, &lo))
>> break;
>> +
>> if (list_empty(&cpts->pool)) {
>> - pr_err("cpts: event pool is empty\n");
>> - return -1;
>> + removed = cpts_event_list_clean_up(cpts);
>> + if (!removed) {
>> + dev_err(cpts->dev,
>> + "cpts: event pool is empty\n");
>> + return -1;
>> + }
>
> if (cpts_purge_events(cpts)) {
> dev_err(cpts->dev, "cpts: event pool empty\n");
> return -1;
> }
>
> Notice how I avoided the ugly line break?
>
>> + dev_dbg(cpts->dev,
>> + "cpts: event pool cleaned up %d\n", removed);
>> }
>> +
>> event = list_first_entry(&cpts->pool, struct cpts_event, list);
>> event->tmo = jiffies + 2;
>> event->high = hi;
>> --
>> 2.9.3
>>
>
NP here - will update. Thanks for you review.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 14:22 ` Richard Cochran
@ 2016-09-14 19:59 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 19:59 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 05:22 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
>> @@ -35,6 +33,8 @@ Optional properties:
>> For example in dra72x-evm, pcf gpio has to be
>> driven low so that cpsw slave 0 and phy data
>> lines are connected via mux.
>> +- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
>> +- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
>
> You should explain to the reader how these will be calculated when the
> properties are missing.
Not sure how full should it be explained in bindings - I'll try.
>
>> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
>> index ff8bb85..8046a21 100644
>> --- a/drivers/net/ethernet/ti/cpts.c
>> +++ b/drivers/net/ethernet/ti/cpts.c
>> @@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts)
>> clk_disable(cpts->refclk);
>> }
>>
>> +static void cpts_calc_mult_shift(struct cpts *cpts)
>> +{
>> + u64 maxsec;
>> + u32 freq;
>> + u32 mult;
>> + u32 shift;
>> + u64 ns;
>> + u64 frac;
>
> Why so many new lines? This isn't good style. Please combine
> variables of the same type into one line and sort the lists
> alphabetically.
Its matter of preferences :), but sure - I'll update.
>
>> + if (cpts->cc_mult || cpts->cc.shift)
>> + return;
>> +
>> + freq = clk_get_rate(cpts->refclk);
>> +
>> + /* Calc the maximum number of seconds which we can run before
>> + * wrapping around.
>> + */
>> + maxsec = cpts->cc.mask;
>> + do_div(maxsec, freq);
>> + if (!maxsec)
>> + maxsec = 1;
>> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> + maxsec = 600;
>> +
>> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>> +
>> + cpts->cc_mult = mult;
>> + cpts->cc.mult = mult;
>> + cpts->cc.shift = shift;
>> + /* Check calculations and inform if not precise */
>
> Contrary to this comment, you are not making any kind of judgment
> about whether the calculations are precise or not.
>
>> + frac = 0;
>> + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
>> +
>> + dev_info(cpts->dev,
>> + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",
>> + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
>> +}
>> +
>
Thanks for the review.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
@ 2016-09-14 19:59 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 19:59 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 05:22 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
>> @@ -35,6 +33,8 @@ Optional properties:
>> For example in dra72x-evm, pcf gpio has to be
>> driven low so that cpsw slave 0 and phy data
>> lines are connected via mux.
>> +- cpts_clock_mult : Numerator to convert input clock ticks into nanoseconds
>> +- cpts_clock_shift : Denominator to convert input clock ticks into nanoseconds
>
> You should explain to the reader how these will be calculated when the
> properties are missing.
Not sure how full should it be explained in bindings - I'll try.
>
>> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
>> index ff8bb85..8046a21 100644
>> --- a/drivers/net/ethernet/ti/cpts.c
>> +++ b/drivers/net/ethernet/ti/cpts.c
>> @@ -418,18 +418,60 @@ void cpts_unregister(struct cpts *cpts)
>> clk_disable(cpts->refclk);
>> }
>>
>> +static void cpts_calc_mult_shift(struct cpts *cpts)
>> +{
>> + u64 maxsec;
>> + u32 freq;
>> + u32 mult;
>> + u32 shift;
>> + u64 ns;
>> + u64 frac;
>
> Why so many new lines? This isn't good style. Please combine
> variables of the same type into one line and sort the lists
> alphabetically.
Its matter of preferences :), but sure - I'll update.
>
>> + if (cpts->cc_mult || cpts->cc.shift)
>> + return;
>> +
>> + freq = clk_get_rate(cpts->refclk);
>> +
>> + /* Calc the maximum number of seconds which we can run before
>> + * wrapping around.
>> + */
>> + maxsec = cpts->cc.mask;
>> + do_div(maxsec, freq);
>> + if (!maxsec)
>> + maxsec = 1;
>> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> + maxsec = 600;
>> +
>> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>> +
>> + cpts->cc_mult = mult;
>> + cpts->cc.mult = mult;
>> + cpts->cc.shift = shift;
>> + /* Check calculations and inform if not precise */
>
> Contrary to this comment, you are not making any kind of judgment
> about whether the calculations are precise or not.
>
>> + frac = 0;
>> + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
>> +
>> + dev_info(cpts->dev,
>> + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",
>> + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
>> +}
>> +
>
Thanks for the review.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 14:25 ` Richard Cochran
@ 2016-09-14 20:03 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:03 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 05:25 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
>> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>> u64 ns;
>> u64 frac;
>>
>> - if (cpts->cc_mult || cpts->cc.shift)
>> - return;
>> -
>> freq = clk_get_rate(cpts->refclk);
>>
>> /* Calc the maximum number of seconds which we can run before
>
> This hunk has nothing to do with $subject.
Sry, but I did not get your comment here :(
I'd happy to update patch according to your request, but could you provide more info here, pls?
>
>> @@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>> else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> maxsec = 600;
>>
>> + /* Calc overflow check period (maxsec / 2) */
>> + cpts->ov_check_period = (HZ * maxsec) / 2;
>> + dev_info(cpts->dev, "cpts: overflow check period %lu\n",
>> + cpts->ov_check_period);
>> +
>> + if (cpts->cc_mult || cpts->cc.shift)
>> + return;
>> +
>> clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>>
>> cpts->cc_mult = mult;
>> cpts->cc.mult = mult;
>> cpts->cc.shift = shift;
>> +
>
> Nor does this.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
@ 2016-09-14 20:03 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:03 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 05:25 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
>> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>> u64 ns;
>> u64 frac;
>>
>> - if (cpts->cc_mult || cpts->cc.shift)
>> - return;
>> -
>> freq = clk_get_rate(cpts->refclk);
>>
>> /* Calc the maximum number of seconds which we can run before
>
> This hunk has nothing to do with $subject.
Sry, but I did not get your comment here :(
I'd happy to update patch according to your request, but could you provide more info here, pls?
>
>> @@ -442,11 +439,20 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>> else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> maxsec = 600;
>>
>> + /* Calc overflow check period (maxsec / 2) */
>> + cpts->ov_check_period = (HZ * maxsec) / 2;
>> + dev_info(cpts->dev, "cpts: overflow check period %lu\n",
>> + cpts->ov_check_period);
>> +
>> + if (cpts->cc_mult || cpts->cc.shift)
>> + return;
>> +
>> clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>>
>> cpts->cc_mult = mult;
>> cpts->cc.mult = mult;
>> cpts->cc.shift = shift;
>> +
>
> Nor does this.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver
2016-09-14 19:45 ` Grygorii Strashko
(?)
@ 2016-09-14 20:03 ` Richard Cochran
-1 siblings, 0 replies; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 20:03 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 10:45:54PM +0300, Grygorii Strashko wrote:
> With this change It will not be required to add the same DT parsing code
> in Keystone 2 netcp driver, so overall number of lines will be reduced.
This explanation would make the commit message much more informative.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 20:03 ` Grygorii Strashko
(?)
@ 2016-09-14 20:08 ` Richard Cochran
2016-09-14 20:23 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 20:08 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
> On 09/14/2016 05:25 PM, Richard Cochran wrote:
> > On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
> >> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
> >> u64 ns;
> >> u64 frac;
> >>
> >> - if (cpts->cc_mult || cpts->cc.shift)
> >> - return;
> >> -
> >> freq = clk_get_rate(cpts->refclk);
> >>
> >> /* Calc the maximum number of seconds which we can run before
> >
> > This hunk has nothing to do with $subject.
>
> Sry, but I did not get your comment here :(
> I'd happy to update patch according to your request, but could you provide more info here, pls?
You added that code in patch #7. Then you moved it in patch #8. You
could have made the code correct in patch #7 to begin with.
> >> cpts->cc_mult = mult;
> >> cpts->cc.mult = mult;
> >> cpts->cc.shift = shift;
> >> +
If you want a blank line here, then put into the original patch #7.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 13:52 ` Richard Cochran
@ 2016-09-14 20:10 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:10 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 04:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
>> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> u64 ns;
>> struct skb_shared_hwtstamps *ssh;
>>
>> - if (!cpts->rx_enable)
>> + if (!cpts || !cpts->rx_enable)
>> return;
>
> This function is in the hot path, and you have added a pointless new
> test. Don't do that.
>
>> ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
>> if (!ns)
>> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> u64 ns;
>> struct skb_shared_hwtstamps ssh;
>>
>> - if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> + if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> return;
>
> Same here.
>
>> ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
>> if (!ns)
>> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> skb_tstamp_tx(skb, &ssh);
>> }
>>
>> -int cpts_register(struct device *dev, struct cpts *cpts,
>> - u32 mult, u32 shift)
>> +int cpts_register(struct cpts *cpts)
>> {
>> int err, i;
>> - unsigned long flags;
>>
>> - cpts->info = cpts_info;
>> - cpts->clock = ptp_clock_register(&cpts->info, dev);
>> - if (IS_ERR(cpts->clock)) {
>> - err = PTR_ERR(cpts->clock);
>> - cpts->clock = NULL;
>> - return err;
>> - }
>> - spin_lock_init(&cpts->lock);
>> -
>> - cpts->cc.read = cpts_systim_read;
>> - cpts->cc.mask = CLOCKSOURCE_MASK(32);
>> - cpts->cc_mult = mult;
>> - cpts->cc.mult = mult;
>> - cpts->cc.shift = shift;
>> + if (!cpts)
>> + return -EINVAL;
>
> Not hot path, but still silly. The caller should never pass NULL.
Ok. I can't say I'd like all this checks, but there are internal requirement
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.
But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
@ 2016-09-14 20:10 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:10 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 04:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
>> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> u64 ns;
>> struct skb_shared_hwtstamps *ssh;
>>
>> - if (!cpts->rx_enable)
>> + if (!cpts || !cpts->rx_enable)
>> return;
>
> This function is in the hot path, and you have added a pointless new
> test. Don't do that.
>
>> ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
>> if (!ns)
>> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> u64 ns;
>> struct skb_shared_hwtstamps ssh;
>>
>> - if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> + if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> return;
>
> Same here.
>
>> ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
>> if (!ns)
>> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>> skb_tstamp_tx(skb, &ssh);
>> }
>>
>> -int cpts_register(struct device *dev, struct cpts *cpts,
>> - u32 mult, u32 shift)
>> +int cpts_register(struct cpts *cpts)
>> {
>> int err, i;
>> - unsigned long flags;
>>
>> - cpts->info = cpts_info;
>> - cpts->clock = ptp_clock_register(&cpts->info, dev);
>> - if (IS_ERR(cpts->clock)) {
>> - err = PTR_ERR(cpts->clock);
>> - cpts->clock = NULL;
>> - return err;
>> - }
>> - spin_lock_init(&cpts->lock);
>> -
>> - cpts->cc.read = cpts_systim_read;
>> - cpts->cc.mask = CLOCKSOURCE_MASK(32);
>> - cpts->cc_mult = mult;
>> - cpts->cc.mult = mult;
>> - cpts->cc.shift = shift;
>> + if (!cpts)
>> + return -EINVAL;
>
> Not hot path, but still silly. The caller should never pass NULL.
Ok. I can't say I'd like all this checks, but there are internal requirement
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.
But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 20:08 ` Richard Cochran
@ 2016-09-14 20:23 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:23 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:08 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
>> On 09/14/2016 05:25 PM, Richard Cochran wrote:
>>> On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
>>>> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>>>> u64 ns;
>>>> u64 frac;
>>>>
>>>> - if (cpts->cc_mult || cpts->cc.shift)
>>>> - return;
>>>> -
>>>> freq = clk_get_rate(cpts->refclk);
>>>>
>>>> /* Calc the maximum number of seconds which we can run before
>>>
>>> This hunk has nothing to do with $subject.
>>
>> Sry, but I did not get your comment here :(
>> I'd happy to update patch according to your request, but could you provide more info here, pls?
>
> You added that code in patch #7. Then you moved it in patch #8. You
> could have made the code correct in patch #7 to begin with.
>
Do you mean
- if (cpts->cc_mult || cpts->cc.shift)
- return;
??
if yes then those changes are correct as from patch#7 point of
view, as from patch#8 because they are separate standalone changes.
In patch patch#7 it reasonable to ball out earlier, while in patch#8
it required to move forward a bit as I need to know maxsec.
Sry, that I'm bothering you with stupid questions.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
@ 2016-09-14 20:23 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:23 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:08 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:03:18PM +0300, Grygorii Strashko wrote:
>> On 09/14/2016 05:25 PM, Richard Cochran wrote:
>>> On Wed, Sep 14, 2016 at 04:02:30PM +0300, Grygorii Strashko wrote:
>>>> @@ -427,9 +427,6 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
>>>> u64 ns;
>>>> u64 frac;
>>>>
>>>> - if (cpts->cc_mult || cpts->cc.shift)
>>>> - return;
>>>> -
>>>> freq = clk_get_rate(cpts->refclk);
>>>>
>>>> /* Calc the maximum number of seconds which we can run before
>>>
>>> This hunk has nothing to do with $subject.
>>
>> Sry, but I did not get your comment here :(
>> I'd happy to update patch according to your request, but could you provide more info here, pls?
>
> You added that code in patch #7. Then you moved it in patch #8. You
> could have made the code correct in patch #7 to begin with.
>
Do you mean
- if (cpts->cc_mult || cpts->cc.shift)
- return;
??
if yes then those changes are correct as from patch#7 point of
view, as from patch#8 because they are separate standalone changes.
In patch patch#7 it reasonable to ball out earlier, while in patch#8
it required to move forward a bit as I need to know maxsec.
Sry, that I'm bothering you with stupid questions.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 13:02 ` Grygorii Strashko
(?)
(?)
@ 2016-09-14 20:26 ` Richard Cochran
2016-09-14 20:47 ` Grygorii Strashko
2016-09-15 11:58 ` Richard Cochran
-1 siblings, 2 replies; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 20:26 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
> +static void cpts_calc_mult_shift(struct cpts *cpts)
> +{
> + u64 maxsec;
> + u32 freq;
> + u32 mult;
> + u32 shift;
> + u64 ns;
> + u64 frac;
> +
> + if (cpts->cc_mult || cpts->cc.shift)
> + return;
> +
> + freq = clk_get_rate(cpts->refclk);
> +
> + /* Calc the maximum number of seconds which we can run before
> + * wrapping around.
> + */
> + maxsec = cpts->cc.mask;
> + do_div(maxsec, freq);
> + if (!maxsec)
> + maxsec = 1;
This doesn't pass the smell test. If the max counter value is M, you
are figuring M*1/F which is the time in seconds corresponding to M.
We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in
order for 'maxsec' to be zero. Do you really expect such high
frequency input clocks?
> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
> + maxsec = 600;
What is this all about? cc.mask is always 2^32 - 1.
> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
> +
> + cpts->cc_mult = mult;
> + cpts->cc.mult = mult;
In order to get good resolution on the frequency adjustment, we want
to keep 'mult' as large as possible. I don't see your code doing
this. We can rely on the watchdog reader (work queue) to prevent
overflows.
Thanks,
Richard
> + cpts->cc.shift = shift;
> + /* Check calculations and inform if not precise */
> + frac = 0;
> + ns = cyclecounter_cyc2ns(&cpts->cc, freq, cpts->cc.mask, &frac);
> +
> + dev_info(cpts->dev,
> + "CPTS: ref_clk_freq:%u calc_mult:%u calc_shift:%u error:%lld nsec/sec\n",
> + freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
> +}
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 20:10 ` Grygorii Strashko
(?)
@ 2016-09-14 20:32 ` Richard Cochran
2016-09-14 20:37 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 20:32 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
> On 09/14/2016 04:52 PM, Richard Cochran wrote:
> > On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> >> - if (!cpts->rx_enable)
> >> + if (!cpts || !cpts->rx_enable)
> >> return;
> Ok. I can't say I'd like all this checks, but there are internal requirement
> to allow CPTS to be disabled though DT on KS2 (even if built in).
> I'd try to clarify and return back here.
>
> But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
Look at that code snippet. We already test for @cpts->rx_enable. If
you want to disable cpts at run time, just avoid setting that field.
There is no need for any new tests or return codes.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 20:32 ` Richard Cochran
@ 2016-09-14 20:37 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:37 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:32 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
>> On 09/14/2016 04:52 PM, Richard Cochran wrote:
>>> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
>
>>>> - if (!cpts->rx_enable)
>>>> + if (!cpts || !cpts->rx_enable)
>>>> return;
>
>> Ok. I can't say I'd like all this checks, but there are internal requirement
>> to allow CPTS to be disabled though DT on KS2 (even if built in).
>> I'd try to clarify and return back here.
>>
>> But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
>
> Look at that code snippet. We already test for @cpts->rx_enable. If
> you want to disable cpts at run time, just avoid setting that field.
> There is no need for any new tests or return codes.
>
The problem is that if cpts not initialized than pinter on
cpts (in consumer/parent driver - NETCP) will be NULL.
So, rx_enable will be unaccessible and cause crash :(
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
@ 2016-09-14 20:37 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:37 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:32 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
>> On 09/14/2016 04:52 PM, Richard Cochran wrote:
>>> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
>
>>>> - if (!cpts->rx_enable)
>>>> + if (!cpts || !cpts->rx_enable)
>>>> return;
>
>> Ok. I can't say I'd like all this checks, but there are internal requirement
>> to allow CPTS to be disabled though DT on KS2 (even if built in).
>> I'd try to clarify and return back here.
>>
>> But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
>
> Look at that code snippet. We already test for @cpts->rx_enable. If
> you want to disable cpts at run time, just avoid setting that field.
> There is no need for any new tests or return codes.
>
The problem is that if cpts not initialized than pinter on
cpts (in consumer/parent driver - NETCP) will be NULL.
So, rx_enable will be unaccessible and cause crash :(
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 20:23 ` Grygorii Strashko
(?)
@ 2016-09-14 20:43 ` Richard Cochran
2016-09-14 20:48 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 20:43 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
> if yes then those changes are correct as from patch#7 point of
> view, as from patch#8 because they are separate standalone changes.
> In patch patch#7 it reasonable to ball out earlier, while in patch#8
> it required to move forward a bit as I need to know maxsec.
And what about the extra blank line? AFAICT, placing the test later
in patch #7 is correct logic and has the advantage of not distracting
reviews with pointless churn!
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 20:26 ` Richard Cochran
@ 2016-09-14 20:47 ` Grygorii Strashko
2016-09-15 11:58 ` Richard Cochran
1 sibling, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:47 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok, John Stultz
On 09/14/2016 11:26 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
>> +static void cpts_calc_mult_shift(struct cpts *cpts)
>> +{
>> + u64 maxsec;
>> + u32 freq;
>> + u32 mult;
>> + u32 shift;
>> + u64 ns;
>> + u64 frac;
>> +
>> + if (cpts->cc_mult || cpts->cc.shift)
>> + return;
>> +
>> + freq = clk_get_rate(cpts->refclk);
>> +
>> + /* Calc the maximum number of seconds which we can run before
>> + * wrapping around.
>> + */
>> + maxsec = cpts->cc.mask;
>> + do_div(maxsec, freq);
>> + if (!maxsec)
>> + maxsec = 1;
>
> This doesn't pass the smell test. If the max counter value is M, you
> are figuring M*1/F which is the time in seconds corresponding to M.
> We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in
> order for 'maxsec' to be zero. Do you really expect such high
> frequency input clocks?
no. can drop it.
>
>> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> + maxsec = 600;
>
> What is this all about? cc.mask is always 2^32 - 1.
Oh. Not sure if we will update CPTS to support this, but on
KS2 E/L (66AK2E) CPTS counter can work in 64bit mode.
>
>> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>> +
>> + cpts->cc_mult = mult;
>> + cpts->cc.mult = mult;
>
> In order to get good resolution on the frequency adjustment, we want
> to keep 'mult' as large as possible. I don't see your code doing
> this. We can rely on the watchdog reader (work queue) to prevent
> overflows.
As I understand (and tested), clocks_calc_mult_shift() will
return max possible mult which can be used without overflow.
if calculated results do not satisfy end user - the custom values can
be passed in DT.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
@ 2016-09-14 20:47 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:47 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok, John Stultz
On 09/14/2016 11:26 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
>> +static void cpts_calc_mult_shift(struct cpts *cpts)
>> +{
>> + u64 maxsec;
>> + u32 freq;
>> + u32 mult;
>> + u32 shift;
>> + u64 ns;
>> + u64 frac;
>> +
>> + if (cpts->cc_mult || cpts->cc.shift)
>> + return;
>> +
>> + freq = clk_get_rate(cpts->refclk);
>> +
>> + /* Calc the maximum number of seconds which we can run before
>> + * wrapping around.
>> + */
>> + maxsec = cpts->cc.mask;
>> + do_div(maxsec, freq);
>> + if (!maxsec)
>> + maxsec = 1;
>
> This doesn't pass the smell test. If the max counter value is M, you
> are figuring M*1/F which is the time in seconds corresponding to M.
> We set M=2^32-1, and so 'freq' would have to be greater than 4 GHz in
> order for 'maxsec' to be zero. Do you really expect such high
> frequency input clocks?
no. can drop it.
>
>> + else if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> + maxsec = 600;
>
> What is this all about? cc.mask is always 2^32 - 1.
Oh. Not sure if we will update CPTS to support this, but on
KS2 E/L (66AK2E) CPTS counter can work in 64bit mode.
>
>> + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
>> +
>> + cpts->cc_mult = mult;
>> + cpts->cc.mult = mult;
>
> In order to get good resolution on the frequency adjustment, we want
> to keep 'mult' as large as possible. I don't see your code doing
> this. We can rely on the watchdog reader (work queue) to prevent
> overflows.
As I understand (and tested), clocks_calc_mult_shift() will
return max possible mult which can be used without overflow.
if calculated results do not satisfy end user - the custom values can
be passed in DT.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
2016-09-14 20:43 ` Richard Cochran
@ 2016-09-14 20:48 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:48 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:43 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
>> if yes then those changes are correct as from patch#7 point of
>> view, as from patch#8 because they are separate standalone changes.
>> In patch patch#7 it reasonable to ball out earlier, while in patch#8
>> it required to move forward a bit as I need to know maxsec.
>
> And what about the extra blank line? AFAICT, placing the test later
> in patch #7 is correct logic and has the advantage of not distracting
> reviews with pointless churn!
>
NP. I'll change it.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period
@ 2016-09-14 20:48 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:48 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:43 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:23:43PM +0300, Grygorii Strashko wrote:
>> if yes then those changes are correct as from patch#7 point of
>> view, as from patch#8 because they are separate standalone changes.
>> In patch patch#7 it reasonable to ball out earlier, while in patch#8
>> it required to move forward a bit as I need to know maxsec.
>
> And what about the extra blank line? AFAICT, placing the test later
> in patch #7 is correct logic and has the advantage of not distracting
> reviews with pointless churn!
>
NP. I'll change it.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 20:37 ` Grygorii Strashko
(?)
@ 2016-09-14 20:52 ` Richard Cochran
2016-09-14 20:59 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 20:52 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 11:37:45PM +0300, Grygorii Strashko wrote:
> The problem is that if cpts not initialized than pinter on
> cpts (in consumer/parent driver - NETCP) will be NULL.
You made that problem with your "clean up" in this series.
Previously, cpts was always allocated.
> So, rx_enable will be unaccessible and cause crash :(
So just make sure cpts is initialized.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 20:52 ` Richard Cochran
@ 2016-09-14 20:59 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:59 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:37:45PM +0300, Grygorii Strashko wrote:
>> The problem is that if cpts not initialized than pinter on
>> cpts (in consumer/parent driver - NETCP) will be NULL.
>
> You made that problem with your "clean up" in this series.
> Previously, cpts was always allocated.
not exactly - it was allocated in CPSW .probe() manually,
in without this re-work it has to be allocated in NTCP the
same way - manually. So, checks we are discussing here will be still
present, but they will need to be done in CPSW/NETCP drivers -
just one level up and duplicated in both these drivers.
>
>> So, rx_enable will be unaccessible and cause crash :(
>
> So just make sure cpts is initialized.
OK. I'll update code this way.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
@ 2016-09-14 20:59 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 20:59 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On 09/14/2016 11:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:37:45PM +0300, Grygorii Strashko wrote:
>> The problem is that if cpts not initialized than pinter on
>> cpts (in consumer/parent driver - NETCP) will be NULL.
>
> You made that problem with your "clean up" in this series.
> Previously, cpts was always allocated.
not exactly - it was allocated in CPSW .probe() manually,
in without this re-work it has to be allocated in NTCP the
same way - manually. So, checks we are discussing here will be still
present, but they will need to be done in CPSW/NETCP drivers -
just one level up and duplicated in both these drivers.
>
>> So, rx_enable will be unaccessible and cause crash :(
>
> So just make sure cpts is initialized.
OK. I'll update code this way.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 20:47 ` Grygorii Strashko
(?)
@ 2016-09-14 21:03 ` Richard Cochran
2016-09-14 21:14 ` Grygorii Strashko
-1 siblings, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-14 21:03 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok, John Stultz
On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote:
> As I understand (and tested), clocks_calc_mult_shift() will
> return max possible mult which can be used without overflow.
Yes, BUT the returned values depends on the @maxsec input. As the
kerneldec says,
* Larger ranges may reduce the conversion accuracy by chosing smaller
* mult and shift factors.
In addition to that, frequency tuning by calculating a percentage of
'mult', and if 'mult' is small, then the tuning resolution is poor.
So we don't want @maxsec as large as possible, but as small as
possible.
> if calculated results do not satisfy end user - the custom values can
> be passed in DT.
If we calculate automatically, then the result had better well be
optimal or nearly so. Otherwise, we should leave it as a manual input
via DTS, IMHO, so that someone is forced to check the values.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 21:03 ` Richard Cochran
@ 2016-09-14 21:14 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 21:14 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok, John Stultz
On 09/15/2016 12:03 AM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote:
>> As I understand (and tested), clocks_calc_mult_shift() will
>> return max possible mult which can be used without overflow.
>
> Yes, BUT the returned values depends on the @maxsec input. As the
> kerneldec says,
>
> * Larger ranges may reduce the conversion accuracy by chosing smaller
> * mult and shift factors.
>
> In addition to that, frequency tuning by calculating a percentage of
> 'mult', and if 'mult' is small, then the tuning resolution is poor.
>
> So we don't want @maxsec as large as possible, but as small as
> possible.
>
Ok. So, will it work if maxsec will be ~= (maxsec / 2) + 1?
+ 1 to cover potential delays of overflow check work execution.
[...]
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
@ 2016-09-14 21:14 ` Grygorii Strashko
0 siblings, 0 replies; 58+ messages in thread
From: Grygorii Strashko @ 2016-09-14 21:14 UTC (permalink / raw)
To: Richard Cochran
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok, John Stultz
On 09/15/2016 12:03 AM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:47:46PM +0300, Grygorii Strashko wrote:
>> As I understand (and tested), clocks_calc_mult_shift() will
>> return max possible mult which can be used without overflow.
>
> Yes, BUT the returned values depends on the @maxsec input. As the
> kerneldec says,
>
> * Larger ranges may reduce the conversion accuracy by chosing smaller
> * mult and shift factors.
>
> In addition to that, frequency tuning by calculating a percentage of
> 'mult', and if 'mult' is small, then the tuning resolution is poor.
>
> So we don't want @maxsec as large as possible, but as small as
> possible.
>
Ok. So, will it work if maxsec will be ~= (maxsec / 2) + 1?
+ 1 to cover potential delays of overflow check work execution.
[...]
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization
2016-09-14 13:02 ` Grygorii Strashko
(?)
(?)
@ 2016-09-15 8:13 ` Richard Cochran
-1 siblings, 0 replies; 58+ messages in thread
From: Richard Cochran @ 2016-09-15 8:13 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> The current implementation CPTS initialization and deinitialization
> (represented by cpts_register/unregister()) is pretty entangled and
> has some issues, like:
> - ptp clock registered before spinlock, which is protecting it, and
> before timecounter and cyclecounter initialization;
> - CPTS ref_clk requested using devm API while cpts_register() is
> called from .ndo_open(), as result additional checks required;
> - CPTS ref_clk is prepared, but never unprepared;
> - CPTS is not disabled even when unregistered..
This list of four items is a clear sign that this one patch should be
broken into a series of four.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-14 20:26 ` Richard Cochran
2016-09-14 20:47 ` Grygorii Strashko
@ 2016-09-15 11:58 ` Richard Cochran
2016-09-15 13:49 ` Richard Cochran
1 sibling, 1 reply; 58+ messages in thread
From: Richard Cochran @ 2016-09-15 11:58 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Wed, Sep 14, 2016 at 10:26:19PM +0200, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:29PM +0300, Grygorii Strashko wrote:
> > + clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, maxsec);
> > +
> > + cpts->cc_mult = mult;
> > + cpts->cc.mult = mult;
>
> In order to get good resolution on the frequency adjustment, we want
> to keep 'mult' as large as possible. I don't see your code doing
> this. We can rely on the watchdog reader (work queue) to prevent
> overflows.
I took a closer look, and assuming cc.mask = 2^32 - 1, then using
clocks_calc_mult_shift() produces good results for a reasonable range
of input frequencies. Keeping 'maxsec' constant at 4 we have:
| Freq. MHz | mult | shift |
|-----------+------------+-------|
| 100 | 0xa0000000 | 28 |
| 250 | 0x80000000 | 29 |
| 500 | 0x80000000 | 30 |
| 1000 | 0x80000000 | 31 |
Can the input clock be higher than 1 GHz? If not, I suggest using
clocks_calc_mult_shift() with maxsec=4 and a setting the watchdog also
to 4*HZ.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq
2016-09-15 11:58 ` Richard Cochran
@ 2016-09-15 13:49 ` Richard Cochran
0 siblings, 0 replies; 58+ messages in thread
From: Richard Cochran @ 2016-09-15 13:49 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori,
linux-kernel, linux-omap, WingMan Kwok
On Thu, Sep 15, 2016 at 01:58:15PM +0200, Richard Cochran wrote:
> Can the input clock be higher than 1 GHz? If not, I suggest using
> clocks_calc_mult_shift() with maxsec=4 and a setting the watchdog also
> to 4*HZ.
On second thought, with the new 12% timer batching, using 4*HZ for 32
bits of 1 GHz is cutting it too close. So just keep it like you had
it, with maxsec=mask/freq and timeout=maxsec/2, to stay on the safe
side.
Thanks,
Richard
^ permalink raw reply [flat|nested] 58+ messages in thread
end of thread, other threads:[~2016-09-15 13:49 UTC | newest]
Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-14 13:02 [PATCH 0/9] net: ethernet: ti: cpts: update and fixes Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 13:02 ` [PATCH 1/9] net: ethernet: ti: exclude cpts from build when disabled Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 13:02 ` [PATCH 2/9] net: ethernet: ti: cpsw: minimize direct access to struct cpts Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 13:02 ` [PATCH 3/9] net: ethernet: ti: cpts: rework initialization/deinitialization Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 13:52 ` Richard Cochran
2016-09-14 20:10 ` Grygorii Strashko
2016-09-14 20:10 ` Grygorii Strashko
2016-09-14 20:32 ` Richard Cochran
2016-09-14 20:37 ` Grygorii Strashko
2016-09-14 20:37 ` Grygorii Strashko
2016-09-14 20:52 ` Richard Cochran
2016-09-14 20:59 ` Grygorii Strashko
2016-09-14 20:59 ` Grygorii Strashko
2016-09-15 8:13 ` Richard Cochran
2016-09-14 13:02 ` [PATCH 4/9] net: ethernet: ti: cpts: move dt props parsing to cpts driver Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 13:55 ` Richard Cochran
2016-09-14 19:45 ` Grygorii Strashko
2016-09-14 19:45 ` Grygorii Strashko
2016-09-14 20:03 ` Richard Cochran
2016-09-14 13:02 ` [PATCH 5/9] net: ethernet: ti: cpts: add return value to tx and rx timestamp funcitons Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 14:00 ` Richard Cochran
2016-09-14 13:02 ` [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 14:14 ` Richard Cochran
2016-09-14 19:54 ` Grygorii Strashko
2016-09-14 19:54 ` Grygorii Strashko
2016-09-14 13:02 ` [PATCH 7/9] net: ethernet: ti: cpts: calc mult and shift from refclk freq Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 14:22 ` Richard Cochran
2016-09-14 19:59 ` Grygorii Strashko
2016-09-14 19:59 ` Grygorii Strashko
2016-09-14 20:26 ` Richard Cochran
2016-09-14 20:47 ` Grygorii Strashko
2016-09-14 20:47 ` Grygorii Strashko
2016-09-14 21:03 ` Richard Cochran
2016-09-14 21:14 ` Grygorii Strashko
2016-09-14 21:14 ` Grygorii Strashko
2016-09-15 11:58 ` Richard Cochran
2016-09-15 13:49 ` Richard Cochran
2016-09-14 13:02 ` [PATCH 8/9] net: ethernet: ti: cpts: fix overflow check period Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
2016-09-14 14:25 ` Richard Cochran
2016-09-14 20:03 ` Grygorii Strashko
2016-09-14 20:03 ` Grygorii Strashko
2016-09-14 20:08 ` Richard Cochran
2016-09-14 20:23 ` Grygorii Strashko
2016-09-14 20:23 ` Grygorii Strashko
2016-09-14 20:43 ` Richard Cochran
2016-09-14 20:48 ` Grygorii Strashko
2016-09-14 20:48 ` Grygorii Strashko
2016-09-14 13:02 ` [PATCH 9/9] net: ethernet: ti: cpts: switch to readl/writel_relaxed() Grygorii Strashko
2016-09-14 13:02 ` Grygorii Strashko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.