Hi,
I have a system where I need to handle an HDLC interface.
The HDLC data are transferred using a TDM bus on which a PEF2256 is
present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
This PEF2256 is also connected to a PowerQUICC SoC for the control path
and the TDM is connected to the SoC (QMC component) for the data path.
From the HDLC driver, I need to handle data using the QMC and carrier
detection using the PEF2256 (E1 line carrier).
The HDLC driver consider the PEF2256 as a generic PHY.
So, the design is the following:
+----------+ +-------------+ +---------+
| HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 |
+----------+ +-------------+ | | <--> E1
^ +---------+ +---------+ | |
+-> | Gen PHY | <-> | PEF2256 | <- local bus -> | |
+---------+ | PHY drv | +---------+
+---------+
In order to implement this, I had to:
1 - Extend the generic PHY API to support get_status() and notification
on status change.
2 - Introduce a new kind of generic PHY named "basic phy". This PHY
familly can provide a link status in the get_status() data.
3 - Support the PEF2256 PHY as a "basic phy"
The purpose of this RFC series is to discuss this design.
The QMC driver code is available on linux-next. In this series:
- patch 1: driver HDLC using the QMC channel
- patch 2: Extend the generic PHY API
- patch 3: Use the "basic phy" in the HDLC driver
- patch 4: Implement the PEF2256 PHY driver
I did 2 patches for the HDLC driver in order to point the new PHY family
usage in the HDLC driver. In the end, these two patches will be squashed
and the bindings will be added.
Hope to have some feedback on this proposal.
Best regards,
Hervé
Herve Codina (4):
net: wan: Add support for QMC HDLC
phy: Extend API to support 'status' get and notification
net: wan: fsl_qmc_hdlc: Add PHY support
phy: lantiq: Add PEF2256 PHY support
drivers/net/wan/Kconfig | 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 558 ++++++++++++++++++++++++
drivers/phy/lantiq/Kconfig | 15 +
drivers/phy/lantiq/Makefile | 1 +
drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++
drivers/phy/phy-core.c | 88 ++++
include/linux/phy/phy-basic.h | 27 ++
include/linux/phy/phy.h | 89 +++-
9 files changed, 921 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c
create mode 100644 include/linux/phy/phy-basic.h
--
2.39.2
The PHY API provides functions to control and pass information from the
PHY consumer to the PHY provider.
There is no way for the consumer to get direct information from the PHY
or be notified by the PHY.
To fill this hole, two API function are provided:
- phy_get_status()
This function can be used to get a "status" from the PHY. It is built
as the same ways as the configure() function. The status information
present in the status retrieved depends on the PHY's phy_mode.
This allows to get a "status" depending on the kind of PHY.
- phy_atomic_notifier_(un)register()
These functions can be used to register/unregister an atomic notifier
block. The event available at this time is the PHY_EVENT_STATUS status
event which purpose is to signal some changes in the status available
using phy_get_status().
An new kind of PHY is added: PHY_MODE_BASIC.
This new kind of PHY represents a basic PHY offering a basic status This
status contains a link state indication.
With the new API, a link state indication can be retrieve using
phy_get_status() and link state changes can be notified.
Signed-off-by: Herve Codina <[email protected]>
---
drivers/phy/phy-core.c | 88 ++++++++++++++++++++++++++++++++++
include/linux/phy/phy-basic.h | 27 +++++++++++
include/linux/phy/phy.h | 89 ++++++++++++++++++++++++++++++++++-
3 files changed, 203 insertions(+), 1 deletion(-)
create mode 100644 include/linux/phy/phy-basic.h
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 9951efc03eaa..c7b568b99dce 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -551,6 +551,94 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
}
EXPORT_SYMBOL_GPL(phy_validate);
+/**
+ * phy_get_status() - Gets the phy status
+ * @phy: the phy returned by phy_get()
+ * @status: the status to retrieve
+ *
+ * Used to get the PHY status. phy_init() must have been called
+ * on the phy. The status will be retrieved from the current phy mode,
+ * that can be changed using phy_set_mode().
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
+int phy_get_status(struct phy *phy, union phy_status *status)
+{
+ int ret;
+
+ if (!phy)
+ return -EINVAL;
+
+ if (!phy->ops->get_status)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&phy->mutex);
+ ret = phy->ops->get_status(phy, status);
+ mutex_unlock(&phy->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_get_status);
+
+/**
+ * phy_atomic_notifier_register() - Registers an atomic notifier
+ * @phy: the phy returned by phy_get()
+ * @nb: the notifier block to register
+ *
+ * Used to register a notifier block on PHY events. phy_init() must have
+ * been called on the phy.
+ * The notifier function given in the notifier_block must not sleep.
+ * The available PHY events are present in enum phy_events
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
+int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
+{
+ int ret;
+
+ if (!phy)
+ return -EINVAL;
+
+ if (!phy->ops->atomic_notifier_register ||
+ !phy->ops->atomic_notifier_unregister)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&phy->mutex);
+ ret = phy->ops->atomic_notifier_register(phy, nb);
+ mutex_unlock(&phy->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_atomic_notifier_register);
+
+/**
+ * phy_atomic_notifier_unregister() - Unregisters an atomic notifier
+ * @phy: the phy returned by phy_get()
+ * @nb: the notifier block to unregister
+ *
+ * Used to unregister a notifier block. phy_init() must have
+ * been called on the phy.
+ *
+ * Return: %0 if successful, a negative error code otherwise
+ */
+int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
+{
+ int ret;
+
+ if (!phy)
+ return -EINVAL;
+
+ if (!phy->ops->atomic_notifier_unregister)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&phy->mutex);
+ ret = phy->ops->atomic_notifier_unregister(phy, nb);
+ mutex_unlock(&phy->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_atomic_notifier_unregister);
+
/**
* _of_phy_get() - lookup and obtain a reference to a phy by phandle
* @np: device_node for which to get the phy
diff --git a/include/linux/phy/phy-basic.h b/include/linux/phy/phy-basic.h
new file mode 100644
index 000000000000..95668c610c78
--- /dev/null
+++ b/include/linux/phy/phy-basic.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <[email protected]>
+ */
+
+#ifndef __PHY_BASIC_H_
+#define __PHY_BASIC_H_
+
+#include <linux/types.h>
+
+/**
+ * struct phy_status_basic - Basic PHY status
+ *
+ * This structure is used to represent the status of a Basic phy.
+ */
+struct phy_status_basic {
+ /**
+ * @link_state:
+ *
+ * Link state. true, the link is on, false, the link is off.
+ */
+ bool link_is_on;
+};
+
+#endif /* __PHY_DP_H_ */
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 3a570bc59fc7..40370d41012b 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -16,6 +16,7 @@
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
+#include <linux/phy/phy-basic.h>
#include <linux/phy/phy-dp.h>
#include <linux/phy/phy-lvds.h>
#include <linux/phy/phy-mipi-dphy.h>
@@ -42,7 +43,8 @@ enum phy_mode {
PHY_MODE_MIPI_DPHY,
PHY_MODE_SATA,
PHY_MODE_LVDS,
- PHY_MODE_DP
+ PHY_MODE_DP,
+ PHY_MODE_BASIC,
};
enum phy_media {
@@ -67,6 +69,22 @@ union phy_configure_opts {
struct phy_configure_opts_lvds lvds;
};
+/**
+ * union phy_status - Opaque generic phy status
+ *
+ * @basic: Status availbale phys supporting the Basic phy mode.
+ */
+union phy_status {
+ struct phy_status_basic basic;
+};
+
+/**
+ * phy_event - event available for notification
+ */
+enum phy_event {
+ PHY_EVENT_STATUS, /* Event notified on phy_status changes */
+};
+
/**
* struct phy_ops - set of function pointers for performing phy operations
* @init: operation to be performed for initializing phy
@@ -120,6 +138,45 @@ struct phy_ops {
*/
int (*validate)(struct phy *phy, enum phy_mode mode, int submode,
union phy_configure_opts *opts);
+
+ /**
+ * @get_status:
+ *
+ * Optional.
+ *
+ * Used to get the PHY status. phy_init() must have
+ * been called on the phy.
+ *
+ * Returns: 0 if successful, an negative error code otherwise
+ */
+ int (*get_status)(struct phy *phy, union phy_status *status);
+
+ /**
+ * @atomic_notifier_register:
+ *
+ * Optional.
+ *
+ * Used to register a notifier block on PHY events. phy_init() must have
+ * been called on the phy.
+ * The notifier function given in the notifier_block must not sleep.
+ * The available PHY events are present in enum phy_events
+ *
+ * Returns: 0 if successful, an negative error code otherwise
+ */
+ int (*atomic_notifier_register)(struct phy *phy, struct notifier_block *nb);
+
+ /**
+ * @atomic_notifier_unregister:
+ *
+ * Mandatoty if @atomic_notifier_register is set.
+ *
+ * Used to unregister a notifier block on PHY events. phy_init() must have
+ * been called on the phy.
+ *
+ * Returns: 0 if successful, an negative error code otherwise
+ */
+ int (*atomic_notifier_unregister)(struct phy *phy, struct notifier_block *nb);
+
int (*reset)(struct phy *phy);
int (*calibrate)(struct phy *phy);
void (*release)(struct phy *phy);
@@ -234,6 +291,10 @@ int phy_set_speed(struct phy *phy, int speed);
int phy_configure(struct phy *phy, union phy_configure_opts *opts);
int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
union phy_configure_opts *opts);
+int phy_get_status(struct phy *phy, union phy_status *status);
+int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb);
+int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb);
+
static inline enum phy_mode phy_get_mode(struct phy *phy)
{
@@ -412,6 +473,32 @@ static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
return -ENOSYS;
}
+static inline int phy_get_status(struct phy *phy, union phy_status *status)
+{
+ if (!phy)
+ return 0;
+
+ return -ENOSYS;
+}
+
+static inline int phy_atomic_notifier_register(struct phy *phy,
+ struct notifier_block *nb)
+{
+ if (!phy)
+ return 0;
+
+ return -ENOSYS;
+}
+
+static inline int phy_atomic_notifier_unregister(struct phy *phy,
+ struct notifier_block *nb)
+{
+ if (!phy)
+ return 0;
+
+ return -ENOSYS;
+}
+
static inline int phy_get_bus_width(struct phy *phy)
{
return -ENOSYS;
--
2.39.2
Hi all,
I haven't received any feedback on this RFC.
Have you had a chance to review it ?
Best regards,
Hervé
On Thu, 23 Mar 2023 11:31:50 +0100
Herve Codina <[email protected]> wrote:
> Hi,
>
> I have a system where I need to handle an HDLC interface.
>
> The HDLC data are transferred using a TDM bus on which a PEF2256 is
> present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
> This PEF2256 is also connected to a PowerQUICC SoC for the control path
> and the TDM is connected to the SoC (QMC component) for the data path.
>
> From the HDLC driver, I need to handle data using the QMC and carrier
> detection using the PEF2256 (E1 line carrier).
>
> The HDLC driver consider the PEF2256 as a generic PHY.
> So, the design is the following:
>
> +----------+ +-------------+ +---------+
> | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 |
> +----------+ +-------------+ | | <--> E1
> ^ +---------+ +---------+ | |
> +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | |
> +---------+ | PHY drv | +---------+
> +---------+
>
> In order to implement this, I had to:
> 1 - Extend the generic PHY API to support get_status() and notification
> on status change.
> 2 - Introduce a new kind of generic PHY named "basic phy". This PHY
> familly can provide a link status in the get_status() data.
> 3 - Support the PEF2256 PHY as a "basic phy"
>
> The purpose of this RFC series is to discuss this design.
>
> The QMC driver code is available on linux-next. In this series:
> - patch 1: driver HDLC using the QMC channel
> - patch 2: Extend the generic PHY API
> - patch 3: Use the "basic phy" in the HDLC driver
> - patch 4: Implement the PEF2256 PHY driver
>
> I did 2 patches for the HDLC driver in order to point the new PHY family
> usage in the HDLC driver. In the end, these two patches will be squashed
> and the bindings will be added.
>
> Hope to have some feedback on this proposal.
>
> Best regards,
> Hervé
>
> Herve Codina (4):
> net: wan: Add support for QMC HDLC
> phy: Extend API to support 'status' get and notification
> net: wan: fsl_qmc_hdlc: Add PHY support
> phy: lantiq: Add PEF2256 PHY support
>
> drivers/net/wan/Kconfig | 12 +
> drivers/net/wan/Makefile | 1 +
> drivers/net/wan/fsl_qmc_hdlc.c | 558 ++++++++++++++++++++++++
> drivers/phy/lantiq/Kconfig | 15 +
> drivers/phy/lantiq/Makefile | 1 +
> drivers/phy/lantiq/phy-lantiq-pef2256.c | 131 ++++++
> drivers/phy/phy-core.c | 88 ++++
> include/linux/phy/phy-basic.h | 27 ++
> include/linux/phy/phy.h | 89 +++-
> 9 files changed, 921 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
> create mode 100644 drivers/phy/lantiq/phy-lantiq-pef2256.c
> create mode 100644 include/linux/phy/phy-basic.h
>
On 23-03-23, 11:31, Herve Codina wrote:
> The PHY API provides functions to control and pass information from the
> PHY consumer to the PHY provider.
> There is no way for the consumer to get direct information from the PHY
> or be notified by the PHY.
>
> To fill this hole, two API function are provided:
>
> - phy_get_status()
> This function can be used to get a "status" from the PHY. It is built
> as the same ways as the configure() function. The status information
> present in the status retrieved depends on the PHY's phy_mode.
> This allows to get a "status" depending on the kind of PHY.
what does 'status' mean and communicate to used? How does having this
help?
>
> - phy_atomic_notifier_(un)register()
> These functions can be used to register/unregister an atomic notifier
> block. The event available at this time is the PHY_EVENT_STATUS status
> event which purpose is to signal some changes in the status available
> using phy_get_status().
>
> An new kind of PHY is added: PHY_MODE_BASIC.
> This new kind of PHY represents a basic PHY offering a basic status This
> status contains a link state indication.
> With the new API, a link state indication can be retrieve using
> phy_get_status() and link state changes can be notified.
>
> Signed-off-by: Herve Codina <[email protected]>
> ---
> drivers/phy/phy-core.c | 88 ++++++++++++++++++++++++++++++++++
> include/linux/phy/phy-basic.h | 27 +++++++++++
> include/linux/phy/phy.h | 89 ++++++++++++++++++++++++++++++++++-
> 3 files changed, 203 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/phy/phy-basic.h
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 9951efc03eaa..c7b568b99dce 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -551,6 +551,94 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> }
> EXPORT_SYMBOL_GPL(phy_validate);
>
> +/**
> + * phy_get_status() - Gets the phy status
> + * @phy: the phy returned by phy_get()
> + * @status: the status to retrieve
> + *
> + * Used to get the PHY status. phy_init() must have been called
> + * on the phy. The status will be retrieved from the current phy mode,
> + * that can be changed using phy_set_mode().
> + *
> + * Return: %0 if successful, a negative error code otherwise
> + */
> +int phy_get_status(struct phy *phy, union phy_status *status)
> +{
> + int ret;
> +
> + if (!phy)
> + return -EINVAL;
> +
> + if (!phy->ops->get_status)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&phy->mutex);
> + ret = phy->ops->get_status(phy, status);
> + mutex_unlock(&phy->mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_get_status);
> +
> +/**
> + * phy_atomic_notifier_register() - Registers an atomic notifier
> + * @phy: the phy returned by phy_get()
> + * @nb: the notifier block to register
> + *
> + * Used to register a notifier block on PHY events. phy_init() must have
> + * been called on the phy.
> + * The notifier function given in the notifier_block must not sleep.
> + * The available PHY events are present in enum phy_events
> + *
> + * Return: %0 if successful, a negative error code otherwise
> + */
> +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
> +{
> + int ret;
> +
> + if (!phy)
> + return -EINVAL;
> +
> + if (!phy->ops->atomic_notifier_register ||
> + !phy->ops->atomic_notifier_unregister)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&phy->mutex);
> + ret = phy->ops->atomic_notifier_register(phy, nb);
> + mutex_unlock(&phy->mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_atomic_notifier_register);
> +
> +/**
> + * phy_atomic_notifier_unregister() - Unregisters an atomic notifier
> + * @phy: the phy returned by phy_get()
> + * @nb: the notifier block to unregister
> + *
> + * Used to unregister a notifier block. phy_init() must have
> + * been called on the phy.
> + *
> + * Return: %0 if successful, a negative error code otherwise
> + */
> +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
> +{
> + int ret;
> +
> + if (!phy)
> + return -EINVAL;
> +
> + if (!phy->ops->atomic_notifier_unregister)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&phy->mutex);
> + ret = phy->ops->atomic_notifier_unregister(phy, nb);
> + mutex_unlock(&phy->mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_atomic_notifier_unregister);
> +
> /**
> * _of_phy_get() - lookup and obtain a reference to a phy by phandle
> * @np: device_node for which to get the phy
> diff --git a/include/linux/phy/phy-basic.h b/include/linux/phy/phy-basic.h
> new file mode 100644
> index 000000000000..95668c610c78
> --- /dev/null
> +++ b/include/linux/phy/phy-basic.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2023 CS GROUP France
> + *
> + * Author: Herve Codina <[email protected]>
> + */
> +
> +#ifndef __PHY_BASIC_H_
> +#define __PHY_BASIC_H_
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct phy_status_basic - Basic PHY status
> + *
> + * This structure is used to represent the status of a Basic phy.
> + */
> +struct phy_status_basic {
> + /**
> + * @link_state:
> + *
> + * Link state. true, the link is on, false, the link is off.
> + */
> + bool link_is_on;
> +};
> +
> +#endif /* __PHY_DP_H_ */
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 3a570bc59fc7..40370d41012b 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -16,6 +16,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
>
> +#include <linux/phy/phy-basic.h>
> #include <linux/phy/phy-dp.h>
> #include <linux/phy/phy-lvds.h>
> #include <linux/phy/phy-mipi-dphy.h>
> @@ -42,7 +43,8 @@ enum phy_mode {
> PHY_MODE_MIPI_DPHY,
> PHY_MODE_SATA,
> PHY_MODE_LVDS,
> - PHY_MODE_DP
> + PHY_MODE_DP,
> + PHY_MODE_BASIC,
> };
>
> enum phy_media {
> @@ -67,6 +69,22 @@ union phy_configure_opts {
> struct phy_configure_opts_lvds lvds;
> };
>
> +/**
> + * union phy_status - Opaque generic phy status
> + *
> + * @basic: Status availbale phys supporting the Basic phy mode.
> + */
> +union phy_status {
> + struct phy_status_basic basic;
> +};
> +
> +/**
> + * phy_event - event available for notification
> + */
> +enum phy_event {
> + PHY_EVENT_STATUS, /* Event notified on phy_status changes */
> +};
> +
> /**
> * struct phy_ops - set of function pointers for performing phy operations
> * @init: operation to be performed for initializing phy
> @@ -120,6 +138,45 @@ struct phy_ops {
> */
> int (*validate)(struct phy *phy, enum phy_mode mode, int submode,
> union phy_configure_opts *opts);
> +
> + /**
> + * @get_status:
> + *
> + * Optional.
> + *
> + * Used to get the PHY status. phy_init() must have
> + * been called on the phy.
> + *
> + * Returns: 0 if successful, an negative error code otherwise
> + */
> + int (*get_status)(struct phy *phy, union phy_status *status);
> +
> + /**
> + * @atomic_notifier_register:
> + *
> + * Optional.
> + *
> + * Used to register a notifier block on PHY events. phy_init() must have
> + * been called on the phy.
> + * The notifier function given in the notifier_block must not sleep.
> + * The available PHY events are present in enum phy_events
> + *
> + * Returns: 0 if successful, an negative error code otherwise
> + */
> + int (*atomic_notifier_register)(struct phy *phy, struct notifier_block *nb);
> +
> + /**
> + * @atomic_notifier_unregister:
> + *
> + * Mandatoty if @atomic_notifier_register is set.
> + *
> + * Used to unregister a notifier block on PHY events. phy_init() must have
> + * been called on the phy.
> + *
> + * Returns: 0 if successful, an negative error code otherwise
> + */
> + int (*atomic_notifier_unregister)(struct phy *phy, struct notifier_block *nb);
> +
> int (*reset)(struct phy *phy);
> int (*calibrate)(struct phy *phy);
> void (*release)(struct phy *phy);
> @@ -234,6 +291,10 @@ int phy_set_speed(struct phy *phy, int speed);
> int phy_configure(struct phy *phy, union phy_configure_opts *opts);
> int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> union phy_configure_opts *opts);
> +int phy_get_status(struct phy *phy, union phy_status *status);
> +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb);
> +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb);
> +
>
> static inline enum phy_mode phy_get_mode(struct phy *phy)
> {
> @@ -412,6 +473,32 @@ static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> return -ENOSYS;
> }
>
> +static inline int phy_get_status(struct phy *phy, union phy_status *status)
> +{
> + if (!phy)
> + return 0;
> +
> + return -ENOSYS;
> +}
> +
> +static inline int phy_atomic_notifier_register(struct phy *phy,
> + struct notifier_block *nb)
> +{
> + if (!phy)
> + return 0;
> +
> + return -ENOSYS;
> +}
> +
> +static inline int phy_atomic_notifier_unregister(struct phy *phy,
> + struct notifier_block *nb)
> +{
> + if (!phy)
> + return 0;
> +
> + return -ENOSYS;
> +}
> +
> static inline int phy_get_bus_width(struct phy *phy)
> {
> return -ENOSYS;
> --
> 2.39.2
>
>
> --
> linux-phy mailing list
> [email protected]
> https://lists.infradead.org/mailman/listinfo/linux-phy
--
~Vinod
Hi Vinod,
On Wed, 12 Apr 2023 22:18:44 +0530
Vinod Koul <[email protected]> wrote:
> On 23-03-23, 11:31, Herve Codina wrote:
> > The PHY API provides functions to control and pass information from the
> > PHY consumer to the PHY provider.
> > There is no way for the consumer to get direct information from the PHY
> > or be notified by the PHY.
> >
> > To fill this hole, two API function are provided:
> >
> > - phy_get_status()
> > This function can be used to get a "status" from the PHY. It is built
> > as the same ways as the configure() function. The status information
> > present in the status retrieved depends on the PHY's phy_mode.
> > This allows to get a "status" depending on the kind of PHY.
>
> what does 'status' mean and communicate to used? How does having this
> help?
'status' can be some information that the PHY can provide to the consumer.
The existing API does not provide a generic way to get some information from
the PHY and 'status' with phy_get_status() provides this generic way.
Its exact meaning depends on the kind of PHY. For the PHY_MODE_BASIC,
introduced in this series, 'status' contains information related to the link
state.
And so, the consumer using a PHY_MODE_BASIC PHY can retreive the link state
getting the 'status' from the PHY.
The patch 3 in this RFC details a consumer usage. An HDLC driver uses a
PHY_MODE_BASIC PHY status to know the PHY link state and calls
netif_carrier_{on,off}() accordingly.
Best regards,
Hervé
>
> >
> > - phy_atomic_notifier_(un)register()
> > These functions can be used to register/unregister an atomic notifier
> > block. The event available at this time is the PHY_EVENT_STATUS status
> > event which purpose is to signal some changes in the status available
> > using phy_get_status().
> >
> > An new kind of PHY is added: PHY_MODE_BASIC.
> > This new kind of PHY represents a basic PHY offering a basic status This
> > status contains a link state indication.
> > With the new API, a link state indication can be retrieve using
> > phy_get_status() and link state changes can be notified.
> >
> > Signed-off-by: Herve Codina <[email protected]>
> > ---
> > drivers/phy/phy-core.c | 88 ++++++++++++++++++++++++++++++++++
> > include/linux/phy/phy-basic.h | 27 +++++++++++
> > include/linux/phy/phy.h | 89 ++++++++++++++++++++++++++++++++++-
> > 3 files changed, 203 insertions(+), 1 deletion(-)
> > create mode 100644 include/linux/phy/phy-basic.h
> >
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > index 9951efc03eaa..c7b568b99dce 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> > @@ -551,6 +551,94 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> > }
> > EXPORT_SYMBOL_GPL(phy_validate);
> >
> > +/**
> > + * phy_get_status() - Gets the phy status
> > + * @phy: the phy returned by phy_get()
> > + * @status: the status to retrieve
> > + *
> > + * Used to get the PHY status. phy_init() must have been called
> > + * on the phy. The status will be retrieved from the current phy mode,
> > + * that can be changed using phy_set_mode().
> > + *
> > + * Return: %0 if successful, a negative error code otherwise
> > + */
> > +int phy_get_status(struct phy *phy, union phy_status *status)
> > +{
> > + int ret;
> > +
> > + if (!phy)
> > + return -EINVAL;
> > +
> > + if (!phy->ops->get_status)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&phy->mutex);
> > + ret = phy->ops->get_status(phy, status);
> > + mutex_unlock(&phy->mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_get_status);
> > +
> > +/**
> > + * phy_atomic_notifier_register() - Registers an atomic notifier
> > + * @phy: the phy returned by phy_get()
> > + * @nb: the notifier block to register
> > + *
> > + * Used to register a notifier block on PHY events. phy_init() must have
> > + * been called on the phy.
> > + * The notifier function given in the notifier_block must not sleep.
> > + * The available PHY events are present in enum phy_events
> > + *
> > + * Return: %0 if successful, a negative error code otherwise
> > + */
> > +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb)
> > +{
> > + int ret;
> > +
> > + if (!phy)
> > + return -EINVAL;
> > +
> > + if (!phy->ops->atomic_notifier_register ||
> > + !phy->ops->atomic_notifier_unregister)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&phy->mutex);
> > + ret = phy->ops->atomic_notifier_register(phy, nb);
> > + mutex_unlock(&phy->mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_atomic_notifier_register);
> > +
> > +/**
> > + * phy_atomic_notifier_unregister() - Unregisters an atomic notifier
> > + * @phy: the phy returned by phy_get()
> > + * @nb: the notifier block to unregister
> > + *
> > + * Used to unregister a notifier block. phy_init() must have
> > + * been called on the phy.
> > + *
> > + * Return: %0 if successful, a negative error code otherwise
> > + */
> > +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb)
> > +{
> > + int ret;
> > +
> > + if (!phy)
> > + return -EINVAL;
> > +
> > + if (!phy->ops->atomic_notifier_unregister)
> > + return -EOPNOTSUPP;
> > +
> > + mutex_lock(&phy->mutex);
> > + ret = phy->ops->atomic_notifier_unregister(phy, nb);
> > + mutex_unlock(&phy->mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_atomic_notifier_unregister);
> > +
> > /**
> > * _of_phy_get() - lookup and obtain a reference to a phy by phandle
> > * @np: device_node for which to get the phy
> > diff --git a/include/linux/phy/phy-basic.h b/include/linux/phy/phy-basic.h
> > new file mode 100644
> > index 000000000000..95668c610c78
> > --- /dev/null
> > +++ b/include/linux/phy/phy-basic.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2023 CS GROUP France
> > + *
> > + * Author: Herve Codina <[email protected]>
> > + */
> > +
> > +#ifndef __PHY_BASIC_H_
> > +#define __PHY_BASIC_H_
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct phy_status_basic - Basic PHY status
> > + *
> > + * This structure is used to represent the status of a Basic phy.
> > + */
> > +struct phy_status_basic {
> > + /**
> > + * @link_state:
> > + *
> > + * Link state. true, the link is on, false, the link is off.
> > + */
> > + bool link_is_on;
> > +};
> > +
> > +#endif /* __PHY_DP_H_ */
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index 3a570bc59fc7..40370d41012b 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -16,6 +16,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/regulator/consumer.h>
> >
> > +#include <linux/phy/phy-basic.h>
> > #include <linux/phy/phy-dp.h>
> > #include <linux/phy/phy-lvds.h>
> > #include <linux/phy/phy-mipi-dphy.h>
> > @@ -42,7 +43,8 @@ enum phy_mode {
> > PHY_MODE_MIPI_DPHY,
> > PHY_MODE_SATA,
> > PHY_MODE_LVDS,
> > - PHY_MODE_DP
> > + PHY_MODE_DP,
> > + PHY_MODE_BASIC,
> > };
> >
> > enum phy_media {
> > @@ -67,6 +69,22 @@ union phy_configure_opts {
> > struct phy_configure_opts_lvds lvds;
> > };
> >
> > +/**
> > + * union phy_status - Opaque generic phy status
> > + *
> > + * @basic: Status availbale phys supporting the Basic phy mode.
> > + */
> > +union phy_status {
> > + struct phy_status_basic basic;
> > +};
> > +
> > +/**
> > + * phy_event - event available for notification
> > + */
> > +enum phy_event {
> > + PHY_EVENT_STATUS, /* Event notified on phy_status changes */
> > +};
> > +
> > /**
> > * struct phy_ops - set of function pointers for performing phy operations
> > * @init: operation to be performed for initializing phy
> > @@ -120,6 +138,45 @@ struct phy_ops {
> > */
> > int (*validate)(struct phy *phy, enum phy_mode mode, int submode,
> > union phy_configure_opts *opts);
> > +
> > + /**
> > + * @get_status:
> > + *
> > + * Optional.
> > + *
> > + * Used to get the PHY status. phy_init() must have
> > + * been called on the phy.
> > + *
> > + * Returns: 0 if successful, an negative error code otherwise
> > + */
> > + int (*get_status)(struct phy *phy, union phy_status *status);
> > +
> > + /**
> > + * @atomic_notifier_register:
> > + *
> > + * Optional.
> > + *
> > + * Used to register a notifier block on PHY events. phy_init() must have
> > + * been called on the phy.
> > + * The notifier function given in the notifier_block must not sleep.
> > + * The available PHY events are present in enum phy_events
> > + *
> > + * Returns: 0 if successful, an negative error code otherwise
> > + */
> > + int (*atomic_notifier_register)(struct phy *phy, struct notifier_block *nb);
> > +
> > + /**
> > + * @atomic_notifier_unregister:
> > + *
> > + * Mandatoty if @atomic_notifier_register is set.
> > + *
> > + * Used to unregister a notifier block on PHY events. phy_init() must have
> > + * been called on the phy.
> > + *
> > + * Returns: 0 if successful, an negative error code otherwise
> > + */
> > + int (*atomic_notifier_unregister)(struct phy *phy, struct notifier_block *nb);
> > +
> > int (*reset)(struct phy *phy);
> > int (*calibrate)(struct phy *phy);
> > void (*release)(struct phy *phy);
> > @@ -234,6 +291,10 @@ int phy_set_speed(struct phy *phy, int speed);
> > int phy_configure(struct phy *phy, union phy_configure_opts *opts);
> > int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> > union phy_configure_opts *opts);
> > +int phy_get_status(struct phy *phy, union phy_status *status);
> > +int phy_atomic_notifier_register(struct phy *phy, struct notifier_block *nb);
> > +int phy_atomic_notifier_unregister(struct phy *phy, struct notifier_block *nb);
> > +
> >
> > static inline enum phy_mode phy_get_mode(struct phy *phy)
> > {
> > @@ -412,6 +473,32 @@ static inline int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> > return -ENOSYS;
> > }
> >
> > +static inline int phy_get_status(struct phy *phy, union phy_status *status)
> > +{
> > + if (!phy)
> > + return 0;
> > +
> > + return -ENOSYS;
> > +}
> > +
> > +static inline int phy_atomic_notifier_register(struct phy *phy,
> > + struct notifier_block *nb)
> > +{
> > + if (!phy)
> > + return 0;
> > +
> > + return -ENOSYS;
> > +}
> > +
> > +static inline int phy_atomic_notifier_unregister(struct phy *phy,
> > + struct notifier_block *nb)
> > +{
> > + if (!phy)
> > + return 0;
> > +
> > + return -ENOSYS;
> > +}
> > +
> > static inline int phy_get_bus_width(struct phy *phy)
> > {
> > return -ENOSYS;
> > --
> > 2.39.2
> >
> >
> > --
> > linux-phy mailing list
> > [email protected]
> > https://lists.infradead.org/mailman/listinfo/linux-phy
>
On Thu, Mar 23, 2023 at 11:31:50AM +0100, Herve Codina wrote:
> Hi,
>
> I have a system where I need to handle an HDLC interface.
>
> The HDLC data are transferred using a TDM bus on which a PEF2256 is
> present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
> This PEF2256 is also connected to a PowerQUICC SoC for the control path
> and the TDM is connected to the SoC (QMC component) for the data path.
>
> From the HDLC driver, I need to handle data using the QMC and carrier
> detection using the PEF2256 (E1 line carrier).
>
> The HDLC driver consider the PEF2256 as a generic PHY.
> So, the design is the following:
>
> +----------+ +-------------+ +---------+
> | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 |
> +----------+ +-------------+ | | <--> E1
> ^ +---------+ +---------+ | |
> +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | |
> +---------+ | PHY drv | +---------+
> +---------+
Hi Herver
Sorry, i'm late to the conversation. I'm looking at this from two
different perspectives. I help maintain Ethernet PHYs. And i have
hacked on the IDT 82P2288 E1/T1/J1 framer.
I think there is a block missing from this diagram. There appears to
be an MFD driver for the PEF2256? At least, i see an include for
linux/mfd/pef2256.h.
When i look at the 'phy' driver, i don't see anything a typical PHY
driver used for networking would have. A networking PHY driver often
has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
The equivalent here would be changing between E1, T1 and J1. It has
the ability to change the speed, 1G, 2.5G, 10G etc. This could be
implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
configured?
In fact, this PHY driver does not seem to do any configuration of any
sort on the framer. All it seems to be doing is take notification from
one chain and send them out another chain!
I also wounder if this get_status() call is sufficient. Don't you also
want Red, Yellow and Blue alarms? It is not just the carrier is down,
but why it is down.
Overall, i don't see why you want a PHY. What value does it add?
Andrew
Hi Andrew,
On Thu, 13 Apr 2023 14:48:30 +0200
Andrew Lunn <[email protected]> wrote:
> On Thu, Mar 23, 2023 at 11:31:50AM +0100, Herve Codina wrote:
> > Hi,
> >
> > I have a system where I need to handle an HDLC interface.
> >
> > The HDLC data are transferred using a TDM bus on which a PEF2256 is
> > present. The PEF2256 transfers data from/to the TDM bus to/from E1 line.
> > This PEF2256 is also connected to a PowerQUICC SoC for the control path
> > and the TDM is connected to the SoC (QMC component) for the data path.
> >
> > From the HDLC driver, I need to handle data using the QMC and carrier
> > detection using the PEF2256 (E1 line carrier).
> >
> > The HDLC driver consider the PEF2256 as a generic PHY.
> > So, the design is the following:
> >
> > +----------+ +-------------+ +---------+
> > | HDLC drv | <-data-> | QMC channel | <-- TDM --> | PEF2256 |
> > +----------+ +-------------+ | | <--> E1
> > ^ +---------+ +---------+ | |
> > +-> | Gen PHY | <-> | PEF2256 | <- local bus -> | |
> > +---------+ | PHY drv | +---------+
> > +---------+
>
> Hi Herver
>
> Sorry, i'm late to the conversation. I'm looking at this from two
> different perspectives. I help maintain Ethernet PHYs. And i have
> hacked on the IDT 82P2288 E1/T1/J1 framer.
>
> I think there is a block missing from this diagram. There appears to
> be an MFD driver for the PEF2256? At least, i see an include for
> linux/mfd/pef2256.h.
Indeed, there is the MFD driver and this MFD driver does the PEF2256
setup (line configuration, speed, ...).
>
> When i look at the 'phy' driver, i don't see anything a typical PHY
> driver used for networking would have. A networking PHY driver often
> has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
> The equivalent here would be changing between E1, T1 and J1. It has
> the ability to change the speed, 1G, 2.5G, 10G etc. This could be
> implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
> J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
> configured?
All of these are set by the MFD driver during its probe().
The expected setting come from several properties present in the pef2256
DT node. The binding can be found here:
https://lore.kernel.org/all/[email protected]/
Further more, the QMC HDLC is not the only PEF2256 consumer.
The PEF2256 is also used for audio path (ie audio over E1) and so the
configuration is shared between network and audio. The setting cannot be
handle by the network part as the PEF2256 must be available and correctly
configured even if the network part is not present.
>
> In fact, this PHY driver does not seem to do any configuration of any
> sort on the framer. All it seems to be doing is take notification from
> one chain and send them out another chain!
Configuration is done by the parent MFD driver.
The PHY driver has nothing more to do.
>
> I also wounder if this get_status() call is sufficient. Don't you also
> want Red, Yellow and Blue alarms? It is not just the carrier is down,
> but why it is down.
I don't need them in my use case but if needed can't they be added later?
Also, from the HDLC device point of view what can be done with these alarms?
>
> Overall, i don't see why you want a PHY. What value does it add?
I need to detect carrier on/off according to the E1 link state.
The HDLC driver is a driver for a QMC device.
The QMC device present in some PowerPC SOC offers the possibility to send
data over a TDM bus.
From the QMC HDLC driver I don't want to refer the PEF2256 as the driver has
nothing to do with the PEF2256 directly.
The QMC HDLC driver send data to a TDM bus using the QMC device.
The PEF2256 is an interface in the data path between the QMC output (TDM bus) and
the E1 line.
We send HDLC over E1 because there is this kind of interface but we would
have sent HDLC over anything else if this interface was different.
Using a PHY to represent this interface was coherent for me.
Using the generic PHY subsystem allows to abstract the specific provider (PEF2256)
from the consumer (QMC HDLC).
Best regards,
Hervé
> > When i look at the 'phy' driver, i don't see anything a typical PHY
> > driver used for networking would have. A networking PHY driver often
> > has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
> > The equivalent here would be changing between E1, T1 and J1. It has
> > the ability to change the speed, 1G, 2.5G, 10G etc. This could be
> > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
> > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
> > configured?
>
> All of these are set by the MFD driver during its probe().
> The expected setting come from several properties present in the pef2256
> DT node. The binding can be found here:
> https://lore.kernel.org/all/[email protected]/
I'm surprised to see so much in the binding. I assume you are familiar
with DAHDI. It allows nearly everything to be configured at
runtime. The systems i've used allow you to select the clock
configuration, line build out, user side vs networks side signalling
CRC4 enables or not, etc.
> Further more, the QMC HDLC is not the only PEF2256 consumer.
> The PEF2256 is also used for audio path (ie audio over E1) and so the
> configuration is shared between network and audio. The setting cannot be
> handle by the network part as the PEF2256 must be available and correctly
> configured even if the network part is not present.
But there is no reason why the MFD could not provide a generic PHY to
actually configure the 'PHY'. The HDLC driver can then also use the
generic PHY. It would make your generic PHY less 'pointless'. I'm not
saying it has to be this way, but it is an option.
> > In fact, this PHY driver does not seem to do any configuration of any
> > sort on the framer. All it seems to be doing is take notification from
> > one chain and send them out another chain!
>
> Configuration is done by the parent MFD driver.
> The PHY driver has nothing more to do.
>
> >
> > I also wounder if this get_status() call is sufficient. Don't you also
> > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > but why it is down.
>
> I don't need them in my use case but if needed can't they be added later?
> Also, from the HDLC device point of view what can be done with these alarms?
https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472
> Requests link state information. Link up/down flag (as provided by
> ``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended
> state might be provided as well. In general, extended state
> describes reasons for why a port is down, or why it operates in some
> non-obvious mode.
The colour of the Alarm gives you an idea which end of the system has
the problem.
> > Overall, i don't see why you want a PHY. What value does it add?
>
> I need to detect carrier on/off according to the E1 link state.
Why not just use the MFD notifier? What is the value of a PHY driver
translating one notifier into another?
And why is the notifier specific to the PEF2256? What would happen if
i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
82P2281? Would each have its own notifier? And hence each would need
its own PHY which translates one notifier into another?
There are enough E1/T1/J1 framers we should have a generic API between
the framer and the HDLC device.
Andrew
Hi Andrew
On Fri, 14 Apr 2023 18:15:30 +0200
Andrew Lunn <[email protected]> wrote:
> > > When i look at the 'phy' driver, i don't see anything a typical PHY
> > > driver used for networking would have. A networking PHY driver often
> > > has the ability to change between modes, like SGMII, QSGMII, 10GBASER.
> > > The equivalent here would be changing between E1, T1 and J1. It has
> > > the ability to change the speed, 1G, 2.5G, 10G etc. This could be
> > > implied via the mode, E1 is 2.048Mbps, T1 1.544Mbps, and i forget what
> > > J1 is. The PEF2256 also seems to support E1/T1/J1. How is its modes
> > > configured?
> >
> > All of these are set by the MFD driver during its probe().
> > The expected setting come from several properties present in the pef2256
> > DT node. The binding can be found here:
> > https://lore.kernel.org/all/[email protected]/
>
> I'm surprised to see so much in the binding. I assume you are familiar
> with DAHDI. It allows nearly everything to be configured at
> runtime. The systems i've used allow you to select the clock
> configuration, line build out, user side vs networks side signalling
> CRC4 enables or not, etc.
Well, I am not familiar with DAHDI at all.
I didn't even know about the DAHDI project.
The project seems to use specific kernel driver and I would like to avoid
these external drivers.
>
> > Further more, the QMC HDLC is not the only PEF2256 consumer.
> > The PEF2256 is also used for audio path (ie audio over E1) and so the
> > configuration is shared between network and audio. The setting cannot be
> > handle by the network part as the PEF2256 must be available and correctly
> > configured even if the network part is not present.
>
> But there is no reason why the MFD could not provide a generic PHY to
> actually configure the 'PHY'. The HDLC driver can then also use the
> generic PHY. It would make your generic PHY less 'pointless'. I'm not
> saying it has to be this way, but it is an option.
If the pef2256 PHY provides a configure function, who is going to call this
configure(). I mean the one calling the configure will be the configuration
owner. None of the MFD child can own the configuration as this configuration
will impact other children. So the MFD (top level node) owns the configuration.
>
> > > In fact, this PHY driver does not seem to do any configuration of any
> > > sort on the framer. All it seems to be doing is take notification from
> > > one chain and send them out another chain!
> >
> > Configuration is done by the parent MFD driver.
> > The PHY driver has nothing more to do.
> >
> > >
> > > I also wounder if this get_status() call is sufficient. Don't you also
> > > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > > but why it is down.
> >
> > I don't need them in my use case but if needed can't they be added later?
> > Also, from the HDLC device point of view what can be done with these alarms?
>
> https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472
Thanks for pointing this interface.
It is specific to ethtool but I can see the idea.
The 'get_status' I proposed could be extended later to provide more
information related to the colour of the alarm if needed.
ethtool and related interfaces are very well fitted with Ethernet and
related PHYs. I am not sure that ethtool will be usable for the pef2256.
>
> > Requests link state information. Link up/down flag (as provided by
> > ``ETHTOOL_GLINK`` ioctl command) is provided. Optionally, extended
> > state might be provided as well. In general, extended state
> > describes reasons for why a port is down, or why it operates in some
> > non-obvious mode.
>
> The colour of the Alarm gives you an idea which end of the system has
> the problem.
>
> > > Overall, i don't see why you want a PHY. What value does it add?
> >
> > I need to detect carrier on/off according to the E1 link state.
>
> Why not just use the MFD notifier? What is the value of a PHY driver
> translating one notifier into another?
I translated to another notifier to keep the core (MFD) pef2256 API
independent.
But indeed this could be changed.
If changed, the MFD pef2256 will have to handle the full struct
phy_status_basic as the translation will not be there anymore.
Right now, this structure is pretty simple and contains only the link state
flag. But in the future, this PHY structure can move to something more
complex and I am not sure that filling this struct is the MFD pef2256
responsibility. The PHY pef2256 is responsible for the correct structure
contents not sure that this should be moved to the MFD part.
>
> And why is the notifier specific to the PEF2256? What would happen if
> i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
> 82P2281? Would each have its own notifier? And hence each would need
> its own PHY which translates one notifier into another?
Each of them should have their own notifier if they can notify.
At least they will need their own notifier at they PHY driver level.
Having or not a translation from something else would depend on each device
PHY driver implementation.
Also, maybe some PHY will not be able to provide notifications but only
the get_status(). In this case, the notifier is not needed.
The proposed QMC HDLC driver handles this case switching to polling mode
if the PHY is not able to provide notification.
>
> There are enough E1/T1/J1 framers we should have a generic API between
> the framer and the HDLC device.
>
I agree.
I would like this first implementation without too much API restriction
in order to see how it goes.
The actual proposal imposes nothing on the PHY internal implementation.
the pef2256 implementation chooses to have two notifiers (one at MFD
level and one at PHY level) but it was not imposed by the API.
Best regards,
Hervé
> > I'm surprised to see so much in the binding. I assume you are familiar
> > with DAHDI. It allows nearly everything to be configured at
> > runtime. The systems i've used allow you to select the clock
> > configuration, line build out, user side vs networks side signalling
> > CRC4 enables or not, etc.
>
> Well, I am not familiar with DAHDI at all.
> I didn't even know about the DAHDI project.
> The project seems to use specific kernel driver and I would like to avoid
> these external drivers.
DAHDI is kind of the reference. Pretty much any Linux system being
used as a open source PBX runs Asterisk, and has the out of tree DAHDI
code to provide access to E1/T1 hardware and analogue POTS. I doubt it
will ever be merged into mainline, but i think it gives a good idea
what is needed to fully make use of such hardware.
I don't know what you application is. Are you using libpri for
signalling? How are you exposing the B channel to user space so that
libpri can use it?
> > > Further more, the QMC HDLC is not the only PEF2256 consumer.
> > > The PEF2256 is also used for audio path (ie audio over E1) and so the
> > > configuration is shared between network and audio. The setting cannot be
> > > handle by the network part as the PEF2256 must be available and correctly
> > > configured even if the network part is not present.
> >
> > But there is no reason why the MFD could not provide a generic PHY to
> > actually configure the 'PHY'. The HDLC driver can then also use the
> > generic PHY. It would make your generic PHY less 'pointless'. I'm not
> > saying it has to be this way, but it is an option.
>
> If the pef2256 PHY provides a configure function, who is going to call this
> configure(). I mean the one calling the configure will be the configuration
> owner. None of the MFD child can own the configuration as this configuration
> will impact other children. So the MFD (top level node) owns the configuration.
Fine. Nothing unusual there. The netdev owns the configuration for an
Ethernet device. The MAC driver passes a subset down to any generic
PHY being used to implement a SERDES.
You could have the same architecture here. The MFD implements a
standardised netlink API for configuring E1/T1/J1 devices. Part of it
gets passed to the framer, which could be part of a generic PHY. I
assume you also need to configure the HDLC hardware. It needs to know
if it is using the whole E1 channel in unframed mode, or it should do
a fractional link, using a subset of slots, or is just using one slot
for 64Kbps, which would be an ISDN B channel.
> > > > In fact, this PHY driver does not seem to do any configuration of any
> > > > sort on the framer. All it seems to be doing is take notification from
> > > > one chain and send them out another chain!
> > >
> > > Configuration is done by the parent MFD driver.
> > > The PHY driver has nothing more to do.
> > >
> > > >
> > > > I also wounder if this get_status() call is sufficient. Don't you also
> > > > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > > > but why it is down.
> > >
> > > I don't need them in my use case but if needed can't they be added later?
> > > Also, from the HDLC device point of view what can be done with these alarms?
> >
> > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472
>
> Thanks for pointing this interface.
> It is specific to ethtool but I can see the idea.
Don't equate ethtool with Ethernet. Any netdev can implement it, and a
HDLC device is a netdev. So it could well return link down reason.
> But indeed this could be changed.
> If changed, the MFD pef2256 will have to handle the full struct
> phy_status_basic as the translation will not be there anymore.
> Right now, this structure is pretty simple and contains only the link state
> flag. But in the future, this PHY structure can move to something more
> complex and I am not sure that filling this struct is the MFD pef2256
> responsibility. The PHY pef2256 is responsible for the correct structure
> contents not sure that this should be moved to the MFD part.
Framers, like Ethernet PHYs, are reasonably well defined, because
there are clear standards to follow. You could put the datasheets for
the various frames side by side and quickly get an idea of the common
properties. So you could define a structure now. In order to make it
extendable, just avoid 0 having any meaning other than UNKNOWN. If you
look at ethernet, SPEED_UNKNOWN is 0, DUPLEX_UNKNOWN is 0. So if a new
field is added, we know if a driver has not filled it in.
> > And why is the notifier specific to the PEF2256? What would happen if
> > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
> > 82P2281? Would each have its own notifier? And hence each would need
> > its own PHY which translates one notifier into another?
>
> Each of them should have their own notifier if they can notify.
I doubt you will find a framer which cannot report lost of framing. It
is too much a part of the standards. There are signalling actions you
need to do when a link goes does. So all framer drivers will have a
notifier.
> At least they will need their own notifier at they PHY driver level.
> Having or not a translation from something else would depend on each device
> PHY driver implementation.
Have you ever look at Ethernet PHYs? They all export the same API. You
can plug any Linux MAC driver into any Linux Ethernet PHY driver. It
should be the same here. You should be able to plug any HDLC driver
into any Framer. I should be able to take the same SoC you have
implementing the TDM interface, and plug it into the IDT framer i know
of. We want a standardised API between the HDLC device and the framer.
> I would like this first implementation without too much API restriction
> in order to see how it goes.
> The actual proposal imposes nothing on the PHY internal implementation.
> the pef2256 implementation chooses to have two notifiers (one at MFD
> level and one at PHY level) but it was not imposed by the API.
What i would like to see is some indication the API you are proposing
is generic, and could be implemented by multiple frames and HDLC
devices. The interface between the HDLC driver and the framer should
be generic. The HDLC driver has an abstract reference to a framer. The
framer has a reference to a netdev for the HDLC device.
You can keep this API very simple, just have link up/down
notification, since that is all you want at the moment. But the
implementation should give hints how it can be extended.
Andrew
On Mon, 17 Apr 2023 15:12:14 +0200
Andrew Lunn <[email protected]> wrote:
> > > I'm surprised to see so much in the binding. I assume you are familiar
> > > with DAHDI. It allows nearly everything to be configured at
> > > runtime. The systems i've used allow you to select the clock
> > > configuration, line build out, user side vs networks side signalling
> > > CRC4 enables or not, etc.
> >
> > Well, I am not familiar with DAHDI at all.
> > I didn't even know about the DAHDI project.
> > The project seems to use specific kernel driver and I would like to avoid
> > these external drivers.
>
> DAHDI is kind of the reference. Pretty much any Linux system being
> used as a open source PBX runs Asterisk, and has the out of tree DAHDI
> code to provide access to E1/T1 hardware and analogue POTS. I doubt it
> will ever be merged into mainline, but i think it gives a good idea
> what is needed to fully make use of such hardware.
>
> I don't know what you application is. Are you using libpri for
> signalling? How are you exposing the B channel to user space so that
> libpri can use it?
My application (probably a subset of what we can do with E1 lines) does
not use signaling.
I don't expose any channel to the user space but just:
- One hdlc interface using one timeslot.
- One or more dai links (audio channels) that can be used using ALSA library.
>
> > > > Further more, the QMC HDLC is not the only PEF2256 consumer.
> > > > The PEF2256 is also used for audio path (ie audio over E1) and so the
> > > > configuration is shared between network and audio. The setting cannot be
> > > > handle by the network part as the PEF2256 must be available and correctly
> > > > configured even if the network part is not present.
> > >
> > > But there is no reason why the MFD could not provide a generic PHY to
> > > actually configure the 'PHY'. The HDLC driver can then also use the
> > > generic PHY. It would make your generic PHY less 'pointless'. I'm not
> > > saying it has to be this way, but it is an option.
> >
> > If the pef2256 PHY provides a configure function, who is going to call this
> > configure(). I mean the one calling the configure will be the configuration
> > owner. None of the MFD child can own the configuration as this configuration
> > will impact other children. So the MFD (top level node) owns the configuration.
>
> Fine. Nothing unusual there. The netdev owns the configuration for an
> Ethernet device. The MAC driver passes a subset down to any generic
> PHY being used to implement a SERDES.
>
> You could have the same architecture here. The MFD implements a
> standardised netlink API for configuring E1/T1/J1 devices. Part of it
> gets passed to the framer, which could be part of a generic PHY. I
> assume you also need to configure the HDLC hardware. It needs to know
> if it is using the whole E1 channel in unframed mode, or it should do
> a fractional link, using a subset of slots, or is just using one slot
> for 64Kbps, which would be an ISDN B channel.
The HDLC driver uses a QMC channel and the DT binding related to this
QMC channel defines the timeslots used by this channel.
From the QMC HDLC nothing is configured. This is done at the QMC level.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml#n91
>
> > > > > In fact, this PHY driver does not seem to do any configuration of any
> > > > > sort on the framer. All it seems to be doing is take notification from
> > > > > one chain and send them out another chain!
> > > >
> > > > Configuration is done by the parent MFD driver.
> > > > The PHY driver has nothing more to do.
> > > >
> > > > >
> > > > > I also wounder if this get_status() call is sufficient. Don't you also
> > > > > want Red, Yellow and Blue alarms? It is not just the carrier is down,
> > > > > but why it is down.
> > > >
> > > > I don't need them in my use case but if needed can't they be added later?
> > > > Also, from the HDLC device point of view what can be done with these alarms?
> > >
> > > https://elixir.bootlin.com/linux/latest/source/Documentation/networking/ethtool-netlink.rst#L472
> >
> > Thanks for pointing this interface.
> > It is specific to ethtool but I can see the idea.
>
> Don't equate ethtool with Ethernet. Any netdev can implement it, and a
> HDLC device is a netdev. So it could well return link down reason.
Ok.
So the link down reason should be returned by the newly introduced QMC HDLC.
Adding this in the generic PHY infrastructure I used (drivers/phy) is adding
some specific netdev stuff in this subsystem.
Don't forget that the pef2256 can be used without any network part by the
audio subsystem in order to have the user audio data sent to E1 using pure
ALSA path.
>
> > But indeed this could be changed.
> > If changed, the MFD pef2256 will have to handle the full struct
> > phy_status_basic as the translation will not be there anymore.
> > Right now, this structure is pretty simple and contains only the link state
> > flag. But in the future, this PHY structure can move to something more
> > complex and I am not sure that filling this struct is the MFD pef2256
> > responsibility. The PHY pef2256 is responsible for the correct structure
> > contents not sure that this should be moved to the MFD part.
>
> Framers, like Ethernet PHYs, are reasonably well defined, because
> there are clear standards to follow. You could put the datasheets for
> the various frames side by side and quickly get an idea of the common
> properties. So you could define a structure now. In order to make it
> extendable, just avoid 0 having any meaning other than UNKNOWN. If you
> look at ethernet, SPEED_UNKNOWN is 0, DUPLEX_UNKNOWN is 0. So if a new
> field is added, we know if a driver has not filled it in.
>
> > > And why is the notifier specific to the PEF2256? What would happen if
> > > i used a analog devices DS2155, DS21Q55, and DS2156, or the IDT
> > > 82P2281? Would each have its own notifier? And hence each would need
> > > its own PHY which translates one notifier into another?
> >
> > Each of them should have their own notifier if they can notify.
>
> I doubt you will find a framer which cannot report lost of framing. It
> is too much a part of the standards. There are signalling actions you
> need to do when a link goes does. So all framer drivers will have a
> notifier.
>
> > At least they will need their own notifier at they PHY driver level.
> > Having or not a translation from something else would depend on each device
> > PHY driver implementation.
>
> Have you ever look at Ethernet PHYs? They all export the same API. You
> can plug any Linux MAC driver into any Linux Ethernet PHY driver. It
> should be the same here. You should be able to plug any HDLC driver
> into any Framer. I should be able to take the same SoC you have
> implementing the TDM interface, and plug it into the IDT framer i know
> of. We want a standardised API between the HDLC device and the framer.
Can you tell me more.
I thought I was providing a "standardised" API between the HDLC device
and the framer. Maybe it was not as complete as you could expect but I had
the feeling that it was standardised.
Right now it is just a link state notification provided by a simple 'basic
PHY'. Any HDLC device that uses a 'basic PHY' can be notified of any changes
of this link state provided by the PHY without knowing what is the exact
PHY behind this 'basic phy'.
>
> > I would like this first implementation without too much API restriction
> > in order to see how it goes.
> > The actual proposal imposes nothing on the PHY internal implementation.
> > the pef2256 implementation chooses to have two notifiers (one at MFD
> > level and one at PHY level) but it was not imposed by the API.
>
> What i would like to see is some indication the API you are proposing
> is generic, and could be implemented by multiple frames and HDLC
> devices. The interface between the HDLC driver and the framer should
> be generic. The HDLC driver has an abstract reference to a framer. The
> framer has a reference to a netdev for the HDLC device.
Here, I do not fully understand.
Why does the framer need a reference to the netdev ?
>
> You can keep this API very simple, just have link up/down
> notification, since that is all you want at the moment. But the
> implementation should give hints how it can be extended.
>
Best regards,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
> My application (probably a subset of what we can do with E1 lines) does
> not use signaling.
>
> I don't expose any channel to the user space but just:
> - One hdlc interface using one timeslot.
> - One or more dai links (audio channels) that can be used using ALSA library.
Well, that obviously does what you need for you application, but it
does not sound very generic. I don't think you could build a PBX on
top of that. I'd think you can build an E1 trunk to VoIP gateway. So
i also have to wounder how generic vs one specific use case your
building blocks are.
Looking at the binding, i assume you could in fact instantiate 32 hdlc
instances? And that 'transparent' actually means DAI? So you can
instantiate as many DAI as you want. If you wanted to implement
signalling, a third type could be added?
> I thought I was providing a "standardised" API between the HDLC device
> and the framer. Maybe it was not as complete as you could expect but I had
> the feeling that it was standardised.
You are abusing the Generic PHY interface, for something which is not
a PHY. Your PHY driver does nothing a PHY driver normally does,
because you keep arguing the MFD does it all. So to me this is the
wrong abstraction.
You need an abstraction of a framer. You do however want a similar API
to generic PHY. devm_of_framer_optional_get() for the consumer of a
framer, which looks in DT for a phandle to a device.
devm_framer_create() which registers a framer provider, your MFD, with
the framer core. The hdlc driver can then get an instance of the
framer, and either put a notifier call block on its chain, or register
a function to be called when there is change in status.
What i also don't like is you have a very static configuration. You
are putting configuration into DT, which i'm surprised Rob Herring and
Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware
property, it is purely software configuration, so should not be in DT.
So there should be a mechanism to configure how slots are used. You
can then dynamically instantiate HDLC interfaces and DAI links. This
is something which should be in the framer core. But since you have
managed to get this binding accepted, you can skip this. But
implementing the basic framer abstraction will give a place holder for
somebody to implement a much more generic solution in the future.
Andrew
Hi Andrew,
On Mon, 17 Apr 2023 19:22:26 +0200
Andrew Lunn <[email protected]> wrote:
> > My application (probably a subset of what we can do with E1 lines) does
> > not use signaling.
> >
> > I don't expose any channel to the user space but just:
> > - One hdlc interface using one timeslot.
> > - One or more dai links (audio channels) that can be used using ALSA library.
>
> Well, that obviously does what you need for you application, but it
> does not sound very generic. I don't think you could build a PBX on
> top of that. I'd think you can build an E1 trunk to VoIP gateway. So
> i also have to wounder how generic vs one specific use case your
> building blocks are.
>
> Looking at the binding, i assume you could in fact instantiate 32 hdlc
> instances? And that 'transparent' actually means DAI? So you can
> instantiate as many DAI as you want. If you wanted to implement
> signalling, a third type could be added?
>
> > I thought I was providing a "standardised" API between the HDLC device
> > and the framer. Maybe it was not as complete as you could expect but I had
> > the feeling that it was standardised.
>
> You are abusing the Generic PHY interface, for something which is not
> a PHY. Your PHY driver does nothing a PHY driver normally does,
> because you keep arguing the MFD does it all. So to me this is the
> wrong abstraction.
>
> You need an abstraction of a framer. You do however want a similar API
> to generic PHY. devm_of_framer_optional_get() for the consumer of a
> framer, which looks in DT for a phandle to a device.
> devm_framer_create() which registers a framer provider, your MFD, with
> the framer core. The hdlc driver can then get an instance of the
> framer, and either put a notifier call block on its chain, or register
> a function to be called when there is change in status.
>
> What i also don't like is you have a very static configuration. You
> are putting configuration into DT, which i'm surprised Rob Herring and
> Krzysztof Kozlowski accepted. How you use E1 slots is not a hardware
> property, it is purely software configuration, so should not be in DT.
> So there should be a mechanism to configure how slots are used. You
> can then dynamically instantiate HDLC interfaces and DAI links. This
> is something which should be in the framer core. But since you have
> managed to get this binding accepted, you can skip this. But
> implementing the basic framer abstraction will give a place holder for
> somebody to implement a much more generic solution in the future.
>
> Andrew
I can move to a basic framer abstraction as you suggested. At least:
- devm_of_framer_optional_get()
- devm_framer_create()
- framer_notifier_register() or something similar.
Where do you expect to see this framer abstraction and the pef2256
framer part ?
driver/net/wan/framer/, other place ?
I think driver/net/wan/framer/ can be a good place to start as only HDLC
will use this abstraction.
I can use the framer abstraction from the QMC HDLC driver itself or try
to move it to the HDLC core. Do you think it will be interesting to have
it move to the HDLC core ?
Best regards,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
> I can move to a basic framer abstraction as you suggested. At least:
> - devm_of_framer_optional_get()
> - devm_framer_create()
> - framer_notifier_register() or something similar.
>
> Where do you expect to see this framer abstraction and the pef2256
> framer part ?
> driver/net/wan/framer/, other place ?
That seems like a good location.
> I think driver/net/wan/framer/ can be a good place to start as only HDLC
> will use this abstraction.
> I can use the framer abstraction from the QMC HDLC driver itself or try
> to move it to the HDLC core. Do you think it will be interesting to have
> it move to the HDLC core ?
Having it in the core would be nice. But i don't know that code, so i
cannot say how easy/hard it will be do to. hdlc.c already seems to
have some code for carrier. So you need to be careful not to break
that.
Andrew