lkml.org 
[lkml]   [2022]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/2] net: dsa: LAN9303: Add basic support for LAN9354
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
actually be.

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

Is this patch fixing a problem for any existing platforms supported by
this driver?

>
> 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 b/drivers/net/dsa/lan9303-core.c
> index e03ff1f267bb..17ae02a56bfe 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 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 LAN9303_HW_CFG_AMDX_EN_PORT2 BIT(26)
> @@ -847,9 +848,10 @@ static int lan9303_check_device(struct lan9303 *chip)
> int ret;
> u32 reg;
>
> - ret = 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.

> + ret = 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
have 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 = lan9303_read(chip->regmap, LAN9303_BYTE_ORDER, &reg);
if (ret) {
dev_err(chip->dev, "failed to access the device: %pe\n",
ERR_PTR(ret));
return ret;
}
}

ret = lan9303_check_device(chip);

> 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 GPIO\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.

> @@ -858,6 +860,13 @@ static int lan9303_check_device(struct lan9303 *chip)
> return ret;
> }
>
> + ret = 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) != LAN9303_CHIP_ID) {
> dev_err(chip->dev, "expecting LAN9303 chip, but found: %X\n",
> reg >> 16);
> --
> 2.17.1
>

\
 
 \ /
  Last update: 2022-08-30 12:34    [W:0.164 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site