| Message ID | 20211212070210.141664-2-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hi folks, | expand |
On 2021-12-12 07:02, Hector Martin wrote: > This is required on some Apple ARM64 laptops using this controller. > As is typical on DT platforms, pull these quirks from the device tree > using the standard mmc bindings. > > See Documentation/devicetree/bindings/mmc/mmc-controller.yaml > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c > index 4fd99c1e82ba..ad742743a494 100644 > --- a/drivers/mmc/host/sdhci-pci-gli.c > +++ b/drivers/mmc/host/sdhci-pci-gli.c > @@ -12,6 +12,7 @@ > #include <linux/pci.h> > #include <linux/mmc/mmc.h> > #include <linux/delay.h> > +#include <linux/of.h> > #include "sdhci.h" > #include "sdhci-pci.h" > #include "cqhci.h" > @@ -114,8 +115,10 @@ > #define GLI_9755_WT_EN_OFF 0x0 > > #define PCI_GLI_9755_PECONF 0x44 > -#define PCI_GLI_9755_LFCLK GENMASK(14, 12) > -#define PCI_GLI_9755_DMACLK BIT(29) > +#define PCI_GLI_9755_LFCLK GENMASK(14, 12) > +#define PCI_GLI_9755_DMACLK BIT(29) > +#define PCI_GLI_9755_INVERT_CD BIT(30) > +#define PCI_GLI_9755_INVERT_WP BIT(31) > > #define PCI_GLI_9755_CFG2 0x48 > #define PCI_GLI_9755_CFG2_L1DLY GENMASK(28, 24) > @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot) > gl9755_wt_on(pdev); > > pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value); > +#ifdef CONFIG_OF > + if (pdev->dev.of_node) { FWIW, of_property_read_bool() should handle a NULL node correctly, and its stub be optimised away for !OF, so you don't really need the explicit check or the #ifdef. Robin. > + /* > + * Apple ARM64 platforms using these chips may have > + * inverted CD/WP detection. > + */ > + if (of_property_read_bool(pdev->dev.of_node, "cd-inverted")) > + value |= PCI_GLI_9755_INVERT_CD; > + if (of_property_read_bool(pdev->dev.of_node, "wp-inverted")) > + value |= PCI_GLI_9755_INVERT_WP; > + } > +#endif > value &= ~PCI_GLI_9755_LFCLK; > value &= ~PCI_GLI_9755_DMACLK; > pci_write_config_dword(pdev, PCI_GLI_9755_PECONF, value); >
On 12/12/2021 09:02, Hector Martin wrote: > This is required on some Apple ARM64 laptops using this controller. > As is typical on DT platforms, pull these quirks from the device tree > using the standard mmc bindings. > > See Documentation/devicetree/bindings/mmc/mmc-controller.yaml > > Signed-off-by: Hector Martin <marcan@marcan.st> A couple of kernel style issues, but fix those and you can add: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c > index 4fd99c1e82ba..ad742743a494 100644 > --- a/drivers/mmc/host/sdhci-pci-gli.c > +++ b/drivers/mmc/host/sdhci-pci-gli.c > @@ -12,6 +12,7 @@ > #include <linux/pci.h> > #include <linux/mmc/mmc.h> > #include <linux/delay.h> > +#include <linux/of.h> > #include "sdhci.h" > #include "sdhci-pci.h" > #include "cqhci.h" > @@ -114,8 +115,10 @@ > #define GLI_9755_WT_EN_OFF 0x0 > > #define PCI_GLI_9755_PECONF 0x44 > -#define PCI_GLI_9755_LFCLK GENMASK(14, 12) > -#define PCI_GLI_9755_DMACLK BIT(29) > +#define PCI_GLI_9755_LFCLK GENMASK(14, 12) > +#define PCI_GLI_9755_DMACLK BIT(29) Please don't mix in white space changes. > +#define PCI_GLI_9755_INVERT_CD BIT(30) > +#define PCI_GLI_9755_INVERT_WP BIT(31) > > #define PCI_GLI_9755_CFG2 0x48 > #define PCI_GLI_9755_CFG2_L1DLY GENMASK(28, 24) > @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot) > gl9755_wt_on(pdev); > > pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value); > +#ifdef CONFIG_OF > + if (pdev->dev.of_node) { As Robin wrote, please remove #ifdef and if (pdev->dev.of_node) because they are not needed. > + /* > + * Apple ARM64 platforms using these chips may have > + * inverted CD/WP detection. > + */ > + if (of_property_read_bool(pdev->dev.of_node, "cd-inverted")) > + value |= PCI_GLI_9755_INVERT_CD; > + if (of_property_read_bool(pdev->dev.of_node, "wp-inverted")) > + value |= PCI_GLI_9755_INVERT_WP; > + } > +#endif > value &= ~PCI_GLI_9755_LFCLK; > value &= ~PCI_GLI_9755_DMACLK; > pci_write_config_dword(pdev, PCI_GLI_9755_PECONF, value); >
On 14/12/2021 19.41, Adrian Hunter wrote: >> #define PCI_GLI_9755_PECONF 0x44 >> -#define PCI_GLI_9755_LFCLK GENMASK(14, 12) >> -#define PCI_GLI_9755_DMACLK BIT(29) >> +#define PCI_GLI_9755_LFCLK GENMASK(14, 12) >> +#define PCI_GLI_9755_DMACLK BIT(29) > > Please don't mix in white space changes. This is aligning the existing code with the additions; is it preferable to have the new ifdefs below misaligned? >> +#define PCI_GLI_9755_INVERT_CD BIT(30) >> +#define PCI_GLI_9755_INVERT_WP BIT(31) >> >> #define PCI_GLI_9755_CFG2 0x48 >> #define PCI_GLI_9755_CFG2_L1DLY GENMASK(28, 24) >> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot) >> gl9755_wt_on(pdev); >> >> pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value); >> +#ifdef CONFIG_OF >> + if (pdev->dev.of_node) { > > As Robin wrote, please remove #ifdef and if (pdev->dev.of_node) > because they are not needed. Ack, will send out a v3 soon with the requested changes and hopefully it should be good to go :)
On 14/12/2021 13:17, Hector Martin wrote: > On 14/12/2021 19.41, Adrian Hunter wrote: >>> #define PCI_GLI_9755_PECONF 0x44 >>> -#define PCI_GLI_9755_LFCLK GENMASK(14, 12) >>> -#define PCI_GLI_9755_DMACLK BIT(29) >>> +#define PCI_GLI_9755_LFCLK GENMASK(14, 12) >>> +#define PCI_GLI_9755_DMACLK BIT(29) >> >> Please don't mix in white space changes. > > This is aligning the existing code with the additions; is it preferable to have the new ifdefs below misaligned? White space changes should be a separate patch > >>> +#define PCI_GLI_9755_INVERT_CD BIT(30) >>> +#define PCI_GLI_9755_INVERT_WP BIT(31) >>> #define PCI_GLI_9755_CFG2 0x48 >>> #define PCI_GLI_9755_CFG2_L1DLY GENMASK(28, 24) >>> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot) >>> gl9755_wt_on(pdev); >>> pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value); >>> +#ifdef CONFIG_OF >>> + if (pdev->dev.of_node) { >> >> As Robin wrote, please remove #ifdef and if (pdev->dev.of_node) >> because they are not needed. > > Ack, will send out a v3 soon with the requested changes and hopefully it should be good to go :) >
On 13/12/2021 20.45, Robin Murphy wrote: > On 2021-12-12 07:02, Hector Martin wrote: >> This is required on some Apple ARM64 laptops using this controller. >> As is typical on DT platforms, pull these quirks from the device tree >> using the standard mmc bindings. >> >> See Documentation/devicetree/bindings/mmc/mmc-controller.yaml >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c >> index 4fd99c1e82ba..ad742743a494 100644 >> --- a/drivers/mmc/host/sdhci-pci-gli.c >> +++ b/drivers/mmc/host/sdhci-pci-gli.c >> @@ -12,6 +12,7 @@ >> #include <linux/pci.h> >> #include <linux/mmc/mmc.h> >> #include <linux/delay.h> >> +#include <linux/of.h> >> #include "sdhci.h" >> #include "sdhci-pci.h" >> #include "cqhci.h" >> @@ -114,8 +115,10 @@ >> #define GLI_9755_WT_EN_OFF 0x0 >> >> #define PCI_GLI_9755_PECONF 0x44 >> -#define PCI_GLI_9755_LFCLK GENMASK(14, 12) >> -#define PCI_GLI_9755_DMACLK BIT(29) >> +#define PCI_GLI_9755_LFCLK GENMASK(14, 12) >> +#define PCI_GLI_9755_DMACLK BIT(29) >> +#define PCI_GLI_9755_INVERT_CD BIT(30) >> +#define PCI_GLI_9755_INVERT_WP BIT(31) >> >> #define PCI_GLI_9755_CFG2 0x48 >> #define PCI_GLI_9755_CFG2_L1DLY GENMASK(28, 24) >> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot) >> gl9755_wt_on(pdev); >> >> pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value); >> +#ifdef CONFIG_OF >> + if (pdev->dev.of_node) { > > FWIW, of_property_read_bool() should handle a NULL node correctly, and > its stub be optimised away for !OF, so you don't really need the > explicit check or the #ifdef. Thanks, removed it for v3!
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c index 4fd99c1e82ba..ad742743a494 100644 --- a/drivers/mmc/host/sdhci-pci-gli.c +++ b/drivers/mmc/host/sdhci-pci-gli.c @@ -12,6 +12,7 @@ #include <linux/pci.h> #include <linux/mmc/mmc.h> #include <linux/delay.h> +#include <linux/of.h> #include "sdhci.h" #include "sdhci-pci.h" #include "cqhci.h" @@ -114,8 +115,10 @@ #define GLI_9755_WT_EN_OFF 0x0 #define PCI_GLI_9755_PECONF 0x44 -#define PCI_GLI_9755_LFCLK GENMASK(14, 12) -#define PCI_GLI_9755_DMACLK BIT(29) +#define PCI_GLI_9755_LFCLK GENMASK(14, 12) +#define PCI_GLI_9755_DMACLK BIT(29) +#define PCI_GLI_9755_INVERT_CD BIT(30) +#define PCI_GLI_9755_INVERT_WP BIT(31) #define PCI_GLI_9755_CFG2 0x48 #define PCI_GLI_9755_CFG2_L1DLY GENMASK(28, 24) @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot) gl9755_wt_on(pdev); pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value); +#ifdef CONFIG_OF + if (pdev->dev.of_node) { + /* + * Apple ARM64 platforms using these chips may have + * inverted CD/WP detection. + */ + if (of_property_read_bool(pdev->dev.of_node, "cd-inverted")) + value |= PCI_GLI_9755_INVERT_CD; + if (of_property_read_bool(pdev->dev.of_node, "wp-inverted")) + value |= PCI_GLI_9755_INVERT_WP; + } +#endif value &= ~PCI_GLI_9755_LFCLK; value &= ~PCI_GLI_9755_DMACLK; pci_write_config_dword(pdev, PCI_GLI_9755_PECONF, value);
This is required on some Apple ARM64 laptops using this controller. As is typical on DT platforms, pull these quirks from the device tree using the standard mmc bindings. See Documentation/devicetree/bindings/mmc/mmc-controller.yaml Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)