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;
From:   <Jerry.Ray@microchip.com>
To:     <olteanv@gmail.com>
CC:     <andrew@lunn.ch>, <vivien.didelot@gmail.com>,
        <f.fainelli@gmail.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/2] net: dsa: LAN9303: Add basic support for LAN9354
Thread-Topic: [PATCH 1/2] net: dsa: LAN9303: Add basic support for LAN9354
Thread-Index: AQHYu9FZPa9+/Wmd806ISn/Vxl3m2K3HQE0AgAVXz2A=
Date:   Fri, 2 Sep 2022 20:10:46 +0000
Message-ID: <MWHPR11MB16939B94E6A64234CA14167DEF7A9@MWHPR11MB1693.namprd11.prod.outlook.com>
References: <20220829180037.31078-1-jerry.ray@microchip.com>
 <20220830103340.bqgzmcztb57m7jgd@skbuf>
In-Reply-To: <20220830103340.bqgzmcztb57m7jgd@skbuf>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?jW7nkl8nKz+nSijC2Bic3M1gOJHoBXalTWQkI5+a0Ok5iuLXimUj+ZMRBcXA?=
 =?us-ascii?Q?KMfooA5KfPPekkI1o3NF2TPE7nkwi04co5SWJbH/jRFeIgIlA2zVCTdK8dWg?=
 =?us-ascii?Q?iAHOwpV8KM19mnYV2Kj8wjwZLaYDoC8gc0ctT3iZXqmt/i+7HgjFElL6ez23?=
 =?us-ascii?Q?rvKWU017CQzcz3KgtxHbsLIF+r6UcnEttemLvEAndeg2++9/nap+osXqnj0K?=
 =?us-ascii?Q?N5UmAG/tFVRMAbtd1bJkmn1ET0rfKm48jCiJMNPrpCQmbXnJ9n2ZqDvNxBFP?=
 =?us-ascii?Q?nbD7IjM3Noy0GmdHzxloFL5RAHcXgUSx/7UFA6F5Fghe+bMc71kWdeRNf/Ec?=
 =?us-ascii?Q?T1Z/d3ZRzoPXNxusFYyIWEvFDM/d9pR7Y9p66/ekDbYOaCAJqGF48iZdJLI7?=
 =?us-ascii?Q?mW8zLP7hNFqXJ6f2ckquRfCJb/k1nO37OD+pxb5l8I80E7PblLpP1X2QyWDX?=
 =?us-ascii?Q?2V10sMqY2OO4BA0bs2tQOac9zuIA9kp7CpwrXGljmEhXY7wPwQxJwefQCMNZ?=
 =?us-ascii?Q?9J9frG7IkPcjxgLGZKogVHZl5fqFgzJYfGItpkB/CDiypOpuTRB6vI1/5i/q?=
 =?us-ascii?Q?Up6mEcAb7T+lx6FZzb2bf/gniI9HRrVk1RKQEVJCPsx/FC55WeqjoOfqeDhR?=
 =?us-ascii?Q?V1lcqZ8K7/mAHo6ilEc+TnhCY4r/2ZNPS+IRA6fVSAxQg/HwwLUFe6oX3y6Z?=
 =?us-ascii?Q?ZwFkBrgX5HYCq22JZpIs/AP+Xf/+R0CXZqJR0mjq8/asjacDVTC9xNz24v7T?=
 =?us-ascii?Q?rLP5DZ08ZDQPJLORpjxyNFLD670fnJvek6BBZBWfxiUDkC3DQW2ycF4S5QhD?=
 =?us-ascii?Q?K4W+dpYb4GlTSYaq4eOn9uTYEdhRymvliHW3QH2BdY75iIIJhAxyXIJTLcCI?=
 =?us-ascii?Q?7tymRnGvL4UBfhbjm2fL8gD/LkdRlsi4xFpLXx6NUd5V3ARYuhnPVcixk8vQ?=
 =?us-ascii?Q?VvpdD/M74DqkqmyOf9bhtNQBwAFnU/TTdPKei2cyfATapfCQ9ko3by/Y/Mmw?=
 =?us-ascii?Q?xVwx3VkWhO1jtRqSuOy5pDJk63D9nV/Yawe039O/CdjZgBNMuhZdhzKtO0VP?=
 =?us-ascii?Q?0NAPfFwmV47xAQXQJyoPUt7TeM4t6DmRe8EsET4ryXvjhuRZq/rcq8GBUfRp?=
 =?us-ascii?Q?Krryc8uCxavW6nTyJZnLBHrE63KzpjzLiMHcF8vWtl2NeOlM9H3QTBo6pIdO?=
 =?us-ascii?Q?RMSpYOjEtCBKMh3+4las3hO+B2Jsv3YRwMZjW8Fg4C8eEKvexT8EnEcrnAFi?=
 =?us-ascii?Q?uUZoQs0I27NE+L5j78DpQv9P5t59f7qQuV9nO98pZxSIaJybNC6XjHpjxZOi?=
 =?us-ascii?Q?F9K5sNhinGFwX++dlRAERj3F+G4fP3aBzVERaiSSDA7EinFeZi6bDqAJlFnd?=
 =?us-ascii?Q?/+pWvDu8lJib1/ma4QslrlvI2r6voX5hmqaMxOw3Id2PEbQinHTjFRDVVLW7?=
 =?us-ascii?Q?ucgCj4nWAhvVnA9ghoC0qNt4Ri5GJu6eECIgscx8fwgQMlNC2xfOBXKuVies?=
 =?us-ascii?Q?X84S780ADVZn2iZuiyKvdSBzx1NAUXgOcmvVJ7M/4K+ujUxTyWDy5ZFt4i6h?=
 =?us-ascii?Q?mQQUbaG18e3aa9W0+JCjnKI8JjtWTiw3e+YYx6UI?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1693.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a333c62-defb-43f7-1654-08da8d1f38f1
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Sep 2022 20:10:46.4548
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: zF5Wgcq7gZcp1uVCa9lCJ4fqw1RDbynATxSWVMy6VL+M0EO76BKnRHCBGRwUCob4zjaVZawFd/PE2trYtCZ+HA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6805
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>On Mon, Aug 29, 2022 at 01:00:36PM -0500, Jerry Ray wrote:
>> Add initial BYTE_ORDER read to sync to improve driver robustness
>
>Please don't post 2 different patches with the same commit message.
>I think here, the first paragraph is what the commit message should actual=
ly be.
>

Understood.

>>
>> The lan9303 expects two mdio read transactions back-to-back to read a=20
>> 32-bit register. The first read transaction causes the other half of=20
>> the 32-bit register to get latched.  The subsequent read returns the=20
>> latched second half of the 32-bit read. The BYTE_ORDER register is an=20
>> exception to this rule. As it is a constant value, there is no need to=20
>> latch the second half. We read this register first in case there were=20
>> reads during the boot loader process that might have occurred prior to=20
>> this driver taking over ownership of accessing this device.
>>
>> This patch has been tested on the SAMA5D3-EDS with a LAN9303 RMII=20
>> daughter card.
>
>Is this patch fixing a problem for any existing platforms supported by thi=
s driver?
>

This patch is fixing a problem I ran into that probably doesn't occur under
normal use case conditions.  I was probing around on the mdio bus within
u-boot, then booting linux.  This change makes the linux driver more robust
(tolerant) to this situation.

>>
>> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
>> ---
>>  drivers/net/dsa/lan9303-core.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c=20
>> b/drivers/net/dsa/lan9303-core.c index e03ff1f267bb..17ae02a56bfe=20
>> 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -32,6 +32,7 @@
>>  #define LAN9303_INT_EN 0x17
>>  # define LAN9303_INT_EN_PHY_INT2_EN BIT(27)  # define=20
>> LAN9303_INT_EN_PHY_INT1_EN BIT(26)
>> +#define LAN9303_BYTE_ORDER 0x19
>>  #define LAN9303_HW_CFG 0x1D
>>  # define LAN9303_HW_CFG_READY BIT(27)  # define=20
>> LAN9303_HW_CFG_AMDX_EN_PORT2 BIT(26) @@ -847,9 +848,10 @@ static int=20
>> lan9303_check_device(struct lan9303 *chip)
>>       int ret;
>>       u32 reg;
>>
>> -     ret =3D lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>> +     // Dummy read to ensure MDIO access is in 32-bit sync.
>
>C-style comments /* */ are more typical in the Linux kernel coding style.
>

Understood.

>> +     ret =3D lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &reg);
>
>Pretty strange to see the dummy read in lan9303_check_device().
>Bootloader leaving things in a messy state is only a problem if we don't h=
ave a reset GPIO, right?
>
>How about introducing the logic here, right in lan9303_probe():
>
>        lan9303_handle_reset(chip);
>
>        if (!chip->reset_gpio) {
>                /* Dummy read to ensure MDIO access is in 32-bit sync. */
>                ret =3D lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &re=
g);
>                if (ret) {
>                        dev_err(chip->dev, "failed to access the device: %=
pe\n",
>                                ERR_PTR(ret));
>                        return ret;
>                }
>        }
>
>        ret =3D lan9303_check_device(chip);
>

I'll look to move it.

>>       if (ret) {
>> -             dev_err(chip->dev, "failed to read chip revision register:=
 %d\n",
>> +             dev_err(chip->dev, "failed to access the device: %d\n",
>>                       ret);
>>               if (!chip->reset_gpio) {
>>                       dev_dbg(chip->dev,
>
>The context here reads:
>                if (!chip->reset_gpio) {
>                        dev_dbg(chip->dev,
>                                "hint: maybe failed due to missing reset G=
PIO\n");
>                }
>
>Is the comment still accurate after the change, or do you feel that it can=
 be removed? Looks like you are fixing a known issue.
>

The 'hint' message applies to the first chip access, which has now changed =
to the BYTE_ORDER read.

>> @@ -858,6 +860,13 @@ static int lan9303_check_device(struct lan9303 *chi=
p)
>>               return ret;
>>       }
>>
>> +     ret =3D lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>> +     if (ret) {
>> +             dev_err(chip->dev, "failed to read chip revision register:=
 %d\n",
>> +                     ret);
>> +             return ret;
>> +     }
>> +
>>       if ((reg >> 16) !=3D LAN9303_CHIP_ID) {
>>               dev_err(chip->dev, "expecting LAN9303 chip, but found: %X\=
n",
>>                       reg >> 16);
>> --
>> 2.17.1
>>

Thanks,
Jerry.