| Message ID | 1364570381-17605-4-git-send-email-tpiepho@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Dear Trent Piepho, > The ssp struct has a clock rate field, to provide the actual value, in Hz, > of the SSP output clock (the rate of SSP_SCK) after mxs_ssp_set_clk_rate() > is called. It should be read-only, except for mxs_ssp_set_clk_rate(). > > For some reason the spi-mxs driver decides to write to this field on init, > and sets it to the value of the SSP input clock (clk_sspN, from the MXS > clocking block) in kHz. It shouldn't be setting the value, and certainly > shouldn't be setting it with the wrong clock in the wrong units. I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then? > Signed-off-by: Trent Piepho <tpiepho@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Fabio Estevam <fabio.estevam@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/spi/spi-mxs.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c > index 7218006..fc52f78 100644 > --- a/drivers/spi/spi-mxs.c > +++ b/drivers/spi/spi-mxs.c > @@ -587,7 +587,6 @@ static int mxs_spi_probe(struct platform_device *pdev) > > clk_prepare_enable(ssp->clk); > clk_set_rate(ssp->clk, clk_freq); > - ssp->clk_rate = clk_get_rate(ssp->clk) / 1000; > > stmp_reset_block(ssp->base); Best regards, Marek Vasut ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote: >> The ssp struct has a clock rate field, to provide the actual value, in Hz, >> of the SSP output clock (the rate of SSP_SCK) after mxs_ssp_set_clk_rate() >> is called. It should be read-only, except for mxs_ssp_set_clk_rate(). >> >> For some reason the spi-mxs driver decides to write to this field on init, >> and sets it to the value of the SSP input clock (clk_sspN, from the MXS >> clocking block) in kHz. It shouldn't be setting the value, and certainly >> shouldn't be setting it with the wrong clock in the wrong units. > > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then? Why do you say that? I see no problem with clk-ssp.c, as setting the clk_rate field in the ssp struct to the actual programmed rate makes sense. The code in spi-mxs.c just makes no sense. I suspect it was added by mistake when porting the driver. ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
Dear Trent Piepho, > On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote: > >> The ssp struct has a clock rate field, to provide the actual value, in > >> Hz, of the SSP output clock (the rate of SSP_SCK) after > >> mxs_ssp_set_clk_rate() is called. It should be read-only, except for > >> mxs_ssp_set_clk_rate(). > >> > >> For some reason the spi-mxs driver decides to write to this field on > >> init, and sets it to the value of the SSP input clock (clk_sspN, from > >> the MXS clocking block) in kHz. It shouldn't be setting the value, and > >> certainly shouldn't be setting it with the wrong clock in the wrong > >> units. > > > > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then? > > Why do you say that? I see no problem with clk-ssp.c, as setting the > clk_rate field in the ssp struct to the actual programmed rate makes > sense. The code in spi-mxs.c just makes no sense. I suspect it was > added by mistake when porting the driver. Either remove it altogether if it's unused OR make sure it's inited to some sane value from the start. Best regards, Marek Vasut ------------------------------------------------------------------------------ Own the Future-Intel® Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://p.sf.net/sfu/intel_levelupd2d
On Mon, Apr 1, 2013 at 4:37 PM, Marek Vasut <marex@denx.de> wrote: >> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote: >> >> The ssp struct has a clock rate field, to provide the actual value, in >> >> Hz, of the SSP output clock (the rate of SSP_SCK) after >> >> mxs_ssp_set_clk_rate() is called. It should be read-only, except for >> >> mxs_ssp_set_clk_rate(). >> >> >> >> For some reason the spi-mxs driver decides to write to this field on >> >> init, and sets it to the value of the SSP input clock (clk_sspN, from >> >> the MXS clocking block) in kHz. It shouldn't be setting the value, and >> >> certainly shouldn't be setting it with the wrong clock in the wrong >> >> units. >> > >> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then? >> >> Why do you say that? I see no problem with clk-ssp.c, as setting the >> clk_rate field in the ssp struct to the actual programmed rate makes >> sense. The code in spi-mxs.c just makes no sense. I suspect it was >> added by mistake when porting the driver. > > Either remove it altogether if it's unused OR make sure it's inited to some sane > value from the start. This field doesn't need to be initted by a user of the common SSP clock code, as it is not an input to that code. It is set by the SSP clock code. After the SSP clock is programmed, it can be read to find the true SSP rate. There is no need for any user of the SSP code to set the clk_rate field, and other than this one incorrect line, no user does so. It's not used currently in the SPI driver at all, but it certainly could be. The SSP code is shared with the MMC driver that can drive an SSP port in SD/MMC mode. The mxs-mmc code does need the actual SSP clock and does use the field. That code does not write to ssp->clk_rate, because as I have said, it is in output from the SSP clock code. Since the ssp struct is part of the spi master device data, it would be initialized to zero when allocated by spi_master_alloc(). The SPI clock isn't part of the spi master, but of a spi slave and a spi transfer. Thus there is no sensible value to initialize the spi clock to at the time the spi master is created. Which is fine. The code doesn't try to and other spi drivers don't either. At the time a spi slave is created the spi clock could be programmed. But that would introduce a race. The proper thing to do, and what the driver does, is to program the spi clock with the requested clock for a spi transfer. ------------------------------------------------------------------------------ Own the Future-Intel(R) Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2
Dear Trent Piepho, > On Mon, Apr 1, 2013 at 4:37 PM, Marek Vasut <marex@denx.de> wrote: > >> On Mon, Apr 1, 2013 at 4:16 PM, Marek Vasut <marex@denx.de> wrote: > >> >> The ssp struct has a clock rate field, to provide the actual value, > >> >> in Hz, of the SSP output clock (the rate of SSP_SCK) after > >> >> mxs_ssp_set_clk_rate() is called. It should be read-only, except for > >> >> mxs_ssp_set_clk_rate(). > >> >> > >> >> For some reason the spi-mxs driver decides to write to this field on > >> >> init, and sets it to the value of the SSP input clock (clk_sspN, from > >> >> the MXS clocking block) in kHz. It shouldn't be setting the value, > >> >> and certainly shouldn't be setting it with the wrong clock in the > >> >> wrong units. > >> > > >> > I suspect this patch should also fix drivers/clk/mxs/clk-ssp.c then? > >> > >> Why do you say that? I see no problem with clk-ssp.c, as setting the > >> clk_rate field in the ssp struct to the actual programmed rate makes > >> sense. The code in spi-mxs.c just makes no sense. I suspect it was > >> added by mistake when porting the driver. > > > > Either remove it altogether if it's unused OR make sure it's inited to > > some sane value from the start. > > This field doesn't need to be initted by a user of the common SSP > clock code, as it is not an input to that code. It is set by the SSP > clock code. After the SSP clock is programmed, it can be read to find > the true SSP rate. There is no need for any user of the SSP code to > set the clk_rate field, and other than this one incorrect line, no > user does so. It's not used currently in the SPI driver at all, but > it certainly could be. The SSP code is shared with the MMC driver > that can drive an SSP port in SD/MMC mode. The mxs-mmc code does need > the actual SSP clock and does use the field. That code does not write > to ssp->clk_rate, because as I have said, it is in output from the SSP > clock code. If the clk_rate is at least inited to zero, then this patch does makes sense. [...] Best regards, Marek Vasut ------------------------------------------------------------------------------ Own the Future-Intel(R) Level Up Game Demo Contest 2013 Rise to greatness in Intel's independent game demo contest. Compete for recognition, cash, and the chance to get your game on Steam. $5K grand prize plus 10 genre and skill prizes. Submit your demo by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c index 7218006..fc52f78 100644 --- a/drivers/spi/spi-mxs.c +++ b/drivers/spi/spi-mxs.c @@ -587,7 +587,6 @@ static int mxs_spi_probe(struct platform_device *pdev) clk_prepare_enable(ssp->clk); clk_set_rate(ssp->clk, clk_freq); - ssp->clk_rate = clk_get_rate(ssp->clk) / 1000; stmp_reset_block(ssp->base);
The ssp struct has a clock rate field, to provide the actual value, in Hz, of the SSP output clock (the rate of SSP_SCK) after mxs_ssp_set_clk_rate() is called. It should be read-only, except for mxs_ssp_set_clk_rate(). For some reason the spi-mxs driver decides to write to this field on init, and sets it to the value of the SSP input clock (clk_sspN, from the MXS clocking block) in kHz. It shouldn't be setting the value, and certainly shouldn't be setting it with the wrong clock in the wrong units. Signed-off-by: Trent Piepho <tpiepho@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Fabio Estevam <fabio.estevam@freescale.com> Cc: Shawn Guo <shawn.guo@linaro.org> --- drivers/spi/spi-mxs.c | 1 - 1 file changed, 1 deletion(-)