Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
> > -Original Message- > > From: Andrew Lunn [mailto:and...@lunn.ch] > > Sent: Thursday, August 24, 2017 4:51 PM > > To: Antoine Tenart > > Cc: da...@davemloft.net; kis...@ti.com; ja...@lakedaemon.net; > > sebastian.hesselba...@gmail.com; gregory.clem...@free-electrons.com; > > thomas.petazz...@free-electrons.com; Nadav Haklai ; > > li...@armlinux.org.uk; linux-ker...@vger.kernel.org; m...@semihalf.com; > > Stefan Chulski ; miquel.ray...@free-electrons.com; > > netdev@vger.kernel.org > > Subject: Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver > > > > Hi Antione. > > > > > How would you name it if not "comphy-cp110"? > > > > Good question... > > > > '7000-cpmphy-cp110' > > '8000-cpmphy-cp110' > > > > ?? > > > > Andrew > > A8K Marvell SoC has two South Bridge communication controllers > and A7K only one communication controllers, but its identical > communication controllers 110. Next generation will has another number 1xx. Save this email, so we know who to blame when Marvell does something different :-) Andrew
Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
Hi Stefan, On Thu, Aug 24, 2017 at 01:57:04PM +, Stefan Chulski wrote: > > > > How would you name it if not "comphy-cp110"? > > > > Good question... > > > > '7000-cpmphy-cp110' > > '8000-cpmphy-cp110' > > > > ?? > > > > Andrew > > A8K Marvell SoC has two South Bridge communication controllers > and A7K only one communication controllers, but its identical > communication controllers 110. Next generation will has another number 1xx. OK, so I guess we can keep 'comphy-cp110' then. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
Hi Andrew,
On Thu, Aug 24, 2017 at 03:45:04PM +0200, Andrew Lunn wrote:
> > + for_each_available_child_of_node(pdev->dev.of_node, child) {
> > + struct mvebu_comphy_lane *lane;
> > + struct phy *phy;
> > + int ret;
> > + u32 val;
> > +
> > + ret = of_property_read_u32(child, "reg", &val);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "missing 'reg' property (%d)\n",
> > + ret);
> > + continue;
> > + }
>
> I'm just wondering why we need this. We know how many lanes there are
> from the table. So just create a generic PHY for each lane?
At first I did this statically. I moved to this solution because:
1. It represents the h/w correctly (there are 6 lanes duplicated here).
2. It eases the dt representation, we would have something like:
<&cpm_comphy 0 1>; otherwise.
3. If the number of lanes changes in future revisions it'll be quite
easy to handle.
Thanks!
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
RE: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
> -Original Message- > From: Andrew Lunn [mailto:and...@lunn.ch] > Sent: Thursday, August 24, 2017 4:51 PM > To: Antoine Tenart > Cc: da...@davemloft.net; kis...@ti.com; ja...@lakedaemon.net; > sebastian.hesselba...@gmail.com; gregory.clem...@free-electrons.com; > thomas.petazz...@free-electrons.com; Nadav Haklai ; > li...@armlinux.org.uk; linux-ker...@vger.kernel.org; m...@semihalf.com; > Stefan Chulski ; miquel.ray...@free-electrons.com; > netdev@vger.kernel.org > Subject: Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver > > Hi Antione. > > > How would you name it if not "comphy-cp110"? > > Good question... > > '7000-cpmphy-cp110' > '8000-cpmphy-cp110' > > ?? > > Andrew A8K Marvell SoC has two South Bridge communication controllers and A7K only one communication controllers, but its identical communication controllers 110. Next generation will has another number 1xx. Stefan, Regards.
Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
Hi Antione. > How would you name it if not "comphy-cp110"? Good question... '7000-cpmphy-cp110' '8000-cpmphy-cp110' ?? Andrew
Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
Hi Andrew,
On Thu, Aug 24, 2017 at 03:39:22PM +0200, Andrew Lunn wrote:
> > +static const struct mvebu_comhy_conf mvebu_comphy_modes[] = {
> > + /* lane 0 */
> > + MVEBU_COMPHY_CONF(0, 1, PHY_MODE_SGMII, 0x1),
> > + /* lane 1 */
> > + MVEBU_COMPHY_CONF(1, 2, PHY_MODE_SGMII, 0x1),
> > + /* lane 2 */
> > + MVEBU_COMPHY_CONF(2, 0, PHY_MODE_SGMII, 0x1),
> > + MVEBU_COMPHY_CONF(2, 0, PHY_MODE_10GKR, 0x1),
> > + /* lane 3 */
> > + MVEBU_COMPHY_CONF(3, 1, PHY_MODE_SGMII, 0x2),
> > + /* lane 4 */
> > + MVEBU_COMPHY_CONF(4, 0, PHY_MODE_SGMII, 0x2),
> > + MVEBU_COMPHY_CONF(4, 0, PHY_MODE_10GKR, 0x2),
> > + MVEBU_COMPHY_CONF(4, 1, PHY_MODE_SGMII, 0x1),
> > + /* lane 5 */
> > + MVEBU_COMPHY_CONF(5, 2, PHY_MODE_SGMII, 0x1),
> > +};
>
> Do other Marvell SoCs re-use this IP? Maybe add cp110 to the name here
> to indicate what SoC this configuration belongs to? I guess at some
> point, the compatible string will be used to select the correct table
> for the hardware variant.
OK, I'll rename the variable mvebu_comphy_cp110_modes.
> > +static const struct of_device_id mvebu_comphy_of_match_table[] = {
> > + { .compatible = "marvell,comphy-cp110" },
>
> Is that specific enough? It seems like this table is easy to change in
> the VHDL. Could there be another cp110 with a different configuration?
As far as I understand CP110 is the name of the CP, should there be
another one it should be named differently. But I can't be 100% sure,
you never know what comes next :)
How would you name it if not "comphy-cp110"?
Thanks!
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
signature.asc
Description: PGP signature
Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
> + for_each_available_child_of_node(pdev->dev.of_node, child) {
> + struct mvebu_comphy_lane *lane;
> + struct phy *phy;
> + int ret;
> + u32 val;
> +
> + ret = of_property_read_u32(child, "reg", &val);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "missing 'reg' property (%d)\n",
> + ret);
> + continue;
> + }
I'm just wondering why we need this. We know how many lanes there are
from the table. So just create a generic PHY for each lane?
Andrew
Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver
> +static const struct mvebu_comhy_conf mvebu_comphy_modes[] = {
> + /* lane 0 */
> + MVEBU_COMPHY_CONF(0, 1, PHY_MODE_SGMII, 0x1),
> + /* lane 1 */
> + MVEBU_COMPHY_CONF(1, 2, PHY_MODE_SGMII, 0x1),
> + /* lane 2 */
> + MVEBU_COMPHY_CONF(2, 0, PHY_MODE_SGMII, 0x1),
> + MVEBU_COMPHY_CONF(2, 0, PHY_MODE_10GKR, 0x1),
> + /* lane 3 */
> + MVEBU_COMPHY_CONF(3, 1, PHY_MODE_SGMII, 0x2),
> + /* lane 4 */
> + MVEBU_COMPHY_CONF(4, 0, PHY_MODE_SGMII, 0x2),
> + MVEBU_COMPHY_CONF(4, 0, PHY_MODE_10GKR, 0x2),
> + MVEBU_COMPHY_CONF(4, 1, PHY_MODE_SGMII, 0x1),
> + /* lane 5 */
> + MVEBU_COMPHY_CONF(5, 2, PHY_MODE_SGMII, 0x1),
> +};
Do other Marvell SoCs re-use this IP? Maybe add cp110 to the name here
to indicate what SoC this configuration belongs to? I guess at some
point, the compatible string will be used to select the correct table
for the hardware variant.
> +static const struct of_device_id mvebu_comphy_of_match_table[] = {
> + { .compatible = "marvell,comphy-cp110" },
Is that specific enough? It seems like this table is easy to change in
the VHDL. Could there be another cp110 with a different configuration?
Andrew
