| Message ID | 20170222131940.31085-1-jonas.gorski@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ff18e1ef04e2073889569b07a5ddd54a6527917f |
| Headers | show |
On Wed, Feb 22, 2017 at 02:19:38PM +0100, Jonas Gorski wrote: > Instead of requiring the hsspi clock to have a rate, allow using a second > clock for providing the Hz rate, which is probably more correct anyway. I'm sorry but I just can't follow the logic here at all - why would we use "a second clock" to get the rate of a clock and why would that be more correct? There's a few steps in the reasoning missing here I think!
On 22 February 2017 at 19:26, Mark Brown <broonie@kernel.org> wrote: > On Wed, Feb 22, 2017 at 02:19:38PM +0100, Jonas Gorski wrote: >> Instead of requiring the hsspi clock to have a rate, allow using a second >> clock for providing the Hz rate, which is probably more correct anyway. > > I'm sorry but I just can't follow the logic here at all - why would we > use "a second clock" to get the rate of a clock and why would that be > more correct? There's a few steps in the reasoning missing here I > think! The PLL rate is hardcoded in the original broadcom code (with #if-deffery for different SoCs); when I added support for the HSSPI controller in upstream linux, I thought "hey, let's put it as the rate of the (gateable) HSSPI clock, then I don't need to pass it as platform data". Part of the issue is also that bcm63xx has its own clk code, so couldn't use clk lookups even if I wanted. The register for the gated clock control is calld blkEnables (so might be just power gating?), and the #define for the rate is called HS_SPI_PLL_FREQ, which at least gives me the impression these could be two different things/clocks. Whether it's a separate PLL from the gated clock or not someone with actual hardware knowledge would have to tell. To (eventually) get rid of bcm63xx's own clk code and replace it with a proper clock driver with lookups and stuff, it would make it much easier to just move the PLL_FREQ into a separate fixed-rate clock, as it is the only clock in the bcm63xx code that has a rate attached and is gated. So I wouldn't need to create composite clocks to and could just directly make it a simple gated-clock driver. Hope that explanation helps. Regards Jonas -- 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 02/22/2017 12:00 PM, Jonas Gorski wrote: > On 22 February 2017 at 19:26, Mark Brown <broonie@kernel.org> wrote: >> On Wed, Feb 22, 2017 at 02:19:38PM +0100, Jonas Gorski wrote: >>> Instead of requiring the hsspi clock to have a rate, allow using a second >>> clock for providing the Hz rate, which is probably more correct anyway. >> >> I'm sorry but I just can't follow the logic here at all - why would we >> use "a second clock" to get the rate of a clock and why would that be >> more correct? There's a few steps in the reasoning missing here I >> think! > > The PLL rate is hardcoded in the original broadcom code (with > #if-deffery for different SoCs); when I added support for the HSSPI > controller in upstream linux, I thought "hey, let's put it as the rate > of the (gateable) HSSPI clock, then I don't need to pass it as > platform data". Part of the issue is also that bcm63xx has its own clk > code, so couldn't use clk lookups even if I wanted. > > The register for the gated clock control is calld blkEnables (so might > be just power gating?), and the #define for the rate is called > HS_SPI_PLL_FREQ, which at least gives me the impression these could be > two different things/clocks. Whether it's a separate PLL from the > gated clock or not someone with actual hardware knowledge would have > to tell. I looked at the HSSPI datasheet again and there are indeed two separate input clock signals to the HSSPI block, one which is a PLL clock and that is used to control the external SPI output/input clock. The other clock is the UBUS register interface clock and also clock gates the block when unused. So with that: Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > To (eventually) get rid of bcm63xx's own clk code and replace it with > a proper clock driver with lookups and stuff, it would make it much > easier to just move the PLL_FREQ into a separate fixed-rate clock, as > it is the only clock in the bcm63xx code that has a rate attached and > is gated. So I wouldn't need to create composite clocks to and could > just directly make it a simple gated-clock driver. > > Hope that explanation helps. > > > Regards > Jonas >
diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c index 55789f7cda92..79096d17ebde 100644 --- a/drivers/spi/spi-bcm63xx-hsspi.c +++ b/drivers/spi/spi-bcm63xx-hsspi.c @@ -351,8 +351,16 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev) return PTR_ERR(clk); rate = clk_get_rate(clk); - if (!rate) - return -EINVAL; + if (!rate) { + struct clk *pll_clk = devm_clk_get(dev, "pll"); + + if (IS_ERR(pll_clk)) + return PTR_ERR(pll_clk); + + rate = clk_get_rate(pll_clk); + if (!rate) + return -EINVAL; + } ret = clk_prepare_enable(clk); if (ret)
Instead of requiring the hsspi clock to have a rate, allow using a second clock for providing the Hz rate, which is probably more correct anyway. Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> --- drivers/spi/spi-bcm63xx-hsspi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)