[U-Boot,1/3] net: designware: socfpga: adapt to Gen5

Message ID 20190110194954.23950-2-simon.k.r.goldschmidt@gmail.com
State Superseded, archived
Delegated to: Marek Vasut
Headers show
Series
  • arm: socpfpga: gen5 clean up ETH RST & PHY mode
Related show

Commit Message

Simon Goldschmidt Jan. 10, 2019, 7:49 p.m.
This driver was written for Arria10, but it applies to Gen5, too.

The main difference is that Gen5 has 2 MACs (Arria10 has 3) and the
syscon bits are encoded in the same register, thus an offset is needed.

This offset is already read from the devicetree, but for Arria10 it is
always 0, which is probably why it has been ignored. By using this
offset when writing the phy mode into the syscon regiter, we can use
this driver to set the phy mode for both of the MACs on Gen5.

Tested on socfpga_socrates (where the 2nd MAC is connected, so a shift
offset is required).

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 drivers/net/dwmac_socfpga.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Simon Goldschmidt Jan. 10, 2019, 7:54 p.m. | #1
Hi Marek,

Am 10.01.2019 um 20:49 schrieb Simon Goldschmidt:
> This driver was written for Arria10, but it applies to Gen5, too.
> 
> The main difference is that Gen5 has 2 MACs (Arria10 has 3) and the
> syscon bits are encoded in the same register, thus an offset is needed.
> 
> This offset is already read from the devicetree, but for Arria10 it is
> always 0, which is probably why it has been ignored. By using this
> offset when writing the phy mode into the syscon regiter, we can use
> this driver to set the phy mode for both of the MACs on Gen5.
> 
> Tested on socfpga_socrates (where the 2nd MAC is connected, so a shift
> offset is required).
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
> 
>   drivers/net/dwmac_socfpga.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
> index 08fc9677c4..309da69647 100644
> --- a/drivers/net/dwmac_socfpga.c
> +++ b/drivers/net/dwmac_socfpga.c
> @@ -27,6 +27,7 @@ struct dwmac_socfpga_platdata {
>   	struct dw_eth_pdata	dw_eth_pdata;
>   	enum dwmac_type		type;
>   	void			*phy_intf;
> +	u32			reg_shift;
>   };
>   
>   static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
> @@ -63,6 +64,7 @@ static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
>   	}
>   
>   	pdata->phy_intf = range + args.args[0];
> +	pdata->reg_shift = args.args[1];
>   
>   	/*
>   	 * Sadly, the Altera DT bindings don't have SoC-specific compatibles,
> @@ -88,9 +90,11 @@ static int dwmac_socfpga_probe(struct udevice *dev)
>   	struct eth_pdata *edata = &pdata->dw_eth_pdata.eth_pdata;
>   	struct reset_ctl_bulk reset_bulk;
>   	int ret;
> -	u8 modereg;
> +	u32 modereg;
> +	u32 modemask;
>   
> -	if (pdata->type == DWMAC_SOCFPGA_ARRIA10) {
> +	if (pdata->type == DWMAC_SOCFPGA_ARRIA10 ||
> +	    pdata->type == DWMAC_SOCFPGA_GEN5) {

I just checked: the Linux driver does not seem to have special handling 
for Gen5/Arria10/Stratix10. Since Gen5 and Arria10 now both work and the 
registers for Stratix10 seem the same as for Arria10, could we maybe 
drop this special handling?

That would mean we could drop this whole 'type' and the 'enum dwmac_type'...

Regards,
Simon

>   		switch (edata->phy_interface) {
>   		case PHY_INTERFACE_MODE_MII:
>   		case PHY_INTERFACE_MODE_GMII:
> @@ -115,9 +119,9 @@ static int dwmac_socfpga_probe(struct udevice *dev)
>   
>   		reset_assert_bulk(&reset_bulk);
>   
> -		clrsetbits_le32(pdata->phy_intf,
> -				SYSMGR_EMACGRP_CTRL_PHYSEL_MASK,
> -				modereg);
> +		modemask = SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << pdata->reg_shift;
> +		clrsetbits_le32(pdata->phy_intf, modemask,
> +				modereg << pdata->reg_shift);
>   
>   		reset_release_bulk(&reset_bulk);
>   	}
>
Marek Vasut Jan. 10, 2019, 8:38 p.m. | #2
On 1/10/19 8:54 PM, Simon Goldschmidt wrote:
> Hi Marek,
> 
> Am 10.01.2019 um 20:49 schrieb Simon Goldschmidt:
>> This driver was written for Arria10, but it applies to Gen5, too.
>>
>> The main difference is that Gen5 has 2 MACs (Arria10 has 3) and the
>> syscon bits are encoded in the same register, thus an offset is needed.
>>
>> This offset is already read from the devicetree, but for Arria10 it is
>> always 0, which is probably why it has been ignored. By using this
>> offset when writing the phy mode into the syscon regiter, we can use
>> this driver to set the phy mode for both of the MACs on Gen5.
>>
>> Tested on socfpga_socrates (where the 2nd MAC is connected, so a shift
>> offset is required).
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>> ---
>>
>>   drivers/net/dwmac_socfpga.c | 14 +++++++++-----
>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
>> index 08fc9677c4..309da69647 100644
>> --- a/drivers/net/dwmac_socfpga.c
>> +++ b/drivers/net/dwmac_socfpga.c
>> @@ -27,6 +27,7 @@ struct dwmac_socfpga_platdata {
>>       struct dw_eth_pdata    dw_eth_pdata;
>>       enum dwmac_type        type;
>>       void            *phy_intf;
>> +    u32            reg_shift;
>>   };
>>     static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
>> @@ -63,6 +64,7 @@ static int dwmac_socfpga_ofdata_to_platdata(struct
>> udevice *dev)
>>       }
>>         pdata->phy_intf = range + args.args[0];
>> +    pdata->reg_shift = args.args[1];
>>         /*
>>        * Sadly, the Altera DT bindings don't have SoC-specific
>> compatibles,
>> @@ -88,9 +90,11 @@ static int dwmac_socfpga_probe(struct udevice *dev)
>>       struct eth_pdata *edata = &pdata->dw_eth_pdata.eth_pdata;
>>       struct reset_ctl_bulk reset_bulk;
>>       int ret;
>> -    u8 modereg;
>> +    u32 modereg;
>> +    u32 modemask;
>>   -    if (pdata->type == DWMAC_SOCFPGA_ARRIA10) {
>> +    if (pdata->type == DWMAC_SOCFPGA_ARRIA10 ||
>> +        pdata->type == DWMAC_SOCFPGA_GEN5) {
> 
> I just checked: the Linux driver does not seem to have special handling
> for Gen5/Arria10/Stratix10. Since Gen5 and Arria10 now both work and the
> registers for Stratix10 seem the same as for Arria10, could we maybe
> drop this special handling?
> 
> That would mean we could drop this whole 'type' and the 'enum
> dwmac_type'...

I seem to remember the register layout for selecting the PHY mode on
Gen5 and Arria10 was different, please check the datasheets. Of course,
the less special-casing, the better.
Simon Goldschmidt Jan. 10, 2019, 8:42 p.m. | #3
Am Do., 10. Jan. 2019, 21:38 hat Marek Vasut <marex@denx.de> geschrieben:

> On 1/10/19 8:54 PM, Simon Goldschmidt wrote:
> > Hi Marek,
> >
> > Am 10.01.2019 um 20:49 schrieb Simon Goldschmidt:
> >> This driver was written for Arria10, but it applies to Gen5, too.
> >>
> >> The main difference is that Gen5 has 2 MACs (Arria10 has 3) and the
> >> syscon bits are encoded in the same register, thus an offset is needed.
> >>
> >> This offset is already read from the devicetree, but for Arria10 it is
> >> always 0, which is probably why it has been ignored. By using this
> >> offset when writing the phy mode into the syscon regiter, we can use
> >> this driver to set the phy mode for both of the MACs on Gen5.
> >>
> >> Tested on socfpga_socrates (where the 2nd MAC is connected, so a shift
> >> offset is required).
> >>
> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >> ---
> >>
> >>   drivers/net/dwmac_socfpga.c | 14 +++++++++-----
> >>   1 file changed, 9 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
> >> index 08fc9677c4..309da69647 100644
> >> --- a/drivers/net/dwmac_socfpga.c
> >> +++ b/drivers/net/dwmac_socfpga.c
> >> @@ -27,6 +27,7 @@ struct dwmac_socfpga_platdata {
> >>       struct dw_eth_pdata    dw_eth_pdata;
> >>       enum dwmac_type        type;
> >>       void            *phy_intf;
> >> +    u32            reg_shift;
> >>   };
> >>     static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
> >> @@ -63,6 +64,7 @@ static int dwmac_socfpga_ofdata_to_platdata(struct
> >> udevice *dev)
> >>       }
> >>         pdata->phy_intf = range + args.args[0];
> >> +    pdata->reg_shift = args.args[1];
> >>         /*
> >>        * Sadly, the Altera DT bindings don't have SoC-specific
> >> compatibles,
> >> @@ -88,9 +90,11 @@ static int dwmac_socfpga_probe(struct udevice *dev)
> >>       struct eth_pdata *edata = &pdata->dw_eth_pdata.eth_pdata;
> >>       struct reset_ctl_bulk reset_bulk;
> >>       int ret;
> >> -    u8 modereg;
> >> +    u32 modereg;
> >> +    u32 modemask;
> >>   -    if (pdata->type == DWMAC_SOCFPGA_ARRIA10) {
> >> +    if (pdata->type == DWMAC_SOCFPGA_ARRIA10 ||
> >> +        pdata->type == DWMAC_SOCFPGA_GEN5) {
> >
> > I just checked: the Linux driver does not seem to have special handling
> > for Gen5/Arria10/Stratix10. Since Gen5 and Arria10 now both work and the
> > registers for Stratix10 seem the same as for Arria10, could we maybe
> > drop this special handling?
> >
> > That would mean we could drop this whole 'type' and the 'enum
> > dwmac_type'...
>
> I seem to remember the register layout for selecting the PHY mode on
> Gen5 and Arria10 was different, please check the datasheets. Of course,
> the less special-casing, the better.
>

Yes, the register layout is different. But the reg_shift value takes care
of that. Luckily, the bits have the same meaning even if the offset is
different.

What remains is the special case for Stratix10. Looking at the Linux driver
and the register descriptions, I think we can drop the special cases, but
someone would need to test it on Stratix10...

Regards,
Simon
Marek Vasut Jan. 10, 2019, 8:44 p.m. | #4
On 1/10/19 9:42 PM, Simon Goldschmidt wrote:
> 
> 
> Am Do., 10. Jan. 2019, 21:38 hat Marek Vasut <marex@denx.de
> <mailto:marex@denx.de>> geschrieben:
> 
>     On 1/10/19 8:54 PM, Simon Goldschmidt wrote:
>     > Hi Marek,
>     >
>     > Am 10.01.2019 um 20:49 schrieb Simon Goldschmidt:
>     >> This driver was written for Arria10, but it applies to Gen5, too.
>     >>
>     >> The main difference is that Gen5 has 2 MACs (Arria10 has 3) and the
>     >> syscon bits are encoded in the same register, thus an offset is
>     needed.
>     >>
>     >> This offset is already read from the devicetree, but for Arria10
>     it is
>     >> always 0, which is probably why it has been ignored. By using this
>     >> offset when writing the phy mode into the syscon regiter, we can use
>     >> this driver to set the phy mode for both of the MACs on Gen5.
>     >>
>     >> Tested on socfpga_socrates (where the 2nd MAC is connected, so a
>     shift
>     >> offset is required).
>     >>
>     >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com
>     <mailto:simon.k.r.goldschmidt@gmail.com>>
>     >> ---
>     >>
>     >>   drivers/net/dwmac_socfpga.c | 14 +++++++++-----
>     >>   1 file changed, 9 insertions(+), 5 deletions(-)
>     >>
>     >> diff --git a/drivers/net/dwmac_socfpga.c
>     b/drivers/net/dwmac_socfpga.c
>     >> index 08fc9677c4..309da69647 100644
>     >> --- a/drivers/net/dwmac_socfpga.c
>     >> +++ b/drivers/net/dwmac_socfpga.c
>     >> @@ -27,6 +27,7 @@ struct dwmac_socfpga_platdata {
>     >>       struct dw_eth_pdata    dw_eth_pdata;
>     >>       enum dwmac_type        type;
>     >>       void            *phy_intf;
>     >> +    u32            reg_shift;
>     >>   };
>     >>     static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
>     >> @@ -63,6 +64,7 @@ static int dwmac_socfpga_ofdata_to_platdata(struct
>     >> udevice *dev)
>     >>       }
>     >>         pdata->phy_intf = range + args.args[0];
>     >> +    pdata->reg_shift = args.args[1];
>     >>         /*
>     >>        * Sadly, the Altera DT bindings don't have SoC-specific
>     >> compatibles,
>     >> @@ -88,9 +90,11 @@ static int dwmac_socfpga_probe(struct udevice
>     *dev)
>     >>       struct eth_pdata *edata = &pdata->dw_eth_pdata.eth_pdata;
>     >>       struct reset_ctl_bulk reset_bulk;
>     >>       int ret;
>     >> -    u8 modereg;
>     >> +    u32 modereg;
>     >> +    u32 modemask;
>     >>   -    if (pdata->type == DWMAC_SOCFPGA_ARRIA10) {
>     >> +    if (pdata->type == DWMAC_SOCFPGA_ARRIA10 ||
>     >> +        pdata->type == DWMAC_SOCFPGA_GEN5) {
>     >
>     > I just checked: the Linux driver does not seem to have special
>     handling
>     > for Gen5/Arria10/Stratix10. Since Gen5 and Arria10 now both work
>     and the
>     > registers for Stratix10 seem the same as for Arria10, could we maybe
>     > drop this special handling?
>     >
>     > That would mean we could drop this whole 'type' and the 'enum
>     > dwmac_type'...
> 
>     I seem to remember the register layout for selecting the PHY mode on
>     Gen5 and Arria10 was different, please check the datasheets. Of course,
>     the less special-casing, the better.
> 
> 
> Yes, the register layout is different. But the reg_shift value takes
> care of that. Luckily, the bits have the same meaning even if the offset
> is different.

Fantastic

> What remains is the special case for Stratix10. Looking at the Linux
> driver and the register descriptions, I think we can drop the special
> cases, but someone would need to test it on Stratix10...

+CC Joyce.

Patch

diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
index 08fc9677c4..309da69647 100644
--- a/drivers/net/dwmac_socfpga.c
+++ b/drivers/net/dwmac_socfpga.c
@@ -27,6 +27,7 @@  struct dwmac_socfpga_platdata {
 	struct dw_eth_pdata	dw_eth_pdata;
 	enum dwmac_type		type;
 	void			*phy_intf;
+	u32			reg_shift;
 };
 
 static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
@@ -63,6 +64,7 @@  static int dwmac_socfpga_ofdata_to_platdata(struct udevice *dev)
 	}
 
 	pdata->phy_intf = range + args.args[0];
+	pdata->reg_shift = args.args[1];
 
 	/*
 	 * Sadly, the Altera DT bindings don't have SoC-specific compatibles,
@@ -88,9 +90,11 @@  static int dwmac_socfpga_probe(struct udevice *dev)
 	struct eth_pdata *edata = &pdata->dw_eth_pdata.eth_pdata;
 	struct reset_ctl_bulk reset_bulk;
 	int ret;
-	u8 modereg;
+	u32 modereg;
+	u32 modemask;
 
-	if (pdata->type == DWMAC_SOCFPGA_ARRIA10) {
+	if (pdata->type == DWMAC_SOCFPGA_ARRIA10 ||
+	    pdata->type == DWMAC_SOCFPGA_GEN5) {
 		switch (edata->phy_interface) {
 		case PHY_INTERFACE_MODE_MII:
 		case PHY_INTERFACE_MODE_GMII:
@@ -115,9 +119,9 @@  static int dwmac_socfpga_probe(struct udevice *dev)
 
 		reset_assert_bulk(&reset_bulk);
 
-		clrsetbits_le32(pdata->phy_intf,
-				SYSMGR_EMACGRP_CTRL_PHYSEL_MASK,
-				modereg);
+		modemask = SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << pdata->reg_shift;
+		clrsetbits_le32(pdata->phy_intf, modemask,
+				modereg << pdata->reg_shift);
 
 		reset_release_bulk(&reset_bulk);
 	}