On 31/05/17 08:18, Corentin Labbe wrote:
> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> allwinner.
> In fact the only common part is the descriptor management and the first
> register function.
Hi,
I know I am a bit late with this, but while adapting the U-Boot driver
to the new binding I was wondering about the internal PHY detection:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> new file mode 100644
> index 000000000000..1a6bfe6c958f
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -0,0 +1,990 @@
....
> +static int sun8i_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct sunxi_priv_data *gmac;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + gmac = devm_kzalloc(dev, sizeof(*gmac), GFP_KERNEL);
> + if (!gmac)
> + return -ENOMEM;
> +
> + gmac->variant = of_device_get_match_data(&pdev->dev);
> + if (!gmac->variant) {
> + dev_err(&pdev->dev, "Missing dwmac-sun8i variant\n");
> + return -EINVAL;
> + }
> +
> + gmac->tx_clk = devm_clk_get(dev, "stmmaceth");
> + if (IS_ERR(gmac->tx_clk)) {
> + dev_err(dev, "Could not get TX clock\n");
> + return PTR_ERR(gmac->tx_clk);
> + }
> +
> + /* Optional regulator for PHY */
> + gmac->regulator = devm_regulator_get_optional(dev, "phy");
> + if (IS_ERR(gmac->regulator)) {
> + if (PTR_ERR(gmac->regulator) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + dev_info(dev, "No regulator found\n");
> + gmac->regulator = NULL;
> + }
> +
> + gmac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + "syscon");
> + if (IS_ERR(gmac->regmap)) {
> + ret = PTR_ERR(gmac->regmap);
> + dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret);
> + return ret;
> + }
> +
> + plat_dat->interface = of_get_phy_mode(dev->of_node);
> + if (plat_dat->interface == gmac->variant->internal_phy) {
> + dev_info(&pdev->dev, "Will use internal PHY\n");
So here you seem to deduce the usage of the internal PHY by the PHY
interface specified in the DT (MII = internal, RGMII = external).
I think I raised this question before, but isn't it perfectly legal for
a board to use MII with an external PHY even on those SoCs that feature
an internal PHY?
On the first glance that does not make too much sense, but apart from
not being the correct binding to describe all of the SoCs features I see
two scenarios:
1) A board vendor might choose to not use the internal PHY because it
has bugs, lacks features (configurability) or has other issues. For
instance I have heard reports that the internal PHY makes the SoC go
rather hot, possibly limiting the CPU frequency. By using an external
MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
2) A PHY does not necessarily need to be directly connected to
magnetics. Indeed quite some boards use (RG)MII to connect to a switch
IC or some other network circuitry, for instance fibre connectors.
So I was wondering if we would need an explicit:
allwinner,use-internal-phy;
boolean DT property to signal the usage of the internal PHY?
Alternatively we could go with the negative version:
allwinner,disable-internal-phy;
Or what about introducing a new "allwinner,internal-mii-phy" compatible
string for the *PHY* node and use that?
I just want to avoid that we introduce a binding that causes us
headaches later. I think we can still fix this with a followup patch
before the driver and its binding hit a release kernel.
Cheers,
Andre.
> + gmac->use_internal_phy = true;
> + gmac->ephy_clk = of_clk_get(plat_dat->phy_node, 0);
> + if (IS_ERR(gmac->ephy_clk)) {
> + ret = PTR_ERR(gmac->ephy_clk);
> + dev_err(&pdev->dev, "Cannot get EPHY clock: %d\n", ret);
> + return -EINVAL;
> + }
> +
> + gmac->rst_ephy = of_reset_control_get(plat_dat->phy_node, NULL);
> + if (IS_ERR(gmac->rst_ephy)) {
> + ret = PTR_ERR(gmac->rst_ephy);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> + dev_err(&pdev->dev, "No EPHY reset control found %d\n",
> + ret);
> + return -EINVAL;
> + }
> + } else {
> + dev_info(&pdev->dev, "Will use external PHY\n");
> + gmac->use_internal_phy = false;
> + }
> +
> + /* platform data specifying hardware features and callbacks.
> + * hardware features were copied from Allwinner drivers.
> + */
> + plat_dat->rx_coe = STMMAC_RX_COE_TYPE2;
> + plat_dat->tx_coe = 1;
> + plat_dat->has_sun8i = true;
> + plat_dat->bsp_priv = gmac;
> + plat_dat->init = sun8i_dwmac_init;
> + plat_dat->exit = sun8i_dwmac_exit;
> + plat_dat->setup = sun8i_dwmac_setup;
> +
> + ret = sun8i_dwmac_init(pdev, plat_dat->bsp_priv);
> + if (ret)
> + return ret;
> +
> + ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> + if (ret)
> + sun8i_dwmac_exit(pdev, plat_dat->bsp_priv);
> +
> + return ret;
> +}
> +
> +static const struct of_device_id sun8i_dwmac_match[] = {
> + { .compatible = "allwinner,sun8i-h3-emac",
> + .data = &emac_variant_h3 },
> + { .compatible = "allwinner,sun8i-a83t-emac",
> + .data = &emac_variant_a83t },
> + { .compatible = "allwinner,sun50i-a64-emac",
> + .data = &emac_variant_a64 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_dwmac_match);
> +
> +static struct platform_driver sun8i_dwmac_driver = {
> + .probe = sun8i_dwmac_probe,
> + .remove = stmmac_pltfr_remove,
> + .driver = {
> + .name = "dwmac-sun8i",
> + .pm = &stmmac_pltfr_pm_ops,
> + .of_match_table = sun8i_dwmac_match,
> + },
> +};
> +module_platform_driver(sun8i_dwmac_driver);
> +
> +MODULE_AUTHOR("Corentin Labbe <[email protected]>");
> +MODULE_DESCRIPTION("Allwinner sun8i DWMAC specific glue layer");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c80c9c3b67db..68a188e74c54 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -235,6 +235,17 @@ static void stmmac_clk_csr_set(struct stmmac_priv *priv)
> else if ((clk_rate >= CSR_F_250M) && (clk_rate < CSR_F_300M))
> priv->clk_csr = STMMAC_CSR_250_300M;
> }
> +
> + if (priv->plat->has_sun8i) {
> + if (clk_rate > 160000000)
> + priv->clk_csr = 0x03;
> + else if (clk_rate > 80000000)
> + priv->clk_csr = 0x02;
> + else if (clk_rate > 40000000)
> + priv->clk_csr = 0x01;
> + else
> + priv->clk_csr = 0;
> + }
> }
>
> static void print_pkt(unsigned char *buf, int len)
> @@ -3955,6 +3966,10 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>
> priv->hw = mac;
>
> + /* dwmac-sun8i only work in chain mode */
> + if (priv->plat->has_sun8i)
> + chain_mode = 1;
> +
> /* To use the chained or ring mode */
> if (priv->synopsys_id >= DWMAC_CORE_4_00) {
> priv->hw->mode = &dwmac4_ring_mode_ops;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 7fc3a1ef395a..3840529344ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -309,6 +309,12 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> struct device_node *np, struct device *dev)
> {
> bool mdio = true;
> + static const struct of_device_id need_mdio_ids[] = {
> + { .compatible = "snps,dwc-qos-ethernet-4.10" },
> + { .compatible = "allwinner,sun8i-a83t-emac" },
> + { .compatible = "allwinner,sun8i-h3-emac" },
> + { .compatible = "allwinner,sun50i-a64-emac" },
> + };
>
> /* If phy-handle property is passed from DT, use it as the PHY */
> plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
> @@ -325,8 +331,7 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
> mdio = false;
> }
>
> - /* exception for dwmac-dwc-qos-eth glue logic */
> - if (of_device_is_compatible(np, "snps,dwc-qos-ethernet-4.10")) {
> + if (of_match_node(need_mdio_ids, np)) {
> plat->mdio_node = of_get_child_by_name(np, "mdio");
> } else {
> /**
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 8bb550bca96d..108739ff9223 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -186,6 +186,7 @@ struct plat_stmmacenet_data {
> struct reset_control *stmmac_rst;
> struct stmmac_axi *axi;
> int has_gmac4;
> + bool has_sun8i;
> bool tso_en;
> int mac_port_sel_speed;
> bool en_tx_lpi_clockgating;
>
On Mon, Jun 26, 2017 at 01:18:23AM +0100, Andr? Przywara wrote:
> On 31/05/17 08:18, Corentin Labbe wrote:
> > The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> > allwinner.
> > In fact the only common part is the descriptor management and the first
> > register function.
>
> Hi,
>
> I know I am a bit late with this, but while adapting the U-Boot driver
> to the new binding I was wondering about the internal PHY detection:
>
>
> So here you seem to deduce the usage of the internal PHY by the PHY
> interface specified in the DT (MII = internal, RGMII = external).
> I think I raised this question before, but isn't it perfectly legal for
> a board to use MII with an external PHY even on those SoCs that feature
> an internal PHY?
> On the first glance that does not make too much sense, but apart from
> not being the correct binding to describe all of the SoCs features I see
> two scenarios:
> 1) A board vendor might choose to not use the internal PHY because it
> has bugs, lacks features (configurability) or has other issues. For
> instance I have heard reports that the internal PHY makes the SoC go
> rather hot, possibly limiting the CPU frequency. By using an external
> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
> 2) A PHY does not necessarily need to be directly connected to
> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
> IC or some other network circuitry, for instance fibre connectors.
>
> So I was wondering if we would need an explicit:
> allwinner,use-internal-phy;
> boolean DT property to signal the usage of the internal PHY?
> Alternatively we could go with the negative version:
> allwinner,disable-internal-phy;
>
> Or what about introducing a new "allwinner,internal-mii-phy" compatible
> string for the *PHY* node and use that?
>
> I just want to avoid that we introduce a binding that causes us
> headaches later. I think we can still fix this with a followup patch
> before the driver and its binding hit a release kernel.
>
> Cheers,
> Andre.
>
I just see some patch, where "phy-mode = internal" is valid.
I will try to find a way to use it
Regards
On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
<[email protected]> wrote:
> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>> On 31/05/17 08:18, Corentin Labbe wrote:
>> > The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>> > allwinner.
>> > In fact the only common part is the descriptor management and the first
>> > register function.
>>
>> Hi,
>>
>> I know I am a bit late with this, but while adapting the U-Boot driver
>> to the new binding I was wondering about the internal PHY detection:
>>
>>
>> So here you seem to deduce the usage of the internal PHY by the PHY
>> interface specified in the DT (MII = internal, RGMII = external).
>> I think I raised this question before, but isn't it perfectly legal for
>> a board to use MII with an external PHY even on those SoCs that feature
>> an internal PHY?
>> On the first glance that does not make too much sense, but apart from
>> not being the correct binding to describe all of the SoCs features I see
>> two scenarios:
>> 1) A board vendor might choose to not use the internal PHY because it
>> has bugs, lacks features (configurability) or has other issues. For
>> instance I have heard reports that the internal PHY makes the SoC go
>> rather hot, possibly limiting the CPU frequency. By using an external
>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
>> 2) A PHY does not necessarily need to be directly connected to
>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
>> IC or some other network circuitry, for instance fibre connectors.
>>
>> So I was wondering if we would need an explicit:
>> allwinner,use-internal-phy;
>> boolean DT property to signal the usage of the internal PHY?
>> Alternatively we could go with the negative version:
>> allwinner,disable-internal-phy;
>>
>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
>> string for the *PHY* node and use that?
>>
>> I just want to avoid that we introduce a binding that causes us
>> headaches later. I think we can still fix this with a followup patch
>> before the driver and its binding hit a release kernel.
>>
>> Cheers,
>> Andre.
>>
>
> I just see some patch, where "phy-mode = internal" is valid.
> I will try to find a way to use it
Can you provide a link?
I'm not a fan of using phy-mode for this. There's no guarantee what
mode the internal PHY uses. That's what phy-mode is for.
In any case, we should fix this before 4.13 is released.
ChenYu
On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> <[email protected]> wrote:
> > On Mon, Jun 26, 2017 at 01:18:23AM +0100, Andr? Przywara wrote:
> >> On 31/05/17 08:18, Corentin Labbe wrote:
> >> > The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >> > allwinner.
> >> > In fact the only common part is the descriptor management and the first
> >> > register function.
> >>
> >> Hi,
> >>
> >> I know I am a bit late with this, but while adapting the U-Boot driver
> >> to the new binding I was wondering about the internal PHY detection:
> >>
> >>
> >> So here you seem to deduce the usage of the internal PHY by the PHY
> >> interface specified in the DT (MII = internal, RGMII = external).
> >> I think I raised this question before, but isn't it perfectly legal for
> >> a board to use MII with an external PHY even on those SoCs that feature
> >> an internal PHY?
> >> On the first glance that does not make too much sense, but apart from
> >> not being the correct binding to describe all of the SoCs features I see
> >> two scenarios:
> >> 1) A board vendor might choose to not use the internal PHY because it
> >> has bugs, lacks features (configurability) or has other issues. For
> >> instance I have heard reports that the internal PHY makes the SoC go
> >> rather hot, possibly limiting the CPU frequency. By using an external
> >> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
> >> 2) A PHY does not necessarily need to be directly connected to
> >> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
> >> IC or some other network circuitry, for instance fibre connectors.
> >>
> >> So I was wondering if we would need an explicit:
> >> allwinner,use-internal-phy;
> >> boolean DT property to signal the usage of the internal PHY?
> >> Alternatively we could go with the negative version:
> >> allwinner,disable-internal-phy;
> >>
> >> Or what about introducing a new "allwinner,internal-mii-phy" compatible
> >> string for the *PHY* node and use that?
> >>
> >> I just want to avoid that we introduce a binding that causes us
> >> headaches later. I think we can still fix this with a followup patch
> >> before the driver and its binding hit a release kernel.
> >>
> >> Cheers,
> >> Andre.
> >>
> >
> > I just see some patch, where "phy-mode = internal" is valid.
> > I will try to find a way to use it
>
> Can you provide a link?
https://lkml.org/lkml/2017/6/23/479
>
> I'm not a fan of using phy-mode for this. There's no guarantee what
> mode the internal PHY uses. That's what phy-mode is for.
For each soc the internal PHY mode is know and setted in emac_variant/internal_phy
So its not a problem.
Patch comming soon
Hi,
(CC:ing some people from that Rockchip dmwac series)
On 27/06/17 09:21, Corentin Labbe wrote:
> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>> <[email protected]> wrote:
>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>> allwinner.
>>>>> In fact the only common part is the descriptor management and the first
>>>>> register function.
>>>>
>>>> Hi,
>>>>
>>>> I know I am a bit late with this, but while adapting the U-Boot driver
>>>> to the new binding I was wondering about the internal PHY detection:
>>>>
>>>>
>>>> So here you seem to deduce the usage of the internal PHY by the PHY
>>>> interface specified in the DT (MII = internal, RGMII = external).
>>>> I think I raised this question before, but isn't it perfectly legal for
>>>> a board to use MII with an external PHY even on those SoCs that feature
>>>> an internal PHY?
>>>> On the first glance that does not make too much sense, but apart from
>>>> not being the correct binding to describe all of the SoCs features I see
>>>> two scenarios:
>>>> 1) A board vendor might choose to not use the internal PHY because it
>>>> has bugs, lacks features (configurability) or has other issues. For
>>>> instance I have heard reports that the internal PHY makes the SoC go
>>>> rather hot, possibly limiting the CPU frequency. By using an external
>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
>>>> 2) A PHY does not necessarily need to be directly connected to
>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
>>>> IC or some other network circuitry, for instance fibre connectors.
>>>>
>>>> So I was wondering if we would need an explicit:
>>>> allwinner,use-internal-phy;
>>>> boolean DT property to signal the usage of the internal PHY?
>>>> Alternatively we could go with the negative version:
>>>> allwinner,disable-internal-phy;
>>>>
>>>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
>>>> string for the *PHY* node and use that?
>>>>
>>>> I just want to avoid that we introduce a binding that causes us
>>>> headaches later. I think we can still fix this with a followup patch
>>>> before the driver and its binding hit a release kernel.
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>
>>> I just see some patch, where "phy-mode = internal" is valid.
>>> I will try to find a way to use it
>>
>> Can you provide a link?
>
> https://lkml.org/lkml/2017/6/23/479
>
>>
>> I'm not a fan of using phy-mode for this. There's no guarantee what
>> mode the internal PHY uses. That's what phy-mode is for.
I can understand Chen-Yu's concerns, but ...
> For each soc the internal PHY mode is know and setted in emac_variant/internal_phy
> So its not a problem.
that is true as well, at least for now.
So while I agree that having a separate property to indicate the usage
of the internal PHY would be nice, I am bit tempted to use this easier
approach and piggy back on the existing phy-mode property.
Are there any insights from the people involved with the Rockchip
internal PHY?
It is worth to introduce a generic boolean property for an internal PHY?
Or shall we actually move this more into the PHY code, introducing new
compatibles for the internal Allwinner and Rockchip Ethernet PHYs?
Cheers,
Andre.
On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> Hi,
>
> (CC:ing some people from that Rockchip dmwac series)
>
> On 27/06/17 09:21, Corentin Labbe wrote:
> > On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >> <[email protected]> wrote:
> >>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, Andr? Przywara wrote:
> >>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>> allwinner.
> >>>>> In fact the only common part is the descriptor management and the first
> >>>>> register function.
> >>>>
> >>>> Hi,
> >>>>
> >>>> I know I am a bit late with this, but while adapting the U-Boot driver
> >>>> to the new binding I was wondering about the internal PHY detection:
> >>>>
> >>>>
> >>>> So here you seem to deduce the usage of the internal PHY by the PHY
> >>>> interface specified in the DT (MII = internal, RGMII = external).
> >>>> I think I raised this question before, but isn't it perfectly legal for
> >>>> a board to use MII with an external PHY even on those SoCs that feature
> >>>> an internal PHY?
> >>>> On the first glance that does not make too much sense, but apart from
> >>>> not being the correct binding to describe all of the SoCs features I see
> >>>> two scenarios:
> >>>> 1) A board vendor might choose to not use the internal PHY because it
> >>>> has bugs, lacks features (configurability) or has other issues. For
> >>>> instance I have heard reports that the internal PHY makes the SoC go
> >>>> rather hot, possibly limiting the CPU frequency. By using an external
> >>>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
> >>>> 2) A PHY does not necessarily need to be directly connected to
> >>>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
> >>>> IC or some other network circuitry, for instance fibre connectors.
> >>>>
> >>>> So I was wondering if we would need an explicit:
> >>>> allwinner,use-internal-phy;
> >>>> boolean DT property to signal the usage of the internal PHY?
> >>>> Alternatively we could go with the negative version:
> >>>> allwinner,disable-internal-phy;
> >>>>
> >>>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
> >>>> string for the *PHY* node and use that?
> >>>>
> >>>> I just want to avoid that we introduce a binding that causes us
> >>>> headaches later. I think we can still fix this with a followup patch
> >>>> before the driver and its binding hit a release kernel.
> >>>>
> >>>> Cheers,
> >>>> Andre.
> >>>>
> >>>
> >>> I just see some patch, where "phy-mode = internal" is valid.
> >>> I will try to find a way to use it
> >>
> >> Can you provide a link?
> >
> > https://lkml.org/lkml/2017/6/23/479
> >
> >>
> >> I'm not a fan of using phy-mode for this. There's no guarantee what
> >> mode the internal PHY uses. That's what phy-mode is for.
>
> I can understand Chen-Yu's concerns, but ...
>
> > For each soc the internal PHY mode is know and setted in emac_variant/internal_phy
> > So its not a problem.
>
> that is true as well, at least for now.
>
> So while I agree that having a separate property to indicate the usage
> of the internal PHY would be nice, I am bit tempted to use this easier
> approach and piggy back on the existing phy-mode property.
We're trying to fix an issue that works for now too.
If we want to consider future weird cases, then we must consider all
of them. And the phy mode changing is definitely not really far
fetched.
I agree with Chen-Yu, and I really feel like the compatible solution
you suggested would cover both your concerns, and ours.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Tue, Jun 27, 2017 at 5:41 PM, Maxime Ripard
<[email protected]> wrote:
> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>> Hi,
>>
>> (CC:ing some people from that Rockchip dmwac series)
>>
>> On 27/06/17 09:21, Corentin Labbe wrote:
>> > On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>> >> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>> >> <[email protected]> wrote:
>> >>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>> >>>> On 31/05/17 08:18, Corentin Labbe wrote:
>> >>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>> >>>>> allwinner.
>> >>>>> In fact the only common part is the descriptor management and the first
>> >>>>> register function.
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> I know I am a bit late with this, but while adapting the U-Boot driver
>> >>>> to the new binding I was wondering about the internal PHY detection:
>> >>>>
>> >>>>
>> >>>> So here you seem to deduce the usage of the internal PHY by the PHY
>> >>>> interface specified in the DT (MII = internal, RGMII = external).
>> >>>> I think I raised this question before, but isn't it perfectly legal for
>> >>>> a board to use MII with an external PHY even on those SoCs that feature
>> >>>> an internal PHY?
>> >>>> On the first glance that does not make too much sense, but apart from
>> >>>> not being the correct binding to describe all of the SoCs features I see
>> >>>> two scenarios:
>> >>>> 1) A board vendor might choose to not use the internal PHY because it
>> >>>> has bugs, lacks features (configurability) or has other issues. For
>> >>>> instance I have heard reports that the internal PHY makes the SoC go
>> >>>> rather hot, possibly limiting the CPU frequency. By using an external
>> >>>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
>> >>>> 2) A PHY does not necessarily need to be directly connected to
>> >>>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
>> >>>> IC or some other network circuitry, for instance fibre connectors.
>> >>>>
>> >>>> So I was wondering if we would need an explicit:
>> >>>> allwinner,use-internal-phy;
>> >>>> boolean DT property to signal the usage of the internal PHY?
>> >>>> Alternatively we could go with the negative version:
>> >>>> allwinner,disable-internal-phy;
>> >>>>
>> >>>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
>> >>>> string for the *PHY* node and use that?
>> >>>>
>> >>>> I just want to avoid that we introduce a binding that causes us
>> >>>> headaches later. I think we can still fix this with a followup patch
>> >>>> before the driver and its binding hit a release kernel.
>> >>>>
>> >>>> Cheers,
>> >>>> Andre.
>> >>>>
>> >>>
>> >>> I just see some patch, where "phy-mode = internal" is valid.
>> >>> I will try to find a way to use it
>> >>
>> >> Can you provide a link?
>> >
>> > https://lkml.org/lkml/2017/6/23/479
>> >
>> >>
>> >> I'm not a fan of using phy-mode for this. There's no guarantee what
>> >> mode the internal PHY uses. That's what phy-mode is for.
>>
>> I can understand Chen-Yu's concerns, but ...
>>
>> > For each soc the internal PHY mode is know and setted in emac_variant/internal_phy
>> > So its not a problem.
>>
>> that is true as well, at least for now.
>>
>> So while I agree that having a separate property to indicate the usage
>> of the internal PHY would be nice, I am bit tempted to use this easier
>> approach and piggy back on the existing phy-mode property.
>
> We're trying to fix an issue that works for now too.
>
> If we want to consider future weird cases, then we must consider all
> of them. And the phy mode changing is definitely not really far
> fetched.
I guess the issue is whether it's likely that the vendor puts 2 internal
PHYs in one SoC, and they use different modes and can be switched around.
Otherwise it's fixed for a given SoC, and we can just handle that with
the per-SoC GMAC compatible.
Maybe Florian could tell us if this was one of the intended use cases
for the "internal" phy mode.
As for Rockchip, AFAIK they have 2 MACs, one is connected to the internal
PHY, while the other is connected to the external interface, and there is
no muxing involved, unlike Allwinner's solution.
> I agree with Chen-Yu, and I really feel like the compatible solution
> you suggested would cover both your concerns, and ours.
If using a PHY compatible is the solution, we could just use the
"ethernet-phy-idAAAA.BBBB" style one, and put in the bogus ID that
Allwinner used.
Care must be taken to put this at the board level for boards using
the internal PHY, or we'd have to delete or override the property
in all other boards.
Ideally I think the internal PHY device node should _not_ be in
the SoC level .dtsi file. If we select the external interface, then
there's no connection to the internal PHY, and that device node becomes
unusable and bogus. This is something I think should be fixed regardless
of the phy-mode discussion above.
ChenYu
Hi,
On 27/06/17 10:41, Maxime Ripard wrote:
> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>> Hi,
>>
>> (CC:ing some people from that Rockchip dmwac series)
>>
>> On 27/06/17 09:21, Corentin Labbe wrote:
>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>> <[email protected]> wrote:
>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, Andr? Przywara wrote:
>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>>>> allwinner.
>>>>>>> In fact the only common part is the descriptor management and the first
>>>>>>> register function.
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I know I am a bit late with this, but while adapting the U-Boot driver
>>>>>> to the new binding I was wondering about the internal PHY detection:
>>>>>>
>>>>>>
>>>>>> So here you seem to deduce the usage of the internal PHY by the PHY
>>>>>> interface specified in the DT (MII = internal, RGMII = external).
>>>>>> I think I raised this question before, but isn't it perfectly legal for
>>>>>> a board to use MII with an external PHY even on those SoCs that feature
>>>>>> an internal PHY?
>>>>>> On the first glance that does not make too much sense, but apart from
>>>>>> not being the correct binding to describe all of the SoCs features I see
>>>>>> two scenarios:
>>>>>> 1) A board vendor might choose to not use the internal PHY because it
>>>>>> has bugs, lacks features (configurability) or has other issues. For
>>>>>> instance I have heard reports that the internal PHY makes the SoC go
>>>>>> rather hot, possibly limiting the CPU frequency. By using an external
>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
>>>>>> 2) A PHY does not necessarily need to be directly connected to
>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
>>>>>> IC or some other network circuitry, for instance fibre connectors.
>>>>>>
>>>>>> So I was wondering if we would need an explicit:
>>>>>> allwinner,use-internal-phy;
>>>>>> boolean DT property to signal the usage of the internal PHY?
>>>>>> Alternatively we could go with the negative version:
>>>>>> allwinner,disable-internal-phy;
>>>>>>
>>>>>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
>>>>>> string for the *PHY* node and use that?
>>>>>>
>>>>>> I just want to avoid that we introduce a binding that causes us
>>>>>> headaches later. I think we can still fix this with a followup patch
>>>>>> before the driver and its binding hit a release kernel.
>>>>>>
>>>>>> Cheers,
>>>>>> Andre.
>>>>>>
>>>>>
>>>>> I just see some patch, where "phy-mode = internal" is valid.
>>>>> I will try to find a way to use it
>>>>
>>>> Can you provide a link?
>>>
>>> https://lkml.org/lkml/2017/6/23/479
>>>
>>>>
>>>> I'm not a fan of using phy-mode for this. There's no guarantee what
>>>> mode the internal PHY uses. That's what phy-mode is for.
>>
>> I can understand Chen-Yu's concerns, but ...
>>
>>> For each soc the internal PHY mode is know and setted in emac_variant/internal_phy
>>> So its not a problem.
>>
>> that is true as well, at least for now.
>>
>> So while I agree that having a separate property to indicate the usage
>> of the internal PHY would be nice, I am bit tempted to use this easier
>> approach and piggy back on the existing phy-mode property.
>
> We're trying to fix an issue that works for now too.
>
> If we want to consider future weird cases, then we must consider all
> of them. And the phy mode changing is definitely not really far
> fetched.
>
> I agree with Chen-Yu, and I really feel like the compatible solution
> you suggested would cover both your concerns, and ours.
So something like this?
emac: emac@1c30000 {
compatible = "allwinner,sun8i-h3-emac";
...
phy-mode = "mii";
phy-handle = <&int_mii_phy>;
...
mdio: mdio {
#address-cells = <1>;
#size-cells = <0>;
int_mii_phy: ethernet-phy@1 {
compatible = "allwinner,sun8i-h3-ephy";
syscon = <&syscon>;
reg = <1>;
clocks = <&ccu CLK_BUS_EPHY>;
resets = <&ccu RST_BUS_EPHY>;
};
};
};
And then move the internal-PHY setup code into a separate PHY driver?
That looks like the architecturally best solution to me, but is probably
also a bit involved since it would require a separate PHY driver.
Or can we make it simpler, but still use this binding?
Cheers,
Andre.
于 2017年6月27日 GMT+08:00 下午6:11:47, Chen-Yu Tsai <[email protected]> 写到:
>On Tue, Jun 27, 2017 at 5:41 PM, Maxime Ripard
><[email protected]> wrote:
>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>> Hi,
>>>
>>> (CC:ing some people from that Rockchip dmwac series)
>>>
>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>> > On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>> >> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>> >> <[email protected]> wrote:
>>> >>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>> >>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>> >>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware
>by
>>> >>>>> allwinner.
>>> >>>>> In fact the only common part is the descriptor management and
>the first
>>> >>>>> register function.
>>> >>>>
>>> >>>> Hi,
>>> >>>>
>>> >>>> I know I am a bit late with this, but while adapting the U-Boot
>driver
>>> >>>> to the new binding I was wondering about the internal PHY
>detection:
>>> >>>>
>>> >>>>
>>> >>>> So here you seem to deduce the usage of the internal PHY by the
>PHY
>>> >>>> interface specified in the DT (MII = internal, RGMII =
>external).
>>> >>>> I think I raised this question before, but isn't it perfectly
>legal for
>>> >>>> a board to use MII with an external PHY even on those SoCs that
>feature
>>> >>>> an internal PHY?
>>> >>>> On the first glance that does not make too much sense, but
>apart from
>>> >>>> not being the correct binding to describe all of the SoCs
>features I see
>>> >>>> two scenarios:
>>> >>>> 1) A board vendor might choose to not use the internal PHY
>because it
>>> >>>> has bugs, lacks features (configurability) or has other issues.
>For
>>> >>>> instance I have heard reports that the internal PHY makes the
>SoC go
>>> >>>> rather hot, possibly limiting the CPU frequency. By using an
>external
>>> >>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
>avoided.
>>> >>>> 2) A PHY does not necessarily need to be directly connected to
>>> >>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
>switch
>>> >>>> IC or some other network circuitry, for instance fibre
>connectors.
>>> >>>>
>>> >>>> So I was wondering if we would need an explicit:
>>> >>>> allwinner,use-internal-phy;
>>> >>>> boolean DT property to signal the usage of the internal PHY?
>>> >>>> Alternatively we could go with the negative version:
>>> >>>> allwinner,disable-internal-phy;
>>> >>>>
>>> >>>> Or what about introducing a new "allwinner,internal-mii-phy"
>compatible
>>> >>>> string for the *PHY* node and use that?
>>> >>>>
>>> >>>> I just want to avoid that we introduce a binding that causes us
>>> >>>> headaches later. I think we can still fix this with a followup
>patch
>>> >>>> before the driver and its binding hit a release kernel.
>>> >>>>
>>> >>>> Cheers,
>>> >>>> Andre.
>>> >>>>
>>> >>>
>>> >>> I just see some patch, where "phy-mode = internal" is valid.
>>> >>> I will try to find a way to use it
>>> >>
>>> >> Can you provide a link?
>>> >
>>> > https://lkml.org/lkml/2017/6/23/479
>>> >
>>> >>
>>> >> I'm not a fan of using phy-mode for this. There's no guarantee
>what
>>> >> mode the internal PHY uses. That's what phy-mode is for.
>>>
>>> I can understand Chen-Yu's concerns, but ...
>>>
>>> > For each soc the internal PHY mode is know and setted in
>emac_variant/internal_phy
>>> > So its not a problem.
>>>
>>> that is true as well, at least for now.
>>>
>>> So while I agree that having a separate property to indicate the
>usage
>>> of the internal PHY would be nice, I am bit tempted to use this
>easier
>>> approach and piggy back on the existing phy-mode property.
>>
>> We're trying to fix an issue that works for now too.
>>
>> If we want to consider future weird cases, then we must consider all
>> of them. And the phy mode changing is definitely not really far
>> fetched.
>
>I guess the issue is whether it's likely that the vendor puts 2
>internal
>PHYs in one SoC, and they use different modes and can be switched
>around.
>Otherwise it's fixed for a given SoC, and we can just handle that with
>the per-SoC GMAC compatible.
>
>Maybe Florian could tell us if this was one of the intended use cases
>for the "internal" phy mode.
>
>As for Rockchip, AFAIK they have 2 MACs, one is connected to the
>internal
>PHY, while the other is connected to the external interface, and there
>is
>no muxing involved, unlike Allwinner's solution.
>
>> I agree with Chen-Yu, and I really feel like the compatible solution
>> you suggested would cover both your concerns, and ours.
>
>If using a PHY compatible is the solution, we could just use the
>"ethernet-phy-idAAAA.BBBB" style one, and put in the bogus ID that
>Allwinner used.
>
>Care must be taken to put this at the board level for boards using
>the internal PHY, or we'd have to delete or override the property
>in all other boards.
>
>Ideally I think the internal PHY device node should _not_ be in
>the SoC level .dtsi file. If we select the external interface, then
>there's no connection to the internal PHY, and that device node becomes
>unusable and bogus. This is something I think should be fixed
>regardless
>of the phy-mode discussion above.
I think it should be in the SoC DTSI, as it's part of the SoC.
But it makes sense to set status to disabled defaultly.
>
>ChenYu
On Tue, Jun 27, 2017 at 6:17 PM, Icenowy Zheng <[email protected]> wrote:
>
>
> 于 2017年6月27日 GMT+08:00 下午6:11:47, Chen-Yu Tsai <[email protected]> 写到:
>>On Tue, Jun 27, 2017 at 5:41 PM, Maxime Ripard
>><[email protected]> wrote:
>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> (CC:ing some people from that Rockchip dmwac series)
>>>>
>>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>>> > On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>> >> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>> >> <[email protected]> wrote:
>>>> >>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>> >>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>> >>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware
>>by
>>>> >>>>> allwinner.
>>>> >>>>> In fact the only common part is the descriptor management and
>>the first
>>>> >>>>> register function.
>>>> >>>>
>>>> >>>> Hi,
>>>> >>>>
>>>> >>>> I know I am a bit late with this, but while adapting the U-Boot
>>driver
>>>> >>>> to the new binding I was wondering about the internal PHY
>>detection:
>>>> >>>>
>>>> >>>>
>>>> >>>> So here you seem to deduce the usage of the internal PHY by the
>>PHY
>>>> >>>> interface specified in the DT (MII = internal, RGMII =
>>external).
>>>> >>>> I think I raised this question before, but isn't it perfectly
>>legal for
>>>> >>>> a board to use MII with an external PHY even on those SoCs that
>>feature
>>>> >>>> an internal PHY?
>>>> >>>> On the first glance that does not make too much sense, but
>>apart from
>>>> >>>> not being the correct binding to describe all of the SoCs
>>features I see
>>>> >>>> two scenarios:
>>>> >>>> 1) A board vendor might choose to not use the internal PHY
>>because it
>>>> >>>> has bugs, lacks features (configurability) or has other issues.
>>For
>>>> >>>> instance I have heard reports that the internal PHY makes the
>>SoC go
>>>> >>>> rather hot, possibly limiting the CPU frequency. By using an
>>external
>>>> >>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
>>avoided.
>>>> >>>> 2) A PHY does not necessarily need to be directly connected to
>>>> >>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
>>switch
>>>> >>>> IC or some other network circuitry, for instance fibre
>>connectors.
>>>> >>>>
>>>> >>>> So I was wondering if we would need an explicit:
>>>> >>>> allwinner,use-internal-phy;
>>>> >>>> boolean DT property to signal the usage of the internal PHY?
>>>> >>>> Alternatively we could go with the negative version:
>>>> >>>> allwinner,disable-internal-phy;
>>>> >>>>
>>>> >>>> Or what about introducing a new "allwinner,internal-mii-phy"
>>compatible
>>>> >>>> string for the *PHY* node and use that?
>>>> >>>>
>>>> >>>> I just want to avoid that we introduce a binding that causes us
>>>> >>>> headaches later. I think we can still fix this with a followup
>>patch
>>>> >>>> before the driver and its binding hit a release kernel.
>>>> >>>>
>>>> >>>> Cheers,
>>>> >>>> Andre.
>>>> >>>>
>>>> >>>
>>>> >>> I just see some patch, where "phy-mode = internal" is valid.
>>>> >>> I will try to find a way to use it
>>>> >>
>>>> >> Can you provide a link?
>>>> >
>>>> > https://lkml.org/lkml/2017/6/23/479
>>>> >
>>>> >>
>>>> >> I'm not a fan of using phy-mode for this. There's no guarantee
>>what
>>>> >> mode the internal PHY uses. That's what phy-mode is for.
>>>>
>>>> I can understand Chen-Yu's concerns, but ...
>>>>
>>>> > For each soc the internal PHY mode is know and setted in
>>emac_variant/internal_phy
>>>> > So its not a problem.
>>>>
>>>> that is true as well, at least for now.
>>>>
>>>> So while I agree that having a separate property to indicate the
>>usage
>>>> of the internal PHY would be nice, I am bit tempted to use this
>>easier
>>>> approach and piggy back on the existing phy-mode property.
>>>
>>> We're trying to fix an issue that works for now too.
>>>
>>> If we want to consider future weird cases, then we must consider all
>>> of them. And the phy mode changing is definitely not really far
>>> fetched.
>>
>>I guess the issue is whether it's likely that the vendor puts 2
>>internal
>>PHYs in one SoC, and they use different modes and can be switched
>>around.
>>Otherwise it's fixed for a given SoC, and we can just handle that with
>>the per-SoC GMAC compatible.
>>
>>Maybe Florian could tell us if this was one of the intended use cases
>>for the "internal" phy mode.
>>
>>As for Rockchip, AFAIK they have 2 MACs, one is connected to the
>>internal
>>PHY, while the other is connected to the external interface, and there
>>is
>>no muxing involved, unlike Allwinner's solution.
>>
>>> I agree with Chen-Yu, and I really feel like the compatible solution
>>> you suggested would cover both your concerns, and ours.
>>
>>If using a PHY compatible is the solution, we could just use the
>>"ethernet-phy-idAAAA.BBBB" style one, and put in the bogus ID that
>>Allwinner used.
>>
>>Care must be taken to put this at the board level for boards using
>>the internal PHY, or we'd have to delete or override the property
>>in all other boards.
>>
>>Ideally I think the internal PHY device node should _not_ be in
>>the SoC level .dtsi file. If we select the external interface, then
>>there's no connection to the internal PHY, and that device node becomes
>>unusable and bogus. This is something I think should be fixed
>>regardless
>>of the phy-mode discussion above.
>
> I think it should be in the SoC DTSI, as it's part of the SoC.
>
> But it makes sense to set status to disabled defaultly.
Then you end up stepping on it when you add an external PHY
with the same address, and start wondering why things don't
work.
ChenYu
On Tue, Jun 27, 2017 at 6:15 PM, Andre Przywara <[email protected]> wrote:
> Hi,
>
> On 27/06/17 10:41, Maxime Ripard wrote:
>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>> Hi,
>>>
>>> (CC:ing some people from that Rockchip dmwac series)
>>>
>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>>> <[email protected]> wrote:
>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>>>>> allwinner.
>>>>>>>> In fact the only common part is the descriptor management and the first
>>>>>>>> register function.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I know I am a bit late with this, but while adapting the U-Boot driver
>>>>>>> to the new binding I was wondering about the internal PHY detection:
>>>>>>>
>>>>>>>
>>>>>>> So here you seem to deduce the usage of the internal PHY by the PHY
>>>>>>> interface specified in the DT (MII = internal, RGMII = external).
>>>>>>> I think I raised this question before, but isn't it perfectly legal for
>>>>>>> a board to use MII with an external PHY even on those SoCs that feature
>>>>>>> an internal PHY?
>>>>>>> On the first glance that does not make too much sense, but apart from
>>>>>>> not being the correct binding to describe all of the SoCs features I see
>>>>>>> two scenarios:
>>>>>>> 1) A board vendor might choose to not use the internal PHY because it
>>>>>>> has bugs, lacks features (configurability) or has other issues. For
>>>>>>> instance I have heard reports that the internal PHY makes the SoC go
>>>>>>> rather hot, possibly limiting the CPU frequency. By using an external
>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
>>>>>>> 2) A PHY does not necessarily need to be directly connected to
>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
>>>>>>> IC or some other network circuitry, for instance fibre connectors.
>>>>>>>
>>>>>>> So I was wondering if we would need an explicit:
>>>>>>> allwinner,use-internal-phy;
>>>>>>> boolean DT property to signal the usage of the internal PHY?
>>>>>>> Alternatively we could go with the negative version:
>>>>>>> allwinner,disable-internal-phy;
>>>>>>>
>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
>>>>>>> string for the *PHY* node and use that?
>>>>>>>
>>>>>>> I just want to avoid that we introduce a binding that causes us
>>>>>>> headaches later. I think we can still fix this with a followup patch
>>>>>>> before the driver and its binding hit a release kernel.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Andre.
>>>>>>>
>>>>>>
>>>>>> I just see some patch, where "phy-mode = internal" is valid.
>>>>>> I will try to find a way to use it
>>>>>
>>>>> Can you provide a link?
>>>>
>>>> https://lkml.org/lkml/2017/6/23/479
>>>>
>>>>>
>>>>> I'm not a fan of using phy-mode for this. There's no guarantee what
>>>>> mode the internal PHY uses. That's what phy-mode is for.
>>>
>>> I can understand Chen-Yu's concerns, but ...
>>>
>>>> For each soc the internal PHY mode is know and setted in emac_variant/internal_phy
>>>> So its not a problem.
>>>
>>> that is true as well, at least for now.
>>>
>>> So while I agree that having a separate property to indicate the usage
>>> of the internal PHY would be nice, I am bit tempted to use this easier
>>> approach and piggy back on the existing phy-mode property.
>>
>> We're trying to fix an issue that works for now too.
>>
>> If we want to consider future weird cases, then we must consider all
>> of them. And the phy mode changing is definitely not really far
>> fetched.
>>
>> I agree with Chen-Yu, and I really feel like the compatible solution
>> you suggested would cover both your concerns, and ours.
>
> So something like this?
> emac: emac@1c30000 {
> compatible = "allwinner,sun8i-h3-emac";
> ...
> phy-mode = "mii";
> phy-handle = <&int_mii_phy>;
> ...
>
> mdio: mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> int_mii_phy: ethernet-phy@1 {
> compatible = "allwinner,sun8i-h3-ephy";
> syscon = <&syscon>;
> reg = <1>;
> clocks = <&ccu CLK_BUS_EPHY>;
> resets = <&ccu RST_BUS_EPHY>;
> };
> };
> };
>
> And then move the internal-PHY setup code into a separate PHY driver?
>
> That looks like the architecturally best solution to me, but is probably
> also a bit involved since it would require a separate PHY driver.
> Or can we make it simpler, but still use this binding?
This was my initial approach prior to handing it off to Corentin.
The MDIO bus is discoverable, so in the kernel MDIO bus driver code, the
devices are only created if something responds. However, for the EPHY to
respond, you must first configure the clocks, reset controls, and syscon
registers. You need either a platform device driver for that, or do it
in the MAC driver. The latter made more sense at the time, looking at
how the device tree is structured.
ChenYu
于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
>Hi,
>
>On 27/06/17 10:41, Maxime Ripard wrote:
>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>> Hi,
>>>
>>> (CC:ing some people from that Rockchip dmwac series)
>>>
>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>>> <[email protected]> wrote:
>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>>>>> allwinner.
>>>>>>>> In fact the only common part is the descriptor management and
>the first
>>>>>>>> register function.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
>driver
>>>>>>> to the new binding I was wondering about the internal PHY
>detection:
>>>>>>>
>>>>>>>
>>>>>>> So here you seem to deduce the usage of the internal PHY by the
>PHY
>>>>>>> interface specified in the DT (MII = internal, RGMII =
>external).
>>>>>>> I think I raised this question before, but isn't it perfectly
>legal for
>>>>>>> a board to use MII with an external PHY even on those SoCs that
>feature
>>>>>>> an internal PHY?
>>>>>>> On the first glance that does not make too much sense, but apart
>from
>>>>>>> not being the correct binding to describe all of the SoCs
>features I see
>>>>>>> two scenarios:
>>>>>>> 1) A board vendor might choose to not use the internal PHY
>because it
>>>>>>> has bugs, lacks features (configurability) or has other issues.
>For
>>>>>>> instance I have heard reports that the internal PHY makes the
>SoC go
>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
>external
>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
>avoided.
>>>>>>> 2) A PHY does not necessarily need to be directly connected to
>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
>switch
>>>>>>> IC or some other network circuitry, for instance fibre
>connectors.
>>>>>>>
>>>>>>> So I was wondering if we would need an explicit:
>>>>>>> allwinner,use-internal-phy;
>>>>>>> boolean DT property to signal the usage of the internal PHY?
>>>>>>> Alternatively we could go with the negative version:
>>>>>>> allwinner,disable-internal-phy;
>>>>>>>
>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
>compatible
>>>>>>> string for the *PHY* node and use that?
>>>>>>>
>>>>>>> I just want to avoid that we introduce a binding that causes us
>>>>>>> headaches later. I think we can still fix this with a followup
>patch
>>>>>>> before the driver and its binding hit a release kernel.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Andre.
>>>>>>>
>>>>>>
>>>>>> I just see some patch, where "phy-mode = internal" is valid.
>>>>>> I will try to find a way to use it
>>>>>
>>>>> Can you provide a link?
>>>>
>>>> https://lkml.org/lkml/2017/6/23/479
>>>>
>>>>>
>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
>what
>>>>> mode the internal PHY uses. That's what phy-mode is for.
>>>
>>> I can understand Chen-Yu's concerns, but ...
>>>
>>>> For each soc the internal PHY mode is know and setted in
>emac_variant/internal_phy
>>>> So its not a problem.
>>>
>>> that is true as well, at least for now.
>>>
>>> So while I agree that having a separate property to indicate the
>usage
>>> of the internal PHY would be nice, I am bit tempted to use this
>easier
>>> approach and piggy back on the existing phy-mode property.
>>
>> We're trying to fix an issue that works for now too.
>>
>> If we want to consider future weird cases, then we must consider all
>> of them. And the phy mode changing is definitely not really far
>> fetched.
>>
>> I agree with Chen-Yu, and I really feel like the compatible solution
>> you suggested would cover both your concerns, and ours.
>
>So something like this?
> emac: emac@1c30000 {
> compatible = "allwinner,sun8i-h3-emac";
> ...
> phy-mode = "mii";
> phy-handle = <&int_mii_phy>;
> ...
>
> mdio: mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> int_mii_phy: ethernet-phy@1 {
> compatible = "allwinner,sun8i-h3-ephy";
> syscon = <&syscon>;
The MAC still needs to set some bits of syscon register.
> reg = <1>;
> clocks = <&ccu CLK_BUS_EPHY>;
> resets = <&ccu RST_BUS_EPHY>;
> };
> };
> };
>
>And then move the internal-PHY setup code into a separate PHY driver?
>
>That looks like the architecturally best solution to me, but is
>probably
>also a bit involved since it would require a separate PHY driver.
>Or can we make it simpler, but still use this binding?
>
>Cheers,
>Andre.
Hi,
On 27/06/17 11:23, Icenowy Zheng wrote:
>
>
> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
>> Hi,
>>
>> On 27/06/17 10:41, Maxime Ripard wrote:
>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> (CC:ing some people from that Rockchip dmwac series)
>>>>
>>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>>>> <[email protected]> wrote:
>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>>>>>> allwinner.
>>>>>>>>> In fact the only common part is the descriptor management and
>> the first
>>>>>>>>> register function.
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
>> driver
>>>>>>>> to the new binding I was wondering about the internal PHY
>> detection:
>>>>>>>>
>>>>>>>>
>>>>>>>> So here you seem to deduce the usage of the internal PHY by the
>> PHY
>>>>>>>> interface specified in the DT (MII = internal, RGMII =
>> external).
>>>>>>>> I think I raised this question before, but isn't it perfectly
>> legal for
>>>>>>>> a board to use MII with an external PHY even on those SoCs that
>> feature
>>>>>>>> an internal PHY?
>>>>>>>> On the first glance that does not make too much sense, but apart
>> from
>>>>>>>> not being the correct binding to describe all of the SoCs
>> features I see
>>>>>>>> two scenarios:
>>>>>>>> 1) A board vendor might choose to not use the internal PHY
>> because it
>>>>>>>> has bugs, lacks features (configurability) or has other issues.
>> For
>>>>>>>> instance I have heard reports that the internal PHY makes the
>> SoC go
>>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
>> external
>>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
>> avoided.
>>>>>>>> 2) A PHY does not necessarily need to be directly connected to
>>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
>> switch
>>>>>>>> IC or some other network circuitry, for instance fibre
>> connectors.
>>>>>>>>
>>>>>>>> So I was wondering if we would need an explicit:
>>>>>>>> allwinner,use-internal-phy;
>>>>>>>> boolean DT property to signal the usage of the internal PHY?
>>>>>>>> Alternatively we could go with the negative version:
>>>>>>>> allwinner,disable-internal-phy;
>>>>>>>>
>>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
>> compatible
>>>>>>>> string for the *PHY* node and use that?
>>>>>>>>
>>>>>>>> I just want to avoid that we introduce a binding that causes us
>>>>>>>> headaches later. I think we can still fix this with a followup
>> patch
>>>>>>>> before the driver and its binding hit a release kernel.
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Andre.
>>>>>>>>
>>>>>>>
>>>>>>> I just see some patch, where "phy-mode = internal" is valid.
>>>>>>> I will try to find a way to use it
>>>>>>
>>>>>> Can you provide a link?
>>>>>
>>>>> https://lkml.org/lkml/2017/6/23/479
>>>>>
>>>>>>
>>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
>> what
>>>>>> mode the internal PHY uses. That's what phy-mode is for.
>>>>
>>>> I can understand Chen-Yu's concerns, but ...
>>>>
>>>>> For each soc the internal PHY mode is know and setted in
>> emac_variant/internal_phy
>>>>> So its not a problem.
>>>>
>>>> that is true as well, at least for now.
>>>>
>>>> So while I agree that having a separate property to indicate the
>> usage
>>>> of the internal PHY would be nice, I am bit tempted to use this
>> easier
>>>> approach and piggy back on the existing phy-mode property.
>>>
>>> We're trying to fix an issue that works for now too.
>>>
>>> If we want to consider future weird cases, then we must consider all
>>> of them. And the phy mode changing is definitely not really far
>>> fetched.
>>>
>>> I agree with Chen-Yu, and I really feel like the compatible solution
>>> you suggested would cover both your concerns, and ours.
>>
>> So something like this?
>> emac: emac@1c30000 {
>> compatible = "allwinner,sun8i-h3-emac";
>> ...
>> phy-mode = "mii";
>> phy-handle = <&int_mii_phy>;
>> ...
>>
>> mdio: mdio {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> int_mii_phy: ethernet-phy@1 {
>> compatible = "allwinner,sun8i-h3-ephy";
>> syscon = <&syscon>;
>
> The MAC still needs to set some bits of syscon register.
Yes, the syscon property needs also to be in the MAC node, that was
meant to be somewhere in the second "..." ;-)
But now since Chen-Yu mentioned that we need to set up the PHY *first*
to make it actually discoverable via MDIO, I wonder if we could change
this to:
1) have the DT as described here
2) Let the dwmac-sun8i driver peek into the node referenced by
phy-handle and check the compatible string there.
3) If that matches some allwinner internal PHY name, it sets up the PHY
to make it respond when the MDIO driver queries its bus.
Or can a PHY driver set itself up (since we have clocks and resets
properties in there) *before* the MDIO bus gets scanned?
Chen-Yu's comment in the other mail hints at that this is not easily
possible.
Cheers,
Andre.
>
>> reg = <1>;
>> clocks = <&ccu CLK_BUS_EPHY>;
>> resets = <&ccu RST_BUS_EPHY>;
>> };
>> };
>> };
>>
>> And then move the internal-PHY setup code into a separate PHY driver?
>>
>> That looks like the architecturally best solution to me, but is
>> probably
>> also a bit involved since it would require a separate PHY driver.
>> Or can we make it simpler, but still use this binding?
>>
>> Cheers,
>> Andre.
On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> Hi,
>
> On 27/06/17 11:23, Icenowy Zheng wrote:
> >
> >
> > 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
> >> Hi,
> >>
> >> On 27/06/17 10:41, Maxime Ripard wrote:
> >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> >>>> Hi,
> >>>>
> >>>> (CC:ing some people from that Rockchip dmwac series)
> >>>>
> >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >>>>>> <[email protected]> wrote:
> >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>>>>>> allwinner.
> >>>>>>>>> In fact the only common part is the descriptor management and
> >> the first
> >>>>>>>>> register function.
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> >> driver
> >>>>>>>> to the new binding I was wondering about the internal PHY
> >> detection:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> >> PHY
> >>>>>>>> interface specified in the DT (MII = internal, RGMII =
> >> external).
> >>>>>>>> I think I raised this question before, but isn't it perfectly
> >> legal for
> >>>>>>>> a board to use MII with an external PHY even on those SoCs that
> >> feature
> >>>>>>>> an internal PHY?
> >>>>>>>> On the first glance that does not make too much sense, but apart
> >> from
> >>>>>>>> not being the correct binding to describe all of the SoCs
> >> features I see
> >>>>>>>> two scenarios:
> >>>>>>>> 1) A board vendor might choose to not use the internal PHY
> >> because it
> >>>>>>>> has bugs, lacks features (configurability) or has other issues.
> >> For
> >>>>>>>> instance I have heard reports that the internal PHY makes the
> >> SoC go
> >>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> >> external
> >>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> >> avoided.
> >>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> >>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> >> switch
> >>>>>>>> IC or some other network circuitry, for instance fibre
> >> connectors.
> >>>>>>>>
> >>>>>>>> So I was wondering if we would need an explicit:
> >>>>>>>> allwinner,use-internal-phy;
> >>>>>>>> boolean DT property to signal the usage of the internal PHY?
> >>>>>>>> Alternatively we could go with the negative version:
> >>>>>>>> allwinner,disable-internal-phy;
> >>>>>>>>
> >>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> >> compatible
> >>>>>>>> string for the *PHY* node and use that?
> >>>>>>>>
> >>>>>>>> I just want to avoid that we introduce a binding that causes us
> >>>>>>>> headaches later. I think we can still fix this with a followup
> >> patch
> >>>>>>>> before the driver and its binding hit a release kernel.
> >>>>>>>>
> >>>>>>>> Cheers,
> >>>>>>>> Andre.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I just see some patch, where "phy-mode = internal" is valid.
> >>>>>>> I will try to find a way to use it
> >>>>>>
> >>>>>> Can you provide a link?
> >>>>>
> >>>>> https://lkml.org/lkml/2017/6/23/479
> >>>>>
> >>>>>>
> >>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
> >> what
> >>>>>> mode the internal PHY uses. That's what phy-mode is for.
> >>>>
> >>>> I can understand Chen-Yu's concerns, but ...
> >>>>
> >>>>> For each soc the internal PHY mode is know and setted in
> >> emac_variant/internal_phy
> >>>>> So its not a problem.
> >>>>
> >>>> that is true as well, at least for now.
> >>>>
> >>>> So while I agree that having a separate property to indicate the
> >> usage
> >>>> of the internal PHY would be nice, I am bit tempted to use this
> >> easier
> >>>> approach and piggy back on the existing phy-mode property.
> >>>
> >>> We're trying to fix an issue that works for now too.
> >>>
> >>> If we want to consider future weird cases, then we must consider all
> >>> of them. And the phy mode changing is definitely not really far
> >>> fetched.
> >>>
> >>> I agree with Chen-Yu, and I really feel like the compatible solution
> >>> you suggested would cover both your concerns, and ours.
> >>
> >> So something like this?
> >> emac: emac@1c30000 {
> >> compatible = "allwinner,sun8i-h3-emac";
> >> ...
> >> phy-mode = "mii";
> >> phy-handle = <&int_mii_phy>;
> >> ...
> >>
> >> mdio: mdio {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> int_mii_phy: ethernet-phy@1 {
> >> compatible = "allwinner,sun8i-h3-ephy";
> >> syscon = <&syscon>;
> >
> > The MAC still needs to set some bits of syscon register.
>
> Yes, the syscon property needs also to be in the MAC node, that was
> meant to be somewhere in the second "..." ;-)
>
> But now since Chen-Yu mentioned that we need to set up the PHY *first*
> to make it actually discoverable via MDIO, I wonder if we could change
> this to:
> 1) have the DT as described here
> 2) Let the dwmac-sun8i driver peek into the node referenced by
> phy-handle and check the compatible string there.
> 3) If that matches some allwinner internal PHY name, it sets up the PHY
> to make it respond when the MDIO driver queries its bus.
>
> Or can a PHY driver set itself up (since we have clocks and resets
> properties in there) *before* the MDIO bus gets scanned?
> Chen-Yu's comment in the other mail hints at that this is not easily
> possible.
>
> Cheers,
> Andre.
>
I think adding phy compatible just make things more complex.
I think the patch series I sent early fix all problems without more complexity since:
- it does not add more DT stuff
- it use an already used in tree DT phy-mode "internal" (and so phy mode PHY_INTERFACE_MODE_INTERNAL)
Regards
On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> Hi,
>
> On 27/06/17 11:23, Icenowy Zheng wrote:
> >
> >
> > 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
> >> Hi,
> >>
> >> On 27/06/17 10:41, Maxime Ripard wrote:
> >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> >>>> Hi,
> >>>>
> >>>> (CC:ing some people from that Rockchip dmwac series)
> >>>>
> >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >>>>>> <[email protected]> wrote:
> >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>>>>>> allwinner.
> >>>>>>>>> In fact the only common part is the descriptor management and
> >> the first
> >>>>>>>>> register function.
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> >> driver
> >>>>>>>> to the new binding I was wondering about the internal PHY
> >> detection:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> >> PHY
> >>>>>>>> interface specified in the DT (MII = internal, RGMII =
> >> external).
> >>>>>>>> I think I raised this question before, but isn't it perfectly
> >> legal for
> >>>>>>>> a board to use MII with an external PHY even on those SoCs that
> >> feature
> >>>>>>>> an internal PHY?
> >>>>>>>> On the first glance that does not make too much sense, but apart
> >> from
> >>>>>>>> not being the correct binding to describe all of the SoCs
> >> features I see
> >>>>>>>> two scenarios:
> >>>>>>>> 1) A board vendor might choose to not use the internal PHY
> >> because it
> >>>>>>>> has bugs, lacks features (configurability) or has other issues.
> >> For
> >>>>>>>> instance I have heard reports that the internal PHY makes the
> >> SoC go
> >>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> >> external
> >>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> >> avoided.
> >>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> >>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> >> switch
> >>>>>>>> IC or some other network circuitry, for instance fibre
> >> connectors.
> >>>>>>>>
> >>>>>>>> So I was wondering if we would need an explicit:
> >>>>>>>> allwinner,use-internal-phy;
> >>>>>>>> boolean DT property to signal the usage of the internal PHY?
> >>>>>>>> Alternatively we could go with the negative version:
> >>>>>>>> allwinner,disable-internal-phy;
> >>>>>>>>
> >>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> >> compatible
> >>>>>>>> string for the *PHY* node and use that?
> >>>>>>>>
> >>>>>>>> I just want to avoid that we introduce a binding that causes us
> >>>>>>>> headaches later. I think we can still fix this with a followup
> >> patch
> >>>>>>>> before the driver and its binding hit a release kernel.
> >>>>>>>>
> >>>>>>>> Cheers,
> >>>>>>>> Andre.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I just see some patch, where "phy-mode = internal" is valid.
> >>>>>>> I will try to find a way to use it
> >>>>>>
> >>>>>> Can you provide a link?
> >>>>>
> >>>>> https://lkml.org/lkml/2017/6/23/479
> >>>>>
> >>>>>>
> >>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
> >> what
> >>>>>> mode the internal PHY uses. That's what phy-mode is for.
> >>>>
> >>>> I can understand Chen-Yu's concerns, but ...
> >>>>
> >>>>> For each soc the internal PHY mode is know and setted in
> >> emac_variant/internal_phy
> >>>>> So its not a problem.
> >>>>
> >>>> that is true as well, at least for now.
> >>>>
> >>>> So while I agree that having a separate property to indicate the
> >> usage
> >>>> of the internal PHY would be nice, I am bit tempted to use this
> >> easier
> >>>> approach and piggy back on the existing phy-mode property.
> >>>
> >>> We're trying to fix an issue that works for now too.
> >>>
> >>> If we want to consider future weird cases, then we must consider all
> >>> of them. And the phy mode changing is definitely not really far
> >>> fetched.
> >>>
> >>> I agree with Chen-Yu, and I really feel like the compatible solution
> >>> you suggested would cover both your concerns, and ours.
> >>
> >> So something like this?
> >> emac: emac@1c30000 {
> >> compatible = "allwinner,sun8i-h3-emac";
> >> ...
> >> phy-mode = "mii";
> >> phy-handle = <&int_mii_phy>;
> >> ...
> >>
> >> mdio: mdio {
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> int_mii_phy: ethernet-phy@1 {
> >> compatible = "allwinner,sun8i-h3-ephy";
> >> syscon = <&syscon>;
> >
> > The MAC still needs to set some bits of syscon register.
>
> Yes, the syscon property needs also to be in the MAC node, that was
> meant to be somewhere in the second "..." ;-)
>
> But now since Chen-Yu mentioned that we need to set up the PHY *first*
> to make it actually discoverable via MDIO, I wonder if we could change
> this to:
> 1) have the DT as described here
> 2) Let the dwmac-sun8i driver peek into the node referenced by
> phy-handle and check the compatible string there.
> 3) If that matches some allwinner internal PHY name, it sets up the PHY
> to make it respond when the MDIO driver queries its bus.
That would be quite easy to do, and would be my implementation of
choice yes.
> Or can a PHY driver set itself up (since we have clocks and resets
> properties in there) *before* the MDIO bus gets scanned?
> Chen-Yu's comment in the other mail hints at that this is not easily
> possible.
Yeah, that's not easily doable, it would require your driver to probe
before your device has showed up, which is not quite what the driver
model is made like.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> > Hi,
> >
> > On 27/06/17 11:23, Icenowy Zheng wrote:
> > >
> > >
> > > 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
> > >> Hi,
> > >>
> > >> On 27/06/17 10:41, Maxime Ripard wrote:
> > >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> > >>>> Hi,
> > >>>>
> > >>>> (CC:ing some people from that Rockchip dmwac series)
> > >>>>
> > >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> > >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> > >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> > >>>>>> <[email protected]> wrote:
> > >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> > >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> > >>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> > >>>>>>>>> allwinner.
> > >>>>>>>>> In fact the only common part is the descriptor management and
> > >> the first
> > >>>>>>>>> register function.
> > >>>>>>>>
> > >>>>>>>> Hi,
> > >>>>>>>>
> > >>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> > >> driver
> > >>>>>>>> to the new binding I was wondering about the internal PHY
> > >> detection:
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> > >> PHY
> > >>>>>>>> interface specified in the DT (MII = internal, RGMII =
> > >> external).
> > >>>>>>>> I think I raised this question before, but isn't it perfectly
> > >> legal for
> > >>>>>>>> a board to use MII with an external PHY even on those SoCs that
> > >> feature
> > >>>>>>>> an internal PHY?
> > >>>>>>>> On the first glance that does not make too much sense, but apart
> > >> from
> > >>>>>>>> not being the correct binding to describe all of the SoCs
> > >> features I see
> > >>>>>>>> two scenarios:
> > >>>>>>>> 1) A board vendor might choose to not use the internal PHY
> > >> because it
> > >>>>>>>> has bugs, lacks features (configurability) or has other issues.
> > >> For
> > >>>>>>>> instance I have heard reports that the internal PHY makes the
> > >> SoC go
> > >>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> > >> external
> > >>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> > >> avoided.
> > >>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> > >>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> > >> switch
> > >>>>>>>> IC or some other network circuitry, for instance fibre
> > >> connectors.
> > >>>>>>>>
> > >>>>>>>> So I was wondering if we would need an explicit:
> > >>>>>>>> allwinner,use-internal-phy;
> > >>>>>>>> boolean DT property to signal the usage of the internal PHY?
> > >>>>>>>> Alternatively we could go with the negative version:
> > >>>>>>>> allwinner,disable-internal-phy;
> > >>>>>>>>
> > >>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> > >> compatible
> > >>>>>>>> string for the *PHY* node and use that?
> > >>>>>>>>
> > >>>>>>>> I just want to avoid that we introduce a binding that causes us
> > >>>>>>>> headaches later. I think we can still fix this with a followup
> > >> patch
> > >>>>>>>> before the driver and its binding hit a release kernel.
> > >>>>>>>>
> > >>>>>>>> Cheers,
> > >>>>>>>> Andre.
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> I just see some patch, where "phy-mode = internal" is valid.
> > >>>>>>> I will try to find a way to use it
> > >>>>>>
> > >>>>>> Can you provide a link?
> > >>>>>
> > >>>>> https://lkml.org/lkml/2017/6/23/479
> > >>>>>
> > >>>>>>
> > >>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
> > >> what
> > >>>>>> mode the internal PHY uses. That's what phy-mode is for.
> > >>>>
> > >>>> I can understand Chen-Yu's concerns, but ...
> > >>>>
> > >>>>> For each soc the internal PHY mode is know and setted in
> > >> emac_variant/internal_phy
> > >>>>> So its not a problem.
> > >>>>
> > >>>> that is true as well, at least for now.
> > >>>>
> > >>>> So while I agree that having a separate property to indicate
> > >>>> the usage of the internal PHY would be nice, I am bit tempted
> > >>>> to use this easier approach and piggy back on the existing
> > >>>> phy-mode property.
> > >>>
> > >>> We're trying to fix an issue that works for now too.
> > >>>
> > >>> If we want to consider future weird cases, then we must
> > >>> consider all of them. And the phy mode changing is definitely
> > >>> not really far fetched.
> > >>>
> > >>> I agree with Chen-Yu, and I really feel like the compatible
> > >>> solution you suggested would cover both your concerns, and
> > >>> ours.
> > >>
> > >> So something like this?
> > >> emac: emac@1c30000 {
> > >> compatible = "allwinner,sun8i-h3-emac";
> > >> ...
> > >> phy-mode = "mii";
> > >> phy-handle = <&int_mii_phy>;
> > >> ...
> > >>
> > >> mdio: mdio {
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >> int_mii_phy: ethernet-phy@1 {
> > >> compatible = "allwinner,sun8i-h3-ephy";
> > >> syscon = <&syscon>;
> > >
> > > The MAC still needs to set some bits of syscon register.
> >
> > Yes, the syscon property needs also to be in the MAC node, that
> > was meant to be somewhere in the second "..." ;-)
> >
> > But now since Chen-Yu mentioned that we need to set up the PHY *first*
> > to make it actually discoverable via MDIO, I wonder if we could change
> > this to:
> > 1) have the DT as described here
> > 2) Let the dwmac-sun8i driver peek into the node referenced by
> > phy-handle and check the compatible string there.
> > 3) If that matches some allwinner internal PHY name, it sets up the PHY
> > to make it respond when the MDIO driver queries its bus.
> >
> > Or can a PHY driver set itself up (since we have clocks and resets
> > properties in there) *before* the MDIO bus gets scanned?
> > Chen-Yu's comment in the other mail hints at that this is not easily
> > possible.
>
> I think adding phy compatible just make things more complex.
>
> I think the patch series I sent early fix all problems without more
> complexity since:
>
> - it does not add more DT stuff
> - it use an already used in tree DT phy-mode "internal" (and so phy
> mode PHY_INTERFACE_MODE_INTERNAL)
- it doesn't cover all the concerns we had
- it uses an undocumented value, with an unclear implication
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 06/27/2017 10:29 AM, Maxime Ripard wrote:
> On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
>> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 27/06/17 11:23, Icenowy Zheng wrote:
>>>>
>>>>
>>>> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
>>>>> Hi,
>>>>>
>>>>> On 27/06/17 10:41, Maxime Ripard wrote:
>>>>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> (CC:ing some people from that Rockchip dmwac series)
>>>>>>>
>>>>>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>>>>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>>>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>>>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>>>>>>>>> allwinner.
>>>>>>>>>>>> In fact the only common part is the descriptor management and
>>>>> the first
>>>>>>>>>>>> register function.
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
>>>>> driver
>>>>>>>>>>> to the new binding I was wondering about the internal PHY
>>>>> detection:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So here you seem to deduce the usage of the internal PHY by the
>>>>> PHY
>>>>>>>>>>> interface specified in the DT (MII = internal, RGMII =
>>>>> external).
>>>>>>>>>>> I think I raised this question before, but isn't it perfectly
>>>>> legal for
>>>>>>>>>>> a board to use MII with an external PHY even on those SoCs that
>>>>> feature
>>>>>>>>>>> an internal PHY?
>>>>>>>>>>> On the first glance that does not make too much sense, but apart
>>>>> from
>>>>>>>>>>> not being the correct binding to describe all of the SoCs
>>>>> features I see
>>>>>>>>>>> two scenarios:
>>>>>>>>>>> 1) A board vendor might choose to not use the internal PHY
>>>>> because it
>>>>>>>>>>> has bugs, lacks features (configurability) or has other issues.
>>>>> For
>>>>>>>>>>> instance I have heard reports that the internal PHY makes the
>>>>> SoC go
>>>>>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
>>>>> external
>>>>>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
>>>>> avoided.
>>>>>>>>>>> 2) A PHY does not necessarily need to be directly connected to
>>>>>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
>>>>> switch
>>>>>>>>>>> IC or some other network circuitry, for instance fibre
>>>>> connectors.
>>>>>>>>>>>
>>>>>>>>>>> So I was wondering if we would need an explicit:
>>>>>>>>>>> allwinner,use-internal-phy;
>>>>>>>>>>> boolean DT property to signal the usage of the internal PHY?
>>>>>>>>>>> Alternatively we could go with the negative version:
>>>>>>>>>>> allwinner,disable-internal-phy;
>>>>>>>>>>>
>>>>>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
>>>>> compatible
>>>>>>>>>>> string for the *PHY* node and use that?
>>>>>>>>>>>
>>>>>>>>>>> I just want to avoid that we introduce a binding that causes us
>>>>>>>>>>> headaches later. I think we can still fix this with a followup
>>>>> patch
>>>>>>>>>>> before the driver and its binding hit a release kernel.
>>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> Andre.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I just see some patch, where "phy-mode = internal" is valid.
>>>>>>>>>> I will try to find a way to use it
>>>>>>>>>
>>>>>>>>> Can you provide a link?
>>>>>>>>
>>>>>>>> https://lkml.org/lkml/2017/6/23/479
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
>>>>> what
>>>>>>>>> mode the internal PHY uses. That's what phy-mode is for.
>>>>>>>
>>>>>>> I can understand Chen-Yu's concerns, but ...
>>>>>>>
>>>>>>>> For each soc the internal PHY mode is know and setted in
>>>>> emac_variant/internal_phy
>>>>>>>> So its not a problem.
>>>>>>>
>>>>>>> that is true as well, at least for now.
>>>>>>>
>>>>>>> So while I agree that having a separate property to indicate
>>>>>>> the usage of the internal PHY would be nice, I am bit tempted
>>>>>>> to use this easier approach and piggy back on the existing
>>>>>>> phy-mode property.
>>>>>>
>>>>>> We're trying to fix an issue that works for now too.
>>>>>>
>>>>>> If we want to consider future weird cases, then we must
>>>>>> consider all of them. And the phy mode changing is definitely
>>>>>> not really far fetched.
>>>>>>
>>>>>> I agree with Chen-Yu, and I really feel like the compatible
>>>>>> solution you suggested would cover both your concerns, and
>>>>>> ours.
>>>>>
>>>>> So something like this?
>>>>> emac: emac@1c30000 {
>>>>> compatible = "allwinner,sun8i-h3-emac";
>>>>> ...
>>>>> phy-mode = "mii";
>>>>> phy-handle = <&int_mii_phy>;
>>>>> ...
>>>>>
>>>>> mdio: mdio {
>>>>> #address-cells = <1>;
>>>>> #size-cells = <0>;
>>>>> int_mii_phy: ethernet-phy@1 {
>>>>> compatible = "allwinner,sun8i-h3-ephy";
>>>>> syscon = <&syscon>;
>>>>
>>>> The MAC still needs to set some bits of syscon register.
>>>
>>> Yes, the syscon property needs also to be in the MAC node, that
>>> was meant to be somewhere in the second "..." ;-)
>>>
>>> But now since Chen-Yu mentioned that we need to set up the PHY *first*
>>> to make it actually discoverable via MDIO, I wonder if we could change
>>> this to:
>>> 1) have the DT as described here
>>> 2) Let the dwmac-sun8i driver peek into the node referenced by
>>> phy-handle and check the compatible string there.
>>> 3) If that matches some allwinner internal PHY name, it sets up the PHY
>>> to make it respond when the MDIO driver queries its bus.
>>>
>>> Or can a PHY driver set itself up (since we have clocks and resets
>>> properties in there) *before* the MDIO bus gets scanned?
>>> Chen-Yu's comment in the other mail hints at that this is not easily
>>> possible.
>>
>> I think adding phy compatible just make things more complex.
>>
>> I think the patch series I sent early fix all problems without more
>> complexity since:
>>
>> - it does not add more DT stuff
>> - it use an already used in tree DT phy-mode "internal" (and so phy
>> mode PHY_INTERFACE_MODE_INTERNAL)
>
> - it doesn't cover all the concerns we ha> - it uses an undocumented value, with an unclear implication
No it's no longer undocumented since [1]
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=29b65f5f97632722bb80969377e5b0e2401fb392
Due to the timezone difference, you guys have already managed to have
several exchanges, hopefully I will have a chance to review your
discussions a little later today.
--
Florian
On Tue, Jun 27, 2017 at 07:29:37PM +0200, Maxime Ripard wrote:
> On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
> > On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> > > Hi,
> > >
> > > On 27/06/17 11:23, Icenowy Zheng wrote:
> > > >
> > > >
> > > > 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
> > > >> Hi,
> > > >>
> > > >> On 27/06/17 10:41, Maxime Ripard wrote:
> > > >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> > > >>>> Hi,
> > > >>>>
> > > >>>> (CC:ing some people from that Rockchip dmwac series)
> > > >>>>
> > > >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> > > >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> > > >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> > > >>>>>> <[email protected]> wrote:
> > > >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> > > >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> > > >>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> > > >>>>>>>>> allwinner.
> > > >>>>>>>>> In fact the only common part is the descriptor management and
> > > >> the first
> > > >>>>>>>>> register function.
> > > >>>>>>>>
> > > >>>>>>>> Hi,
> > > >>>>>>>>
> > > >>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> > > >> driver
> > > >>>>>>>> to the new binding I was wondering about the internal PHY
> > > >> detection:
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> > > >> PHY
> > > >>>>>>>> interface specified in the DT (MII = internal, RGMII =
> > > >> external).
> > > >>>>>>>> I think I raised this question before, but isn't it perfectly
> > > >> legal for
> > > >>>>>>>> a board to use MII with an external PHY even on those SoCs that
> > > >> feature
> > > >>>>>>>> an internal PHY?
> > > >>>>>>>> On the first glance that does not make too much sense, but apart
> > > >> from
> > > >>>>>>>> not being the correct binding to describe all of the SoCs
> > > >> features I see
> > > >>>>>>>> two scenarios:
> > > >>>>>>>> 1) A board vendor might choose to not use the internal PHY
> > > >> because it
> > > >>>>>>>> has bugs, lacks features (configurability) or has other issues.
> > > >> For
> > > >>>>>>>> instance I have heard reports that the internal PHY makes the
> > > >> SoC go
> > > >>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> > > >> external
> > > >>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> > > >> avoided.
> > > >>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> > > >>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> > > >> switch
> > > >>>>>>>> IC or some other network circuitry, for instance fibre
> > > >> connectors.
> > > >>>>>>>>
> > > >>>>>>>> So I was wondering if we would need an explicit:
> > > >>>>>>>> allwinner,use-internal-phy;
> > > >>>>>>>> boolean DT property to signal the usage of the internal PHY?
> > > >>>>>>>> Alternatively we could go with the negative version:
> > > >>>>>>>> allwinner,disable-internal-phy;
> > > >>>>>>>>
> > > >>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> > > >> compatible
> > > >>>>>>>> string for the *PHY* node and use that?
> > > >>>>>>>>
> > > >>>>>>>> I just want to avoid that we introduce a binding that causes us
> > > >>>>>>>> headaches later. I think we can still fix this with a followup
> > > >> patch
> > > >>>>>>>> before the driver and its binding hit a release kernel.
> > > >>>>>>>>
> > > >>>>>>>> Cheers,
> > > >>>>>>>> Andre.
> > > >>>>>>>>
> > > >>>>>>>
> > > >>>>>>> I just see some patch, where "phy-mode = internal" is valid.
> > > >>>>>>> I will try to find a way to use it
> > > >>>>>>
> > > >>>>>> Can you provide a link?
> > > >>>>>
> > > >>>>> https://lkml.org/lkml/2017/6/23/479
> > > >>>>>
> > > >>>>>>
> > > >>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
> > > >> what
> > > >>>>>> mode the internal PHY uses. That's what phy-mode is for.
> > > >>>>
> > > >>>> I can understand Chen-Yu's concerns, but ...
> > > >>>>
> > > >>>>> For each soc the internal PHY mode is know and setted in
> > > >> emac_variant/internal_phy
> > > >>>>> So its not a problem.
> > > >>>>
> > > >>>> that is true as well, at least for now.
> > > >>>>
> > > >>>> So while I agree that having a separate property to indicate
> > > >>>> the usage of the internal PHY would be nice, I am bit tempted
> > > >>>> to use this easier approach and piggy back on the existing
> > > >>>> phy-mode property.
> > > >>>
> > > >>> We're trying to fix an issue that works for now too.
> > > >>>
> > > >>> If we want to consider future weird cases, then we must
> > > >>> consider all of them. And the phy mode changing is definitely
> > > >>> not really far fetched.
> > > >>>
> > > >>> I agree with Chen-Yu, and I really feel like the compatible
> > > >>> solution you suggested would cover both your concerns, and
> > > >>> ours.
> > > >>
> > > >> So something like this?
> > > >> emac: emac@1c30000 {
> > > >> compatible = "allwinner,sun8i-h3-emac";
> > > >> ...
> > > >> phy-mode = "mii";
> > > >> phy-handle = <&int_mii_phy>;
> > > >> ...
> > > >>
> > > >> mdio: mdio {
> > > >> #address-cells = <1>;
> > > >> #size-cells = <0>;
> > > >> int_mii_phy: ethernet-phy@1 {
> > > >> compatible = "allwinner,sun8i-h3-ephy";
> > > >> syscon = <&syscon>;
> > > >
> > > > The MAC still needs to set some bits of syscon register.
> > >
> > > Yes, the syscon property needs also to be in the MAC node, that
> > > was meant to be somewhere in the second "..." ;-)
> > >
> > > But now since Chen-Yu mentioned that we need to set up the PHY *first*
> > > to make it actually discoverable via MDIO, I wonder if we could change
> > > this to:
> > > 1) have the DT as described here
> > > 2) Let the dwmac-sun8i driver peek into the node referenced by
> > > phy-handle and check the compatible string there.
> > > 3) If that matches some allwinner internal PHY name, it sets up the PHY
> > > to make it respond when the MDIO driver queries its bus.
> > >
> > > Or can a PHY driver set itself up (since we have clocks and resets
> > > properties in there) *before* the MDIO bus gets scanned?
> > > Chen-Yu's comment in the other mail hints at that this is not easily
> > > possible.
> >
> > I think adding phy compatible just make things more complex.
> >
> > I think the patch series I sent early fix all problems without more
> > complexity since:
> >
> > - it does not add more DT stuff
> > - it use an already used in tree DT phy-mode "internal" (and so phy
> > mode PHY_INTERFACE_MODE_INTERNAL)
>
> - it doesn't cover all the concerns we had
> - it uses an undocumented value, with an unclear implication
>
The answer from Florian anyway breaks my logic, internal is for "internal phy with non-xMII protocol" not just internal PHY
Regards
On Tue, Jun 27, 2017 at 10:37:34AM -0700, Florian Fainelli wrote:
> On 06/27/2017 10:29 AM, Maxime Ripard wrote:
> > On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
> >> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> >>> Hi,
> >>>
> >>> On 27/06/17 11:23, Icenowy Zheng wrote:
> >>>>
> >>>>
> >>>> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
> >>>>> Hi,
> >>>>>
> >>>>> On 27/06/17 10:41, Maxime Ripard wrote:
> >>>>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> (CC:ing some people from that Rockchip dmwac series)
> >>>>>>>
> >>>>>>> On 27/06/17 09:21, Corentin Labbe wrote:
> >>>>>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >>>>>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> >>>>>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>>>>>>>>> allwinner.
> >>>>>>>>>>>> In fact the only common part is the descriptor management and
> >>>>> the first
> >>>>>>>>>>>> register function.
> >>>>>>>>>>>
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> >>>>> driver
> >>>>>>>>>>> to the new binding I was wondering about the internal PHY
> >>>>> detection:
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> >>>>> PHY
> >>>>>>>>>>> interface specified in the DT (MII = internal, RGMII =
> >>>>> external).
> >>>>>>>>>>> I think I raised this question before, but isn't it perfectly
> >>>>> legal for
> >>>>>>>>>>> a board to use MII with an external PHY even on those SoCs that
> >>>>> feature
> >>>>>>>>>>> an internal PHY?
> >>>>>>>>>>> On the first glance that does not make too much sense, but apart
> >>>>> from
> >>>>>>>>>>> not being the correct binding to describe all of the SoCs
> >>>>> features I see
> >>>>>>>>>>> two scenarios:
> >>>>>>>>>>> 1) A board vendor might choose to not use the internal PHY
> >>>>> because it
> >>>>>>>>>>> has bugs, lacks features (configurability) or has other issues.
> >>>>> For
> >>>>>>>>>>> instance I have heard reports that the internal PHY makes the
> >>>>> SoC go
> >>>>>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> >>>>> external
> >>>>>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> >>>>> avoided.
> >>>>>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> >>>>>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> >>>>> switch
> >>>>>>>>>>> IC or some other network circuitry, for instance fibre
> >>>>> connectors.
> >>>>>>>>>>>
> >>>>>>>>>>> So I was wondering if we would need an explicit:
> >>>>>>>>>>> allwinner,use-internal-phy;
> >>>>>>>>>>> boolean DT property to signal the usage of the internal PHY?
> >>>>>>>>>>> Alternatively we could go with the negative version:
> >>>>>>>>>>> allwinner,disable-internal-phy;
> >>>>>>>>>>>
> >>>>>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> >>>>> compatible
> >>>>>>>>>>> string for the *PHY* node and use that?
> >>>>>>>>>>>
> >>>>>>>>>>> I just want to avoid that we introduce a binding that causes us
> >>>>>>>>>>> headaches later. I think we can still fix this with a followup
> >>>>> patch
> >>>>>>>>>>> before the driver and its binding hit a release kernel.
> >>>>>>>>>>>
> >>>>>>>>>>> Cheers,
> >>>>>>>>>>> Andre.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I just see some patch, where "phy-mode = internal" is valid.
> >>>>>>>>>> I will try to find a way to use it
> >>>>>>>>>
> >>>>>>>>> Can you provide a link?
> >>>>>>>>
> >>>>>>>> https://lkml.org/lkml/2017/6/23/479
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
> >>>>> what
> >>>>>>>>> mode the internal PHY uses. That's what phy-mode is for.
> >>>>>>>
> >>>>>>> I can understand Chen-Yu's concerns, but ...
> >>>>>>>
> >>>>>>>> For each soc the internal PHY mode is know and setted in
> >>>>> emac_variant/internal_phy
> >>>>>>>> So its not a problem.
> >>>>>>>
> >>>>>>> that is true as well, at least for now.
> >>>>>>>
> >>>>>>> So while I agree that having a separate property to indicate
> >>>>>>> the usage of the internal PHY would be nice, I am bit tempted
> >>>>>>> to use this easier approach and piggy back on the existing
> >>>>>>> phy-mode property.
> >>>>>>
> >>>>>> We're trying to fix an issue that works for now too.
> >>>>>>
> >>>>>> If we want to consider future weird cases, then we must
> >>>>>> consider all of them. And the phy mode changing is definitely
> >>>>>> not really far fetched.
> >>>>>>
> >>>>>> I agree with Chen-Yu, and I really feel like the compatible
> >>>>>> solution you suggested would cover both your concerns, and
> >>>>>> ours.
> >>>>>
> >>>>> So something like this?
> >>>>> emac: emac@1c30000 {
> >>>>> compatible = "allwinner,sun8i-h3-emac";
> >>>>> ...
> >>>>> phy-mode = "mii";
> >>>>> phy-handle = <&int_mii_phy>;
> >>>>> ...
> >>>>>
> >>>>> mdio: mdio {
> >>>>> #address-cells = <1>;
> >>>>> #size-cells = <0>;
> >>>>> int_mii_phy: ethernet-phy@1 {
> >>>>> compatible = "allwinner,sun8i-h3-ephy";
> >>>>> syscon = <&syscon>;
> >>>>
> >>>> The MAC still needs to set some bits of syscon register.
> >>>
> >>> Yes, the syscon property needs also to be in the MAC node, that
> >>> was meant to be somewhere in the second "..." ;-)
> >>>
> >>> But now since Chen-Yu mentioned that we need to set up the PHY *first*
> >>> to make it actually discoverable via MDIO, I wonder if we could change
> >>> this to:
> >>> 1) have the DT as described here
> >>> 2) Let the dwmac-sun8i driver peek into the node referenced by
> >>> phy-handle and check the compatible string there.
> >>> 3) If that matches some allwinner internal PHY name, it sets up the PHY
> >>> to make it respond when the MDIO driver queries its bus.
> >>>
> >>> Or can a PHY driver set itself up (since we have clocks and resets
> >>> properties in there) *before* the MDIO bus gets scanned?
> >>> Chen-Yu's comment in the other mail hints at that this is not easily
> >>> possible.
> >>
> >> I think adding phy compatible just make things more complex.
> >>
> >> I think the patch series I sent early fix all problems without more
> >> complexity since:
> >>
> >> - it does not add more DT stuff
> >> - it use an already used in tree DT phy-mode "internal" (and so phy
> >> mode PHY_INTERFACE_MODE_INTERNAL)
> >
> > - it doesn't cover all the concerns we ha> - it uses an undocumented value, with an unclear implication
>
> No it's no longer undocumented since [1]
>
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=29b65f5f97632722bb80969377e5b0e2401fb392
>
> Due to the timezone difference, you guys have already managed to have
> several exchanges, hopefully I will have a chance to review your
> discussions a little later today.
Hello
I wait for your comment before sending my reverts patch for http://www.mail-archive.com/[email protected]/msg1431579.html
Could you confirm that internal is only meant for "non xMII internal protocol"
Regards
On 30/06/2017 23:53, Corentin Labbe wrote:
> On Tue, Jun 27, 2017 at 10:37:34AM -0700, Florian Fainelli wrote:
>> On 06/27/2017 10:29 AM, Maxime Ripard wrote:
>>> On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
>>>> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
>>>>> Hi,
>>>>>
>>>>> On 27/06/17 11:23, Icenowy Zheng wrote:
>>>>>>
>>>>>>
>>>>>> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 27/06/17 10:41, Maxime Ripard wrote:
>>>>>>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> (CC:ing some people from that Rockchip dmwac series)
>>>>>>>>>
>>>>>>>>> On 27/06/17 09:21, Corentin Labbe wrote:
>>>>>>>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
>>>>>>>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
>>>>>>>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
>>>>>>>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
>>>>>>>>>>>>>> allwinner.
>>>>>>>>>>>>>> In fact the only common part is the descriptor management and
>>>>>>> the first
>>>>>>>>>>>>>> register function.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
>>>>>>> driver
>>>>>>>>>>>>> to the new binding I was wondering about the internal PHY
>>>>>>> detection:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> So here you seem to deduce the usage of the internal PHY by the
>>>>>>> PHY
>>>>>>>>>>>>> interface specified in the DT (MII = internal, RGMII =
>>>>>>> external).
>>>>>>>>>>>>> I think I raised this question before, but isn't it perfectly
>>>>>>> legal for
>>>>>>>>>>>>> a board to use MII with an external PHY even on those SoCs that
>>>>>>> feature
>>>>>>>>>>>>> an internal PHY?
>>>>>>>>>>>>> On the first glance that does not make too much sense, but apart
>>>>>>> from
>>>>>>>>>>>>> not being the correct binding to describe all of the SoCs
>>>>>>> features I see
>>>>>>>>>>>>> two scenarios:
>>>>>>>>>>>>> 1) A board vendor might choose to not use the internal PHY
>>>>>>> because it
>>>>>>>>>>>>> has bugs, lacks features (configurability) or has other issues.
>>>>>>> For
>>>>>>>>>>>>> instance I have heard reports that the internal PHY makes the
>>>>>>> SoC go
>>>>>>>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
>>>>>>> external
>>>>>>>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
>>>>>>> avoided.
>>>>>>>>>>>>> 2) A PHY does not necessarily need to be directly connected to
>>>>>>>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
>>>>>>> switch
>>>>>>>>>>>>> IC or some other network circuitry, for instance fibre
>>>>>>> connectors.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So I was wondering if we would need an explicit:
>>>>>>>>>>>>> allwinner,use-internal-phy;
>>>>>>>>>>>>> boolean DT property to signal the usage of the internal PHY?
>>>>>>>>>>>>> Alternatively we could go with the negative version:
>>>>>>>>>>>>> allwinner,disable-internal-phy;
>>>>>>>>>>>>>
>>>>>>>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
>>>>>>> compatible
>>>>>>>>>>>>> string for the *PHY* node and use that?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I just want to avoid that we introduce a binding that causes us
>>>>>>>>>>>>> headaches later. I think we can still fix this with a followup
>>>>>>> patch
>>>>>>>>>>>>> before the driver and its binding hit a release kernel.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>> Andre.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I just see some patch, where "phy-mode = internal" is valid.
>>>>>>>>>>>> I will try to find a way to use it
>>>>>>>>>>>
>>>>>>>>>>> Can you provide a link?
>>>>>>>>>>
>>>>>>>>>> https://lkml.org/lkml/2017/6/23/479
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
>>>>>>> what
>>>>>>>>>>> mode the internal PHY uses. That's what phy-mode is for.
>>>>>>>>>
>>>>>>>>> I can understand Chen-Yu's concerns, but ...
>>>>>>>>>
>>>>>>>>>> For each soc the internal PHY mode is know and setted in
>>>>>>> emac_variant/internal_phy
>>>>>>>>>> So its not a problem.
>>>>>>>>>
>>>>>>>>> that is true as well, at least for now.
>>>>>>>>>
>>>>>>>>> So while I agree that having a separate property to indicate
>>>>>>>>> the usage of the internal PHY would be nice, I am bit tempted
>>>>>>>>> to use this easier approach and piggy back on the existing
>>>>>>>>> phy-mode property.
>>>>>>>>
>>>>>>>> We're trying to fix an issue that works for now too.
>>>>>>>>
>>>>>>>> If we want to consider future weird cases, then we must
>>>>>>>> consider all of them. And the phy mode changing is definitely
>>>>>>>> not really far fetched.
>>>>>>>>
>>>>>>>> I agree with Chen-Yu, and I really feel like the compatible
>>>>>>>> solution you suggested would cover both your concerns, and
>>>>>>>> ours.
>>>>>>>
>>>>>>> So something like this?
>>>>>>> emac: emac@1c30000 {
>>>>>>> compatible = "allwinner,sun8i-h3-emac";
>>>>>>> ...
>>>>>>> phy-mode = "mii";
>>>>>>> phy-handle = <&int_mii_phy>;
>>>>>>> ...
>>>>>>>
>>>>>>> mdio: mdio {
>>>>>>> #address-cells = <1>;
>>>>>>> #size-cells = <0>;
>>>>>>> int_mii_phy: ethernet-phy@1 {
>>>>>>> compatible = "allwinner,sun8i-h3-ephy";
>>>>>>> syscon = <&syscon>;
>>>>>>
>>>>>> The MAC still needs to set some bits of syscon register.
>>>>>
>>>>> Yes, the syscon property needs also to be in the MAC node, that
>>>>> was meant to be somewhere in the second "..." ;-)
>>>>>
>>>>> But now since Chen-Yu mentioned that we need to set up the PHY *first*
>>>>> to make it actually discoverable via MDIO, I wonder if we could change
>>>>> this to:
>>>>> 1) have the DT as described here
>>>>> 2) Let the dwmac-sun8i driver peek into the node referenced by
>>>>> phy-handle and check the compatible string there.
>>>>> 3) If that matches some allwinner internal PHY name, it sets up the PHY
>>>>> to make it respond when the MDIO driver queries its bus.
>>>>>
>>>>> Or can a PHY driver set itself up (since we have clocks and resets
>>>>> properties in there) *before* the MDIO bus gets scanned?
>>>>> Chen-Yu's comment in the other mail hints at that this is not easily
>>>>> possible.
>>>>
>>>> I think adding phy compatible just make things more complex.
>>>>
>>>> I think the patch series I sent early fix all problems without more
>>>> complexity since:
>>>>
>>>> - it does not add more DT stuff
>>>> - it use an already used in tree DT phy-mode "internal" (and so phy
>>>> mode PHY_INTERFACE_MODE_INTERNAL)
>>>
>>> - it doesn't cover all the concerns we ha> - it uses an undocumented value, with an unclear implication
>>
>> No it's no longer undocumented since [1]
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=29b65f5f97632722bb80969377e5b0e2401fb392
>>
>> Due to the timezone difference, you guys have already managed to have
>> several exchanges, hopefully I will have a chance to review your
>> discussions a little later today.
>
> Hello
>
> I wait for your comment before sending my reverts patch for http://www.mail-archive.com/[email protected]/msg1431579.html
> Could you confirm that internal is only meant for "non xMII internal protocol"
Yes that's what is it meant for. There are possibly two ways here:
1) assuming there is already a specific PHY driver for that internal
PHY, you should have this PHY driver set PHY_IS_INTERNAL (see bcm63xx.c
and bcm7xxx.c that do that) and so you would know that you did bind to
the correct internal PHY driver. Problem with that is if you need to
perform a particular action such that you will successfully bind to the
internal PHY (e.g: power on, reset, etc.)
2) We could create a new set of phy-mode values that are, e.g:
'internal-mii': internal MII connection to the PHY
and so on in order to cover the internal variants of those modes. I am
not sure this is strictly needed here though.
--
Florian
On Sat, Jul 01, 2017 at 02:42:14PM -0700, Florian Fainelli wrote:
> On 30/06/2017 23:53, Corentin Labbe wrote:
> > On Tue, Jun 27, 2017 at 10:37:34AM -0700, Florian Fainelli wrote:
> >> On 06/27/2017 10:29 AM, Maxime Ripard wrote:
> >>> On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
> >>>> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 27/06/17 11:23, Icenowy Zheng wrote:
> >>>>>>
> >>>>>>
> >>>>>> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <[email protected]> 写到:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On 27/06/17 10:41, Maxime Ripard wrote:
> >>>>>>>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> (CC:ing some people from that Rockchip dmwac series)
> >>>>>>>>>
> >>>>>>>>> On 27/06/17 09:21, Corentin Labbe wrote:
> >>>>>>>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >>>>>>>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >>>>>>>>>>> <[email protected]> wrote:
> >>>>>>>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> >>>>>>>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>>>>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>>>>>>>>>>> allwinner.
> >>>>>>>>>>>>>> In fact the only common part is the descriptor management and
> >>>>>>> the first
> >>>>>>>>>>>>>> register function.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hi,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> >>>>>>> driver
> >>>>>>>>>>>>> to the new binding I was wondering about the internal PHY
> >>>>>>> detection:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> >>>>>>> PHY
> >>>>>>>>>>>>> interface specified in the DT (MII = internal, RGMII =
> >>>>>>> external).
> >>>>>>>>>>>>> I think I raised this question before, but isn't it perfectly
> >>>>>>> legal for
> >>>>>>>>>>>>> a board to use MII with an external PHY even on those SoCs that
> >>>>>>> feature
> >>>>>>>>>>>>> an internal PHY?
> >>>>>>>>>>>>> On the first glance that does not make too much sense, but apart
> >>>>>>> from
> >>>>>>>>>>>>> not being the correct binding to describe all of the SoCs
> >>>>>>> features I see
> >>>>>>>>>>>>> two scenarios:
> >>>>>>>>>>>>> 1) A board vendor might choose to not use the internal PHY
> >>>>>>> because it
> >>>>>>>>>>>>> has bugs, lacks features (configurability) or has other issues.
> >>>>>>> For
> >>>>>>>>>>>>> instance I have heard reports that the internal PHY makes the
> >>>>>>> SoC go
> >>>>>>>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> >>>>>>> external
> >>>>>>>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> >>>>>>> avoided.
> >>>>>>>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> >>>>>>>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> >>>>>>> switch
> >>>>>>>>>>>>> IC or some other network circuitry, for instance fibre
> >>>>>>> connectors.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> So I was wondering if we would need an explicit:
> >>>>>>>>>>>>> allwinner,use-internal-phy;
> >>>>>>>>>>>>> boolean DT property to signal the usage of the internal PHY?
> >>>>>>>>>>>>> Alternatively we could go with the negative version:
> >>>>>>>>>>>>> allwinner,disable-internal-phy;
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> >>>>>>> compatible
> >>>>>>>>>>>>> string for the *PHY* node and use that?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I just want to avoid that we introduce a binding that causes us
> >>>>>>>>>>>>> headaches later. I think we can still fix this with a followup
> >>>>>>> patch
> >>>>>>>>>>>>> before the driver and its binding hit a release kernel.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Cheers,
> >>>>>>>>>>>>> Andre.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> I just see some patch, where "phy-mode = internal" is valid.
> >>>>>>>>>>>> I will try to find a way to use it
> >>>>>>>>>>>
> >>>>>>>>>>> Can you provide a link?
> >>>>>>>>>>
> >>>>>>>>>> https://lkml.org/lkml/2017/6/23/479
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I'm not a fan of using phy-mode for this. There's no guarantee
> >>>>>>> what
> >>>>>>>>>>> mode the internal PHY uses. That's what phy-mode is for.
> >>>>>>>>>
> >>>>>>>>> I can understand Chen-Yu's concerns, but ...
> >>>>>>>>>
> >>>>>>>>>> For each soc the internal PHY mode is know and setted in
> >>>>>>> emac_variant/internal_phy
> >>>>>>>>>> So its not a problem.
> >>>>>>>>>
> >>>>>>>>> that is true as well, at least for now.
> >>>>>>>>>
> >>>>>>>>> So while I agree that having a separate property to indicate
> >>>>>>>>> the usage of the internal PHY would be nice, I am bit tempted
> >>>>>>>>> to use this easier approach and piggy back on the existing
> >>>>>>>>> phy-mode property.
> >>>>>>>>
> >>>>>>>> We're trying to fix an issue that works for now too.
> >>>>>>>>
> >>>>>>>> If we want to consider future weird cases, then we must
> >>>>>>>> consider all of them. And the phy mode changing is definitely
> >>>>>>>> not really far fetched.
> >>>>>>>>
> >>>>>>>> I agree with Chen-Yu, and I really feel like the compatible
> >>>>>>>> solution you suggested would cover both your concerns, and
> >>>>>>>> ours.
> >>>>>>>
> >>>>>>> So something like this?
> >>>>>>> emac: emac@1c30000 {
> >>>>>>> compatible = "allwinner,sun8i-h3-emac";
> >>>>>>> ...
> >>>>>>> phy-mode = "mii";
> >>>>>>> phy-handle = <&int_mii_phy>;
> >>>>>>> ...
> >>>>>>>
> >>>>>>> mdio: mdio {
> >>>>>>> #address-cells = <1>;
> >>>>>>> #size-cells = <0>;
> >>>>>>> int_mii_phy: ethernet-phy@1 {
> >>>>>>> compatible = "allwinner,sun8i-h3-ephy";
> >>>>>>> syscon = <&syscon>;
> >>>>>>
> >>>>>> The MAC still needs to set some bits of syscon register.
> >>>>>
> >>>>> Yes, the syscon property needs also to be in the MAC node, that
> >>>>> was meant to be somewhere in the second "..." ;-)
> >>>>>
> >>>>> But now since Chen-Yu mentioned that we need to set up the PHY *first*
> >>>>> to make it actually discoverable via MDIO, I wonder if we could change
> >>>>> this to:
> >>>>> 1) have the DT as described here
> >>>>> 2) Let the dwmac-sun8i driver peek into the node referenced by
> >>>>> phy-handle and check the compatible string there.
> >>>>> 3) If that matches some allwinner internal PHY name, it sets up the PHY
> >>>>> to make it respond when the MDIO driver queries its bus.
> >>>>>
> >>>>> Or can a PHY driver set itself up (since we have clocks and resets
> >>>>> properties in there) *before* the MDIO bus gets scanned?
> >>>>> Chen-Yu's comment in the other mail hints at that this is not easily
> >>>>> possible.
> >>>>
> >>>> I think adding phy compatible just make things more complex.
> >>>>
> >>>> I think the patch series I sent early fix all problems without more
> >>>> complexity since:
> >>>>
> >>>> - it does not add more DT stuff
> >>>> - it use an already used in tree DT phy-mode "internal" (and so phy
> >>>> mode PHY_INTERFACE_MODE_INTERNAL)
> >>>
> >>> - it doesn't cover all the concerns we ha> - it uses an undocumented value, with an unclear implication
> >>
> >> No it's no longer undocumented since [1]
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=29b65f5f97632722bb80969377e5b0e2401fb392
> >>
> >> Due to the timezone difference, you guys have already managed to have
> >> several exchanges, hopefully I will have a chance to review your
> >> discussions a little later today.
> >
> > Hello
> >
> > I wait for your comment before sending my reverts patch for http://www.mail-archive.com/[email protected]/msg1431579.html
> > Could you confirm that internal is only meant for "non xMII internal protocol"
>
> Yes that's what is it meant for. There are possibly two ways here:
>
> 1) assuming there is already a specific PHY driver for that internal
> PHY, you should have this PHY driver set PHY_IS_INTERNAL (see bcm63xx.c
> and bcm7xxx.c that do that) and so you would know that you did bind to
> the correct internal PHY driver. Problem with that is if you need to
> perform a particular action such that you will successfully bind to the
> internal PHY (e.g: power on, reset, etc.)
>
> 2) We could create a new set of phy-mode values that are, e.g:
>
> 'internal-mii': internal MII connection to the PHY
>
> and so on in order to cover the internal variants of those modes. I am
> not sure this is strictly needed here though.
Or perhaps a phy-location = "external|internal" ?