Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Wed, 10 Feb 2021 10:14:59 +0100
From:   Michael Walle <michael@walle.cc>
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Andrew Lunn <andrew@lunn.ch>,
        Russell King <linux@armlinux.org.uk>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH net-next 7/9] net: phy: icplus: select page before writing
 control register
In-Reply-To: <1656b889-12c4-b376-5cdf-38e1dcc500bc@gmail.com>
References: <20210209164051.18156-1-michael@walle.cc>
 <20210209164051.18156-8-michael@walle.cc>
 <d5672062-c619-02a4-3bbe-dad44371331d@gmail.com>
 <e9d26cd6634a8c066809aa92e1481112@walle.cc>
 <1656b889-12c4-b376-5cdf-38e1dcc500bc@gmail.com>
User-Agent: Roundcube Webmail/1.4.10
Message-ID: <f3f831f453378fe3b55e8fc5606266eb@walle.cc>
Precedence: bulk

Hi,

Am 2021-02-10 10:03, schrieb Heiner Kallweit:
[..]
>>>> +    return phy_restore_page(phydev, oldpage, err);
>>> 
>>> If a random page was set before entering config_init, do we actually 
>>> want
>>> to restore it? Or wouldn't it be better to set the default page as 
>>> part
>>> of initialization?
>> 
>> First, I want to convert this to the match_phy_device() and while at 
>> it,
>> I noticed that there is this one "problem". Short summary: the IP101A 
>> isn't
>> paged, the IP101G has serveral and if page 16 is selected it is more 
>> or
>> less compatible with the IP101A. My problem here is now how to share 
>> the
>> functions for both PHYs without duplicating all the code. Eg. the 
>> IP101A
>> will phy_read/phy_write/phy_modify(), that is, all the locked 
>> versions.
>> For the IP101G I'd either need the _paged() versions or the __phy ones
>> which don't take the mdio_bus lock.
>> 
>> Here is what I came up with:
>> (1) provide a common function which uses the __phy ones, then the
>>     callback for the A version will take the mdio_bus lock and calls
>>     the common one. The G version will use 
>> phy_{select,restore}_page().
>> (2) the phy_driver ops for A will also provde a .read/write_page()
>>     callback which is just a no-op. So A can just use the G versions.
>> (3) What Heiner mentioned here, just set the default page in
>>     config_init().
>> 
>> (1) will still bloat the code; (3) has the disadvantage, that the
>> userspace might fiddle around with the page register and then the
>> whole PHY driver goes awry. I don't know if we have to respect that
>> use case in general. I know there is an API to read/write the PHY
>> registers and it could happen.
>> 
> 
> The potential issue you mention here we have with all PHY's using
> pages. As one example, the genphy functions rely on the PHY being
> set to the default page. In general userspace can write PHY register
> values that break processing, independent of paging.
> I'm not aware of any complaints regarding this behavior, therefore
> wouldn't be too concerned here.

I'm fine with that, that will make the driver easier.

> Regarding (2) I'd like to come back to my proposal from yesterday,
> implement match_phy_device to completely decouple the A and G versions.
> Did you consider this option?

Yes, that is what I was talking about above: "First, I want to convert
this to the match_phy_device()" ;) And then I stumbled across that 
problem
I was describing above.

It will likely go away if I just set the page to the default page.

>> That being said, I'm either fine with (2) and (3) but I'm preferring
>> (2).
>> 
>> BTW, this patch is still missing read/writes to the interrupt status
>> and control registers which is also paged.

-- 
-michael