Message-ID: <5d062410-ca12-8e57-1cad-02051f2a13f3@silicom-usa.com>
Date:   Fri, 30 Nov 2018 18:43:57 +0000
From:   Steve Douthit <stephend@...icom-usa.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
CC:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

On 11/30/18 12:43 PM, Florian Fainelli wrote:
> 
> 
> On 11/30/2018 9:34 AM, Steve Douthit wrote:
>> On 11/30/18 11:34 AM, Andrew Lunn wrote:
>>>> Yep, registering multiple interfaces is wrong.  The first board I tested
>>>> against only had a single MAC enabled (they can be disabled/hidden via
>>>> straps) so it just happened to work.
>>>
>>> Hi Steve
>>>
>>> Can you hide any/all via straps, or is 00.0 always guaranteed to
>>> exist?
>>
>> You can hide all the devices, but if function 1 is enabled then function
>> 0 must also be enabled, so not all combinations are valid.
>>
>>>> The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
>>>> structured as two devices of two functions each on fixed internal root
>>>> ports.
>>>>
>>>> from lspci:
>>>> <snip>
>>>>               +-16.0-[05]--+-00.0
>>>>               |            \-00.1
>>>>               +-17.0-[06]--+-00.0
>>>>               |            \-00.1
>>>> <snip>
>>>
>>> Is there any other hardware resource which is shared between the MAC
>>> interfaces? I'm just wondering if the driver has already solved this
>>> once. Is there an EEPROM per interface for the MAC address, or one
>>> shared EEPROM?
>>
>> NVM is shared, it's actually the same SPI flash that holds the BIOS for
>> this SoC.  Access is serialized by the swfw_sync semaphore.  I think the
>> device firmware is automagically handling offset translation.
>>
>> I don't think that helps for this case.
>>
>> There might be a better match for shared resources, but nothing springs
>> to mind.
>>
>>> Ah, how about using the 'cards_found' found variable. It is not
>>> perfect, in that it is not decremented in ixgb_remove(), and i wonder
>>> about race conditions since there does not appear to be any lock when
>>> it is incremented. But if cards_found == 0, register the MDIO bus.
>>
>> 'cards_found' doesn't exist for the ixgbe driver.  I could add it and
>> fix the race/decrement issues you mention, but it'd have to be a device
>> type specific count.  It's still possible there are other non-x550em_a
>> ixgbe devices in external PCIe slots that have different resource
>> sharing setups.
>>
>> It's still a lighter weight solution than poking around the parent bus
>> so I'll add a 'x550em_a_devs_found' counter to v2.
>>
> 
> If the MDIO bus block is hardwired to function 0, would it be acceptable
> to walk the PCI bus hierarchy from any driver's probe routine where PCI
> function != 0 and see if it is claimed/enabled already, and if so, skip
> registering the MDIO bus entirely?

It's not really hardwired to any function at the hardware level AFAICT.
Unless by 'hardwired' you mean we always register it to the same devfn?
Walking the bus to find the lowest numbered devfn was my original
suggestion.  I'm good with a PCI walk or a counter to choose which MAC
registers the MDIO bus.  Let me know what the consensus is.

> There is also possibly a problem if you have a shared MDIO device, and
> you turn off/power off the PCI function that does provide it. How can we
> make sure it remains alive for other functions to use it?

I'm not sure this is an issue.  I wouldn't expect the other MACs to lose
the ability to talk to their PHYs.  Those callbacks weren't changed to
use the registered MDIO bus.

Powered by blists - more mailing lists