Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9d17ab9f-1679-4af1-f85c-a538cb330d7b@ti.com>
Date:   Thu, 11 Aug 2022 12:30:00 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2 net-next] net: ethernet: ti: davinci_mdio: Add
 workaround for errata i2329
Content-Language: en-US
To:     Andrew Lunn <andrew@lunn.ch>
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <linux-omap@vger.kernel.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <kishon@ti.com>,
        <vigneshr@ti.com>
References: <20220810111345.31200-1-r-gunasekaran@ti.com>
 <YvRNpAdG7/edUEc+@lunn.ch>
From:   Ravi Gunasekaran <r-gunasekaran@ti.com>
In-Reply-To: <YvRNpAdG7/edUEc+@lunn.ch>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/08/22 6:00 am, Andrew Lunn wrote:
>> +static int davinci_mdiobb_read(struct mii_bus *bus, int phy, int reg)
>> +{
>> +	int ret;
>> +	struct mdiobb_ctrl *ctrl = bus->priv;
>> +	struct davinci_mdio_data *data;
>> +
>> +	data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
>> +
>> +	if (phy & ~PHY_REG_MASK || reg & ~PHY_ID_MASK)
>> +		return -EINVAL;
> 
> You don't need this. Leave it up to the bit banging code to do the
> validation. This also breaks C45, which the bit banging code can do,
> and it looks like the hardware cannot.

I will remove these validation.

>> +
>> +	ret = pm_runtime_resume_and_get(data->dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = mdiobb_read(bus, phy, reg);
>> +
>> +	pm_runtime_mark_last_busy(data->dev);
>> +	pm_runtime_put_autosuspend(data->dev);
> 
> Once you take the validation out, this function then all becomes about
> runtime power management. Should the bit banging core actually be
> doing this? It seems like it is something which could be useful for
> other devices.
> 
> struct mii_bus has a parent member. If set, you could apply these run
> time PM functions to that. Please add a patch to modify the core bit
> banging code, and then you should be able to remove these helpers.

Devices may or may not be configured for runtime autosuspend, and 
perhaps may not even use runtime PM. pm_runtime_enabled() and the 
autosuspend configuration could be addressed by checking against 
dev->power.use_autosuspend flag. But if the runtime PM functions are 
added to the bit banging core, would it not restrict the usage of 
pm_runtime_put_*() variants for others?

There is atleast one device sh_eth, which is not configured for 
autosuspend but uses the bit bang core in sh_mdiobb_read() and invokes 
regular runtime PM functions.

>>   static int davinci_mdio_probe(struct platform_device *pdev)
>>   {
>>   	struct mdio_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> @@ -340,12 +535,30 @@ static int davinci_mdio_probe(struct platform_device *pdev)
>>   	struct phy_device *phy;
>>   	int ret, addr;
>>   	int autosuspend_delay_ms = -1;
>> +	const struct soc_device_attribute *soc_match_data;
> 
> netdev uses reverse christmas tree. Variables should be sorted longest
> first, shortest last.

Noted. I will fix this as well.


-- 
Regards,
Ravi