| Message ID | 20241024170540.2721307-7-masterr3c0rd@epochal.quest (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sunxi: A100/A133 second stage support | expand |
On Thu, 24 Oct 2024 14:05:24 -0300 Cody Eksal <masterr3c0rd@epochal.quest> wrote: Hi, > From: Yangtao Li <frank@allwinnertech.com> > > Add support for a100's usb phy, which with 2 PHYs. > > Signed-off-by: Yangtao Li <frank@allwinnertech.com> > [masterr3c0rd@epochal.quest: modified to use quirk flags] > Signed-off-by: Cody Eksal <masterr3c0rd@epochal.quest> > --- > drivers/phy/allwinner/phy-sun4i-usb.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c > index b0f19e950601..a3942b2ee90b 100644 > --- a/drivers/phy/allwinner/phy-sun4i-usb.c > +++ b/drivers/phy/allwinner/phy-sun4i-usb.c > @@ -1006,6 +1006,16 @@ static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = { > .phy0_dual_route = true, > }; > > +static const struct sun4i_usb_phy_cfg sun50i_a100_cfg = { > + .num_phys = 2, > + .disc_thresh = 3, This member is never used when .siddq_in_base is true (and yes, this is wrong for the H616 too), ... > + .phyctl_offset = REG_PHYCTL_A33, > + .dedicated_clocks = true, > + .hci_phy_ctl_clear = PHY_CTL_SIDDQ, > + .phy0_dual_route = true, > + .siddq_in_base = true, ... which makes this whole description identical to the D1 version. So at the very least we wouldn't this new a100_cfg, but instead just point to the existing d1_cfg. > +}; > + > static const struct sun4i_usb_phy_cfg sun50i_h6_cfg = { > .num_phys = 4, > .phyctl_offset = REG_PHYCTL_A33, > @@ -1040,6 +1050,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] = { > { .compatible = "allwinner,sun20i-d1-usb-phy", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a64-usb-phy", > .data = &sun50i_a64_cfg}, > + { .compatible = "allwinner,sun50i-a100-usb-phy", .data = &sun50i_a100_cfg }, And this also brings up the question whether we need a new compatible string. As it stands now, we could also use: compatible = "allwinner,sun50i-a100-usb-phy", "allwinner,sun20i-d1-usb-phy"; and wouldn't need any driver changes at all. Which would have the neat side effect to make USB work already with v5.18 kernels. The only downside is the somewhat weird ordering of the compatible strings, with the much newer chip as the fallback. What do other people think here? Cheers, Andre > { .compatible = "allwinner,sun50i-h6-usb-phy", .data = &sun50i_h6_cfg }, > { .compatible = "allwinner,sun50i-h616-usb-phy", .data = &sun50i_h616_cfg }, > { .compatible = "allwinner,suniv-f1c100s-usb-phy",
On 2024/10/24 4:22 pm, Andre Przywara wrote: > On Thu, 24 Oct 2024 14:05:24 -0300 > Cody Eksal <masterr3c0rd@epochal.quest> wrote: >> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c >> b/drivers/phy/allwinner/phy-sun4i-usb.c >> index b0f19e950601..a3942b2ee90b 100644 >> --- a/drivers/phy/allwinner/phy-sun4i-usb.c >> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c >> @@ -1006,6 +1006,16 @@ static const struct sun4i_usb_phy_cfg >> sun50i_a64_cfg = { >> .phy0_dual_route = true, >> }; >> >> +static const struct sun4i_usb_phy_cfg sun50i_a100_cfg = { >> + .num_phys = 2, >> + .disc_thresh = 3, > > This member is never used when .siddq_in_base is true (and yes, this is > wrong for the H616 too), ... > >> + .phyctl_offset = REG_PHYCTL_A33, >> + .dedicated_clocks = true, >> + .hci_phy_ctl_clear = PHY_CTL_SIDDQ, >> + .phy0_dual_route = true, >> + .siddq_in_base = true, > > ... which makes this whole description identical to the D1 version. > So at the very least we wouldn't this new a100_cfg, but instead just > point to the existing d1_cfg. I did test on my board and confirmed simply using a D1 compatible works. > And this also brings up the question whether we need a new compatible > string. As it stands now, we could also use: > compatible = "allwinner,sun50i-a100-usb-phy", > "allwinner,sun20i-d1-usb-phy"; > > and wouldn't need any driver changes at all. Which would have the neat > side effect to make USB work already with v5.18 kernels. > > The only downside is the somewhat weird ordering of the compatible > strings, with the much newer chip as the fallback. I plan to drop this patch in V2, in favor of just adding the compatible/fallback. Although it's odd ordering, I would think fixing the DTS of other device trees to remedy this would probably not be worth the hassle. Thanks for pointing this out! - Cody > What do other people think here? > > Cheers, > Andre
diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c index b0f19e950601..a3942b2ee90b 100644 --- a/drivers/phy/allwinner/phy-sun4i-usb.c +++ b/drivers/phy/allwinner/phy-sun4i-usb.c @@ -1006,6 +1006,16 @@ static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = { .phy0_dual_route = true, }; +static const struct sun4i_usb_phy_cfg sun50i_a100_cfg = { + .num_phys = 2, + .disc_thresh = 3, + .phyctl_offset = REG_PHYCTL_A33, + .dedicated_clocks = true, + .hci_phy_ctl_clear = PHY_CTL_SIDDQ, + .phy0_dual_route = true, + .siddq_in_base = true, +}; + static const struct sun4i_usb_phy_cfg sun50i_h6_cfg = { .num_phys = 4, .phyctl_offset = REG_PHYCTL_A33, @@ -1040,6 +1050,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] = { { .compatible = "allwinner,sun20i-d1-usb-phy", .data = &sun20i_d1_cfg }, { .compatible = "allwinner,sun50i-a64-usb-phy", .data = &sun50i_a64_cfg}, + { .compatible = "allwinner,sun50i-a100-usb-phy", .data = &sun50i_a100_cfg }, { .compatible = "allwinner,sun50i-h6-usb-phy", .data = &sun50i_h6_cfg }, { .compatible = "allwinner,sun50i-h616-usb-phy", .data = &sun50i_h616_cfg }, { .compatible = "allwinner,suniv-f1c100s-usb-phy",