Re: [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1

[Alyssa Rosenzweig]

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