allwinner question reformulalted: sc->phy_ctrl vs. sc->pmu[phyno], which should be used with PMU_UNK_H3 and PMU_UNK_H3_CLR? (sc->phy_ctrl actually used currently)

[Emmanuel Vadot]

On Tue, 19 Sep 2017 10:51:46 -0700
Mark Millard <markmi at dsl-only.net> wrote:

> The modern, updated sys/arm/allwinner/aw_usbphy.c code uses
> PMU_UNK_H3 and PMU_UNK_H3_CLR with sc->phy_ctrl :
> 
> 	if (sc->phy_conf->pmu_unk1 == true)
> 		CLR4(sc->phy_ctrl, PMU_UNK_H3, PMU_UNK_H3_CLR);

 This is a mistake, it should write using sc->pmu[phyno] as base, I'll
correct that soon.

> but uses sc->pmu[phyno] for PMU_IRQ_ENABLE and:
> 
> PMU_ULPI_BYPASS | PMU_AHB_INCR8 | PMU_AHB_INCR4 | PMU_AHB_INCRX_ALIGN
> 
> in:
>         SET4(sc->pmu[phyno], PMU_IRQ_ENABLE, PMU_ULPI_BYPASS |
>             PMU_AHB_INCR8 | PMU_AHB_INCR4 | PMU_AHB_INCRX_ALIGN);
> 
> Having PMU_<?> not used with sc->pmu[phyno] looks a little odd.
> 
> 
> For reference: the older code used:
> 
> 	if (sc->phy_type == AWUSBPHY_TYPE_A64)  {
> 		CLR4(sc, phyno, PMU_UNK_H3, PMU_UNK_H3_CLR);
> 
> and:
> 
> 	if (phyno > 0) {
> 		/* Enable passby */
> 		SET4(sc, phyno, PMU_IRQ_ENABLE, PMU_ULPI_BYPASS |
> 		    PMU_AHB_INCR8 | PMU_AHB_INCR4 | PMU_AHB_INCRX_ALIGN);
> 	}
> 
> 
> where: that involved use of:
> 
> (sc)->res[(phyno)]
> 
> via:
> 
> #define	RD4(sc, i, o)		bus_read_4((sc)->res[(i)], (o))
> #define	WR4(sc, i, o, v)	bus_write_4((sc)->res[(i)], (o), (v))
> #define	CLR4(sc, i, o, m)	WR4(sc, i, o, RD4(sc, i, o) & ~(m))
> #define	SET4(sc, i, o, m)	WR4(sc, i, o, RD4(sc, i, o) | (m))
> 
> 
> ===
> Mark Millard
> markmi at dsl-only.net
> 
> On 2017-Sep-18, at 1:42 PM, Mark Millard <markmi at dsl-only.net> wrote:
> 
> 
> On 2017-Sep-18, at 1:30 PM, Mark Millard <markmi at dsl-only.net> wrote:
> 
> > It probably just my ignorance of the code's intent
> > but for A64 it used to be that phyno ==1 had code
> > that did CLR4 for phyno==0 (hard coded):
> > 
> > 	if (sc->phy_type == AWUSBPHY_TYPE_A64)  {
> > 		CLR4(sc, phyno, PMU_UNK_H3, PMU_UNK_H3_CLR);
> > 
> > 		/* EHCI0 and OTG share a PHY */
> > 		if (phyno == 0)
> > 			SET4(sc, 0, OTG_PHY_CFG, OTG_PHY_ROUTE_OTG);
> > 		else if (phyno == 1)
> > 			CLR4(sc, 0, OTG_PHY_CFG, OTG_PHY_ROUTE_OTG);
> > 	}
> > 
> > So: that last CLR4 manipulated phyno==0 as far as I can tell,
> > no matter what the passed-in phyno was.
> > 
> > In the new code there seems to be no hook for phyno==1
> > to manipulate phyno==0 similarly:
> > 
> > 	if (sc->phy_conf->phy0_route == true) {
> > 		if (phyno == 0)
> > 			SET4(sc->phy_ctrl, OTG_PHY_CFG, OTG_PHY_ROUTE_OTG);
> > 		else
> > 			CLR4(sc->phy_ctrl, OTG_PHY_CFG, OTG_PHY_ROUTE_OTG);
> > 	}
> > 
> > That CLR4 seems to be manipulating phyno==1 instead and
> > seems to have no means of doing otherwise.
> > 
> > Was the old code wrong?
> 
> May be I asked the reverse of the right question:
> that first CLR 4 in the old code varied by phyno but
> now always uses phy_ctrl:
> 
> 	if (sc->phy_conf->pmu_unk1 == true)
> 		CLR4(sc->phy_ctrl, PMU_UNK_H3, PMU_UNK_H3_CLR);
> 
> 
> Overall one part or the other seems to be a mismatch with the
> old code for A64.
> 
> ===
> Mark Millard
> markmi at dsl-only.net
> 
> _______________________________________________
> freebsd-arm at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arm
> To unsubscribe, send any mail to "freebsd-arm-unsubscribe at freebsd.org"
> 
> _______________________________________________
> freebsd-arm at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arm
> To unsubscribe, send any mail to "freebsd-arm-unsubscribe at freebsd.org"


-- 
Emmanuel Vadot <manu at bidouilliste.com> <manu at freebsd.org>