On 23 August 2016 at 19:41, David Daney <ddaney@xxxxxxxxxxxxxxxxxx> wrote: > On 08/23/2016 07:53 AM, Ulf Hansson wrote: >> >> On 12 July 2016 at 20:18, Steven J. Hill <steven.hill@xxxxxxxxxx> wrote: > > [...] > >>> +#include <asm/byteorder.h> >>> +#include <asm/octeon/octeon.h> >> >> > > OK, we will duplicate any needed definitions from octeon.h into the driver > source file. Why can't you share it via a platfrom data header at include/linux/platform_data/* ? [...] >> > > I guess we will put it in arch/mips/cavium-octeon/octeon-mmc-platform.c or > something. There is also drivers/soc/* to consider. I am not sure what suits you best. [...] >>> + emm_dma.u64 = 0; >>> + emm_dma.s.bus_id = slot->bus_id; >>> + emm_dma.s.dma_val = 1; >>> + emm_dma.s.sector = mmc_card_blockaddr(mmc->card) ? 1 : 0; >>> + emm_dma.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0; >>> + if (mmc_card_mmc(mmc->card) || >>> + (mmc_card_sd(mmc->card) && >>> + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT))) >>> + emm_dma.s.multi = 1; >> >> >> Could you elaborate on exactly what you are doing here. I don't >> understand why you need to check whether the card supports CMD23. > > > > The MMC host hardware doesn't issue single commands, because that would > require the driver and OS MMC core to do work to determine the proper > sequence of commands. Instead, this hardware is superior to most other MMC > bus hosts, the sequence of MMC command required to execute a transfer are > issued automatically by the bus hardware. Oh, nice! Actually I have heard of similar HW, although I am not sure whether there is some mmc driver that has deployed support for this. Anyway, perhaps we at a later point can think of if there is a clever way to optimize the request path for these kind of HWs. > > Because the interface expected by the Linux MMC core is at a lower level > than what the OCTEON MMC host can accept we need to examine the the > mmc_request and card capabilities to determine which operations most closely > match what is being requested. > > In the case of emm_dma.s.multi above, the bus hardware will automatically > issue CMD23 when this bit is set, so we only set it if we think it is a > valid thing to do. I noticed that the similar check is done in the mmc block layer, perhaps we should add a helper function like mmc_card_cmd23() which tells whether the cards supports CMD23. If you like to add a helper as part of this series, it would be nice - although we can also deal with that later if you prefer so. > > >> >> Moreover you must not access mmc->card unless you make sure there is a >> valid pointer for it. > > > OK, it has never been found to be invalid in the wild. When a transfer is > requested, it always targets some card, but we can add a check at the top to > return an error code if the card is somehow NULL. That's probably because you also requires a multi block transfer for dma jobs, which is when the card has been created... I would have verified that the card exists... [...] >>> + >>> + /* Alternatively a GPIO may be specified to control slot power */ >>> + slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", >>> GPIOD_OUT_LOW); >> >> >> No, I am not happy with this as we discussed earlier. You need to >> parse the DTB from SoC specifc code and manage the power GPIO from >> there. >> >> Ideally you should register the power GPIO as a GPIO regulator for the >> cavium mmc slot device. > > > What do we do if the GPIO doensn't really control power to the card, but > rather is just a bus isolator on the data bus lines? The device tree will That more sounds like a pinctrl to me then. > still have a property called "power" (as that is a legacy binding that > cannot be changed), but no power control is done. > > In this case, is it still appropiate to use the regulator framework? Probably not. > > I don't see what is gained. This is an SoC specific MMC controller that is > connected in a manner that doesn't really match the Linux regulator > framework. Is it really cleaner to put 100 lines of ugly hacks in the > platform code instead of a couple of lines here in the driver? What is > achieved? We arn't creating an elegant framework, but instead jumping > through hoops to make an archectual mismatch between various Linux driver > frameworks be bent to fit as a matter of principle. > > If that is what you require to merge the driver we will do it. We have cleaned up lots of mmc hacks that dealt with "regulators" in all kind of home-brewed manners. It was a mess. In this particular case it also seems a little bit unclear what the regulators (power GPIOs) is really used for. Very similar to the experience I had with the earlier hacks. So, please try to describe in detail about what the power GPIO are and how/if they connects to the card. if they are considered as to control I/O voltage level or power to the card, then those shall be modelled as regulators. Sorry if you find me being stubborn on this, although I hope the background described above makes you understand a bit better. [...] Kind regards Uffe
