Message-ID: <52A87A74.8040807@st.com>
Date:	Wed, 11 Dec 2013 14:45:08 +0000
From:	srinivas kandagatla <srinivas.kandagatla@...com>
To:	Chen-Yu Tsai <wens@...e.org>
Cc:	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	netdev <netdev@...r.kernel.org>,
	Rob Herring <rob.herring@...xeda.com>,
	devicetree <devicetree@...r.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-sunxi <linux-sunxi@...glegroups.com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC
 in Allwinner A20 SoC's

Hi Chen,

On 11/12/13 12:17, Chen-Yu Tsai wrote:

>>
>> I would be good to get actual picture of this hw setup, On ST the
>> additional glue logic which sits on top of the GMAC is to resposible for
>> selecting the correct retime clock.
> 
> I would have liked to look at the internal design, how the dwmac core
> is connected to the clock control, but that is out of the question.
> Still, based on the documents, I think our clock controller is partially
> intertwined with the GMAC. It takes GMAC's internally generated clock
> as one of several inputs, then sends it back to the GMAC to time tx data.
> 
This is very much similar to ST glue, one of the selected clk is used
for retime the tx data lines. This selection is more of board dependent.
It totally depends on how the GMAC is wired up with PHY.

> Judging by the register definitions listed in the A20 manual,
> the SoC glue layer clocks is something like this:
> 
>                                _________________________
>   MII TX clock from PHY >-----|___________    _________|----> to GMAC core
>   GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
>   Ext. 125MHz RGMII TX clk >--|__divider__/            |
>                               |________________________|
> 
> 
> For MII mode, the glue layer should select the TX clock from the PHY.
> The gate to the PHY should be disabled.
> 
> For RGMII mode, either the internal clock generated by the GMAC core,
> or the external 125MHz reference generated by the PHY can be selected.
> And the clock gate to the PHY should be enabled.
> If the 125MHz reference is used, the glue layer should select the proper
> divider (/1, /5, /50) based on the link speed.
> 
> For GMII mode, under 10/100 speeds, the operation matches MII mode.
> For gigabit speeds, should use a 125MHz clock (internal or external)
> and enable the output gate.
> 
> The glue layer may indeed sit on top or around the GMAC core.
> Nevertheless, its operational state does depend on the GMAC.
> The current callbacks present in the stmmac driver are a good model
> for this.

Callbacks are OK with me, as they give good level of abstraction as you
said.

But I don't like the idea of glue drivers passing the full platform data
to stmmac or glue driver parsing the platform data, which is going to
look as very ugly fixups.

Also, currently callbacks just take pdev, which seems to be forcing glue
drivers to use platform data as the only data structure to pass information.

My recommendation would be to add new parameter to these callbacks ,
which can be used for to store glue private datastructure, we could
actually use bsp_priv variable from platform data.

So the of_data structure would have some thing like:

struct stmmac_of_data {
        void * (*setup)(struct platform_device *pdev);
        void (*bus_setup)(struct platform_device *pdev, void *priv, void
__iomem *ioaddr);
        int (*init)(struct platform_device *pdev, void *priv);
        void (*exit)(struct platform_device *pdev, void *priv);
        void (*fix_mac_speed)(struct platform_device *pdev, void *priv,
unnsigned int speed);

};

setup() would return a private data struct of glue driver which can be
stored in plat->bsp_priv. Should be done at DT parsing level.

Regarding the bindings, If Peppe is happy to allow optional SOC specific
binding in it, it is Ok with me too.

But all SOC specific resources names and properties have to be properly
prefexed so that its not confused with dwmac properties.

Regarding reset, I think we can add the support in stmmac driver itself.

Regarding clocks, on STi glue we can not represent the configuration in
proper clock infrastructure.

Am happy to change sti glue driver to this interface style, if you are
Ok with this approach Or if you have any other better ideas, lets discuss.

Feel free to change the above proposed new APIs..

> 
>>>> I see issues with this approach, which are addressed in my patches.
>>>>
>>>> Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
>>>> STi SOC glue, which does implement a very similar glue driver.
>>>>
>>>> Do you see any issues not to do that way?
>>>
>>> The glue layer driver would not have access to any of dwmac driver's
>>> internals, nor could it respond to any state changes.
>>
>> Should it have access to this in the first place?
>> Should SOC Glue driver actually change the internal data structures of
>> the stmmac? I guess No.
>>
>> I think all you need is to know the interface type which can be derived
>> from a child node as I did in my glue driver.
> 
> The interface type is not enough. In GMII mode, you would have to change
> the tx clock parent to match the link speed, as I mentioned in a previous
> section.

Ok, this should be specific to the glue driver I guess.


> 
> This I am working on. Here is what I propose:
> 
> stmmac takes a second optional clock, named "tx_clk".

> The clock provider should:
> 
[---
>   1. put the glue layer into MII mode when the clock is disabled
>   2. put the glue layer into RGMII mode when the clock is enabled
>   3. support clk_set_rate at 125, 25, or 2.5 MHz for RGMII link
>      speed changes. This could be a no-op for the internal clock.
>
---]
I think this is capturing your glue implementation details which might
not be true with other glue drivers.

> While we're at it, we can also make stmmac take an optional reset handler.
> 
> I think this solves both our problems, while being both generic and
> part of the gmac core hardware.
> 
> The rest of my requirements for Allwinner A20/A31 gmac can be solved
> with proper PHY node support, which I will implement, and
> "snps,force_sf_dma_mode" and "snps,tx-coe" properties.

This makes sense, I think this aread would need a cleanup any way.

> 
>>>>> +     plat_dat = dev_get_platdata(&pdev->dev);
>>>> This is not valid for DT case.
>>>> In DT case stmmac maintains its own instance of platform data.
>>>>
>>>> Setting platform data dynamically after probe to a device is not the
>>>> right way to do.
>>>
>>> I see. And stmmac's own instance of platform data is associated with the
>>> device in stmmac_dvr_probe. To make this work, we would have to move
>>> setting dev->priv->plat in front of the .init callback.
>>
>> No it would not work either, as platform data is stmmac is stored in its
>> private data structure.
> 
> With these modifications, the .init and .exit callbacks will have access
> to stmmac's private data structure, as they are passed dev as the sole
> argument.

Your suggestion looks like a hack, lets do it properly, as I suggested
and at the start.

>>>
>>> 1. .tx_coe
>>>    This is not exported in the DT bindings.
>>>    Looking at stmmac code, not setting this seems to disable all
>>>    checksum offloading.
>>
>> Why cant this go via DT as well?
> 
> If you and Giuseppe are OK with this, why not?
Am Ok with it.

>>> 3. .mdio_bus_data
>>>    The PHY mask is something we would like to have at the board level.
>>>    The PHY on one of our boards, RTL8211E from Realtek, responds to
>>>    commands sent to phy address 0, regardless what it's actual address is.
>>
>> Sounds correct.as
>>
>>>    This is documented in RTL8211E's datasheet:
>>>
>>>       The RTL8211E-VB(VL)/RTL8211EG-VB support PHY addresses from 00001 to
>>>       00111. PHY address 0 is a broadcast from the MAC; each PHY device
>>>       should respond.
>>>
>>>    Without the PHY mask, probing the MDIO bus would yield two devices,
>>>    when in reality there is only one. This results in some confusion.
>> Can you explain the issue in more detail.
>>
>> Why is it a confusion? PHYs are supposed to respond to address 00000.
> 
> If you look under /sys/bus/mdio_bus/devices/, for PHYs supporting
> address 00000 as broadcast, you would see 2 devices.
> stmmac debug messages will also report 2 devices found.
> 
> Also, not all PHY devices respond to address 00000.
> The Realtek RTL8201CP PHY on one of our other boards, does not.
> 
> I could not find a reference to "supposed to respond to address 00000".
> IEEE 802.3 only defines the address space, not intended uses.
Here is the reference:

"22.2.4.5.5 PHYAD (PHY Address): The PHY Address is five bits, allowing
32 unique PHY addresses. The first PHY address bit transmitted and
received is the MSB of the address. A PHY that is connected to the
station management entity via the mechanical interface defined in 22.6
shall always respond to transactions addressed to PHY Address zero
<00000>. A station management entity that is attached to multiple PHYs
must have prior knowledge of the appropriate PHY Address for each PHY."


> 
>> MDIO bus could have multiple PHYS on the bus, it does not matter as long
>> as you address them with correct PHYID.
> 
> Correct. I assume by PHYID you mean PHY address.
> Florian has pointed out that "snps,phy-addr" is !standard, and we
> should replace it with standard PHY node support. I can address this
> in the patch series.

I agree, if you have bandwidth cleanup in this area would be good.

Anyway I have limited access to emails for the next 3 weeks, so I can
not respond quickly.

Hope you have a good Christmas and new year..

Will talk to you after new year.

Thanks,
srini
> 
> Thanks,
> ChenYu
> 
>> Thanks,
>> srini
>>>
>>>    This should definitely be exported as a DT binding.
>>>
>>> I am also unable to add a generic compatible string for our particular
>>> dwmac, because I do not know the exact version of the IP, only that
>>> it is earlier than 3.50. No hardware DMA capability flags.
>>>
>>> Using something like "snps,dwmac-pre-3.50" is IMO worse.
>>>
>>>>>       { /* sentinel */ }
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
>>>>>
>>>>
>>>
>>> ChenYu
>>>
>>>
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists