Messages in this thread | | | Date | Sun, 15 Aug 2021 17:33:48 -0400 | From | Alyssa Rosenzweig <> | Subject | Re: [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1 |
| |
Hi Rob,
Thanks for the review.
> > +#define CORE_RC_PHYIF_CTL 0x00024 > > +#define CORE_RC_PHYIF_CTL_RUN BIT(0) > > +#define CORE_RC_PHYIF_STAT 0x00028 > > +#define CORE_RC_PHYIF_STAT_REFCLK BIT(4) > > +#define CORE_RC_CTL 0x00050 > > +#define CORE_RC_CTL_RUN BIT(0) > > +#define CORE_RC_STAT 0x00058 > > +#define CORE_RC_STAT_READY BIT(0) > > +#define CORE_FABRIC_STAT 0x04000 > > +#define CORE_FABRIC_STAT_MASK 0x001F001F > > +#define CORE_PHY_CTL 0x80000 > > +#define CORE_PHY_CTL_CLK0REQ BIT(0) > > +#define CORE_PHY_CTL_CLK1REQ BIT(1) > > +#define CORE_PHY_CTL_CLK0ACK BIT(2) > > +#define CORE_PHY_CTL_CLK1ACK BIT(3) > > +#define CORE_PHY_CTL_RESET BIT(7) > > I was going to say these should be a phy driver perhaps, but they are > unused. So for now, just drop them.
Removed in v2.
CORE_PHY_CTRL is used in the asahi linux bootloader (m1n1, shared between linux+uboot+bsd) to do early pcie bringup. They are indeed not used here, nor are they used in the uboot/bsd drivers.
> > +static int apple_pcie_setup_port(struct apple_pcie *pcie, unsigned int i) > > +{ > > + struct fwnode_handle *fwnode = dev_fwnode(pcie->dev); > > Doesn't look like you ever use the fwnode, just get the DT node > pointer. Unless this driver is going to use ACPI someday (and ACPI > changes how PCI is done), there's no point in using fwnode.
Dropped in v2.
That was a copypaste fail splitting off apple_pcie_setup_port from apple_msi_init in an early revision.
> It's preferred to use platform resource api and ioremap over DT functions. > ... > Use devm_platform_ioremap_resource instead.
Done in v2.
Thanks,
Alyssa
| |