Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

Vikram Sethi
2016-04-14T22:05:26Z

A couple of clarifications on the SGMII internal PHY and the DMA capability of the EMAC inline.

On 04/14/2016 04:19 PM, Florian Fainelli wrote:
> On 14/04/16 13:19, Timur Tabi wrote:
>> Florian Fainelli wrote:
>>> On 13/04/16 10:59, Timur Tabi wrote:
>>>> From: Gilad Avidov <gavidov@codeaurora.org>
>>>>
>>>> Add supports for ethernet controller HW on Qualcomm Technologies,
>>>> Inc. SoC.
>>>> This driver supports the following features:
>>>> 1) Checksum offload.
>>>> 2) Runtime power management support.
>>>> 3) Interrupt coalescing support.
>>>> 4) SGMII phy.
>>>> 5) SGMII direct connection without external phy.
>>>
>>>
>>> [snip]
>>>
>>>> +- qcom,no-external-phy : Indicates there is no external PHY
>>>> connected to
>>>> + EMAC. Include this only if the EMAC is directly
>>>> + connected to the peer end without EPHY.
>>> How is the internal PHY accessed, is it responding on the MDIO bus at a
>>> particular address?
>> There is a set of memory-mapped registers. It's not connected via MDIO
>> at all. It's mapped via the "sgmii" addresses in the device tree (see
>> function emac_sgmii_config).
>>
>>> If so, standard MDIO scanning/probing works, and you
>>> can have your PHY driver flag this device has internal. Worst case, you
>>> can do what BCMGENET does, and have a special "phy-mode" value set to
>>> "internal" when this knowledge needs to exist prior to MDIO bus scanning
>>> (e.g: to power on the PHY).
>> So the internal phy is not a real phy. It's not capable of driving an
>> RJ45 port (there's no analog part). It's an SGMII-like device that is
>> hard-wired to the EMAC itself.
There *is* an analog part to the internal SGMII PHY. Please check the SGMII specification. The only non-standard part is that it's not on MDIO.

> OK, that explains things a bit, thanks, this is quite a bit of important
> detail actually.
>
>> In theory, the internal PHY is optional. You could design an SOC that
>> has just the EMAC connected via normal MDIO to an external phy. I
>> really wish our hardware designers has done that. But unfortunately,
>> there are no SOCs like that, and so we have to treat the internal phy as
>> an extension of the EMAC.
>>
>> My preference would be to get rid of the "qcom,no-external-phy" property
>> and have an external phy be required, at least until Qualcomm creates an
>> SOC without the internal phy (which may never happen, for all I know).
>>
> Can we just say that, an absence of PHY specified in the Device Tree (no
> phy-handle property and PHY not a child node of the MDIO bus), means
> that there is no external PHY?
>
> [snip]
>
>
[snip]
>>>> + dev_set_drvdata(&pdev->dev, netdev);
>>>> + SET_NETDEV_DEV(netdev, &pdev->dev);
>>>> +
>>>> + adpt = netdev_priv(netdev);
>>>> + adpt->netdev = netdev;
>>>> + phy = &adpt->phy;
>>>> + adpt->msg_enable = netif_msg_init(debug, EMAC_MSG_DEFAULT);
>>>> +
>>>> + dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>>> Really, is not that supposed to run on ARM64 servers?
>> Well, this version of the driver isn't, which is why it supports DT and
>> not ACPI. I'm planning on adding that support in a later patch.
>> However, I'll add support for 64-bit masks in the next version of this
>> patch.
>>
>> Would this be okay:
>>
>> retval = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> if (retval) {
>> dev_err(&pdev->dev, "failed to set DMA mask err %d\n", retval);
>> goto err_res;
>> }

How can you set the mask to 64 bits when the EMAC IP on FSM9900 and QDF2432 can only do 32 bit DMA?
The mask in that API is a bit mask describing which bits of an address your device supports.

>> I've seen code like this in other drivers:
>>
>> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> if (ret) {
>> ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> if (ret) {
>> dev_err(dev, "failed to set dma mask\n");
>> return ret;
>> }
>> }
>>
>> and I've never understood why it's necessary to fall back to 32-bits if
>> 64 bits fails. Isn't 64 bits a superset of 32 bits? The driver is
>> saying that the hardware supports all of DDR. How could fail, and how
>> could 32-bit succeed if 64-bits fails?
> I believe there could be cases where the HW is capable of addressing
> more physical memory than the CPU itself (usually unlikely, but it
> could), there could be cases where the HW is behind an IOMMMU which only
> has a window into the DDR, and that could prevent a higher DMA_BIT_MASK
> from being successfully configured.

--
Vikram Sethi
Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project