Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Andre Przywara]

Hi,

On 27/06/17 11:23, Icenowy Zheng wrote:
> 
> 
> 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <andre.przyw...@arm.com> 写到:
>> 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
>>>>>> <clabbe.montj...@gmail.com> 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 Ch

Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Andre Przywara]

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
>>>> <clabbe.montj...@gmail.com> 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@1c3 {
compatible = "allwinner,sun8i-h3-emac";
...
phy-mode = "mii";
phy-handle = <_mii_phy>;
...

mdio: mdio {
#address-cells = <1>;
#size-cells = <0>;
int_mii_phy: ethernet-phy@1 {
compatible = "allwinner,sun8i-h3-ephy";
syscon = <>;
reg = <1>;
clocks = < CLK_BUS_EPHY>;
resets = < 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.

Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Andre Przywara]

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
>>  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.

Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Andre Przywara]

Hi,

On 25/07/16 20:54, Maxime Ripard wrote:
> On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote:
>> This patch add support for sun8i-emac ethernet MAC hardware.
>> It could be found in Allwinner H3/A83T/A64 SoCs.
>>
>> It supports 10/100/1000 Mbit/s speed with half/full duplex.
>> It can use an internal PHY (MII 10/100) or an external PHY
>> via RGMII/RMII.
>>
>> Signed-off-by: LABBE Corentin 
>> ---
>>  drivers/net/ethernet/allwinner/Kconfig  |   13 +
>>  drivers/net/ethernet/allwinner/Makefile |1 +
>>  drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 
>> +++
>>  3 files changed, 2143 insertions(+)
>>  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c 
>> b/drivers/net/ethernet/allwinner/sun8i-emac.c
>> new file mode 100644
>> index 000..fc0c1dd
>> --- /dev/null
>> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> +
>> +/* struct dma_desc - Structure of DMA descriptor used by the hardware
>> + * @status: Status of the frame written by HW, so RO for the
>> + *  driver (except for BIT(31) which is R/W)
>> + * @ctl: Information on the frame written by the driver (INT, len,...)
>> + * @buf_addr: physical address of the frame data
>> + * @next: physical address of next dma_desc
>> + */
>> +struct dma_desc {
>> +u32 status;
>> +u32 ctl;
>> +u32 buf_addr;
>> +u32 next;
>> +};
> 
> You should use the endian-aware variants here.

For the records: just doing the sparse annotation with __le32 here will
of course not be sufficient to make it work on BE kernels. I added
proper endianness conversion to all accesses to the descriptors and got
it to work with an arm64 big-endian kernel on the Pine64.

I put a patch here:
https://gist.github.com/apritzel/bc792c4dbbd8789f5f18aef538e8c440

This particular version is untested (though it compiles), since I just
adapted the working patch against the newer driver code and couldn't
test it yet.
I am not really an endianness expert, so don't know if there are smarter
ways to tackle this, if we should for instance provide access wrappers
to the DMA descriptor fields.

I will try to test this later today, if that works, feel free to merge
those changes into your driver.

Cheers,
Andre.