Re: git: 4720afaffe7e - main - Improve RK3568 pcie phy handling codes a bit.

On 7 Apr 2023, at 03:55, Ganbold Tsagaankhuu <ganbold@FreeBSD.org> wrote:
> 
> The branch main has been updated by ganbold:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=4720afaffe7e17e44ee0f8f3ab66da2fd2b0d5da
> 
> commit 4720afaffe7e17e44ee0f8f3ab66da2fd2b0d5da
> Author:     Ganbold Tsagaankhuu <ganbold@FreeBSD.org>
> AuthorDate: 2023-04-07 02:54:13 +0000
> Commit:     Ganbold Tsagaankhuu <ganbold@FreeBSD.org>
> CommitDate: 2023-04-07 02:54:13 +0000
> 
>    Improve RK3568 pcie phy handling codes a bit.
> 
>    Move phy bifurcation code to a separate function
>    that can be called during the attach phase.
>    Also initialize both pcie lanes accordingly.

No attempt at code review, just like all your recent commits?

> ---
> sys/arm64/rockchip/rk3568_pciephy.c | 87 +++++++++++++++++++++++--------------
> 1 file changed, 55 insertions(+), 32 deletions(-)
> 
> diff --git a/sys/arm64/rockchip/rk3568_pciephy.c b/sys/arm64/rockchip/rk3568_pciephy.c
> index 5320f30e31bd..d56675521841 100644
> --- a/sys/arm64/rockchip/rk3568_pciephy.c
> +++ b/sys/arm64/rockchip/rk3568_pciephy.c
> @@ -56,9 +56,12 @@
> #define	GRF_PCIE30PHY_CON4		0x10
> #define	GRF_PCIE30PHY_CON5		0x14
> #define	GRF_PCIE30PHY_CON6		0x18
> +#define	 GRF_BIFURCATION_LANE_1		0
> +#define	 GRF_BIFURCATION_LANE_2		1
> #define	 GRF_PCIE30PHY_WR_EN		(0xf << 16)
> #define	GRF_PCIE30PHY_CON9		0x24
> -#define	 GRF_PCIE30PHY_DA_OCM		((1 << 15) | (1 << (15 + 16)))
> +#define	 GRF_PCIE30PHY_DA_OCM_MASK	(1 << (15 + 16))
> +#define	 GRF_PCIE30PHY_DA_OCM		((1 << 15) | GRF_PCIE30PHY_DA_OCM_MASK)
> #define	GRF_PCIE30PHY_STATUS0		0x80
> #define	 SRAM_INIT_DONE			(1 << 14)
> 
> @@ -80,46 +83,42 @@ struct rk3568_pciephy_softc {
> };
> 
> 
> +static void
> +rk3568_pciephy_bifurcate(device_t dev, int control, uint32_t lane)
> +{
> +	struct rk3568_pciephy_softc *sc = device_get_softc(dev);
> +
> +	switch (lane) {
> +	case 0:
> +		SYSCON_WRITE_4(sc->phy_grf, control, GRF_PCIE30PHY_WR_EN);
> +		return;
> +	case 1:
> +		SYSCON_WRITE_4(sc->phy_grf, control,
> +		    GRF_PCIE30PHY_WR_EN | GRF_BIFURCATION_LANE_1);
> +		break;
> +	case 2:
> +		SYSCON_WRITE_4(sc->phy_grf, control,
> +		    GRF_PCIE30PHY_WR_EN | GRF_BIFURCATION_LANE_2);
> +		break;
> +	default:
> +		device_printf(dev, "Illegal lane %d\n", lane);
> +		return;

This doesn’t report failure?..

> +	}
> +	if (bootverbose)
> +		device_printf(dev, "lane %d @ pcie3x%d\n", lane,
> +		    (control == GRF_PCIE30PHY_CON5) ? 1 : 2);
> +}
> +
> /* PHY class and methods */
> static int
> rk3568_pciephy_enable(struct phynode *phynode, bool enable)
> {
> 	device_t dev = phynode_get_device(phynode);
> 	struct rk3568_pciephy_softc *sc = device_get_softc(dev);
> -	uint32_t data_lanes[2] = { 0, 0 };
> 	int count;
> 
> 	if (enable) {
> -		/* Deassert PCIe PMA output clamp mode */
> -		SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON9,
> -		    GRF_PCIE30PHY_DA_OCM);
> -
> -		/* Set bifurcation according to DT entry */
> -		if (OF_hasprop(sc->node, "data-lanes")) {
> -			OF_getencprop(sc->node, "data-lanes", data_lanes,
> -			    sizeof(data_lanes));
> -			if (data_lanes[0] > 0) {
> -				SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON5,
> -				    GRF_PCIE30PHY_WR_EN | (data_lanes[0] - 1));
> -				device_printf(dev, "pcie3x1 1 lane\n");
> -			}
> -			if (data_lanes[1] > 0) {
> -				SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON6,
> -				    GRF_PCIE30PHY_WR_EN | (data_lanes[1] - 1));
> -				device_printf(dev, "pcie3x2 1 lane\n");
> -			}
> -			if (data_lanes[0] > 1 || data_lanes[1] > 1)
> -				SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON1,
> -				    GRF_PCIE30PHY_DA_OCM);
> -
> -		} else {
> -			SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON5,
> -			    GRF_PCIE30PHY_WR_EN);
> -			SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON6,
> -			    GRF_PCIE30PHY_WR_EN);
> -			device_printf(dev, "pcie3 2 lanes\n");
> -		}
> -
> +		/* Pull PHY out of reset */
> 		hwreset_deassert(sc->phy_reset);
> 
> 		/* Poll for SRAM loaded and ready */
> @@ -165,6 +164,7 @@ rk3568_pciephy_attach(device_t dev)
> 	struct rk3568_pciephy_softc *sc = device_get_softc(dev);
> 	struct phynode_init_def phy_init;
> 	struct phynode *phynode;
> +	uint32_t data_lanes[2] = { 0, 0 };
> 	int rid = 0;
> 
> 	sc->dev = dev;
> @@ -212,6 +212,29 @@ rk3568_pciephy_attach(device_t dev)
> 
> 	/* Set RC/EP mode not implemented yet (RC mode only) */
> 
> +	/* Set bifurcation according to "data-lanes" entry */
> +	if (OF_hasprop(sc->node, "data-lanes")) {
> +		OF_getencprop(sc->node, "data-lanes", data_lanes,
> +		    sizeof(data_lanes));
> +	}
> +	else

This does not conform to style(9).

> +		if (bootverbose)
> +			device_printf(dev, "lane 1 & 2 @pcie3x2\n");

These seems like messy printfs.

Jess

> +
> +	/* Deassert PCIe PMA output clamp mode */
> +	SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON9, GRF_PCIE30PHY_DA_OCM);
> +
> +	/* Configure PHY HW accordingly */
> +	rk3568_pciephy_bifurcate(dev, GRF_PCIE30PHY_CON5, data_lanes[0]);
> +	rk3568_pciephy_bifurcate(dev, GRF_PCIE30PHY_CON6, data_lanes[1]);
> +
> +	if (data_lanes[0] || data_lanes[1])
> +		SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON1,
> +		    GRF_PCIE30PHY_DA_OCM);
> +	else
> +		SYSCON_WRITE_4(sc->phy_grf, GRF_PCIE30PHY_CON1,
> +		    GRF_PCIE30PHY_DA_OCM_MASK);
> +
> 	bzero(&phy_init, sizeof(phy_init));
> 	phy_init.id = PHY_NONE;
> 	phy_init.ofw_node = sc->node;