Message-ID: <95e106fd-d1ce-b9d4-a4f7-03fb69bd4aaa@gmail.com>
Date:   Mon, 20 Mar 2023 12:06:31 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
Content-Language: en-US
To:     =?UTF-8?Q?=c3=81lvaro_Fern=c3=a1ndez_Rojas?= <noltari@gmail.com>,
        jonas.gorski@gmail.com, andrew@lunn.ch, olteanv@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230320182813.963508-1-noltari@gmail.com>
From:   Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20230320182813.963508-1-noltari@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/20/23 11:28, Álvaro Fernández Rojas wrote:
> Currently, B53 MMAP BCM63xx devices with an external switch hang when
> performing PHY read and write operations due to invalid registers access.
> This adds support for PHY ops by using the internal bus from mdio-mux-bcm6368
> when probed by device tree and also falls back to direct MDIO registers if not.
> 
> This is an alternative to:
> - https://patchwork.kernel.org/project/netdevbpf/cover/20230317113427.302162-1-noltari@gmail.com/
> - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-2-noltari@gmail.com/
> - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-3-noltari@gmail.com/
> - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-4-noltari@gmail.com/
> As discussed, it was an ABI break and not the correct way of fixing the issue.

Looks good for the most part, just a few questions below.

> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>   drivers/net/dsa/b53/b53_mmap.c    | 86 +++++++++++++++++++++++++++++++
>   include/linux/platform_data/b53.h |  1 +
>   2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/net/dsa/b53/b53_mmap.c b/drivers/net/dsa/b53/b53_mmap.c
> index 706df04b6cee..7deca1c557c5 100644
> --- a/drivers/net/dsa/b53/b53_mmap.c
> +++ b/drivers/net/dsa/b53/b53_mmap.c
> @@ -19,14 +19,25 @@
>   #include <linux/bits.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/of_mdio.h>
>   #include <linux/io.h>
>   #include <linux/platform_device.h>
>   #include <linux/platform_data/b53.h>
>   
>   #include "b53_priv.h"
>   
> +#define REG_MDIOC		0xb0
> +#define  REG_MDIOC_EXT_MASK	BIT(16)
> +#define  REG_MDIOC_REG_SHIFT	20
> +#define  REG_MDIOC_PHYID_SHIFT	25
> +#define  REG_MDIOC_RD_MASK	BIT(30)
> +#define  REG_MDIOC_WR_MASK	BIT(31)

For some reason, there was no bit introduced to tell us when a 
transaction has finished, so we have to poll after a certain delay has 
elapsed...

> +
> +#define REG_MDIOD		0xb4
> +
>   struct b53_mmap_priv {
>   	void __iomem *regs;
> +	struct mii_bus *bus;
>   };
>   
>   static int b53_mmap_read8(struct b53_device *dev, u8 page, u8 reg, u8 *val)
> @@ -216,6 +227,69 @@ static int b53_mmap_write64(struct b53_device *dev, u8 page, u8 reg,
>   	return 0;
>   }
>   
> +static inline void b53_mmap_mdio_read(struct b53_device *dev, int phy_id,
> +				      int loc, u16 *val)
> +{
> +	uint32_t reg;
> +
> +	b53_mmap_write32(dev, 0, REG_MDIOC, 0);
> +
> +	reg = REG_MDIOC_RD_MASK |
> +	      (phy_id << REG_MDIOC_PHYID_SHIFT) |
> +	      (loc << REG_MDIOC_REG_SHIFT);
> +
> +	b53_mmap_write32(dev, 0, REG_MDIOC, reg);
> +	udelay(50);
> +	b53_mmap_read16(dev, 0, REG_MDIOD, val);
> +}
> +
> +static inline int b53_mmap_mdio_write(struct b53_device *dev, int phy_id,
> +				      int loc, u16 val)
> +{
> +	uint32_t reg;
> +
> +	b53_mmap_write32(dev, 0, REG_MDIOC, 0);
> +
> +	reg = REG_MDIOC_WR_MASK |
> +	      (phy_id << REG_MDIOC_PHYID_SHIFT) |
> +	      (loc << REG_MDIOC_REG_SHIFT) |
> +	      val;
> +
> +	b53_mmap_write32(dev, 0, REG_MDIOC, reg);
> +	udelay(50);
> +
> +	return 0;
> +}
> +
> +static int b53_mmap_phy_read16(struct b53_device *dev, int addr, int reg,
> +			       u16 *value)
> +{
> +	struct b53_mmap_priv *priv = dev->priv;
> +	struct mii_bus *bus = priv->bus;
> +
> +	if (bus)
> +		*value = mdiobus_read_nested(bus, addr, reg);

Since you make the 'mii-bus' property and 'priv->bus' necessary 
prerequisites for the driver to finish probing successfully, when shall 
we not have valid priv->bus reference to work with? Do we end-up taking 
the other path at all?
-- 
Florian