Re: [PATCH] bttv: Add support for PCI-8604PW

2014-03-17 Thread Hans Verkuil
Hi Daniel,

Looks good for the most part, but I have two comments, see below...

On 03/16/2014 11:33 AM, Daniel Glöckner wrote:
> This patch adds support for the PCI-8604PW card equipped with four 878A.
> It is unknown who the manufacturer of this card is and no drivers were
> available during development of the patch. According to images found
> online, the card is originally sold with Linux DVR software.
> 
> A CPLD on the card prevents the 878A from requesting access to the
> bus until an initialization sequence has been issued via GPIOs. The
> implemented sequence uses the minimum number of GPIOs needed to
> successfully unlock bus access. As there are many more GPIOs connected
> to the CPLD, it is very likely that some of the others have an influence
> on the bus arbitration scheduling. This should be investigated further
> in case of performance issues.
> 
> The tested card contains an EEPROM on one of the 878A, but it is
> completely empty (i.e. contains only 0xff), so it is not possible
> to detect the card.
> 
> Signed-off-by: Daniel Glöckner 
> Tested-by: Robert Longbottom 
> ---
>  drivers/media/pci/bt8xx/bttv-cards.c | 102 
> +++
>  drivers/media/pci/bt8xx/bttv.h   |   1 +
>  2 files changed, 103 insertions(+)
> 
> diff --git a/drivers/media/pci/bt8xx/bttv-cards.c 
> b/drivers/media/pci/bt8xx/bttv-cards.c
> index 6662b49..d9c3853 100644
> --- a/drivers/media/pci/bt8xx/bttv-cards.c
> +++ b/drivers/media/pci/bt8xx/bttv-cards.c
> @@ -52,6 +52,7 @@ static void osprey_eeprom(struct bttv *btv, const u8 
> ee[256]);
>  static void modtec_eeprom(struct bttv *btv);
>  static void init_PXC200(struct bttv *btv);
>  static void init_RTV24(struct bttv *btv);
> +static void init_PCI8604PW(struct bttv *btv);
>  
>  static void rv605_muxsel(struct bttv *btv, unsigned int input);
>  static void eagle_muxsel(struct bttv *btv, unsigned int input);
> @@ -2856,6 +2857,22 @@ struct tvcard bttv_tvcards[] = {
>   .tuner_addr = ADDR_UNSET,
>   },
>  
> + /*  card 0xa5-- */
> + [BTTV_BOARD_PCI_8604PW] = {
> + /* PCI-8604PW with special unlock sequence */
> + .name   = "PCI-8604PW",
> + .video_inputs   = 2,
> + /* .audio_inputs= 0, */
> + .svhs   = NO_SVHS,
> + /* The second input is available on CN4, if populated.
> +  * The other 5x2 header (CN2?) connects to the same inputs
> +  * as the on-board BNCs */
> + .muxsel = MUXSEL(2, 3),
> + .tuner_type = TUNER_ABSENT,
> + .no_msp34xx = 1,
> + .no_tda7432 = 1,
> + .pll= PLL_35,
> + },
>  };
>  
>  static const unsigned int bttv_num_tvcards = ARRAY_SIZE(bttv_tvcards);
> @@ -3290,6 +3307,9 @@ void bttv_init_card1(struct bttv *btv)
>   case BTTV_BOARD_ADLINK_RTV24:
>   init_RTV24( btv );
>   break;
> + case BTTV_BOARD_PCI_8604PW:
> + init_PCI8604PW(btv);
> + break;
>  
>   }
>   if (!bttv_tvcards[btv->c.type].has_dvb)
> @@ -4170,6 +4190,88 @@ init_RTV24 (struct bttv *btv)
>  
>  
>  /* --- */
> +/*
> + *  The PCI-8604PW contains a CPLD, probably an ispMACH 4A, that filters
> + *  the PCI REQ signals comming from the four BT878 chips. After power
> + *  up, the CPLD does not forward requests to the bus, which prevents
> + *  the BT878 from fetching RISC instructions from memory. While the
> + *  CPLD is connected to most of the GPIOs of PCI device 0xD, only
> + *  five appear to play a role in unlocking the REQ signal. The following
> + *  sequence has been determined by trial and error without access to the
> + *  original driver.
> + *
> + *  Eight GPIOs of device 0xC are provided on connector CN4 (4 in, 4 out).
> + *  Devices 0xE and 0xF do not appear to have anything connected to their
> + *  GPIOs.
> + *
> + *  The correct GPIO_OUT_EN value might have some more bits set. It should
> + *  be possible to derive it from a boundary scan of the CPLD. Its JTAG
> + *  pins are routed to test points.
> + *
> + */
> +/* --- */
> +static void
> +init_PCI8604PW(struct bttv *btv)
> +{
> + int state;
> +
> + if ((PCI_SLOT(btv->c.pci->devfn) & ~3) != 0xC) {
> + pr_warn("This is not a PCI-8604PW\n");
> + return;
> + }
> +
> + if (PCI_SLOT(btv->c.pci->devfn) != 0xD)
> + return;
> +
> + btwrite(0x080002, BT848_GPIO_OUT_EN);
> +
> + state = (btread(BT848_GPIO_DATA) >> 21) & 7;
> +
> + for (;;) {
> + switch (state) {
> + case 1:
> + case 5:
> + case 6:
> + case 4:
> + pr_debug("PCI-8604PW in state %i, toggling pin\n",
> +  s

Re: [PATCH] bttv: Add support for PCI-8604PW

2014-03-17 Thread Daniel Glöckner
Hi Hans,

On Mon, Mar 17, 2014 at 10:44:29AM +0100, Hans Verkuil wrote:
> > +   switch (state) {
> > +   case 1:
> > +   case 5:
> > +   case 6:
> > +   case 4:
> > +   pr_debug("PCI-8604PW in state %i, toggling pin\n",
> > +state);
> > +   btwrite(0x08, BT848_GPIO_DATA);
> > +   msleep(1);
> > +   btwrite(0x00, BT848_GPIO_DATA);
> > +   msleep(1);
> > +   break;
> > +   case 7:
> > +   pr_info("PCI-8604PW unlocked\n");
> > +   return;
> > +   case 0: /* FIXME */
> 
> Fix what? My guess is that if this state happens, then you have no idea how to
> get out of it. Did you actually see this happen, or is this a theoretical 
> case?

yes, if we are in state 7 and toggle GPIO[19] one more time, the CPLD
goes into state 0, where PCI bus mastering is inhibited again.
We have not managed to get out of that state.

> > +   pr_err("PCI-8604PW locked until reset\n");
> > +   return;

> > +   state = (state << 4) | ((btread(BT848_GPIO_DATA) >> 21) & 7);
> > +
> > +   switch (state) {
> > +   case 0x15:
> > +   case 0x56:
> > +   case 0x64:
> > +   case 0x47:
> > +/* case 0x70: */
> 
> Why is this commented out?

The transition from state 7 to state 0 is, as explained above, valid
but undesired and with this code impossible as we exit as soon as we
are in state 7.

Best regards,

  Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] bttv: Add support for PCI-8604PW

2014-03-17 Thread Hans Verkuil
On 03/17/2014 12:59 PM, Daniel Glöckner wrote:
> Hi Hans,
> 
> On Mon, Mar 17, 2014 at 10:44:29AM +0100, Hans Verkuil wrote:
>>> +   switch (state) {
>>> +   case 1:
>>> +   case 5:
>>> +   case 6:
>>> +   case 4:
>>> +   pr_debug("PCI-8604PW in state %i, toggling pin\n",
>>> +state);
>>> +   btwrite(0x08, BT848_GPIO_DATA);
>>> +   msleep(1);
>>> +   btwrite(0x00, BT848_GPIO_DATA);
>>> +   msleep(1);
>>> +   break;
>>> +   case 7:
>>> +   pr_info("PCI-8604PW unlocked\n");
>>> +   return;
>>> +   case 0: /* FIXME */
>>
>> Fix what? My guess is that if this state happens, then you have no idea how 
>> to
>> get out of it. Did you actually see this happen, or is this a theoretical 
>> case?
> 
> yes, if we are in state 7 and toggle GPIO[19] one more time, the CPLD
> goes into state 0, where PCI bus mastering is inhibited again.
> We have not managed to get out of that state.
> 
>>> +   pr_err("PCI-8604PW locked until reset\n");
>>> +   return;
> 
>>> +   state = (state << 4) | ((btread(BT848_GPIO_DATA) >> 21) & 7);
>>> +
>>> +   switch (state) {
>>> +   case 0x15:
>>> +   case 0x56:
>>> +   case 0x64:
>>> +   case 0x47:
>>> +/* case 0x70: */
>>
>> Why is this commented out?
> 
> The transition from state 7 to state 0 is, as explained above, valid
> but undesired and with this code impossible as we exit as soon as we
> are in state 7.

Can you post a new version with comments that explain this?

This is non-obvious information that should be documented in the code.

Thanks!

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html