| Message ID | 1397751967-20250-3-git-send-email-chiau.ee.chew@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 18, 2014 at 12:26:07AM +0800, Chew Chiau Ee wrote: > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > Intel BayTrail PCI mode LPSS devices inclusive of SPI do not rely > on common clock framework. Thus, this patch allows the PCI mode > SPI host to pass the supported clock rate info to the core layer > which eventually used for speed calculation. As you mention we already have a clock API for passing around information about clocks, it would be much better to fix the fact that this isn't enabled on x86 than to add custom code to deal with clocks (even simple things like this) - this is far from the only place where it causes issues.
On Fri, Apr 18, 2014 at 05:30:54PM +0100, Mark Brown wrote: > On Fri, Apr 18, 2014 at 12:26:07AM +0800, Chew Chiau Ee wrote: > > From: Chew, Chiau Ee <chiau.ee.chew@intel.com> > > > > Intel BayTrail PCI mode LPSS devices inclusive of SPI do not rely > > on common clock framework. Thus, this patch allows the PCI mode > > SPI host to pass the supported clock rate info to the core layer > > which eventually used for speed calculation. > > As you mention we already have a clock API for passing around > information about clocks, it would be much better to fix the fact that > this isn't enabled on x86 than to add custom code to deal with clocks > (even simple things like this) - this is far from the only place where > it causes issues. We actually have clock API enabled on x86 if CONFIG_X86_INTEL_LPSS is selected. In this case the question is more like, where do we create clocks for PCI enumerated devices? Surely we don't want to have board files to do this. In ACPI mode, we do it in drivers/acpi/acpi_lpss.c but not sure where we could do that in PCI code. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 22, 2014 at 02:57:17PM +0300, Mika Westerberg wrote: > On Fri, Apr 18, 2014 at 05:30:54PM +0100, Mark Brown wrote: > > As you mention we already have a clock API for passing around > > information about clocks, it would be much better to fix the fact that > > this isn't enabled on x86 than to add custom code to deal with clocks > > (even simple things like this) - this is far from the only place where > > it causes issues. > We actually have clock API enabled on x86 if CONFIG_X86_INTEL_LPSS is > selected. Why is this conditional? It's really unhelpful for writing generic code handling clocks that the API isn't available as standard. Allowing the user to disable it if it's not required makes sense but not making it available at all is just unhelpful. > In this case the question is more like, where do we create clocks for PCI > enumerated devices? Surely we don't want to have board files to do this. > In ACPI mode, we do it in drivers/acpi/acpi_lpss.c but not sure where we > could do that in PCI code. As far as I can tell the clock here is part of the same glue that allows the generic pxa2xx support to be instantiated from PCI. I would therefore expect the glue to also instantiate the clock. Or if it's a standard PCI clock then perhaps the PCI subsystem ought to be providing it.
On Tue, Apr 22, 2014 at 01:09:48PM +0100, Mark Brown wrote: > On Tue, Apr 22, 2014 at 02:57:17PM +0300, Mika Westerberg wrote: > > On Fri, Apr 18, 2014 at 05:30:54PM +0100, Mark Brown wrote: > > > > As you mention we already have a clock API for passing around > > > information about clocks, it would be much better to fix the fact that > > > this isn't enabled on x86 than to add custom code to deal with clocks > > > (even simple things like this) - this is far from the only place where > > > it causes issues. > > > We actually have clock API enabled on x86 if CONFIG_X86_INTEL_LPSS is > > selected. > > Why is this conditional? It's really unhelpful for writing generic code > handling clocks that the API isn't available as standard. Allowing the > user to disable it if it's not required makes sense but not making it > available at all is just unhelpful. IIRC, it was decided at the time that it was the "safest" option. Anyway, distros seem to have it enabled already. Just checked my Fedora installation and it's there. > > In this case the question is more like, where do we create clocks for PCI > > enumerated devices? Surely we don't want to have board files to do this. > > > In ACPI mode, we do it in drivers/acpi/acpi_lpss.c but not sure where we > > could do that in PCI code. > > As far as I can tell the clock here is part of the same glue that allows > the generic pxa2xx support to be instantiated from PCI. I would > therefore expect the glue to also instantiate the clock. Or if it's a > standard PCI clock then perhaps the PCI subsystem ought to be providing > it. Instantiating the clock in the PCI glue driver makes sense (as it's not a standard PCI clock). Thanks. Chiau Ee, do you see any problems with that solution? -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Why is this conditional? It's really unhelpful for writing generic code > handling clocks that the API isn't available as standard. Allowing the > user to disable it if it's not required makes sense but not making it > available at all is just unhelpful. KConfig allows drivers to select features they need - this seems to be a non issue ? > As far as I can tell the clock here is part of the same glue that allows > the generic pxa2xx support to be instantiated from PCI. I would > therefore expect the glue to also instantiate the clock. Or if it's a > standard PCI clock then perhaps the PCI subsystem ought to be providing > it. For most cases it probably makes sense to do something of that form, although as clk_enable/clk_disable allow NULL it's much less of an issue than it might otherwise be for some of the other drivers. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 22, 2014 at 03:27:23PM +0100, One Thousand Gnomes wrote: > > Why is this conditional? It's really unhelpful for writing generic code > > handling clocks that the API isn't available as standard. Allowing the > > user to disable it if it's not required makes sense but not making it > > available at all is just unhelpful. > KConfig allows drivers to select features they need - this seems to be a > non issue ? No, not for the clock API - the platform has to provide a clock API since currently platforms still have the option of providing their own implementation. We should fix that (just making the common clock API available on all platforms which don't have a custom one would help a lot) but getting anyone to take any interest has been hard.
diff --git a/Documentation/spi/pxa2xx b/Documentation/spi/pxa2xx index 3352f97..3cde429 100644 --- a/Documentation/spi/pxa2xx +++ b/Documentation/spi/pxa2xx @@ -31,6 +31,9 @@ The "pxa2xx_spi_master.clock_enable" field is used to enable/disable the corresponding SSP peripheral block in the "Clock Enable Register (CKEN"). See the "PXA2xx Developer Manual" section "Clocks and Power Management". +The "pxa2xx_spi_master.max_clk_rate" field is used to determine the maximum +clock rate supported by the SPI master. + The "pxa2xx_spi_master.num_chipselect" field is used to determine the number of slave device (chips) attached to this SPI master. diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c index c1865c9..24cd938 100644 --- a/drivers/spi/spi-pxa2xx-pci.c +++ b/drivers/spi/spi-pxa2xx-pci.c @@ -17,6 +17,7 @@ struct pxa_spi_info { enum pxa_ssp_type type; int port_id; int num_chipselect; + unsigned long max_clk_rate; int tx_slave_id; int tx_chan_id; int rx_slave_id; @@ -37,6 +38,7 @@ static struct pxa_spi_info spi_info_configs[] = { .type = LPSS_SSP, .port_id = 0, .num_chipselect = 1, + .max_clk_rate = 50000000, .tx_slave_id = 0, .tx_chan_id = 0, .rx_slave_id = 1, @@ -72,6 +74,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev, spi_pdata.rx_slave_id = c->rx_slave_id; spi_pdata.rx_chan_id = c->rx_chan_id; spi_pdata.enable_dma = c->rx_slave_id >= 0 && c->tx_slave_id >= 0; + spi_pdata.max_clk_rate = c->max_clk_rate; ssp = &spi_pdata.ssp; ssp->phys_base = pci_resource_start(dev, 0); diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index d0fc8e0..b670371 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1182,6 +1182,8 @@ static int pxa2xx_spi_probe(struct platform_device *pdev) clk_prepare_enable(ssp->clk); drv_data->max_clk_rate = clk_get_rate(ssp->clk); + if (!drv_data->max_clk_rate) + drv_data->max_clk_rate = platform_info->max_clk_rate; /* Load default SSP configuration */ write_SSCR0(0, drv_data->ioaddr); diff --git a/include/linux/spi/pxa2xx_spi.h b/include/linux/spi/pxa2xx_spi.h index 82d5111..967903f 100644 --- a/include/linux/spi/pxa2xx_spi.h +++ b/include/linux/spi/pxa2xx_spi.h @@ -28,6 +28,7 @@ struct pxa2xx_spi_master { u32 clock_enable; u16 num_chipselect; u8 enable_dma; + unsigned long max_clk_rate; /* DMA engine specific config */ int rx_chan_id;