| Message ID | 1428340592-3196-2-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 06, 2015 at 05:16:31PM +0000, kernel@martin.sperl.org wrote: > There was a question if we could move the native-cs to GPIO in the driver > itself. > This patch implements this, but it is quite complex for the minimal gains. > Maybe the originally proposed simple warning recommending to use cs_gpio > in DT would be a sufficient alternative that introduces less code. I'm not seeing any particular complexity here. Can you be more explicit please?
> On 06.04.2015, at 19:29, Mark Brown <broonie@kernel.org> wrote: > > I'm not seeing any particular complexity here. Can you be more explicit > please? I just see the high-possibility of bitrot for something that is IMO better moved to a modified device-tree config. -- 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 Mon, Apr 06, 2015 at 07:35:37PM +0200, Martin Sperl wrote: > > On 06.04.2015, at 19:29, Mark Brown <broonie@kernel.org> wrote: > > I'm not seeing any particular complexity here. Can you be more explicit > > please? > I just see the high-possibility of bitrot for something that is IMO better > moved to a modified device-tree config. Requiring every system to update the DT to specify information that we can already get easily (and using a less readable format in that update) doesn't seem like a clear win for me...
> On 06.04.2015, at 19:45, Mark Brown <broonie@kernel.org> wrote: > > Requiring every system to update the DT to specify information that we > can already get easily (and using a less readable format in that update) > doesn't seem like a clear win for me... If you prefer that, then apply it - i just wanted to offer both options! One note though: as most systems will get "deploying" the rpi-foundation pre-build kernels and their corresponding device tree it seems unlikely that a lot of alternate dts will get used... So when they switch, then >99% of all users will be using the new code plus the new device-tree that handles that. That is why I see this as a good example of future bit-rot, where we have to maintain 20+ lines versus about 5 lines with just a message... -- 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 Mon, Apr 06, 2015 at 07:58:15PM +0200, Martin Sperl wrote: > One note though: as most systems will get "deploying" the rpi-foundation > pre-build kernels and their corresponding device tree it seems unlikely > that a lot of alternate dts will get used... > So when they switch, then >99% of all users will be using the new code > plus the new device-tree that handles that. People adding things to the standard kernel (presumably people push their board support in there) would still have to work out which chip select corresponds to which GPIO and write that mapping into their DT with the corresponding loss of legibility there, and of course there's the cost of modifying existing users. It *would* be nice if the mapping to which pin has which function were pushed into the pinctrl driver (and then possibly into the DT for it) but generally this seems like something we should be doing more of not less of.
On 04/06/2015 11:16 AM, kernel@martin.sperl.org wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent. > > This allows for some support of some optimizations that are not > possible due to HW-gliches on the CS line - especially filling > the FIFO before enabling SPI interrupts (by writing to CS register) > while the transfer is already in progress (See commit: e3a2be3030e2) > > This patch also works arround some issues in bcm2835-pinctrl which does not > set the value when setting the GPIO as output - it just sets up output and > (typically) leaves the GPIO as low. When a fix for this is merged then this > gpio_set_value can get removed from bcm2835_spi_setup. > diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c > @@ -360,13 +367,45 @@ static int bcm2835_spi_setup(struct spi_device *spi) > return 0; > if (gpio_is_valid(spi->cs_gpio)) > return 0; > - if (spi->chip_select < 3) > + if (spi->chip_select > 1) { > + /* error in the case of native CS requested with CS > 1 > + * officially there is a CS2, but it is not documented > + * which GPIO is connected with that... > + */ > + dev_err(&spi->dev, > + "setup: only two native chip-selects are supported\n"); I believe the bcm283x have 2 types of SPI controller. There is 1 instance of the HW that this driver controls (SPI0), and that has CS0, CS1. There are two instances of a different SPI HW block (SPI1, SPI2), and those each have CS0, CS1, CS2. At least, that's my interpretation of the table that shows the pinctrl module's per-pin alternate function values. > + return -EINVAL; > + } > + /* now translate native cs to GPIO */ > + > + /* get the gpio chip for the base */ > + chip = gpiochip_find("pinctrl-bcm2835", chip_match_name); > + if (!chip) > return 0; Should that be an error? Not being able to find the gpiochip implies the code can't manipulate the CS lines at all, I think? -- 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 04/06/2015 11:58 AM, Martin Sperl wrote: > >> On 06.04.2015, at 19:45, Mark Brown <broonie@kernel.org> wrote: >> >> Requiring every system to update the DT to specify information that we >> can already get easily (and using a less readable format in that update) >> doesn't seem like a clear win for me... > > If you prefer that, then apply it - i just wanted to offer both options! > > One note though: as most systems will get "deploying" the rpi-foundation > pre-build kernels and their corresponding device tree it seems unlikely > that a lot of alternate dts will get used... > > So when they switch, then >99% of all users will be using the new code > plus the new device-tree that handles that. I wouldn't be at all surprised if many people maintained their own DT in order to add in support for whatever extra peripherals they've connected to the expansion header. The correct way to do this is of course to add a patch on top of the kernel tree, and rebase, rebuild, and reinstall the DTB whenever the kernel gets upgraded. However, the likelihood of /everyone/ doing that seems low; I'd certainly expect to see some people simply not upgrading their DTB. -- 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 07.04.2015, at 05:01, Stephen Warren <swarren@wwwdotorg.org> wrote: > I believe the bcm283x have 2 types of SPI controller. There is 1 > instance of the HW that this driver controls (SPI0), and that has CS0, > CS1. There are two instances of a different SPI HW block (SPI1, SPI2), > and those each have CS0, CS1, CS2. At least, that's my interpretation of > the table that shows the pinctrl module's per-pin alternate function values. Yes and no - SPI1 and SPI2 are a totally different beasts as these are "auxiliar devices" that have 2x2 word fifos, no DMA and minimal interrupt support and use a distinct register layout compared to SPI0. See BCM2835 Arm Peripherials page 20-27 for details. Essentially these are intended to get used for low speed devices or devices that run short transfers (<4-8 bytes) So these devices would need a totally separate SPI driver, which this driver does not and can not handle... > Should that be an error? Not being able to find the gpiochip implies the > code can't manipulate the CS lines at all, I think? Will add an dev_warn_once and contine without optimizations - the code still does check for the cs_gpio, so we would run without it? It could just be a rename of the pinctrl driver that would trigger this. You want it as an error? Martin -- 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 2015-04-06 20:10, Mark Brown wrote: > People adding things to the standard kernel (presumably people push > their board support in there) would still have to work out which > chip select corresponds to which GPIO and write that mapping into > their DT with the corresponding loss of legibility there, and of > course there's the cost of modifying existing users. > > It *would* be nice if the mapping to which pin has which function > were pushed into the pinctrl driver (and then possibly into the DT > for it) but generally this seems like something we should be doing > more of not less of. > The foundation gets to a point of using device-tree overlays to minimize this kind of manual changes - the default still would include CS0=GPIO8 and CS1=GPIO7. If someone needs more CS, then the extra CS needs to get configured in an overlay overriding those pins - so not a huge difference If we add "cs_gpios = <&gpio 8 0> <&gpio 7 0>" to the dtsi or assume it is native, where this code essentially does what is needed to translate it to the above. Just tell me if we need spi_bus_lock on the "change" path to avoid unpleasant behavior and I will create a patch incorporating that. Spi_setup can sleep, so we can wait for the bus to become idle before such a change... Alternatively we could also add something like: int initial_chip_select_setup(int cs, int cspol); as a method in spi_master that the framework would call prior to registering all spi_devices with spi_device_register. In the code for the spi-bcm2835 we can run the native-CS to GPIO translation. I could create a patch that handles this. If you like it as is, merge it. -- 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 04/06/2015 11:45 PM, Martin Sperl wrote: > >> On 07.04.2015, at 05:01, Stephen Warren <swarren@wwwdotorg.org> wrote: >> I believe the bcm283x have 2 types of SPI controller. There is 1 >> instance of the HW that this driver controls (SPI0), and that has CS0, >> CS1. There are two instances of a different SPI HW block (SPI1, SPI2), >> and those each have CS0, CS1, CS2. At least, that's my interpretation of >> the table that shows the pinctrl module's per-pin alternate function values. > > Yes and no - SPI1 and SPI2 are a totally different beasts as these are > "auxiliar devices" that have 2x2 word fifos, no DMA and minimal interrupt > support and use a distinct register layout compared to SPI0. > See BCM2835 Arm Peripherials page 20-27 for details. > > Essentially these are intended to get used for low speed devices or devices > that run short transfers (<4-8 bytes) > > So these devices would need a totally separate SPI driver, which this driver > does not and can not handle... That's exactly what I said. >> Should that be an error? Not being able to find the gpiochip implies the >> code can't manipulate the CS lines at all, I think? > > Will add an dev_warn_once and contine without optimizations - the code > still does check for the cs_gpio, so we would run without it? > It could just be a rename of the pinctrl driver that would trigger this. > > You want it as an error? If the rest of the driver is expected to always check whether spi->cs_gpio is valid or not, and support the gpio-is-not-valid case, I guess it's OK for that case *not* to error out. I think we should at least warn to indicate that something unexpected happened. Shouldn't spi->cs_gpio be set to e.g. -1 if the pinctrl device can't be found, so that the rest of the driver does know that the GPIO is invalid? -- 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 Mon, Apr 06, 2015 at 05:16:31PM +0000, kernel@martin.sperl.org wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent. This doesn't apply against current code, please check and resend.
On Mon, Apr 06, 2015 at 05:16:31PM +0000, kernel@martin.sperl.org wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > Transforms the bcm-2835 native SPI-chip select to their gpio-cs equivalent. Sorry, acutally it does - applied, thanks.
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index 88b808b..b2651fa 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -351,8 +351,15 @@ static void bcm2835_spi_set_cs(struct spi_device *spi, bool gpio_level) bcm2835_wr(bs, BCM2835_SPI_CS, cs); } +static int chip_match_name(struct gpio_chip *chip, void *data) +{ + return !strcmp(chip->label, data); +} + static int bcm2835_spi_setup(struct spi_device *spi) { + int err; + struct gpio_chip *chip; /* * sanity checking the native-chipselects */ @@ -360,13 +367,45 @@ static int bcm2835_spi_setup(struct spi_device *spi) return 0; if (gpio_is_valid(spi->cs_gpio)) return 0; - if (spi->chip_select < 3) + if (spi->chip_select > 1) { + /* error in the case of native CS requested with CS > 1 + * officially there is a CS2, but it is not documented + * which GPIO is connected with that... + */ + dev_err(&spi->dev, + "setup: only two native chip-selects are supported\n"); + return -EINVAL; + } + /* now translate native cs to GPIO */ + + /* get the gpio chip for the base */ + chip = gpiochip_find("pinctrl-bcm2835", chip_match_name); + if (!chip) return 0; - /* error in the case of native CS requested with CS-id > 2 */ - dev_err(&spi->dev, - "setup: only three native chip-selects are supported\n"); - return -EINVAL; + /* and calculate the real CS */ + spi->cs_gpio = chip->base + 8 - spi->chip_select; + + /* and set up the "mode" and level */ + dev_info(&spi->dev, "setting up native-CS%i as GPIO %i\n", + spi->chip_select, spi->cs_gpio); + + /* set up GPIO as output and pull to the correct level */ + err = gpio_direction_output(spi->cs_gpio, + (spi->mode & SPI_CS_HIGH) ? 0 : 1); + if (err) { + dev_err(&spi->dev, + "could not set CS%i gpio %i as output: %i", + spi->chip_select, spi->cs_gpio, err); + return err; + } + /* the implementation of pinctrl-bcm2835 currently does not + * set the GPIO value when using gpio_direction_output + * so we are setting it here explicitly + */ + gpio_set_value(spi->cs_gpio, (spi->mode & SPI_CS_HIGH) ? 0 : 1); + + return 0; } static int bcm2835_spi_probe(struct platform_device *pdev)