Re: [PATCH v3 0/8] arm64: rockchip: Initial GeekBox enablement
Hi Andreas, Am Montag, 7. März 2016, 13:17:54 schrieb Andreas Färber: > Am 06.03.2016 um 20:53 schrieb Andreas Färber: > > On next-20160304 the GMAC seems to have regressed, it no longer finds the > > PHY: > > > > libphy: PHY stmmac-0: not found > > eth0: Could not attach to PHY > > stmmac_open: Cannot attach to PHY (error: -19) > > Still with us in next-20160307. CC'ing stmmac maintainers. do you remember from what revision you rebased away from, when it was still working (as a known-good state)? Heiko > > http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/drivers/ > net/ethernet/stmicro > > Regards, > Andreas
Re: [PATCH 3/6] net: arc_emac: support the phy reset for emac driver
Hi Caesar,
Am Freitag, 11. März 2016, 16:47:59 schrieb Sergei Shtylyov:
> On 3/11/2016 1:55 PM, Caesar Wang wrote:
> > This patch adds to support the emac phy reset.
> >
> > 1) phy-reset-gpios:
> > The phy-reset-gpios is an optional property for arc emac device tree boot.
> > Change the binding document to match the driver code.
> >
> > 2) phy-reset-duration:
> > Different boards may require different phy reset duration. Add property
> > phy-reset-duration for device tree probe, so that the boards that need
> > a longer reset duration can specify it in their device tree.
> >
> > 3) phy-reset-active-high:
> > We need that for a custom hardware that needs the reverse reset sequence.
> >
> > Of course, this patch will fix the issue on
> > https://patchwork.kernel.org/patch/8186801/.
> >
> > In some cases, the emac couldn't work if you don't have reset the phy.
> > Let's add it to happy work.
> >
> > Signed-off-by: Caesar Wang
> > ---
> >
> > drivers/net/ethernet/arc/emac_main.c | 41
> > 1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/arc/emac_main.c
> > b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644
> > --- a/drivers/net/ethernet/arc/emac_main.c
> > +++ b/drivers/net/ethernet/arc/emac_main.c
> > @@ -764,6 +764,45 @@ static const struct net_device_ops
> > arc_emac_netdev_ops = {>
> > #endif
> > };
> >
> > +#ifdef CONFIG_OF
> > +static void emac_reset_phy(struct net_device *pdev)
> > +{
> > + int err, phy_reset;
> > + bool active_high = false;
> > + int msec = 10;
> > + struct device *dev = pdev->dev.parent;
> > + struct device_node *np = dev->of_node;
> > +
> > + if (!np)
> > + return;
> > +
> > + of_property_read_u32(np, "phy-reset-duration", &msec);
> > + /* A sane reset duration should not be longer than 1s */
> > + if (msec > 1000)
> > + msec = 1;
> > +
> > + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> > + if (!gpio_is_valid(phy_reset))
> > + return;
> > +
> > + active_high = of_property_read_bool(np, "phy-reset-active-high");
> > +
> > + err = devm_gpio_request_one(dev, phy_reset, active_high ?
> > + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > + "phy-reset");
> > + if (err) {
> > + dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);
> > + return;
> > + }
> > + msleep(msec);
> > + gpio_set_value_cansleep(phy_reset, !active_high);
> > +}
>
> [...]
>
> Why not make it the mii_bus::reset() method? It gets called before the
> MDIO bus scan.
while meddling with the emac, I've built some sort of preliminary variant of
Sergei's suggestion at [0] - maybe you could take a look for some sort of
inspiration ;-)
The code is lifted from the designware gmac driver, so the binding also is
similar.
Heiko
[0]
https://github.com/mmind/linux-rockchip/commit/a943c588783438ff1c508dfa8c79f1709aa5775e
Re: [PATCH 3/6] net: arc_emac: support the phy reset for emac driver
Am Freitag, 11. März 2016, 15:56:04 schrieb Heiko Stübner:
> Hi Caesar,
>
> Am Freitag, 11. März 2016, 16:47:59 schrieb Sergei Shtylyov:
> > On 3/11/2016 1:55 PM, Caesar Wang wrote:
> > > This patch adds to support the emac phy reset.
> > >
> > > 1) phy-reset-gpios:
> > > The phy-reset-gpios is an optional property for arc emac device tree
> > > boot.
> > > Change the binding document to match the driver code.
> > >
> > > 2) phy-reset-duration:
> > > Different boards may require different phy reset duration. Add property
> > > phy-reset-duration for device tree probe, so that the boards that need
> > > a longer reset duration can specify it in their device tree.
> > >
> > > 3) phy-reset-active-high:
> > > We need that for a custom hardware that needs the reverse reset
> > > sequence.
> > >
> > > Of course, this patch will fix the issue on
> > > https://patchwork.kernel.org/patch/8186801/.
> > >
> > > In some cases, the emac couldn't work if you don't have reset the phy.
> > > Let's add it to happy work.
> > >
> > > Signed-off-by: Caesar Wang
> > > ---
> > >
> > > drivers/net/ethernet/arc/emac_main.c | 41
> > > 1 file changed, 41 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/arc/emac_main.c
> > > b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644
> > > --- a/drivers/net/ethernet/arc/emac_main.c
> > > +++ b/drivers/net/ethernet/arc/emac_main.c
> > > @@ -764,6 +764,45 @@ static const struct net_device_ops
> > > arc_emac_netdev_ops = {>
> > >
> > > #endif
> > > };
> > >
> > > +#ifdef CONFIG_OF
> > > +static void emac_reset_phy(struct net_device *pdev)
> > > +{
> > > + int err, phy_reset;
> > > + bool active_high = false;
> > > + int msec = 10;
> > > + struct device *dev = pdev->dev.parent;
> > > + struct device_node *np = dev->of_node;
> > > +
> > > + if (!np)
> > > + return;
> > > +
> > > + of_property_read_u32(np, "phy-reset-duration", &msec);
> > > + /* A sane reset duration should not be longer than 1s */
> > > + if (msec > 1000)
> > > + msec = 1;
> > > +
> > > + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> > > + if (!gpio_is_valid(phy_reset))
> > > + return;
> > > +
> > > + active_high = of_property_read_bool(np, "phy-reset-active-high");
> > > +
> > > + err = devm_gpio_request_one(dev, phy_reset, active_high ?
> > > + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > + "phy-reset");
> > > + if (err) {
> > > + dev_err(dev, "failed to get phy-reset-gpios: %d\n", err);
> > > + return;
> > > + }
> > > + msleep(msec);
> > > + gpio_set_value_cansleep(phy_reset, !active_high);
> > > +}
> >
> > [...]
> >
> > Why not make it the mii_bus::reset() method? It gets called before the
> >
> > MDIO bus scan.
>
> while meddling with the emac, I've built some sort of preliminary variant of
> Sergei's suggestion at [0] - maybe you could take a look for some sort of
> inspiration ;-)
forgot to add ... of course the newer gpiod_* infrastructure should be used
instead of the old integer-based gpio numbers
>
> The code is lifted from the designware gmac driver, so the binding also is
> similar.
>
>
> Heiko
>
>
> [0]
> https://github.com/mmind/linux-rockchip/commit/a943c588783438ff1c508dfa8c79
> f1709aa5775e
Re: [PATCH] net: stmmac: dwmac-rk: keep PHY up for WoL
Hi, Am Freitag, 3. Juni 2016, 10:29:20 schrieb Vincent Palatin: > Do not shutdown the PHY if Wake-on-Lan is enabled, else it cannot wake > us up. > > Signed-off-by: Vincent Palatin > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..2e45e75 > 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -534,6 +534,10 @@ static int rk_gmac_init(struct platform_device *pdev, > void *priv) struct rk_priv_data *bsp_priv = priv; > int ret; > > + /* Keep the PHY up if we use Wake-on-Lan. */ > + if (device_may_wakeup(&pdev->dev)) > + return 0; > + Hmm, this looks like it would also block the initial setup of clocks and phy? platform_device + device struct are created before probe gets called, so something could set the wakeup flag before the driver initially probes? Confused, Heiko
Re: [PATCH] net: stmmac: dwmac-rk: add rk322x-specific data
Am Dienstag, 21. Juni 2016, 15:13:55 schrieb Xing Zheng:
> Add constants and callback functions for the dwmac on rk322x socs.
> As can be seen, the base structure is the same, only registers and
> the bits in them moved slightly.
>
> Signed-off-by: Xing Zheng
> ---
>
> .../devicetree/bindings/net/rockchip-dwmac.txt |3 +-
> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 117
> 2 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt index
> 93eac7c..5040ed4 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> @@ -3,7 +3,8 @@ Rockchip SoC RK3288 10/100/1000 Ethernet driver(GMAC)
> The device node has following properties.
>
> Required properties:
> - - compatible: Can be one of "rockchip,rk3288-gmac", "rockchip,rk3368-gmac"
> + - compatible: Can be one of "rockchip,rk322x-gmac",
devicetree names are normally expected to be real, aka no "x" as catchall. So
I guess either just add compatibles for both the rk3228 and rk3229 which point
to the same structure in the driver. (So driver-side can stay as it is below,
just add a second compatible).
> "rockchip,rk3288-gmac", +
> "rockchip,rk3368-gmac"
> - reg: addresses and length of the register sets for the device.
> - interrupts: Should contain the GMAC interrupts.
> - interrupt-names: Should contain the interrupt names "macirq".
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..7f045db
> 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -72,6 +72,122 @@ struct rk_priv_data {
> #define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
> #define GRF_CLR_BIT(nr) (BIT(nr+16))
>
> +#define RK322X_GRF_MAC_CON0 0x0900
> +#define RK322X_GRF_MAC_CON1 0x0904
> +
> +/* RK322X_GRF_MAC_CON0 */
> +#define RK322X_GMAC_CLK_RX_DL_CFG(val) HIWORD_UPDATE(val, 0x7F, 7)
> +#define RK322X_GMAC_CLK_TX_DL_CFG(val) HIWORD_UPDATE(val, 0x7F, 0)
> +
> +/* RK322X_GRF_MAC_CON1 */
> +#define RK322X_GMAC_PHY_INTF_SEL_RGMII \
> + (GRF_BIT(4) | GRF_CLR_BIT(5) | GRF_CLR_BIT(6))
> +#define RK322X_GMAC_PHY_INTF_SEL_RMII\
> + (GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | GRF_BIT(6))
> +#define RK322X_GMAC_FLOW_CTRLGRF_BIT(3)
> +#define RK322X_GMAC_FLOW_CTRL_CLRGRF_CLR_BIT(3)
> +#define RK322X_GMAC_SPEED_10MGRF_CLR_BIT(2)
> +#define RK322X_GMAC_SPEED_100M GRF_BIT(2)
> +#define RK322X_GMAC_RMII_CLK_25M GRF_BIT(7)
> +#define RK322X_GMAC_RMII_CLK_2_5MGRF_CLR_BIT(7)
> +#define RK322X_GMAC_CLK_125M (GRF_CLR_BIT(8) | GRF_CLR_BIT(9))
> +#define RK322X_GMAC_CLK_25M (GRF_BIT(8) | GRF_BIT(9))
> +#define RK322X_GMAC_CLK_2_5M (GRF_CLR_BIT(8) | GRF_BIT(9))
> +#define RK322X_GMAC_RMII_MODEGRF_BIT(10)
> +#define RK322X_GMAC_RMII_MODE_CLRGRF_CLR_BIT(10)
> +#define RK322X_GMAC_TXCLK_DLY_ENABLE GRF_BIT(0)
> +#define RK322X_GMAC_TXCLK_DLY_DISABLEGRF_CLR_BIT(0)
> +#define RK322X_GMAC_RXCLK_DLY_ENABLE GRF_BIT(1)
> +#define RK322X_GMAC_RXCLK_DLY_DISABLEGRF_CLR_BIT(1)
> +
> +static void rk322x_set_to_rgmii(struct rk_priv_data *bsp_priv,
> + int tx_delay, int rx_delay)
> +{
> + struct device *dev = &bsp_priv->pdev->dev;
> +
> + if (IS_ERR(bsp_priv->grf)) {
> + dev_err(dev, "Missing rockchip,grf property\n");
> + return;
> + }
> +
> + regmap_write(bsp_priv->grf, RK322X_GRF_MAC_CON1,
> + RK322X_GMAC_PHY_INTF_SEL_RGMII |
> + RK322X_GMAC_RMII_MODE_CLR |
> + RK322X_GMAC_RXCLK_DLY_ENABLE |
> + RK322X_GMAC_TXCLK_DLY_ENABLE);
> +
> + regmap_write(bsp_priv->grf, RK322X_GRF_MAC_CON0,
> + RK322X_GMAC_CLK_RX_DL_CFG(rx_delay) |
> + RK322X_GMAC_CLK_TX_DL_CFG(tx_delay));
> +}
> +
> +static void rk322x_set_to_rmii(struct rk_priv_data *bsp_priv)
> +{
> + struct device *dev = &bsp_priv->pdev->dev;
> +
> + if (IS_ERR(bsp_priv->grf)) {
> + dev_err(dev, "Missing rockchip,grf property\n");
> + return;
> + }
> +
> + regmap_write(bsp_priv->grf, RK322X_GRF_MAC_CON1,
> + RK322X_GMAC_PHY_INTF_SEL_RMII |
> + RK322X_GMAC_RMII_MODE);
> +
> + /* set MAC to RMII mode */
> + regmap_write(bsp_priv->grf, RK322X_GRF_MAC_CON1, GRF_BIT(11));
> +}
> +
> +static void rk322x_set_rgmii_speed(struct rk_priv_data *bsp_priv, int
> speed) +{
> + struct device *dev = &bsp_priv->pdev->dev;
> +
> + if (IS_ERR(bsp_priv->grf)) {
> + dev_err(dev, "Missing rockchip,grf property\n");
> + return;
> + }
Re: [PATCH v2] net: stmmac: dwmac-rk: add rk3228-specific data
Am Dienstag, 21. Juni 2016, 20:33:28 schrieb Xing Zheng: > Add constants and callback functions for the dwmac on rk3228/rk3229 socs. > As can be seen, the base structure is the same, only registers and the > bits in them moved slightly. > > Signed-off-by: Xing Zheng Reviewed-by: Heiko Stuebner
Re: [RESEND PATCH v1 0/4] Add support emac for the RK3036 SoC platform
Hi Dave, Am Dienstag, 29. Dezember 2015, 15:53:14 schrieb David Miller: > You have to submit this series properly, the same problem happend twice > now. > > When you submit a series you should: > > 1) Make it clear which tree you expect these changes to be applied >to. Here it is completely ambiguous, do you want it to go into >my networking tree or some other subsystem tree? > > 2) You MUST keep all parties informed about all patches for a series >like this. That means you cannot drop netdev from patch #4 as >you did both times. Doing this aggravates the situation for >#1 even more, because if a patch is not CC:'d to netdev it does >not enter patchwork. And if it doesn't go into patchwork, I'm >not looking at it. I guess that is some unfortunate result of git send-email combined with get_maintainer.pl . In general I also prefer to see the whole series, but have gotten such partial series from other maintainers as well in the past, so it seems to be depending on preferences somewhat. For the series at hand, the 4th patch is the devicetree addition, which the expected way is me picking it up, after you are comfortable with the code- related changes. Heiko -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v1 3/4] net: ethernet: arc: Add support emac for RK3036
Am Dienstag, 29. Dezember 2015, 14:59:59 schrieb Florian Fainelli:
> On December 27, 2015 11:22:20 PM PST, Xing Zheng
wrote:
> >The RK3036's GRFs offset are different with RK3066/RK3188, and need to
> >set
> >mac TX/RX clock before probe emac.
> >
> >Signed-off-by: Xing Zheng
> >---
>
>
>
> > };
> >
> > static const struct of_device_id emac_rockchip_dt_ids[] = {
> >
> >-{ .compatible = "rockchip,rk3066-emac", .data =
> >&emac_rockchip_dt_data[0] },
> >-{ .compatible = "rockchip,rk3188-emac", .data =
> >&emac_rockchip_dt_data[1] },
> >+{ .compatible = "rockchip,rk3036-emac", .data =
> >&emac_rockchip_dt_data[0] },
> >+{ .compatible = "rockchip,rk3066-emac", .data =
> >&emac_rockchip_dt_data[1] },
> >+{ .compatible = "rockchip,rk3188-emac", .data =
> >&emac_rockchip_dt_data[2] },
> >
> > { /* Sentinel */ }
>
> Food for thought, you might want to use an enum here to index
> emac_rockchip_dt_data which would be less error prone if you add/remove
> entries in this structure.
Or just have the structs separately and not in array-form at all, aka
rk3066_emac_dt_data, rk3036_emac_dt_data.
I don't think the original array really improves anything.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] arm64: dts: rockchip: support gmac for rk3399
Am Mittwoch, 31. August 2016, 04:30:06 schrieb Caesar Wang:
> This patch adds needed gamc information for rk3399,
> also support the gmac pd.
>
> Signed-off-by: Roger Chen
> Signed-off-by: Caesar Wang
> ---
>
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 90
> 1 file changed, 90 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index 32aebc8..53ac651 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -200,6 +200,26 @@
> };
> };
>
> + gmac: eth@fe30 {
> + compatible = "rockchip,rk3399-gmac";
> + reg = <0x0 0xfe30 0x0 0x1>;
> + rockchip,grf = <&grf>;
should move below the reset-names .
> + interrupts = ;
> + interrupt-names = "macirq";
> + clocks = <&cru SCLK_MAC>, <&cru SCLK_MAC_RX>,
> + <&cru SCLK_MAC_TX>, <&cru SCLK_MACREF>,
> + <&cru SCLK_MACREF_OUT>, <&cru ACLK_GMAC>,
> + <&cru PCLK_GMAC>;
> + clock-names = "stmmaceth", "mac_clk_rx",
> + "mac_clk_tx", "clk_mac_ref",
> + "clk_mac_refout", "aclk_mac",
> + "pclk_mac";
> + resets = <&cru SRST_A_GMAC>;
> + reset-names = "stmmaceth";
> + power-domains = <&power RK3399_PD_GMAC>;
The driver core should handle regular power-domain handling on device creation
already, right? So I should be able to apply patches 3 and 4 even without the
dwmac patches, right?
Also if resending please move power-domains above resets
Heiko
Re: [RESEND PATCH 3/4] arm64: dts: rockchip: support gmac for rk3399
Am Mittwoch, 31. August 2016, 13:42:17 schrieb Doug Anderson:
> Caesar,
>
> On Tue, Aug 30, 2016 at 11:13 PM, Caesar Wang wrote:
> > This patch adds needed gamc information for rk3399,
> > also support the gmac pd.
> >
> > Signed-off-by: Roger Chen
> > Signed-off-by: Caesar Wang
> > ---
> >
> > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 90
> > 1 file changed, 90 insertions(+)
>
> I noticed that your subject for this patch contains "RESEND" and not
> "v2" event though there are changes between this version and the last
> one. That's really confusing. This should have been "v2" and the
> next version should be "v3".
>
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index 32aebc8..abf27a4 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> > @@ -200,6 +200,26 @@
> >
> > };
> >
> > };
> >
> > + gmac: eth@fe30 {
>
> nit: on rk3288 the node was "ethernet@" instead of "eth@". Presumably
> "ethernet" is more correct?
>
> > + compatible = "rockchip,rk3399-gmac";
> > + reg = <0x0 0xfe30 0x0 0x1>;
> > + interrupts = ;
> > + interrupt-names = "macirq";
> > + clocks = <&cru SCLK_MAC>, <&cru SCLK_MAC_RX>,
> > +<&cru SCLK_MAC_TX>, <&cru SCLK_MACREF>,
> > +<&cru SCLK_MACREF_OUT>, <&cru ACLK_GMAC>,
> > +<&cru PCLK_GMAC>;
> > + clock-names = "stmmaceth", "mac_clk_rx",
> > + "mac_clk_tx", "clk_mac_ref",
> > + "clk_mac_refout", "aclk_mac",
> > + "pclk_mac";
> > + power-domains = <&power RK3399_PD_GMAC>;
> > + resets = <&cru SRST_A_GMAC>;
> > + reset-names = "stmmaceth";
> > + rockchip,grf = <&grf>;
> > + status = "disabled";
> > + };
> > +
> >
> > sdio0: dwmmc@fe31 {
> >
> > compatible = "rockchip,rk3399-dw-mshc",
> >
> > "rockchip,rk3288-dw-mshc";
> >
> > @@ -611,6 +631,11 @@
> >
> > status = "disabled";
> >
> > };
> >
> > + qos_gmac: qos@ffa5c000 {
> > + compatible = "syscon";
> > + reg = <0x0 0xffa5c000 0x0 0x20>;
> > + };
> > +
> >
> > qos_hdcp: qos@ffa9 {
> >
> > compatible = "syscon";
> > reg = <0x0 0xffa9 0x0 0x20>;
> >
> > @@ -704,6 +729,11 @@
> >
> > #size-cells = <0>;
> >
> > /* These power domains are grouped by VD_CENTER */
> >
> > + pd_gmac@RK3399_PD_GMAC {
>
> RK3399_PD_GMAC is not in VD_CENTER but in VD_LOGIC, right? ...so this
> should move.
>
> > + reg = ;
> > + clocks = <&cru ACLK_GMAC>;
> > + pm_qos = <&qos_gmac>;
> > + };
>
> IMHO it would be nice if this were broken into two patches.
>
> 1. First patch would be the power domain patch and that could land any
> time. You wouldn't actually be able to use the gmac but at least
> you'd be able to turn off its power. This would be a handy patch to
> be able to backport if you happened to not need Ethernet support but
> wanted to save power.
>
> 2. Second patch would actually add the gmac.
according to my talk with Caesar in the real v1, the gmac even with power-
domains should work just nicely even without the dts patches, as the driver
core takes care of powering up the pd before probe.
But I may miss some peculiarity of the dwmac?
Heiko
Re: [PATCH 0/3] support GMAC for RK3399 & RK3366
Hi Roger, Caesar, Am Donnerstag, 11. August 2016, 15:24:45 schrieb Roger Chen: > This series adds registers description for RK3366 & RK3399 GMAC. > And also DT nodes for RK3399 SoC and EVB. > > Roger Chen (3): > net: stmmac: dwmac-rk: add rk3366 & rk3399 specific data > arm64: dts: rockchip: add GMAC dt nodes for rk3399 SoC > arm64: dts: rockchip: add GMAC dt nodes for RK3399 evb can you please respin this series addressing the comments from patch 1? Thanks Heiko
Re: [PATCH v3 3/5] arm64: dts: rockchip: add the gmac power domain on rk3399
Hi Caesar,
Am Donnerstag, 1. September 2016, 07:28:50 schrieb Caesar Wang:
> This patch supports the gmac pd to save power consumption.
> Even though some boards not need Ethernet support, the driver
> core can also take care of powering up the pd before probe.
>
> Signed-off-by: Roger Chen
> Signed-off-by: Caesar Wang
who is the author of this patch? Roger or you? Because we should either drop
Roger's Signed-off or fix the authorship to list him.
Heiko
> ---
>
> Changes in v3:
> - leave into two patches based on patchv2, and fix nits and commit, as
> comment on https://patchwork.kernel.org/patch/9306339/
>
> Changes in v2:
> - Fixes the order, ss Heiko commnets on
> https://patchwork.kernel.org/patch/9305991/
>
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index 32aebc8..2ab233f 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -611,6 +611,11 @@
> status = "disabled";
> };
>
> + qos_gmac: qos@ffa5c000 {
> + compatible = "syscon";
> + reg = <0x0 0xffa5c000 0x0 0x20>;
> + };
> +
> qos_hdcp: qos@ffa9 {
> compatible = "syscon";
> reg = <0x0 0xffa9 0x0 0x20>;
> @@ -739,6 +744,11 @@
> };
>
> /* These power domains are grouped by VD_LOGIC */
> + pd_gmac@RK3399_PD_GMAC {
> + reg = ;
> + clocks = <&cru ACLK_GMAC>;
> + pm_qos = <&qos_gmac>;
> + };
> pd_vio@RK3399_PD_VIO {
> reg = ;
> #address-cells = <1>;
Re: [PATCH v3 4/5] arm64: dts: rockchip: add the gmac needed node for rk3399
Hi Caesar, Am Donnerstag, 1. September 2016, 07:28:51 schrieb Caesar Wang: > The RK3399 GMAC Ethernet Controller provides a complete Ethernet interface > from processor to a Reduced Media Independent Interface (RMII) and Reduced > Gigabit Media Independent Interface (RGMII) compliant Ethernet PHY. > > This patch adds the related needed device information. > e.g.: interrupts, grf, clocks, pinctrl and so on. > > The full details are in [0]. > > [0]: > Documentation/devicetree/bindings/net/rockchip-dwmac.txt > > Signed-off-by: Caesar Wang the core rk3399 gmac support is still stuck and needs a respin. This patch is part of that other series as well which needs a respin itself. Maybe you could just fold both series into one so that we don't have to track two. Thanks Heiko
Re: [PATCH v4 4/6] arm64: dts: rockchip: add the gmac power domain on rk3399
Am Freitag, 2. September 2016, 01:50:02 schrieb Caesar Wang: > This patch supports the gmac pd to save power consumption. > Even though some boards not need Ethernet support, the driver > core can also take care of powering up the pd before probe. > > Signed-off-by: Caesar Wang applied to my dts64 branch for 4.9 with Doug's Review-tag. I'd like to also pick up the other two dts patches after Dave is satisfied with the driver-side changes in patches 1-3. Heiko
Re: [PATCH 1/3] net: stmmac: dwmac-rk: add rk3366 & rk3399-specific data
Hi Roger, Am Mittwoch, 6. Juli 2016, 18:51:29 schrieb Roger Chen: > Add constants and callback functions for the dwmac on rk3368 socs. > As can be seen, the base structure is the same, only registers and > the bits in them moved slightly. > > Signed-off-by: Roger Chen > --- > .../devicetree/bindings/net/rockchip-dwmac.txt | 3 +- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 226 > + 2 files changed, 228 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt index > 93eac7c..8c066e6 100644 > --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > @@ -3,7 +3,8 @@ Rockchip SoC RK3288 10/100/1000 Ethernet driver(GMAC) > The device node has following properties. > > Required properties: > - - compatible: Can be one of "rockchip,rk3288-gmac", "rockchip,rk3368-gmac" > + - compatible: Can be one of "rockchip,rk3288-gmac", > "rockchip,rk3366-gmac", + > "rockchip,rk3368-gmac", "rockchip,rk3399-gmac" - reg: addresses and length > of the register sets for the device. - interrupts: Should contain the GMAC > interrupts. > - interrupt-names: Should contain the interrupt names "macirq". this doesn't apply against 4.8-rc1 anymore, can you please rebase and resend the series? Thanks Heiko
Re: [PATCH 1/3] net: stmmac: dwmac-rk: add rk3366 & rk3399 specific data
Hi Roger, Am Donnerstag, 11. August 2016, 15:24:46 schrieb Roger Chen: > Add constants and callback functions for the dwmac on rk3228/rk3229 socs. > As can be seen, the base structure is the same, only registers and the > bits in them moved slightly. > > Signed-off-by: Roger Chen > --- > .../devicetree/bindings/net/rockchip-dwmac.txt | 4 +- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 226 > + 2 files changed, 228 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt index > cccd945..8c066e6 100644 > --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > @@ -3,8 +3,8 @@ Rockchip SoC RK3288 10/100/1000 Ethernet driver(GMAC) > The device node has following properties. > > Required properties: > - - compatible: Can be one of "rockchip,rk3228-gmac", > "rockchip,rk3288-gmac", - > "rockchip,rk3368-gmac" > + - compatible: Can be one of "rockchip,rk3288-gmac", you're dropping the rk3228 here. Otherwise looks fine, so with the compatible list fixed you can add my Reviewed-by: Heiko Stuebner Heiko
Re: Linux kernel hangs if using RV1108 with MSZ8863 switch with two ports connected
Hi all, Am Sonntag, 18. November 2018, 19:12:30 CET schrieb Andrew Lunn: > > The kernel starts booting normally and then hangs like this when two > > Ethernet cables are connected to the KSZ8863 switch: > > http://dark-code.bulix.org/3xexu5-507563 > > > This has the lock detection, inside kernel hacking, enabled. > > Maybe turn on all the hung-task debug and magic key support. With > magic key, you might be able to get a backtrace of where it is > spinning. > > Maybe also add #define DEBUG at the top of drivers/net/phy/phy.c. > Does it hang during a PHY state transition? > > Maybe both PHYs are interrupting at the same time, and the interrupt > code is broken? > > Maybe look at the switch driver and see if there is any code which is > executed on link up. Put some printk() in there. > > If you PHYs are using interrupt mode, maybe disable that and use > polling. > > Do you know if this ever worked properly before? If you know when it > did work, you can do a git bisect to narrow it down to the one patch > which broke it.. > > Basically, at the moment, you just need to try lots of things, to > narrow it down. Your hang also seems to happen around the time the kernel disables unused clocks and regulators. So you might also try with these functions disabled, as it may be caused by some clock or regulator handled wrongly there (I think it's called clk_ignore_unused as kernel commandline but please double-check and you'll need to check for a regulator equivalent yourself). And as I think it might be some sort of driver-related issue, you could also enable debugging in the driver-core [drivers/base/dd.c] by either #define DEBUG or just redefining dev_dbg temporarily ;-) #define dev_dbg dev_warn or so. That may help finding the culprit of your hang. Heiko
Re: [PATCH net-next 3/3] net: stmmac: Add RK3566/RK3568 SoC support
Am Mittwoch, 14. April 2021, 13:03:25 CEST schrieb Peter Geis:
> On Tue, Apr 13, 2021 at 7:37 PM Ezequiel Garcia
> wrote:
> > > > +static void rk3566_set_to_rmii(struct rk_priv_data *bsp_priv)
> > > > +{
> > > > + struct device *dev = &bsp_priv->pdev->dev;
> > > > +
> > > > + if (IS_ERR(bsp_priv->grf)) {
> > > > + dev_err(dev, "%s: Missing rockchip,grf property\n",
> > > > __func__);
> > > > + return;
> > > > + }
> > > > +
> > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON1,
> > > > +RK3568_GMAC_PHY_INTF_SEL_RMII);
> > > > +}
> > > > +
> > > > +static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
> > > > + int tx_delay, int rx_delay)
> > > > +{
> > > > + struct device *dev = &bsp_priv->pdev->dev;
> > > > +
> > > > + if (IS_ERR(bsp_priv->grf)) {
> > > > + dev_err(dev, "Missing rockchip,grf property\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC0_CON1,
> > > > +RK3568_GMAC_PHY_INTF_SEL_RGMII |
> > > > +RK3568_GMAC_RXCLK_DLY_ENABLE |
> > > > +RK3568_GMAC_TXCLK_DLY_ENABLE);
> > > > +
> > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC0_CON0,
> > > > +RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> > > > +RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> > > > +
> > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON1,
> > > > +RK3568_GMAC_PHY_INTF_SEL_RGMII |
> > > > +RK3568_GMAC_RXCLK_DLY_ENABLE |
> > > > +RK3568_GMAC_TXCLK_DLY_ENABLE);
> > > > +
> > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON0,
> > > > +RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> > > > +RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> > >
> > > Since there are two GMACs on the rk3568, and either, or, or both may
> > > be enabled in various configurations, we should only configure the
> > > controller we are currently operating.
>
> Perhaps we should have match data (such as reg = <0>, or against the
> address) to identify the individual controllers.
Hmm, "reg" will be used by the actual mmio address of the controller,
so matching against that should be the way I guess.
We're already doing something similar for dsi:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1170
Heiko
Re: [PATCH net-next 3/3] net: stmmac: Add RK3566/RK3568 SoC support
Am Mittwoch, 14. April 2021, 18:33:12 CEST schrieb Chen-Yu Tsai:
> On Thu, Apr 15, 2021 at 12:14 AM Ezequiel Garcia
> wrote:
> >
> > Hi Peter, Heiko,
> >
> > On Wed, 2021-04-14 at 13:15 +0200, Heiko Stübner wrote:
> > > Am Mittwoch, 14. April 2021, 13:03:25 CEST schrieb Peter Geis:
> > > > On Tue, Apr 13, 2021 at 7:37 PM Ezequiel Garcia
> > > > wrote:
> > > > > > > +static void rk3566_set_to_rmii(struct rk_priv_data *bsp_priv)
> > > > > > > +{
> > > > > > > + struct device *dev = &bsp_priv->pdev->dev;
> > > > > > > +
> > > > > > > + if (IS_ERR(bsp_priv->grf)) {
> > > > > > > + dev_err(dev, "%s: Missing rockchip,grf
> > > > > > > property\n", __func__);
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON1,
> > > > > > > +RK3568_GMAC_PHY_INTF_SEL_RMII);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
> > > > > > > + int tx_delay, int rx_delay)
> > > > > > > +{
> > > > > > > + struct device *dev = &bsp_priv->pdev->dev;
> > > > > > > +
> > > > > > > + if (IS_ERR(bsp_priv->grf)) {
> > > > > > > + dev_err(dev, "Missing rockchip,grf property\n");
> > > > > > > + return;
> > > > > > > + }
> > > > > > > +
> > > > > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC0_CON1,
> > > > > > > +RK3568_GMAC_PHY_INTF_SEL_RGMII |
> > > > > > > +RK3568_GMAC_RXCLK_DLY_ENABLE |
> > > > > > > +RK3568_GMAC_TXCLK_DLY_ENABLE);
> > > > > > > +
> > > > > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC0_CON0,
> > > > > > > +RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> > > > > > > +RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> > > > > > > +
> > > > > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON1,
> > > > > > > +RK3568_GMAC_PHY_INTF_SEL_RGMII |
> > > > > > > +RK3568_GMAC_RXCLK_DLY_ENABLE |
> > > > > > > +RK3568_GMAC_TXCLK_DLY_ENABLE);
> > > > > > > +
> > > > > > > + regmap_write(bsp_priv->grf, RK3568_GRF_GMAC1_CON0,
> > > > > > > +RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> > > > > > > +RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> > > > > >
> > > > > > Since there are two GMACs on the rk3568, and either, or, or both may
> > > > > > be enabled in various configurations, we should only configure the
> > > > > > controller we are currently operating.
> > > >
> > > > Perhaps we should have match data (such as reg = <0>, or against the
> > > > address) to identify the individual controllers.
> > >
> > > Hmm, "reg" will be used by the actual mmio address of the controller,
> > > so matching against that should be the way I guess.
> > >
> > > We're already doing something similar for dsi:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n1170
> > >
> >
> > I have to admit, I'm not a fan of hardcoding the registers in the kernel.
> >
> > David Wu solved this in the downstream kernel by using bus_id,
> > which parses the devicetree "ethernet@0" node, i.e.:
> >
> > plat->bus_id = of_alias_get_id(np, "ethernet");
>
> What happens when one adds another ethernet controller (USB or PCIe) to
> the board and wants to change the numbering order?
>
> Or maybe only the second ethernet controller is routed on some board
> and the submitter / vendor wants that one to be ethernet0, because
> it's the only usable controller?
Which matches a discussion I had with Arnd about the mmc numbering.
I.e. there the first mmc device is supposed to be mmc0 and so on,
without gaps - for probably the same reasons.
>
> This seems even more fragile than hardcoding the registers.
>
> Regards
> ChenYu
>
Re: [PATCH v1 1/1] time64.h: Consolidated PSEC_PER_SEC definition
Am Dienstag, 12. Januar 2021, 16:37:09 CET schrieb Andy Shevchenko:
> We have currently three users of the PSEC_PER_SEC each of them defining it
> individually. Instead, move it to time64.h to be available for everyone.
>
> There is a new user coming with the same constant in use. It will also
> make its life easier.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/net/ethernet/mscc/ocelot_ptp.c | 2 ++
> drivers/phy/phy-core-mipi-dphy.c | 2 --
> drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c | 8
for the Rockchip-part:
Acked-by: Heiko Stuebner
though not sure if that reordering of other includes should be in there
or separate. Don't have a hard opinion, and will let others decide ;-)
Heiko
> include/soc/mscc/ocelot_ptp.h| 2 --
> include/vdso/time64.h| 1 +
> 5 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_ptp.c
> b/drivers/net/ethernet/mscc/ocelot_ptp.c
> index a33ab315cc6b..87ad2137ba06 100644
> --- a/drivers/net/ethernet/mscc/ocelot_ptp.c
> +++ b/drivers/net/ethernet/mscc/ocelot_ptp.c
> @@ -4,6 +4,8 @@
> * Copyright (c) 2017 Microsemi Corporation
> * Copyright 2020 NXP
> */
> +#include
> +
> #include
> #include
> #include
> diff --git a/drivers/phy/phy-core-mipi-dphy.c
> b/drivers/phy/phy-core-mipi-dphy.c
> index 14e0551cd319..77fe65367ce5 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -12,8 +12,6 @@
> #include
> #include
>
> -#define PSEC_PER_SEC 1LL
> -
> /*
> * Minimum D-PHY timings based on MIPI D-PHY specification. Derived
> * from the valid ranges specified in Section 6.9, Table 14, Page 41
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 8af8c6c5cc02..347dc79a18c1 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -11,16 +11,16 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> +#include
> +
> #include
> #include
> -#include
> -#include
> -
> -#define PSEC_PER_SEC 1LL
>
> #define UPDATE(x, h, l) (((x) << (l)) & GENMASK((h), (l)))
>
> diff --git a/include/soc/mscc/ocelot_ptp.h b/include/soc/mscc/ocelot_ptp.h
> index 6a7388fa7cc5..ded497d72bdb 100644
> --- a/include/soc/mscc/ocelot_ptp.h
> +++ b/include/soc/mscc/ocelot_ptp.h
> @@ -37,8 +37,6 @@ enum {
>
> #define PTP_CFG_MISC_PTP_EN BIT(2)
>
> -#define PSEC_PER_SEC 1LL
> -
> #define PTP_CFG_CLK_ADJ_CFG_ENA BIT(0)
> #define PTP_CFG_CLK_ADJ_CFG_DIR BIT(1)
>
> diff --git a/include/vdso/time64.h b/include/vdso/time64.h
> index 9d43c3f5e89d..b40cfa2aa33c 100644
> --- a/include/vdso/time64.h
> +++ b/include/vdso/time64.h
> @@ -9,6 +9,7 @@
> #define NSEC_PER_MSEC100L
> #define USEC_PER_SEC 100L
> #define NSEC_PER_SEC 10L
> +#define PSEC_PER_SEC 1LL
> #define FSEC_PER_SEC 1000LL
>
> #endif /* __VDSO_TIME64_H */
>
Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants
Am Donnerstag, 18. Juni 2020, 18:40:15 CEST schrieb Russell King - ARM Linux admin: > On Thu, Jun 18, 2020 at 06:01:29PM +0200, Heiko Stübner wrote: > > Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM > > Linux admin: > > > On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote: > > > > Though I'm not sure how this fits in the whole bringup of ethernet phys. > > > > Like the phy is dependent on the underlying ethernet controller to > > > > actually turn it on. > > > > > > > > I guess we should check the phy-state and if it's not accessible, just > > > > keep the values and if it's in a suitable state do the configuration. > > > > > > > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init() > > > > as well as the clk_(un-)prepare and clk_set_rate functions and being > > > > protected by a check against phy_is_started() ? > > > > > > It sounds like it doesn't actually fit the clk API paradym then. I > > > see that Rob suggested it, and from the DT point of view, it makes > > > complete sense, but then if the hardware can't actually be used in > > > the way the clk API expects it to be used, then there's a semantic > > > problem. > > > > > > What is this clock used for? > > > > It provides a source for the mac-clk for the actual transfers, here to > > provide the 125MHz clock needed for the RGMII interface . > > > > So right now the old rk3368-lion devicetree just declares a stub > > fixed-clock and instructs the soc's clock controller to use it [0] . > > And in the cover-letter here, I show the update variant with using > > the clock defined here. > > > > > > I've added the idea from my previous mail like shown below [1]. > > which would take into account the phy-state. > > > > But I guess I'll wait for more input before spamming people with v6. > > Let's get a handle on exactly what this is. > > The RGMII bus has two clocks: RXC and TXC, which are clocked at one > of 125MHz, 25MHz or 2.5MHz depending on the RGMII data rate. Some > PHYs replace TXC with GTX clock, which always runs at 125MHz. These > clocks are not what you're referring to. > > You are referring to another commonly provided clock between the MAC > and the PHY, something which is not unique to your PHY. > > We seem to be heading down a path where different PHYs end up doing > different things in DT for what is basically the same hardware setup, > which really isn't good. :( > > We have at803x using: > > qca,clk-out-frequency > qca,clk-out-strength > qca,keep-pll-enabled > > which are used to control the CLK_25M output pin on the device, which > may be used to provide a reference clock for the MAC side, selecting > between 25M, 50M, 62.5M and 125MHz. This was introduced in November > 2019, so not that long ago. Because it was not that old, was the reason I followed that example in my v1 :-) And Andrew then suggested to at least try to use a common property for the target frequency that other phys could migrate to. > Broadcom PHYs configure their 125MHz clock through the PHY device > flags passed from the MAC at attach/connect time. > > There's the dp83867 and dp83869 configuration (I'm not sure I can > make sense of it from reading the code) using ti,clk-output-sel - > but it looks like it's the same kind of thing. Introduced February > 2018 into one driver, and November 2019 in the other. > > It seems the Micrel PHYs produce a 125MHz clock irrespective of any > configuration (maybe configured by firmware, or hardware strapping?) > > So it seems we have four ways of doing the same thing today, and now > the suggestion is to implement a fifth different way. I think there > needs to be some consolidation here, maybe choosing one approach and > sticking with it. > > Hence, I disagree with Rob - we don't need a fifth approach, we need > to choose one approach and decide that's our policy for this and > apply it evenly across the board, rather than making up something > different each time a new PHY comes along. @Dave, @Andrew: what's you opinion here? As Russell above (and Florian in the binding patch) pointed out, integrating this into the clock-framework may prove difficult not only for consistency but also for dependency reasons. Personally I'm fine with either solution, I just want to achieve a working ethernet on my board :-D . Thanks Heiko
Re: [PATCH v3 1/3] net: phy: mscc: move shared probe code into a helper
Hi, Am Dienstag, 16. Juni 2020, 03:12:25 CEST schrieb David Miller: > From: David Miller > Date: Mon, 15 Jun 2020 18:11:29 -0700 (PDT) > > + return devm_phy_package_join(&phydev->mdio.dev, phydev, > > +vsc8531->base_addr, 0); > > But it is still dereferenced here. > > Did the compiler really not warn you about this when you test built > these changes? I'm wondering that myself ... it probably did and I overlooked it, which also is indicated by the fact that I did add the declaration of the vsc8531 when rebasing. > > Because you removed this devm_kzalloc() code, vsc8531 is never initialized. > > You also need to provide a proper header posting when you repost this series > after fixing this bug. not sure I understand what you mean with "header posting" here. Thanks Heiko
Re: [PATCH v4 2/3] dt-bindings: net: ethernet-phy: add enet-phy-clock-out-frequency
Am Mittwoch, 17. Juni 2020, 23:33:25 CEST schrieb Heiko Stuebner: > From: Heiko Stuebner > > Some ethernet phys have a configurable clock output, so add a generic > property to describe its target rate. > > Suggested-by: Andrew Lunn > Signed-off-by: Heiko Stuebner just now Rob wrote for v3: - 8< -- The correct thing to do here is make the phy a clock provider and then the client side use 'assigned-clock-rate' to set the rate. That has the advantage that it also describes the connection of the clock signal. You might not need that for a simple case, but I could imagine needing that in a more complex case. Rob - 8< -- > Documentation/devicetree/bindings/net/ethernet-phy.yaml | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > index 9b1f1147ca36..4dcf93f1c555 100644 > --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml > +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml > @@ -84,6 +84,11 @@ properties: >the turn around line low at end of the control phase of the >MDIO transaction. > > + enet-phy-clock-out-frequency: > +$ref: /schemas/types.yaml#definitions/uint32 > +description: > + Frequency in Hz to set an available clock output to. > + >enet-phy-lane-swap: > $ref: /schemas/types.yaml#definitions/flag > description: >
Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants
Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux
admin:
> On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote:
> > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> > > From: Heiko Stuebner
> > >
> > > At least VSC8530/8531/8540/8541 contain a clock output that can emit
> > > a predefined rate of 25, 50 or 125MHz.
> > >
> > > This may then feed back into the network interface as source clock.
> > > So expose a clock-provider from the phy using the common clock framework
> > > to allow setting the rate.
> > >
> > > Signed-off-by: Heiko Stuebner
> > > ---
> > > drivers/net/phy/mscc/mscc.h | 13 +++
> > > drivers/net/phy/mscc/mscc_main.c | 182 +--
> > > 2 files changed, 187 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> > > index fbcee5fce7b2..94883dab5cc1 100644
> > > --- a/drivers/net/phy/mscc/mscc.h
> > > +++ b/drivers/net/phy/mscc/mscc.h
> > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
> > > #define INT_MEM_DATA_M 0x00ff
> > > #define INT_MEM_DATA(x)(INT_MEM_DATA_M & (x))
> > >
> > > +#define MSCC_CLKOUT_CNTL 13
> > > +#define CLKOUT_ENABLE BIT(15)
> > > +#define CLKOUT_FREQ_MASK GENMASK(14, 13)
> > > +#define CLKOUT_FREQ_25M(0x0 << 13)
> > > +#define CLKOUT_FREQ_50M(0x1 << 13)
> > > +#define CLKOUT_FREQ_125M (0x2 << 13)
> > > +
> > > #define MSCC_PHY_PROC_CMD 18
> > > #define PROC_CMD_NCOMPLETED0x8000
> > > #define PROC_CMD_FAILED0x4000
> > > @@ -360,6 +367,12 @@ struct vsc8531_private {
> > >*/
> > > unsigned int base_addr;
> > >
> > > +#ifdef CONFIG_COMMON_CLK
> > > + struct clk_hw clkout_hw;
> > > +#endif
> > > + u32 clkout_rate;
> > > + int clkout_enabled;
> > > +
> > > #if IS_ENABLED(CONFIG_MACSEC)
> > > /* MACsec fields:
> > >* - One SecY per device (enforced at the s/w implementation level)
> > > diff --git a/drivers/net/phy/mscc/mscc_main.c
> > > b/drivers/net/phy/mscc/mscc_main.c
> > > index 5d2777522fb4..727a9dd58403 100644
> > > --- a/drivers/net/phy/mscc/mscc_main.c
> > > +++ b/drivers/net/phy/mscc/mscc_main.c
> > > @@ -7,6 +7,7 @@
> > > * Copyright (c) 2016 Microsemi Corporation
> > > */
> > >
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device
> > > *phydev,
> > >
> > > return led_mode;
> > > }
> > > -
> > > #else
> > > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
> > > {
> > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device
> > > *phydev)
> > > return 0;
> > > }
> > >
> > > +static int vsc8531_config_init(struct phy_device *phydev)
> > > +{
> > > + struct vsc8531_private *vsc8531 = phydev->priv;
> > > + u16 val;
> > > + int rc;
> > > +
> > > + rc = vsc85xx_config_init(phydev);
> > > + if (rc)
> > > + return rc;
> > > +
> > > +#ifdef CONFIG_COMMON_CLK
> > > + switch (vsc8531->clkout_rate) {
> > > + case 2500:
> > > + val = CLKOUT_FREQ_25M;
> > > + break;
> > > + case 5000:
> > > + val = CLKOUT_FREQ_50M;
> > > + break;
> > > + case 12500:
> > > + val = CLKOUT_FREQ_125M;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (vsc8531->clkout_enabled)
> > > + val |= CLKOUT_ENABLE;
> > > +
> > > + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > > + MSCC_CLKOUT_CNTL, val);
> > > + if (rc)
> > > + return rc;
> > > +#endif
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > > +static int vsc8531_clkout_prepare(struct clk_hw *hw)
> > > +{
> > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > +
> > > + vsc8531->clkout_enabled = true;
> > > + return 0;
> > > +}
> > > +
> > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw)
> > > +{
> > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw);
> > > +
> > > + vsc8531->clkout_enabled = false;
> > > +}
> > > +
> >
> > > +static const struct clk_ops vsc8531_clkout_ops = {
> > > + .prepare = vsc8531_clkout_prepare,
> > > + .unprepare = vsc8531_clkout_unprepare,
> > > + .is_prepared = vsc8531_clkout_is_prepared,
> > > + .recalc_rate = vsc8531_clkout_recalc_rate,
> > > + .round_rate = vsc8531_clkout_round_rate,
> > > + .set_rate = vsc8531_clkout_set_rate,
> >
> > I'm not sure this is the expected behaviour. The clk itself should
> > only start ticking when the enable callback is called. But this code
> > will enable the clock when config_init() is called. I think you should
> > implement the enable and disable methods.
>
> That is actually incorrect. The whole "prepare" vs "enable" difference
> is that prepare can schedule, enable isn't permi
Re: [PATCH v5 3/3] net: phy: mscc: handle the clkout control on some phy variants
Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux
admin:
> On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote:
> > Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM
> > Linux admin:
> > > On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote:
> > > > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote:
> > > > > From: Heiko Stuebner
> > > > >
> > > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit
> > > > > a predefined rate of 25, 50 or 125MHz.
> > > > >
> > > > > This may then feed back into the network interface as source clock.
> > > > > So expose a clock-provider from the phy using the common clock
> > > > > framework
> > > > > to allow setting the rate.
> > > > >
> > > > > Signed-off-by: Heiko Stuebner
> > > > > ---
> > > > > drivers/net/phy/mscc/mscc.h | 13 +++
> > > > > drivers/net/phy/mscc/mscc_main.c | 182
> > > > > +--
> > > > > 2 files changed, 187 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> > > > > index fbcee5fce7b2..94883dab5cc1 100644
> > > > > --- a/drivers/net/phy/mscc/mscc.h
> > > > > +++ b/drivers/net/phy/mscc/mscc.h
> > > > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay {
> > > > > #define INT_MEM_DATA_M 0x00ff
> > > > > #define INT_MEM_DATA(x)(INT_MEM_DATA_M & (x))
> > > > >
> > > > > +#define MSCC_CLKOUT_CNTL 13
> > > > > +#define CLKOUT_ENABLE BIT(15)
> > > > > +#define CLKOUT_FREQ_MASK GENMASK(14, 13)
> > > > > +#define CLKOUT_FREQ_25M(0x0 << 13)
> > > > > +#define CLKOUT_FREQ_50M(0x1 << 13)
> > > > > +#define CLKOUT_FREQ_125M (0x2 << 13)
> > > > > +
> > > > > #define MSCC_PHY_PROC_CMD 18
> > > > > #define PROC_CMD_NCOMPLETED0x8000
> > > > > #define PROC_CMD_FAILED0x4000
> > > > > @@ -360,6 +367,12 @@ struct vsc8531_private {
> > > > >*/
> > > > > unsigned int base_addr;
> > > > >
> > > > > +#ifdef CONFIG_COMMON_CLK
> > > > > + struct clk_hw clkout_hw;
> > > > > +#endif
> > > > > + u32 clkout_rate;
> > > > > + int clkout_enabled;
> > > > > +
> > > > > #if IS_ENABLED(CONFIG_MACSEC)
> > > > > /* MACsec fields:
> > > > >* - One SecY per device (enforced at the s/w implementation
> > > > > level)
> > > > > diff --git a/drivers/net/phy/mscc/mscc_main.c
> > > > > b/drivers/net/phy/mscc/mscc_main.c
> > > > > index 5d2777522fb4..727a9dd58403 100644
> > > > > --- a/drivers/net/phy/mscc/mscc_main.c
> > > > > +++ b/drivers/net/phy/mscc/mscc_main.c
> > > > > @@ -7,6 +7,7 @@
> > > > > * Copyright (c) 2016 Microsemi Corporation
> > > > > */
> > > > >
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct
> > > > > phy_device *phydev,
> > > > >
> > > > > return led_mode;
> > > > > }
> > > > > -
> > > > > #else
> > > > > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
> > > > > {
> > > > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct
> > > > > phy_device *phydev)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int vsc8531_config_init(struct phy_device *phydev)
> > > > > +{
> > > > > + struct vsc8531_private *vsc8531 = phydev->priv;
> > > > > + u16 val;
> > > > > + int rc;
> > > > > +
> > > > > + rc = vsc85xx_config_ini
