Message-ID: <7bf5761c-abdd-a3cb-267c-5e61641b15f8@gmail.com>
Date:   Sat, 18 Mar 2023 10:40:46 -0400
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 net-next v3 9/9] net: sunhme: Consolidate common probe
 tasks
Content-Language: en-US
To:     Simon Horman <simon.horman@corigine.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230314003613.3874089-1-seanga2@gmail.com>
 <20230314003613.3874089-10-seanga2@gmail.com> <ZBXCWUm/1ffaD1B+@corigine.com>
From:   Sean Anderson <seanga2@gmail.com>
In-Reply-To: <ZBXCWUm/1ffaD1B+@corigine.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/18/23 09:53, Simon Horman wrote:
> On Mon, Mar 13, 2023 at 08:36:13PM -0400, Sean Anderson wrote:
>> Most of the second half of the PCI/SBUS probe functions are the same.
>> Consolidate them into a common function.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v1)
>>
>>   drivers/net/ethernet/sun/sunhme.c | 183 ++++++++++++------------------
>>   1 file changed, 71 insertions(+), 112 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c
>> index a59b998062d9..a384b162c46d 100644
>> --- a/drivers/net/ethernet/sun/sunhme.c
>> +++ b/drivers/net/ethernet/sun/sunhme.c
>> @@ -2430,6 +2430,71 @@ static void happy_meal_addr_init(struct happy_meal *hp,
>>   	}
>>   }
>>   
>> +static int happy_meal_common_probe(struct happy_meal *hp,
>> +				   struct device_node *dp, int minor_rev)
>> +{
>> +	struct net_device *dev = hp->dev;
>> +	int err;
>> +
>> +#ifdef CONFIG_SPARC
>> +	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> +	if (hp->hm_revision == 0xff)
>> +		hp->hm_revision = 0xc0 | minor_rev;
>> +#else
>> +	/* works with this on non-sparc hosts */
>> +	hp->hm_revision = 0x20;
>> +#endif
> 
> ...
> 
>> +#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> +	/* Hook up SBUS register/descriptor accessors. */
>> +	hp->read_desc32 = sbus_hme_read_desc32;
>> +	hp->write_txd = sbus_hme_write_txd;
>> +	hp->write_rxd = sbus_hme_write_rxd;
>> +	hp->read32 = sbus_hme_read32;
>> +	hp->write32 = sbus_hme_write32;
>> +#endif
> 
> This looks correct for the SBUS case.
> But I'm not sure about the PCIE case.
> 
> gcc 12 tells me when compiling with sparc allmodconfig that the following
> functions are now unused.
> 
>    pci_hme_read_desc32
>    pci_hme_write_txd
>    pci_hme_write_rxd
>    pci_hme_read32
>    pci_hme_write32

Oh, looks like I missed that these were different while refactoring.

That said, I haven't seen any issues here...

>> +
>> +	/* Grrr, Happy Meal comes up by default not advertising
>> +	 * full duplex 100baseT capabilities, fix this.
> b
>> +	 */
>> +	spin_lock_irq(&hp->happy_lock);
>> +	happy_meal_set_initial_advertisement(hp);
>> +	spin_unlock_irq(&hp->happy_lock);
>> +
>> +	err = devm_register_netdev(hp->dma_dev, dev);
>> +	if (err)
>> +		dev_err(hp->dma_dev, "Cannot register net device, aborting.\n");
>> +	return err;
>> +}
>> +
>>   #ifdef CONFIG_SBUS
>>   static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>>   {
> 
>> @@ -2511,70 +2576,18 @@ static int happy_meal_sbus_probe_one(struct platform_device *op, int is_qfe)
>>   		goto err_out_clear_quattro;
>>   	}
>>   
>> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> -	if (hp->hm_revision == 0xff)
>> -		hp->hm_revision = 0xa0;
> 
> It's not clear to me that the same value will be set by the call to
> happy_meal_common_probe(hp, dp, 0); where the logic is:
> 
> #ifdef CONFIG_SPARC
> 	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
> 	if (hp->hm_revision == 0xff)
> 		hp->hm_revision = 0xc0 | minor_rev;

OK, so maybe this should be xor, with sbus passing in 0x30.

> #else
> 	/* works with this on non-sparc hosts */
> 	hp->hm_revision = 0x20;
> #endif
> 
> I am assuming that the SPARC logic is run.
> But another question: is it strictly true that SBUS means SPARC?

Yes.

>> -
>> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> -	/* Hook up SBUS register/descriptor accessors. */
>> -	hp->read_desc32 = sbus_hme_read_desc32;
>> -	hp->write_txd = sbus_hme_write_txd;
>> -	hp->write_rxd = sbus_hme_write_rxd;
>> -	hp->read32 = sbus_hme_read32;
>> -	hp->write32 = sbus_hme_write32;
>> -#endif
> 
> ...
> 
>> @@ -2689,21 +2702,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev,
>>   	hp->bigmacregs = (hpreg_base + 0x6000UL);
>>   	hp->tcvregs    = (hpreg_base + 0x7000UL);
>>   
>> -#ifdef CONFIG_SPARC
>> -	hp->hm_revision = of_getintprop_default(dp, "hm-rev", 0xff);
>> -	if (hp->hm_revision == 0xff)
>> -		hp->hm_revision = 0xc0 | (pdev->revision & 0x0f);
>> -#else
>> -	/* works with this on non-sparc hosts */
>> -	hp->hm_revision = 0x20;
>> -#endif
> 
> ...
> 
>> -#if defined(CONFIG_SBUS) && defined(CONFIG_PCI)
>> -	/* Hook up PCI register/descriptor accessors. */
>> -	hp->read_desc32 = pci_hme_read_desc32;
>> -	hp->write_txd = pci_hme_write_txd;
>> -	hp->write_rxd = pci_hme_write_rxd;
>> -	hp->read32 = pci_hme_read32;
>> -	hp->write32 = pci_hme_write32;
>> -#endif
> 
> ...

--Sean