| Message ID | 20230206060708.3574472-1-danishanwar@ti.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce ICSSG based ethernet Driver | expand |
> +enum mii_mode { > + MII_MODE_MII = 0, > + MII_MODE_RGMII, > + MII_MODE_SGMII There is no mention of SGMII anywhere else. And in a couple of places, the code makes the assumption that if it is not RGMII it is MII. Does the hardware really support SGMII? > +static int prueth_config_rgmiidelay(struct prueth *prueth, > + struct device_node *eth_np, > + phy_interface_t phy_if) > +{ ... > + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || > + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; > + > + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); Here you are adding the TX delay if the phy-mode indicates it should be added. > +static int prueth_netdev_init(struct prueth *prueth, > + struct device_node *eth_node) > +{ > + ret = of_get_phy_mode(eth_node, &emac->phy_if); > + if (ret) { > + dev_err(prueth->dev, "could not get phy-mode property\n"); > + goto free; > + } > + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); > + if (ret) > + goto free; > + Reading it from DT and calling the delay function. > +static int prueth_probe(struct platform_device *pdev) > +{ > + /* register the network devices */ > + if (eth0_node) { > + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); > + if (ret) { > + dev_err(dev, "can't register netdev for port MII0"); > + goto netdev_exit; > + } > + > + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; > + > + emac_phy_connect(prueth->emac[PRUETH_MAC0]); And this is connecting the MAC and the PHY, where emac_phy_connect() passes emac->phy_if to phylib. What i don't see anywhere is you changing emac->phy_if to indicate the MAC has inserted the TX delay, and so the PHY should not. Andrew
Hi Andrew, On 06/02/23 19:45, Andrew Lunn wrote: >> +enum mii_mode { >> + MII_MODE_MII = 0, >> + MII_MODE_RGMII, >> + MII_MODE_SGMII > > There is no mention of SGMII anywhere else. And in a couple of places, > the code makes the assumption that if it is not RGMII it is MII. > > Does the hardware really support SGMII? > As far as I know, the hardware does support SGMII but it's not yet supported by the driver. I will drop the SGMII because it's not needed as of now. If in future support for SGMII is there, I'll add it. >> +static int prueth_config_rgmiidelay(struct prueth *prueth, >> + struct device_node *eth_np, >> + phy_interface_t phy_if) >> +{ > > ... > >> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >> + >> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > Here you are adding the TX delay if the phy-mode indicates it should > be added. > >> +static int prueth_netdev_init(struct prueth *prueth, >> + struct device_node *eth_node) >> +{ > >> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >> + if (ret) { >> + dev_err(prueth->dev, "could not get phy-mode property\n"); >> + goto free; >> + } > >> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >> + if (ret) >> + goto free; >> + > > Reading it from DT and calling the delay function. > >> +static int prueth_probe(struct platform_device *pdev) >> +{ > > >> + /* register the network devices */ >> + if (eth0_node) { >> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >> + if (ret) { >> + dev_err(dev, "can't register netdev for port MII0"); >> + goto netdev_exit; >> + } >> + >> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >> + >> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); > > And this is connecting the MAC and the PHY, where emac_phy_connect() > passes emac->phy_if to phylib. > > What i don't see anywhere is you changing emac->phy_if to indicate the > MAC has inserted the TX delay, and so the PHY should not. > Yes, there is no indication whether MAC has enabled TX delay or not. I have changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your suggestion in previous revision. I will keep Tx Internal delay as it is(getting configured in MAC) and inside emac_phy_connect() API, while calling of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only enables Rx delay and keep the existing approach of keepping Tx delay in MAC. Currently, in emac_phy_connect() API, /* connect PHY */ ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, &emac_adjust_link, 0, emac->phy_if); I will change it to, /* connect PHY */ ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, &emac_adjust_link, 0, PHY_INTERFACE_MODE_RGMII_RXID); Let me know if this looks OK. > Andrew >
Danish, On 07/02/2023 17:29, Md Danish Anwar wrote: > Hi Andrew, > > On 06/02/23 19:45, Andrew Lunn wrote: >>> +enum mii_mode { >>> + MII_MODE_MII = 0, >>> + MII_MODE_RGMII, >>> + MII_MODE_SGMII >> >> There is no mention of SGMII anywhere else. And in a couple of places, >> the code makes the assumption that if it is not RGMII it is MII. >> >> Does the hardware really support SGMII? >> > > As far as I know, the hardware does support SGMII but it's not yet supported by > the driver. I will drop the SGMII because it's not needed as of now. If in > future support for SGMII is there, I'll add it. > >>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>> + struct device_node *eth_np, >>> + phy_interface_t phy_if) >>> +{ >> >> ... >> >>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>> + >>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); This is only applicable to some devices so you need to restrict this only to those devices. And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID if it was PHY_INTERFACE_MODE_RGMII_ID. >> >> Here you are adding the TX delay if the phy-mode indicates it should >> be added. >> >>> +static int prueth_netdev_init(struct prueth *prueth, >>> + struct device_node *eth_node) >>> +{ >> >>> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >>> + if (ret) { >>> + dev_err(prueth->dev, "could not get phy-mode property\n"); >>> + goto free; >>> + } >> >>> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >>> + if (ret) >>> + goto free; >>> + >> >> Reading it from DT and calling the delay function. >> >>> +static int prueth_probe(struct platform_device *pdev) >>> +{ >> >> >>> + /* register the network devices */ >>> + if (eth0_node) { >>> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >>> + if (ret) { >>> + dev_err(dev, "can't register netdev for port MII0"); >>> + goto netdev_exit; >>> + } >>> + >>> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >>> + >>> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); >> >> And this is connecting the MAC and the PHY, where emac_phy_connect() >> passes emac->phy_if to phylib. >> >> What i don't see anywhere is you changing emac->phy_if to indicate the >> MAC has inserted the TX delay, and so the PHY should not. >> > > Yes, there is no indication whether MAC has enabled TX delay or not. I have > changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your > suggestion in previous revision. I will keep Tx Internal delay as it is(getting > configured in MAC) and inside emac_phy_connect() API, while calling > of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), > I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only > enables Rx delay and keep the existing approach of keepping Tx delay in MAC. > > Currently, in emac_phy_connect() API, > > /* connect PHY */ > ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, > &emac_adjust_link, 0, > emac->phy_if); > I will change it to, > > /* connect PHY */ > ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, > &emac_adjust_link, 0, > PHY_INTERFACE_MODE_RGMII_RXID); > > Let me know if this looks OK. No, this is not OK. Please keep this as emac->phy_if. In prueth_config_rgmiidelay(), you can change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode *and* MAC TX delay was enabled. cheers, -roger
Hi Roger and Andrew, On 08/02/23 01:26, Roger Quadros wrote: > Danish, > > On 07/02/2023 17:29, Md Danish Anwar wrote: >> Hi Andrew, >> >> On 06/02/23 19:45, Andrew Lunn wrote: >>>> +enum mii_mode { >>>> + MII_MODE_MII = 0, >>>> + MII_MODE_RGMII, >>>> + MII_MODE_SGMII >>> >>> There is no mention of SGMII anywhere else. And in a couple of places, >>> the code makes the assumption that if it is not RGMII it is MII. >>> >>> Does the hardware really support SGMII? >>> >> >> As far as I know, the hardware does support SGMII but it's not yet supported by >> the driver. I will drop the SGMII because it's not needed as of now. If in >> future support for SGMII is there, I'll add it. >> >>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>> + struct device_node *eth_np, >>>> + phy_interface_t phy_if) >>>> +{ >>> >>> ... >>> >>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>> + >>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > This is only applicable to some devices so you need to restrict this only > to those devices. > Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't think there is any need for any device related restriction. Once support for other devices are enabled for upstream, we can modify this accordingly. > And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID > if it was PHY_INTERFACE_MODE_RGMII_ID. > >>> >>> Here you are adding the TX delay if the phy-mode indicates it should >>> be added. >>> >>>> +static int prueth_netdev_init(struct prueth *prueth, >>>> + struct device_node *eth_node) >>>> +{ >>> >>>> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >>>> + if (ret) { >>>> + dev_err(prueth->dev, "could not get phy-mode property\n"); >>>> + goto free; >>>> + } >>> >>>> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >>>> + if (ret) >>>> + goto free; >>>> + >>> >>> Reading it from DT and calling the delay function. >>> >>>> +static int prueth_probe(struct platform_device *pdev) >>>> +{ >>> >>> >>>> + /* register the network devices */ >>>> + if (eth0_node) { >>>> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >>>> + if (ret) { >>>> + dev_err(dev, "can't register netdev for port MII0"); >>>> + goto netdev_exit; >>>> + } >>>> + >>>> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >>>> + >>>> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); >>> >>> And this is connecting the MAC and the PHY, where emac_phy_connect() >>> passes emac->phy_if to phylib. >>> >>> What i don't see anywhere is you changing emac->phy_if to indicate the >>> MAC has inserted the TX delay, and so the PHY should not. >>> >> >> Yes, there is no indication whether MAC has enabled TX delay or not. I have >> changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your >> suggestion in previous revision. I will keep Tx Internal delay as it is(getting >> configured in MAC) and inside emac_phy_connect() API, while calling >> of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), >> I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only >> enables Rx delay and keep the existing approach of keepping Tx delay in MAC. >> >> Currently, in emac_phy_connect() API, >> >> /* connect PHY */ >> ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, >> &emac_adjust_link, 0, >> emac->phy_if); >> I will change it to, >> >> /* connect PHY */ >> ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, >> &emac_adjust_link, 0, >> PHY_INTERFACE_MODE_RGMII_RXID); >> >> Let me know if this looks OK. > > No, this is not OK. > > Please keep this as emac->phy_if. > > In prueth_config_rgmiidelay(), you can change emac->phy_if to > PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode > *and* MAC TX delay was enabled. > I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table 5-624) for AM65 Silicon Revision 2.0. Below is the description in Table 5-624 BIT : 24 Field : RGMII0_ID_MODE Type : R/W Reset : 0h Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay 0h - Internal transmit delay is enabled 1h - Reserved The TX internal delay is always enabled and couldn't be disabled as 1h is reserved. So hardware support for disabling TX internal delay is not there. As, TX internal delay is always there, there is no need to enable it in MAC or PHY. So no need of API prueth_config_rgmiidelay(). My approach to handle delay would be as below. *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. *) Let TX internal delay enabled in Hardware. *) Let PHY configure RX internal delay. *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX Internal delay is always enabled. *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() API, add below if condition. if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID This will check if phy-mode is "rgmii-id", then change the phy-mode to "rgmii-rxid". The same emac->phy_if is passed to emac_phy_connect which passes emac->phy_if to phylib. So that PHY will only enable RX internal delay and TX internal delay is always enabled by default(in Hardware) As of now ICSSG driver is getting upstreamed with support for only AM65 SR2.0, In future when ICSSG driver starts supporting other devices, we can modify this condition to something like below if(device_is_AM65 && emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID So that, for other devices, phy-mode should remain as it is, as other devices don't have hardware restriction. Please let me know if this is the right approach. [1] https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf?ts=1675841023830&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM6548#%5B%7B%22num%22%3A1706%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C717.9%2C0%5D > cheers, > -roger
Danish, On 08/02/2023 09:46, Md Danish Anwar wrote: > Hi Roger and Andrew, > > On 08/02/23 01:26, Roger Quadros wrote: >> Danish, >> >> On 07/02/2023 17:29, Md Danish Anwar wrote: >>> Hi Andrew, >>> >>> On 06/02/23 19:45, Andrew Lunn wrote: >>>>> +enum mii_mode { >>>>> + MII_MODE_MII = 0, >>>>> + MII_MODE_RGMII, >>>>> + MII_MODE_SGMII >>>> >>>> There is no mention of SGMII anywhere else. And in a couple of places, >>>> the code makes the assumption that if it is not RGMII it is MII. >>>> >>>> Does the hardware really support SGMII? >>>> >>> >>> As far as I know, the hardware does support SGMII but it's not yet supported by >>> the driver. I will drop the SGMII because it's not needed as of now. If in >>> future support for SGMII is there, I'll add it. >>> >>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>> + struct device_node *eth_np, >>>>> + phy_interface_t phy_if) >>>>> +{ >>>> >>>> ... >>>> >>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>> + >>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >> >> This is only applicable to some devices so you need to restrict this only >> to those devices. >> > > Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't > think there is any need for any device related restriction. Once support for > other devices are enabled for upstream, we can modify this accordingly. OK then please get rid of prueth_config_rgmiidelay() entirely. > >> And only when you enable MAC TX delay you need to change emac->phy_if to PHY_INTERFACE_MODE_RGMII_RXID >> if it was PHY_INTERFACE_MODE_RGMII_ID. >> >>>> >>>> Here you are adding the TX delay if the phy-mode indicates it should >>>> be added. >>>> >>>>> +static int prueth_netdev_init(struct prueth *prueth, >>>>> + struct device_node *eth_node) >>>>> +{ >>>> >>>>> + ret = of_get_phy_mode(eth_node, &emac->phy_if); >>>>> + if (ret) { >>>>> + dev_err(prueth->dev, "could not get phy-mode property\n"); >>>>> + goto free; >>>>> + } >>>> >>>>> + ret = prueth_config_rgmiidelay(prueth, eth_node, emac->phy_if); >>>>> + if (ret) >>>>> + goto free; >>>>> + >>>> >>>> Reading it from DT and calling the delay function. >>>> >>>>> +static int prueth_probe(struct platform_device *pdev) >>>>> +{ >>>> >>>> >>>>> + /* register the network devices */ >>>>> + if (eth0_node) { >>>>> + ret = register_netdev(prueth->emac[PRUETH_MAC0]->ndev); >>>>> + if (ret) { >>>>> + dev_err(dev, "can't register netdev for port MII0"); >>>>> + goto netdev_exit; >>>>> + } >>>>> + >>>>> + prueth->registered_netdevs[PRUETH_MAC0] = prueth->emac[PRUETH_MAC0]->ndev; >>>>> + >>>>> + emac_phy_connect(prueth->emac[PRUETH_MAC0]); >>>> >>>> And this is connecting the MAC and the PHY, where emac_phy_connect() >>>> passes emac->phy_if to phylib. >>>> >>>> What i don't see anywhere is you changing emac->phy_if to indicate the >>>> MAC has inserted the TX delay, and so the PHY should not. >>>> >>> >>> Yes, there is no indication whether MAC has enabled TX delay or not. I have >>> changed the phy-mode in DT from "rgmii-rxid" to "rgmii-id" as per your >>> suggestion in previous revision. I will keep Tx Internal delay as it is(getting >>> configured in MAC) and inside emac_phy_connect() API, while calling >>> of_phy_connect() instead of passing emac->phy_if (which is rgmii-id as per DT), >>> I will pass PHY_INTERFACE_MODE_RGMII_RXID. This will make sure that phy only >>> enables Rx delay and keep the existing approach of keepping Tx delay in MAC. >>> >>> Currently, in emac_phy_connect() API, >>> >>> /* connect PHY */ >>> ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, >>> &emac_adjust_link, 0, >>> emac->phy_if); >>> I will change it to, >>> >>> /* connect PHY */ >>> ndev->phydev = of_phy_connect(emac->ndev, emac->phy_node, >>> &emac_adjust_link, 0, >>> PHY_INTERFACE_MODE_RGMII_RXID); >>> >>> Let me know if this looks OK. >> >> No, this is not OK. >> >> Please keep this as emac->phy_if. >> >> In prueth_config_rgmiidelay(), you can change emac->phy_if to >> PHY_INTERFACE_MODE_RGMII_RXID only if it was RGMII mode >> *and* MAC TX delay was enabled. >> > > I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table > 5-624) for AM65 Silicon Revision 2.0. > > Below is the description in Table 5-624 > > BIT : 24 > Field : RGMII0_ID_MODE > Type : R/W > Reset : 0h > Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay > 0h - Internal transmit delay is enabled > 1h - Reserved > > The TX internal delay is always enabled and couldn't be disabled as 1h is > reserved. So hardware support for disabling TX internal delay is not there. > > As, TX internal delay is always there, there is no need to enable it in MAC or > PHY. So no need of API prueth_config_rgmiidelay(). > > My approach to handle delay would be as below. > > *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. > *) Let TX internal delay enabled in Hardware. You mean in TX delay in MAC? Why? If Silicon does not have an issue then it should be enabled in PHY not in MAC. > *) Let PHY configure RX internal delay. No. PHY should do whatever is asked in the DT. In this case both TX and RX delay. > *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX Agreed. > Internal delay is always enabled. no. > *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() > API, add below if condition. > > if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) > emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID No. This will remain emac->phy_if. > > This will check if phy-mode is "rgmii-id", then change the phy-mode to > "rgmii-rxid". The same emac->phy_if is passed to emac_phy_connect which passes > emac->phy_if to phylib. So that PHY will only enable RX internal delay and TX > internal delay is always enabled by default(in Hardware) > No need of all this complexity. > As of now ICSSG driver is getting upstreamed with support for only AM65 SR2.0, OK. Then let's just deal with SR2.0 for now. Pre-production devices are never up-streamed. > In future when ICSSG driver starts supporting other devices, we can modify this > condition to something like below > > if(device_is_AM65 && emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) > emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID > > So that, for other devices, phy-mode should remain as it is, as other devices > don't have hardware restriction. > > Please let me know if this is the right approach. > > [1] > https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf?ts=1675841023830&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FAM6548#%5B%7B%22num%22%3A1706%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2C0%2C717.9%2C0%5D > >> cheers, >> -roger cheers, -roger.
> >>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, > >>>> + struct device_node *eth_np, > >>>> + phy_interface_t phy_if) > >>>> +{ > >>> > >>> ... > >>> > >>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || > >>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > >>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; > >>>> + > >>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); > > > > This is only applicable to some devices so you need to restrict this only > > to those devices. > > > > Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't > think there is any need for any device related restriction. Once support for > other devices are enabled for upstream, we can modify this accordingly. The problem is, this is a board property, not a SoC property. What if somebody designs a board with extra long clock lines in order to add the delay? > I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table > 5-624) for AM65 Silicon Revision 2.0. > > Below is the description in Table 5-624 > > BIT : 24 > Field : RGMII0_ID_MODE > Type : R/W > Reset : 0h > Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay > 0h - Internal transmit delay is enabled > 1h - Reserved > > The TX internal delay is always enabled and couldn't be disabled as 1h is > reserved. So hardware support for disabling TX internal delay is not there. So if somebody passes a phy-mode which requires it disabled, you need to return -EINVAL, to indicate the hardware cannot actually do it. > As, TX internal delay is always there, there is no need to enable it in MAC or > PHY. So no need of API prueth_config_rgmiidelay(). > > My approach to handle delay would be as below. > > *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. As i said this depends on the board, not the SoC. In theory, you could design a board with an extra long RX clock line, and then use phy-mode rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. > *) Let TX internal delay enabled in Hardware. > *) Let PHY configure RX internal delay. > *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX > Internal delay is always enabled. > *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() > API, add below if condition. > > if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) > emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID You should handle all cases where a TX delay is requested, not just ID. Andrew
Hi Andrew, On 08/02/23 18:26, Andrew Lunn wrote: >>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>>> + struct device_node *eth_np, >>>>>> + phy_interface_t phy_if) >>>>>> +{ >>>>> >>>>> ... >>>>> >>>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>>> + >>>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>> >>> This is only applicable to some devices so you need to restrict this only >>> to those devices. >>> >> >> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't >> think there is any need for any device related restriction. Once support for >> other devices are enabled for upstream, we can modify this accordingly. > > The problem is, this is a board property, not a SoC property. What if > somebody designs a board with extra long clock lines in order to add > the delay? > >> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table >> 5-624) for AM65 Silicon Revision 2.0. >> >> Below is the description in Table 5-624 >> >> BIT : 24 >> Field : RGMII0_ID_MODE >> Type : R/W >> Reset : 0h >> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay >> 0h - Internal transmit delay is enabled >> 1h - Reserved >> >> The TX internal delay is always enabled and couldn't be disabled as 1h is >> reserved. So hardware support for disabling TX internal delay is not there. > > So if somebody passes a phy-mode which requires it disabled, you need > to return -EINVAL, to indicate the hardware cannot actually do it. > Sure, I'll do that. In the list of all phy modes described in [1], I can only see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other phy-mode that requires enabling/disabling TX internal delays? Please let me know if any other phy-mode also needs this. I will add check for that as well. >> As, TX internal delay is always there, there is no need to enable it in MAC or >> PHY. So no need of API prueth_config_rgmiidelay(). >> >> My approach to handle delay would be as below. >> >> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. > > As i said this depends on the board, not the SoC. In theory, you could > design a board with an extra long RX clock line, and then use phy-mode > rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. > Yes I understand that board can have any phy-mode in it's DTS. We need to be able to handle all different phy modes. >> *) Let TX internal delay enabled in Hardware. >> *) Let PHY configure RX internal delay. >> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX >> Internal delay is always enabled. >> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() >> API, add below if condition. >> >> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >> emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID > > You should handle all cases where a TX delay is requested, not just > ID. > So there could be four different RGMII phy modes as described in [1]. Below is the handling mechanism for different phy modes. 1) # RGMII with internal RX and TX delays provided by the PHY, # the MAC should not add the RX or TX delays in this case - rgmii-id For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But in our SoC TX internal delay is always enabled. So to handle this, we'll change the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY will enable RX internal delay only. 2) # RGMII with internal RX delay provided by the PHY, the MAC # should not add an RX delay in this case - rgmii-rxid For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do nothing in the driver and just pass the same mode to phy, so that PHY will enable RX internal delay only. 3) # RGMII with internal TX delay provided by the PHY, the MAC # should not add an TX delay in this case - rgmii-txid For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC should not add an TX delay in this case. But in our SoC TX internal delay is always enabled. So this scenario can not be handled. We will return -EINVAL in this case. 4) # RX and TX delays are added by the MAC when required - rgmii For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX internal delay is always enabled so no need to add TX delay. For RX I am not sure what should we do as there is no provision of adding RX delay in MAC currently. Should we ask PHY to add RX delay? Andrew, Roger, Can you please comment on this. Apart from Case 4, below code change will be able to handle all other cases. if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID) return -EINVAL; Please let me know if I am missing any other phy modes. [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml > Andrew
On 09/02/2023 12:29, Md Danish Anwar wrote: > Hi Andrew, > > On 08/02/23 18:26, Andrew Lunn wrote: >>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>>>> + struct device_node *eth_np, >>>>>>> + phy_interface_t phy_if) >>>>>>> +{ >>>>>> >>>>>> ... >>>>>> >>>>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>>>> + >>>>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>>> >>>> This is only applicable to some devices so you need to restrict this only >>>> to those devices. >>>> >>> >>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't >>> think there is any need for any device related restriction. Once support for >>> other devices are enabled for upstream, we can modify this accordingly. >> >> The problem is, this is a board property, not a SoC property. What if >> somebody designs a board with extra long clock lines in order to add >> the delay? >> >>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table >>> 5-624) for AM65 Silicon Revision 2.0. >>> >>> Below is the description in Table 5-624 >>> >>> BIT : 24 >>> Field : RGMII0_ID_MODE >>> Type : R/W >>> Reset : 0h >>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay >>> 0h - Internal transmit delay is enabled >>> 1h - Reserved >>> >>> The TX internal delay is always enabled and couldn't be disabled as 1h is >>> reserved. So hardware support for disabling TX internal delay is not there. >> >> So if somebody passes a phy-mode which requires it disabled, you need >> to return -EINVAL, to indicate the hardware cannot actually do it. >> > > Sure, I'll do that. In the list of all phy modes described in [1], I can only > see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other > phy-mode that requires enabling/disabling TX internal delays? Please let me > know if any other phy-mode also needs this. I will add check for that as well. > >>> As, TX internal delay is always there, there is no need to enable it in MAC or >>> PHY. So no need of API prueth_config_rgmiidelay(). >>> >>> My approach to handle delay would be as below. >>> >>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. >> >> As i said this depends on the board, not the SoC. In theory, you could >> design a board with an extra long RX clock line, and then use phy-mode >> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. >> > > Yes I understand that board can have any phy-mode in it's DTS. We need to be > able to handle all different phy modes. > >>> *) Let TX internal delay enabled in Hardware. >>> *) Let PHY configure RX internal delay. >>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX >>> Internal delay is always enabled. >>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() >>> API, add below if condition. >>> >>> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >>> emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID >> >> You should handle all cases where a TX delay is requested, not just >> ID. >> > > So there could be four different RGMII phy modes as described in [1]. Below is > the handling mechanism for different phy modes. > > 1) # RGMII with internal RX and TX delays provided by the PHY, > # the MAC should not add the RX or TX delays in this case > - rgmii-id > > For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But > in our SoC TX internal delay is always enabled. So to handle this, we'll change OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions. But it looks like it hasn't been fixed yet. > the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY > will enable RX internal delay only. OK. > > 2) # RGMII with internal RX delay provided by the PHY, the MAC > # should not add an RX delay in this case > - rgmii-rxid > > For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do > nothing in the driver and just pass the same mode to phy, so that PHY will > enable RX internal delay only. But the MAC is forcing TX-delay right? So this case can't be implemented. you have to return error. > > 3) # RGMII with internal TX delay provided by the PHY, the MAC > # should not add an TX delay in this case > - rgmii-txid > > For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC > should not add an TX delay in this case. But in our SoC TX internal delay is > always enabled. So this scenario can not be handled. We will return -EINVAL in > this case. As you didn't return error for 1st case "rgmii-id" even though TX delay was requested for PHY but you added it in the MAC I see no reason to return error here. You just do the delay in MAC and pass "rgmii" to the PHY. > > > 4) # RX and TX delays are added by the MAC when required > - rgmii > > For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX > internal delay is always enabled so no need to add TX delay. For RX I am not > sure what should we do as there is no provision of adding RX delay in MAC > currently. Should we ask PHY to add RX delay? > I don't think it will work so you can error out in this case. > Andrew, Roger, Can you please comment on this. > > Apart from Case 4, below code change will be able to handle all other cases. > > if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) > emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; > if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID) > return -EINVAL; > > Please let me know if I am missing any other phy modes. > > [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml > >> Andrew > cheers, -roger
On 09/02/23 18:28, Roger Quadros wrote: > > > On 09/02/2023 12:29, Md Danish Anwar wrote: >> Hi Andrew, >> >> On 08/02/23 18:26, Andrew Lunn wrote: >>>>>>>> +static int prueth_config_rgmiidelay(struct prueth *prueth, >>>>>>>> + struct device_node *eth_np, >>>>>>>> + phy_interface_t phy_if) >>>>>>>> +{ >>>>>>> >>>>>>> ... >>>>>>> >>>>>>>> + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || >>>>>>>> + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >>>>>>>> + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; >>>>>>>> + >>>>>>>> + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); >>>>> >>>>> This is only applicable to some devices so you need to restrict this only >>>>> to those devices. >>>>> >>>> >>>> Currently ICSSG driver is getting upstreamed for AM65 SR2.0 device, so I don't >>>> think there is any need for any device related restriction. Once support for >>>> other devices are enabled for upstream, we can modify this accordingly. >>> >>> The problem is, this is a board property, not a SoC property. What if >>> somebody designs a board with extra long clock lines in order to add >>> the delay? >>> >>>> I checked the latest Technical Reference Manual [1] (Section 5.1.3.4.49, Table >>>> 5-624) for AM65 Silicon Revision 2.0. >>>> >>>> Below is the description in Table 5-624 >>>> >>>> BIT : 24 >>>> Field : RGMII0_ID_MODE >>>> Type : R/W >>>> Reset : 0h >>>> Description : Controls the PRU_ICSSG0 RGMII0 port internal transmit delay >>>> 0h - Internal transmit delay is enabled >>>> 1h - Reserved >>>> >>>> The TX internal delay is always enabled and couldn't be disabled as 1h is >>>> reserved. So hardware support for disabling TX internal delay is not there. >>> >>> So if somebody passes a phy-mode which requires it disabled, you need >>> to return -EINVAL, to indicate the hardware cannot actually do it. >>> >> >> Sure, I'll do that. In the list of all phy modes described in [1], I can only >> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other >> phy-mode that requires enabling/disabling TX internal delays? Please let me >> know if any other phy-mode also needs this. I will add check for that as well. >> >>>> As, TX internal delay is always there, there is no need to enable it in MAC or >>>> PHY. So no need of API prueth_config_rgmiidelay(). >>>> >>>> My approach to handle delay would be as below. >>>> >>>> *) Keep phy-mode = "rgmii-id" in DT as asked by Andrew. >>> >>> As i said this depends on the board, not the SoC. In theory, you could >>> design a board with an extra long RX clock line, and then use phy-mode >>> rgmii-txid, meaning the MAC/PHY combination needs to add the TX delay. >>> >> >> Yes I understand that board can have any phy-mode in it's DTS. We need to be >> able to handle all different phy modes. >> >>>> *) Let TX internal delay enabled in Hardware. >>>> *) Let PHY configure RX internal delay. >>>> *) Remove prueth_config_rgmiidelay() API is there is no use of this. TX >>>> Internal delay is always enabled. >>>> *) Instead of calling prueth_config_rgmiidelay() API in prueth_netdev_init() >>>> API, add below if condition. >>>> >>>> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >>>> emac->phy_if == PHY_INTERFACE_MODE_RGMII_RXID >>> >>> You should handle all cases where a TX delay is requested, not just >>> ID. >>> >> >> So there could be four different RGMII phy modes as described in [1]. Below is >> the handling mechanism for different phy modes. >> >> 1) # RGMII with internal RX and TX delays provided by the PHY, >> # the MAC should not add the RX or TX delays in this case >> - rgmii-id >> >> For phy-mode="rgmii-id", phy needs to add both TX and RX internal delays. But >> in our SoC TX internal delay is always enabled. So to handle this, we'll change > > OK. I thought that this MAC forced TX delay issue was fixed in Later Silicon Revisions. > But it looks like it hasn't been fixed yet. > Yes, I confirmed with the Hardware folks, the forced TX delay issue is still there. It's not fixed as of now. > >> the phy-mode in driver to "rgmii-rxid" and then pass it ti PHY, so that PHY >> will enable RX internal delay only. > > OK. > Noted. >> >> 2) # RGMII with internal RX delay provided by the PHY, the MAC >> # should not add an RX delay in this case >> - rgmii-rxid >> >> For phy-mode="rgmii-rxid", phy needs to add only RX internal delay. We will do >> nothing in the driver and just pass the same mode to phy, so that PHY will >> enable RX internal delay only. > > But the MAC is forcing TX-delay right? So this case can't be implemented. > you have to return error. > Yes, My bad. "rgmii-rxid" is not possible. I'll return -EINVAL here. >> >> 3) # RGMII with internal TX delay provided by the PHY, the MAC >> # should not add an TX delay in this case >> - rgmii-txid >> >> For phy-mode="rgmii-txid", phy needs to add only TX internal delay,the MAC >> should not add an TX delay in this case. But in our SoC TX internal delay is >> always enabled. So this scenario can not be handled. We will return -EINVAL in >> this case. > > As you didn't return error for 1st case "rgmii-id" even though TX delay was requested > for PHY but you added it in the MAC I see no reason to return error here. > > You just do the delay in MAC and pass "rgmii" to the PHY. > Yes Noted, TX delay will be forced and phy-mode "rgmii" will be passed to PHY. >> >> >> 4) # RX and TX delays are added by the MAC when required >> - rgmii >> >> For phy-mode="rgmii", MAC needs to add both TX and RX delays. But in our SoC TX >> internal delay is always enabled so no need to add TX delay. For RX I am not >> sure what should we do as there is no provision of adding RX delay in MAC >> currently. Should we ask PHY to add RX delay? >> > > I don't think it will work so you can error out in this case. > Sure, Noted. So just to summarize, For "rgmii-id" phy mode, pass "rgmii-rxid" to phy. For "rgmii-rxid", return error. For "rgmii-txid", TX delay is forced, pass "rgmii" to PHY. For "rgmii", return error. >> Andrew, Roger, Can you please comment on this. >> >> Apart from Case 4, below code change will be able to handle all other cases. >> >> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_ID) >> emac->phy_if = PHY_INTERFACE_MODE_RGMII_RXID; >> if(emac->phy_if == PHY_INTERFACE_MODE_RGMII_TXID) >> return -EINVAL; >> >> Please let me know if I am missing any other phy modes. >> >> [1] Documentation/devicetree/bindings/net/ethernet-controller.yaml >> >>> Andrew >> > > cheers, > -roger
> Sure, I'll do that. In the list of all phy modes described in [1], I can only > see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other > phy-mode that requires enabling/disabling TX internal delays? Please let me > know if any other phy-mode also needs this. I will add check for that as well. There are 4 phy-modes for RGMII. rgmii, rgmii-id, rmgii-rxid, rgmii-txid. rgmii-id, rgmii-txid both require TX delays. If you do that in the MAC you then need to pass rgmii-rxid and rgmii to the PHY respectively. rmii and rgmii-rxid requires no TX delays, which your SoC cannot do, so you need to return -EINVAl, The interpretation of these properties is all really messy and historically not very uniformly done. Which is why i recommend the MAC does nothing, leaving it to the PHY. That generally works since the PHYs have a pretty uniform implementation. But in your case, you don't have that option. So i suggest you do what is described above. Andrew
On 09/02/23 19:24, Andrew Lunn wrote: >> Sure, I'll do that. In the list of all phy modes described in [1], I can only >> see phy-mode "rgmii-txid", for which we can return -EINVAL. Is there any other >> phy-mode that requires enabling/disabling TX internal delays? Please let me >> know if any other phy-mode also needs this. I will add check for that as well. > > There are 4 phy-modes for RGMII. > > rgmii, rgmii-id, rmgii-rxid, rgmii-txid. > > rgmii-id, rgmii-txid both require TX delays. If you do that in the MAC > you then need to pass rgmii-rxid and rgmii to the PHY respectively. > > rmii and rgmii-rxid requires no TX delays, which your SoC cannot do, > so you need to return -EINVAl, > > The interpretation of these properties is all really messy and > historically not very uniformly done. Which is why i recommend the MAC > does nothing, leaving it to the PHY. That generally works since the > PHYs have a pretty uniform implementation. But in your case, you don't > have that option. So i suggest you do what is described above. > > Andrew Sure Andrew, I will post new revision with the changes described above.